[cmake] Fix -z noexecstack portability #4222
Merged
+18
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Issue reported by @ryandesign and @MarcusCalhoun-Lopez. CMake doesn't support spaces in flags. This caused older versions of gcc to ignore the unknown flag "-z noexecstack" on MacOS since it was interpreted as a single flag, not two separate flags. Then, during compilation it was treated as "-z" "noexecstack", which was correctly forwarded to the linker. But the MacOS linker does not support `-z noexecstack` so compilation failed. The fix is to use `-Wl,-z,noexecstack`. This is never misinterpreted by a compiler. However, not all compilers support this syntax to forward flags to the linker. To fix this issue, we check if all the relevant `noexecstack` flags have been successfully set, and if they haven't we disable assembly. See also PR#4056 and PR#4061. I decided to go a different route because this is simpler. It might not successfully set these flags on some compilers, but in that case it also disables assembly, so they aren't required. Test Plan: ``` mkdir build-cmake cmake ../build/cmake/CMakeLists.txt make -j ``` See that the linker flag is successfully detected & that assembly is enabled. Run the same commands on MacOS which doesn't support `-Wl,-z,noexecstack` and see that everything compiles and that `LD_FLAG_WL_Z_NOEXECSTACK` and `ZSTD_HAS_NOEXECSTACK` are both false.
This was referenced Dec 20, 2024
|
Is there a test we could create in CI that would trigger the problem (without this PR) ? |
|
I'm not sure. My understanding is that it requires quite an old version of gcc on MacOS to test. I'm not sure we'll be able to get a gcc old enough in our Mac test environment. |
Cyan4973
approved these changes
Dec 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Issue reported by @ryandesign and @MarcusCalhoun-Lopez. Thanks to @ryandesign for investigating this issue in-depth & making fixing it easy.
CMake doesn't support spaces in flags. This caused older versions of gcc to ignore the unknown flag "-z noexecstack" on MacOS since it was interpreted as a single flag, not two separate flags. Then, during compilation it was treated as "-z" "noexecstack", which was correctly forwarded to the linker. But the MacOS linker does not support
-z noexecstackso compilation failed.The fix is to use
-Wl,-z,noexecstack. This is never misinterpreted by a compiler. However, not all compilers support this syntax to forward flags to the linker. To fix this issue, we check if all the relevantnoexecstackflags have been successfully set, and if they haven't we disable assembly.See also PR #4056 and PR #4061. I decided to go a different route because this is simpler. It might not successfully set these flags on some compilers, but in that case it also disables assembly, so they aren't required.
Test Plan:
See that the linker flag is successfully detected & that assembly is enabled.
Run the same commands on MacOS which doesn't support
-Wl,-z,noexecstackand see that everything compiles and thatLD_FLAG_WL_Z_NOEXECSTACKandZSTD_HAS_NOEXECSTACKare both false.