Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Fix and improve hardening for Windows #74

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jan 9, 2024

Some compile flags, for example -fstack-protector-all, might impact the linking stage as well. Instead of guessing, this PR makes the try_append_cxx_flags function add new flags in both compile and link contexts.

@hebasto
Copy link
Owner Author

hebasto commented Jan 9, 2024

Friendly ping @fanquake @theuni @TheCharlatan :)

@hebasto
Copy link
Owner Author

hebasto commented Jan 9, 2024

Rebased.

@hebasto
Copy link
Owner Author

hebasto commented Jan 16, 2024

Rebased.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK after addressing documentation comment.

@@ -131,3 +135,7 @@ endif()
if(NOT WITH_USDT AND "@no_usdt@" STREQUAL "1")
set(WITH_USDT OFF CACHE STRING "")
endif()

if(NOT HARDENING AND "@no_harden@" STREQUAL "1")
set(HARDENING OFF CACHE STRING "")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting to see this set hardening to true if not @no_harden@, but it's already the default in the main build. So it's doing the same thing as our config.site which makes sense.

@hebasto
Copy link
Owner Author

hebasto commented Jan 26, 2024

As it was mentioned during yesterday's meeting, if it won't hurt or have any significant drawbacks why not providing target_link_libraries(hardening_interface INTERFACE ssp) without a test?

Or, even better, target_link_options(hardening_interface INTERFACE -fstack-protector-all) to allow a compiler to decide whether to link to libssp or not.

@hebasto
Copy link
Owner Author

hebasto commented Jan 27, 2024

Rebased.

@hebasto hebasto added help wanted Extra attention is needed question labels Jan 27, 2024
@hebasto
Copy link
Owner Author

hebasto commented Feb 5, 2024

This PR and its description have been updated per feedback from the recent call on 2024-02-01.

@TheCharlatan
Copy link

LGTM

@hebasto hebasto merged commit 4ac1e24 into cmake-staging Feb 9, 2024
22 checks passed
@hebasto hebasto mentioned this pull request Feb 9, 2024
hebasto added a commit that referenced this pull request Feb 9, 2024
0d985e5 fixup! cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov)

Pull request description:

  This PR is a follow-up on #74.

  No need to try a flag in the linker context if it is not supported in the compiler context.

  For example, on the staging branch:
  ```
  $ env CC=clang CXX=clang++ cmake -B build
  ...
  -- Performing Test CXX_SUPPORTS__FSTACK_REUSE_NONE
  -- Performing Test CXX_SUPPORTS__FSTACK_REUSE_NONE - Failed
  CMake Warning at cmake/module/TryAppendCXXFlags.cmake:125 (message):
    The -fstack-reuse=none fail(s) to link.
  Call Stack (most recent call first):
    CMakeLists.txt:310 (try_append_cxx_flags)

  ...
  ```

Top commit has no ACKs.

Tree-SHA512: 6e73f00838eeb066635cbbf1acb41955b32a83e9939b1cabe62152b312d2e9c1f8dcacb6b11f529769b541925cc778d9e63e0968615ff4404a393115199300f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants