cmake: Add CheckLinkerSupportsPIE module #31359

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241124-pie changing 2 files +32 −10
  1. hebasto commented at 11:23 am on November 24, 2024: member

    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 #30771.

  2. hebasto added the label Bug on Nov 24, 2024
  3. hebasto added the label Build system on Nov 24, 2024
  4. 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 vasild

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

  5. hebasto added this to the milestone 29.0 on Nov 24, 2024
  6. hebasto commented at 11:24 am on November 24, 2024: member
    cc @vasild
  7. hebasto force-pushed on Nov 24, 2024
  8. hebasto force-pushed on Nov 25, 2024
  9. cmake: Add `CheckLinkerSupportsPIE` module
    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
  10. hebasto force-pushed on Nov 26, 2024
  11. hebasto commented at 4:10 pm on November 26, 2024: member
    The upstream fix has been mentioned in the comment.
  12. 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.

  13. vasild approved
  14. 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.

  15. 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?

  16. 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:

    1. The default value, EXECUTABLE, enables both compiler and linker checks.
    2. 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.

  17. 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).
  18. 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 :(


github-metadata-mirror

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