cmake: Add `CheckLinkerSupportsPIE` module

hebasto commented at 11:23 am on November 24, 2024: member

This new module is a wrapper around CMake’s CheckPIESupported module that fixes an upstream bug.

See: https://gitlab.kitware.com/cmake/cmake/-/issues/26463.

Fixes #30771.

hebasto added the label Bug on Nov 24, 2024

hebasto added the label Build system on Nov 24, 2024

DrahtBot commented at 11:23 am on November 24, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31359.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni
Stale ACK	vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31665 (build: Make config warnings fatal if -DWERROR=ON by davidgumberg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto added this to the milestone 29.0 on Nov 24, 2024

hebasto commented at 11:24 am on November 24, 2024: member

cc @vasild

hebasto force-pushed on Nov 24, 2024

hebasto force-pushed on Nov 25, 2024

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

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.

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?

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:

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 :(

fanquake commented at 1:49 pm on January 9, 2025: member

What is the status of this?

cmake: Add `CheckLinkerSupportsPIE` module

This new module serves as a wrapper around CMake's `CheckPIESupported`
module and addresses an upstream bug:
- https://gitlab.kitware.com/cmake/cmake/-/issues/26463
- https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034

65a0920ca6

hebasto force-pushed on Jan 15, 2025

hebasto commented at 4:11 pm on January 15, 2025: member

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.

Undone in #31662.

What is the status of this?

Updated and ready for review.

cmake: Add CheckLinkerSupportsPIE module #31359

Code Coverage & Benchmarks

Reviews

Conflicts

cmake: Add `CheckLinkerSupportsPIE` module #31359