CheckLinkerSupportsPIE module
#31359
This new module is a wrapper around CMake’s CheckPIESupported module that:
CMAKE_TRY_COMPILE_TARGET_TYPE value at the call site.Fixes #30771.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31359.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
This new module is a wrapper around CMake's `CheckPIESupported` module
that:
1. Fixes an upstream bug.
See: https://gitlab.kitware.com/cmake/cmake/-/issues/26463.
2. Enhances robustness by ensuring the linker is invoked, regardless of
the `CMAKE_TRY_COMPILE_TARGET_TYPE` value at the call site.
Fixes an upstream bug Enhances robustness
Wondering if in this case given CMakes tooling has not only been buggy, but then is also apparently not robust enough for production use without wrapping it in more layers of abstraction, maybe we should just revert to (as we did in Autotools) something like using the flags we want directly.
ACK d3f42fa08fc385752881afa5740f40287cfb4d8b
Without this patch I get:
0CMake Warning at CMakeLists.txt:195 (message):
1 PIE is not supported at link time: PIE (CXX): Change Dir:
2 '/tmp/build/clang20-fuzz0/CMakeFiles/CMakeScratch/TryCompile-8aYRFx'
3...
4 ld: error: relocation R_X86_64_32S cannot be used against local symbol;
5 recompile with -fPIC
with this patch - no errors.
Enhances robustness
Can you explain this further? Why is CMakes own check not “robust” enough to use directly? Is this another bug/something that should be improved upstream?
Enhances robustness
Can you explain this further? Why is CMakes own check not “robust” enough to use directly? Is this another bug/something that should be improved upstream?
Sure thing!
(Almost?) every CMake check internally uses the try_compile() command, whose behaviour, in turn, depends on the CMAKE_TRY_COMPILE_TARGET_TYPE variable:
EXECUTABLE, enables both compiler and linker checks.STATIC_LIBRARY value enables only compiler checks.To mimic Autotools’ behaviour, we disabled linker checks by setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY globally (perhaps not the best design). This effectively separates the entire CMake script into regions where CMAKE_TRY_COMPILE_TARGET_TYPE is:
STATIC_LIBRARYEXECUTABLEThe following line:https://github.com/bitcoin/bitcoin/blob/d3f42fa08fc385752881afa5740f40287cfb4d8b/cmake/module/CheckLinkerSupportsPIE.cmake#L8-L9
does not change the current behaviour because, at this point, CMAKE_TRY_COMPILE_TARGET_TYPE is unset, which is equivalent to the EXECUTABLE value. However, with this line, it is safe to move the check_linker_supports_pie() command around.
I was going to ask why we’d need to worry about the value of CMAKE_TRY_COMPILE_TARGET_TYPE but @hebasto answered that above. We broke it ourselves!
This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don’t think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should “just work” (minus the upstream bugs).
Agreed. I forgot that we set CMAKE_TRY_COMPILE_TARGET_TYPE globally. And even worse, it’s buried in a module. If that upsets CMake internal tests, I think we should undo that.
Could we invert this logic? ie turn check_cxx_source_* and friends to functions and set CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC in the necessary local scopes?
Concept ACK to wrapping for the upstream bug though. That’s unfortunate :(