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
ACK	theuni, 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 -DWCONFIGURE_ERROR=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: Refer to the configure log instead of printing PIE test error

This change improves the user experience on systems where the toolchain
does not support PIE.

81c174e318

hebasto commented at 3:40 pm on January 23, 2025: member

Add a commit to address #31724 (comment):

… I’m just not sure this is useful output. It’s verbose compiler output, that also says error multiple times (not warning), and doesn’t explain what anything means, or why it might/might not be a problem.

theuni commented at 8:57 pm on January 23, 2025: member

The real fix here is in #31662, no?

And do we want to be just silently building without pie? Should we error unless hardening is disabled?

hebasto commented at 10:23 am on January 24, 2025: member

@theuni

The real fix here is in #31662, no?

The bug described in #30771#issue-2497352810 / https://gitlab.kitware.com/cmake/cmake/-/issues/26463 is fixed in the first commit:https://github.com/bitcoin/bitcoin/blob/65a0920ca6b11b4250b5cdcf174eaa9ea55fa6ed/cmake/module/CheckLinkerSupportsPIE.cmake#L8-L16

Changes from #31662 do not affect the CheckLinkerSupportsPIE module because the CMAKE_TRY_COMPILE_TARGET_TYPE variable was not modified prior to the module’s usage.

And do we want to be just silently building without pie? Should we error unless hardening is disabled?

IIRC, during the development of the staging branch, we had a discussion on this topic and decided to warn the user when PIE is not available. Since this PR does not alter that behaviour, could we continue this discussion in a dedicated issue or PR?

Besides, it does not affect our Tier 1 (release) builds or Tier 2 builds (documented or tested in the CI).

theuni approved

theuni commented at 6:39 pm on February 12, 2025: member

utACK 81c174e3186efae084829dcde314b081cad3d3cb.

Nice work helping to get this fixed upstream. Makes sense for us to carry the workaround for a few years. No opinion on the second commit.

DrahtBot requested review from vasild on Feb 12, 2025

in cmake/module/CheckLinkerSupportsPIE.cmake:24 in 81c174e318

19+  check_pie_supported(OUTPUT_VARIABLE output LANGUAGES CXX)
20+  if(CMAKE_CXX_LINK_PIE_SUPPORTED)
21+    set(CMAKE_POSITION_INDEPENDENT_CODE ON PARENT_SCOPE)
22+  elseif(NOT WIN32)
23+    # The warning is superfluous for Windows.
24+    message(WARNING "PIE is not supported at link time. See the configure log for details.")

vasild commented at 7:33 am on February 13, 2025:

See the configure log

It would be nice to mention the actual file name of the “configure log” because that may not be obvious.

0    message(WARNING "PIE is not supported at link time. See ${CMAKE_BINARY_DIR}/CMakeFiles/CMakeConfigureLog.yaml for details.")

hebasto commented at 8:10 am on February 13, 2025:

It might depend on the used CMake version: https://github.com/bitcoin/bitcoin/blob/048ef98626b9ed62355923f089ad5cc5b6a898a3/ci/test/GetCMakeLogFiles.cmake#L5-L9

vasild commented at 8:19 am on February 13, 2025:

Ok, not so important. If this recurs then maybe move that snippet to a location where it can be reused by ci/test/GetCMakeLogFiles.cmake and by cmake/module/CheckLinkerSupportsPIE.cmake, so they can use ${log_files}.

vasild approved

vasild commented at 8:05 am on February 13, 2025: contributor

ACK 81c174e3186efae084829dcde314b081cad3d3cb

I re-checked this. Without this PR I get the warning as mentioned in #31359#pullrequestreview-2465699855 and with this PR there is no warning. Further, without this PR, there is no -fPIC or -fPIE when compiling. With this PR -fPIC is added (not -fPIE).

glozow assigned fanquake on Feb 13, 2025

fanquake merged this on Feb 14, 2025

fanquake closed this on Feb 14, 2025

hebasto deleted the branch on Feb 20, 2025

cmake: Add CheckLinkerSupportsPIE module #31359

Code Coverage & Benchmarks

Reviews

Conflicts

cmake: Add `CheckLinkerSupportsPIE` module #31359