cmake: Fix passing APPEND_*FLAGS to secp256k1 subtree #31379

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:241127-append-cflags changing 1 files +7 −7
  1. hebasto commented at 9:52 am on November 27, 2024: member

    On the master branch @ 70e20ea024ce4f39abc4022e1ba19d5a6db2a207, the APPEND_CPPFLAGS, APPEND_CFLAGS and APPEND_LDFLAGS are not correctly applied when building C code in the secp256k1 subtree, as intended.

    This behaviour occurs due to two issues:

    1. The command here: https://github.com/bitcoin/bitcoin/blob/70e20ea024ce4f39abc4022e1ba19d5a6db2a207/src/CMakeLists.txt#L77 does not affect the code in add_subdirectory(secp256k1) above it.

    2. APPEND_LDFLAGS is not passed to the subtree’s build system at all.

    This PR fixes both issues.

    Additionally, the helper variables core_sanitizer_cxx_flags and core_sanitizer_linker_flags have been removed.

  2. hebasto added the label Build system on Nov 27, 2024
  3. DrahtBot commented at 9:52 am on November 27, 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/31379.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

  4. dergoegge commented at 10:03 am on November 27, 2024: member
    How can the broken behavior be observed? It appears to be working to me based on: the MSan job not spitting out false positives and coverage reports including coverage for secp.
  5. fanquake commented at 10:06 am on November 27, 2024: member

    APPEND_LDFLAGS is not passed to the subtree’s build system at all.

    I thought #30791 fixed an issue related to passing down linker flags via APPEND_LDFLAGS. So has that regressed / broken since, or just never worked?

  6. hebasto commented at 10:19 am on November 27, 2024: member

    How can the broken behavior be observed?

    For example:

    0cmake -B build -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic"
    1# Note the absence of the `SECP256K1_APPEND_CFLAGS ...` line in the secp256k1 summary
    2cmake --build build --target secp256k1 --verbose 
    

    It appears to be working to me based on: the MSan job not spitting out false positives and coverage reports including coverage for secp.

    Yes, passing sanitizers flags works on the master branch.

    Another use case to consider: When building for test coverage with GCC the -fprofile-update=prefer-atomic flag has to be passed to a compiler on some build platforms. Using -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic" does not work.

    APPEND_LDFLAGS is not passed to the subtree’s build system at all.

    I thought #30791 fixed an issue related to passing down linker flags via APPEND_LDFLAGS.

    #30791 fixed passing the sanitizer flags.

    So has that regressed / broken since, or just never worked?

    The regression happened somewhere in the CMake staging branch. In this repository, it has never worked since #30454.

  7. dergoegge commented at 10:55 am on November 27, 2024: member

    Yes, passing sanitizers flags works on the master branch.

    Our MSan ci job passes C/CXXFLAGS to depends (including non-"-fsanitize=" flags e.g. -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls), why do those flags propagate to secp but the APPEND_CFLAGS flags do not? Would it make sense to use the same mechanism for both?

    (I’m very confused by all the different ways of passing *FLAGS…)

  8. hebasto commented at 12:55 pm on November 27, 2024: member

    Yes, passing sanitizers flags works on the master branch.

    Our MSan ci job passes C/CXXFLAGS to depends (including non-"-fsanitize=" flags e.g. -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls), why do those flags propagate to secp but the APPEND_CFLAGS flags do not?

    Flags set in depends are propagated via the toolchain file.

    Would it make sense to use the same mechanism for both?

    Using the toolchain file is the proper way to handle tools options, including compiler flags, when building with depends. This mechanism employs the CMAKE_<LANG>_FLAGS_INIT variables, which are not suitable for other uses.

    (I’m very confused by all the different ways of passing *FLAGS…)

    Passing -DCMAKE_<LANG>_FLAGS on the command line works for most trivial cases.

    The APPEND_*FLAGS have unique feature: they are appended to all other flags set by the build system, which guarantees they are enforced—at least for GCC and Clang.

  9. theuni commented at 6:42 pm on December 3, 2024: member

    🤦🤦

    Could you split this into 2 commits? The fixes you describe sound simple enough, but the removal of the helpers makes this hard to review.

  10. cmake: Remove `core_sanitizer_{cxx,linker}_flags` helper variables
    This change make the code more concise and minimizes the diff in the
    subsequent commit.
    eb540a2629
  11. cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree c4c5cf1748
  12. hebasto force-pushed on Dec 3, 2024
  13. hebasto commented at 9:00 pm on December 3, 2024: member

    🤦🤦

    Could you split this into 2 commits? The fixes you describe sound simple enough, but the removal of the helpers makes this hard to review.

    Sure thing! Done.


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