cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree

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:

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

hebasto added the label Build system on Nov 27, 2024

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.

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.

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?

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.

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…)

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.

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.

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

cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree c4c5cf1748

hebasto force-pushed on Dec 3, 2024

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.

theuni commented at 4:46 pm on December 5, 2024: member

I guess I’m a little confused about how this should work. It seems it’s not very consistent.

secp is the only C code we have. So arguably there’s no need for APPEND_CFLAGS at all when SECP256K1_APPEND_CFLAGS can just be used directly, no?

And as for APPEND_LDFLAGS, because secp is static, we don’t actually invoke the linker for the library. Only for tests I guess? I believe that’s why we didn’t forward them in the initial CMake port.

So.. how should this all work ideally? Do we really need to forward the flags at all? Asking because if no, that would explain how this has managed to be silently “broken”.

fanquake commented at 5:08 pm on January 7, 2025: member

I guess I’m a little confused about how this should work. It seems it’s not very consistent.

So.. how should this all work ideally? Do we really need to forward the flags at all?

I agree. It’d be good to know what the goals are here, if some amount of this code is redundant, and what the status of this change is.

hebasto commented at 10:02 am on January 14, 2025: member

secp is the only C code we have. So arguably there’s no need for APPEND_CFLAGS at all when SECP256K1_APPEND_CFLAGS can just be used directly, no?

Correct. It is solely about maintaining consistency with the other APPEND_* flags. I believe the following

0cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic"

is better than

0cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DSECP256K1_APPEND_CFLAGS="-fprofile-update=prefer-atomic"

And as for APPEND_LDFLAGS, because secp is static, we don’t actually invoke the linker for the library. Only for tests I guess?

Correct. Please refer to https://github.com/bitcoin-core/secp256k1/pull/1600.

So.. how should this all work ideally? Do we really need to forward the flags at all? Asking because if no, that would explain how this has managed to be silently “broken”.

Some flags, such as -fprofile-update=prefer-atomic, must be applied when building every object file. Additionally, flags like -fsanitize=memory must be applied during both compilation and linking.

By default, the main build system is configured without engaging any of these flags.

As previously mentioned, MSan-related flags are used without issue. This is because they are passed as CFLAGS to the depends build subsystem and subsequently to the main build system via a toolchain file.

Therefore, the issue has managed to remain silently broken because there are no use cases in our tests.

Building for code coverage on some systems requires the -fprofile-update=prefer-atomic flag for both C++ and C code. This clearly exposes the bug.

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

Code Coverage & Benchmarks

Reviews

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