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.
d3f42fa08f
hebasto force-pushed
on Nov 26, 2024
hebasto
commented at 4:10 pm on November 26, 2024:
member
The upstream fix has been mentioned in the comment.
fanquake
commented at 9:56 am on November 27, 2024:
member
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.
vasild approved
vasild
commented at 6:08 pm on November 27, 2024:
contributor
ACKd3f42fa08fc385752881afa5740f40287cfb4d8b
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.
fanquake
commented at 10:43 am on December 2, 2024:
member
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?
hebasto
commented at 11:12 am on December 2, 2024:
member
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?
The default value, EXECUTABLE, enables both compiler and linker checks.
The 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:
unset
set to STATIC_LIBRARY
set to EXECUTABLE
The 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.
fanquake
commented at 11:15 am on December 2, 2024:
member
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).
theuni
commented at 6:23 pm on December 3, 2024:
member
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 :(
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me