The first commit fixes #30760.
The second commit:
-g -O1 set in MSAN_FLAGS. Since configuration-specific flags override general flags, these are set to empty strings. A similar approach is used in the OSS-Fuzz repository.DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/29506147599</sub>
<details><summary>Hints</summary>
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
16 | @@ -17,8 +17,12 @@ export PACKAGES="ninja-build" 17 | # BDB generates false-positives and will be removed in future 18 | export DEP_OPTS="DEBUG=1 NO_BDB=1 NO_QT=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'" 19 | export GOAL="install" 20 | +# Setting CMAKE_{C,CXX}_FLAGS_DEBUG flags to an empty string ensures that the flags set in MSAN_FLAGS remain unaltered. 21 | # _FORTIFY_SOURCE is not compatible with MSAN. 22 | export BITCOIN_CONFIG="\ 23 | + -DCMAKE_BUILD_TYPE="Debug" \
If setting the build type specific flags to empty is all that's needed, why are you also changing the build type to Debug, rather than leaving it as RelwithDebInfo (same as oss-fuzz).
To pass linux_debug_CPPFLAGS from depends.
(I was going to add this explanation to the PR description after more tests completed).
review-only ACK 331e9761b639f380c457582ae3c4b05e56ff02b0
review-only ACK 331e9761b639f380c457582ae3c4b05e56ff02b0
I'm still working on it. MSan fuzzing CI job still fails when running locally.
Additionally, setting the "Debug" build configuration ensures that
`linux_debug_CPPFLAGS` from depends are passed to the main build system.
131 | @@ -132,6 +132,7 @@ target_link_libraries(fuzz 132 | test_fuzz 133 | bitcoin_cli 134 | bitcoin_common 135 | + bitcoin_util
26c460aa8b5decfd08d931b9b3f80be5c13c7528:
Can you explain this a bit better? Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
Can you explain this a bit better?
I'm afraid I can't (
Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
Those are the questions I asked myself :)
I'm not able to answer them at this moment. There are two reasons for that:
test_fuzz and test_util, has circular symbol dependencies, which is inherited from Autotools.-fsanitize=fuzzer,memory.
Ok, so I guess the reason is that this is required to mirror the autotools approach from https://github.com/bitcoin/bitcoin/blob/5abb9b1af49be9024f95fa2f777285c531785d85/src/Makefile.test.include#L44 ?
Autotools tends to overlink, while in CMake, I aimed to link only the necessary components.
Yes, this PR commit mirrors the line from bitcoin/src/Makefile.test.include that you mentioned. However, simply removing that line from 80f00cafdeef0600fa1a5e906686720786c2336c results in a link-time error.
Sets the "Debug" build configuration when depends are built with DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.
Is this not a bug? If depends is built (in any configuration/with any options/flags) then the expectation would be that everything is passed to CMake (regardless of the CMake build type). However it seems like your saying CMake will actually just ignore some flags depending on which build type has been set? If this was the intended behaviour, has this been documented somewhere? Otherwise how are builders meant to know that certains flags from depends might just be ignored, depending on which build type is selected in CMake?
review-only ACK 787dfaf084a3952319778da9cbcda9d7d619e4ee
Only change since last review is linking with libbitcoin_util.
It would be nice to better understand this change, but this can be done later and is not a blocker to for a test-only change.
Also, the depends behavior can be documented better, or be changed, but this is probably best done in a separate issue or pull request, so that this test-only change stays focussed.
Sets the "Debug" build configuration when depends are built with DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.
Is this not a bug?
No, it is not. Rather it is a design feature.
If depends is built (in any configuration/with any options/flags) then the expectation would be that everything is passed to CMake (regardless of the CMake build type).
While both depends and CMake have configuration-specific flags, it seems unreasonable to expect from CMake configured for the "Release" build type pull in *_debug_* flags from depends.
If this was the intended behaviour, has this been documented somewhere?
Yes, it was intended. No, it has not been documented. FWIW, the fact that flags from depends override flags set in the main build system has also never been documented.
Here are more details regarding that design decision (C++ flags are used as an example, but the same applies for other tool flags):
CMake has (a) flags applied for all build configurations (CMAKE_CXX_FLAGS) and (b) configuration-specific flags (CMAKE_CXX_FLAGS_<CONFIG>). The latter follow the former during the build tool invocation.
The depends build subsystem has a similar design, and configuration-specific variables have _release_ and _debug_ infixes. The depends build configuration is determined by whether DEBUG variable is defined as 1 or not.
IMPORTANT note about behaviour from Autotools: Flags set in depends override ones set in the main build system.
The CMake's way of passing the toolchain details set in depends to the main build system is by using a toolchain file. There are dedicated CMake variables to be defined in the toolchain file, namely CMAKE_CXX_FLAGS_INIT and configuration-specific CMAKE_CXX_FLAGS_<CONFIG>_INIT. The former is assigned the host_CXXFLAGS value, and the latter use host_release_CXXFLAGS and host_debug_CXXFLAGS values. This helps to ensure that the behaviour described in item 3 still holds when using CMake.
it seems unreasonable to expect from CMake configured for the "Release" build type pull in _debug_ flags from depends.
CMake shouldn't have a concept of "depends debug" flags (it shouldn't really even know about depends). It should just get flags/options passed to it in a toolchain file (which is the interface between the two), and it should use them. Having any special coupling/behaviour based on if things are a "depends" build or not, removes the ability for anyone else to bring a CMake toolchain file, and expect it to work properly.
We should open an issue to follow up on this, as at a minimum, it needs better documentation around expections / how things are meant to work, or even reworking, as at the moment, the behaviour seems inconsistent/unexpected.
ACK 787dfaf084a3952319778da9cbcda9d7d619e4ee - as a follow up it would be good to:
For reference, before (https://cirrus-ci.com/task/6668539271053312?logs=ci#L1064):
Cross compiling ....................... FALSE
C++ compiler .......................... Clang 18.1.3, /usr/bin/clang++
CMAKE_BUILD_TYPE ...................... RelWithDebInfo
Preprocessor defined macros ...........
C++ compiler flags .................... -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -nostdlib++ -isystem /msan/cxx_build/include/c++/v1 -L/msan/cxx_build/lib -Wl,-rpath,/msan/cxx_build/lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base=. -fmacro-prefix-map=/ci_container_base=. -Werror -fsanitize=memory -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE
Linker flags .......................... -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -nostdlib++ -isystem /msan/cxx_build/include/c++/v1 -L/msan/cxx_build/lib -Wl,-rpath,/msan/cxx_build/lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument -O2 -g -fsanitize=memory -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
After (https://api.cirrus-ci.com/v1/task/6661080322146304/logs/ci.log):
Cross compiling ....................... FALSE
C++ compiler .......................... Clang 18.1.3, /usr/bin/clang++
CMAKE_BUILD_TYPE ...................... Debug
Preprocessor defined macros ........... DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC _LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG
C++ compiler flags .................... -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -nostdlib++ -isystem /msan/cxx_build/include/c++/v1 -L/msan/cxx_build/lib -Wl,-rpath,/msan/cxx_build/lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base=. -fmacro-prefix-map=/ci_container_base=. -Werror -fsanitize=memory -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE
Linker flags .......................... -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -nostdlib++ -isystem /msan/cxx_build/include/c++/v1 -L/msan/cxx_build/lib -Wl,-rpath,/msan/cxx_build/lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument -fsanitize=memory -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
... as a follow up it would be good to:
- Update oss-fuzz to match what is being done here? Now that they've diverged.
The OSS-Fuzz script controls all flags explicitly, including https://github.com/google/oss-fuzz/blob/5403c34ff006baf410b94cad1112a6c385d1bade/projects/bitcoin-core/build.sh#L50:
export CPPFLAGS="-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
The OSS-Fuzz script controls all flags explicitly
Sure, but you changed the CMake build type here, so why not change it in oss-fuzz? Otherwise, it'd be good to explain somewhere why they (should) differ.
The OSS-Fuzz script controls all flags explicitly
Sure, but you changed the CMake build type here, so why not change it in oss-fuzz? Otherwise, it'd be good to explain somewhere why they (should) differ.
Sure. Please consider https://github.com/google/oss-fuzz/pull/12443.
16 | @@ -17,8 +17,12 @@ export PACKAGES="ninja-build" 17 | # BDB generates false-positives and will be removed in future 18 | export DEP_OPTS="DEBUG=1 NO_BDB=1 NO_QT=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'" 19 | export GOAL="install" 20 | +# Setting CMAKE_{C,CXX}_FLAGS_DEBUG flags to an empty string ensures that the flags set in MSAN_FLAGS remain unaltered.
I think I understand why this needs to pass BUILD_TYPE=Debug so the debug flags from the depends system will be applied. But I don't understand why FLAGS_DEBUG variables need to be set to empty stings. How does setting those to empty strings cause MSAN_FLAGS to remain unaltered? What code is altering MSAN_FLAGS if FLAGS_DEBUG variables are unset?
Perhaps I used the word 'unaltered' incorrectly. It would be more accurate to say that "flags set in MSAN_FLAGS won't be overridden". The CMAKE_CXX_FLAGS_DEBUG follow CMAKE_CXX_FLAGS in the compiler invocation string and competing flags may be overridden.
Good point that "overridden" is probably more appropriate word, but I think I already understood that this meant overridden.
The thing I don't understand is what causes MSAN_FLAGS to be overridden when FLAGS_DEBUG variables are unset, but not overridden when they are empty? Appreciate the clarification. I'm not asking for any particular reason, just curious to understand this better.
Sure. The logic is follows:
MSAN_FLAGS variable's value is ... -g -O1 ....CXXFLAGS for building depends and become the value of CMAKE_CXX_FLAGS in the toolchain file.-DCMAKE_BUILD_TYPE=Debug, the build system may assign -O0 -ftrapv -g3 to CMAKE_CXX_FLAGS_DEBUG.CMAKE_CXX_FLAGS and CMAKE_CXX_FLAGS_DEBUG are concatenated, resulting in .. -g -O1 ... -O0 ... -g3 .... This means that flags set in MSAN_FLAGS will not have any effect.
Very helpful, thanks. I think I would write comment as "Set CMAKE_{C,CXX}_FLAGS_DEBUG options to empty so CMake's default options for debug builds do not interfere with MSAN flags"
Followup points:
I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own ProcessConfigurations.cmake file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C,CXX}_FLAGS"
I'm probably missing something, but maybe a cleaner workaround instead of setting Debug build type and then erasing all the debug flags is just to set a custom build type -DCMAKE_BUILD_TYPE=msan that doesn't have the flags to begin with?
Probably not applicable to this case, but it does seem unfortunate that only having FLAGS and FLAGS_<build_type> options forces a choice between specifying lower priority flags independent of build type, or higher priority flags tied to the build type. Ideally there would be a third option to specify high priority flags not depending on build type, maybe called FLAGS_ALL that would be applied to all build configurations. This seems like something that would not be too hard to support. Not important here, but maybe something to look into if there are more cases like this.
- I'm probably missing something, but maybe a cleaner workaround instead of setting
Debugbuild type and then erasing all the debug flags is just to set a custom build type -DCMAKE_BUILD_TYPE=msan that doesn't have the flags to begin with?
I agree with you, and that was my initial preference when I worked on the CMake staging branch. However, using undefined build configurations is not documented. As a result, the rough consensus among the CMake staging branch contributors was to adhere to documented practices.
- Probably not applicable to this case, but it does seem unfortunate that only having
FLAGSandFLAGS_<build_type>options forces a choice between specifying lower priority flags independent of build type, or higher priority flags tied to the build type. Ideally there would be a third option to specify high priority flags not depending on build type, maybe calledFLAGS_ALLthat would be applied to all build configurations. This seems like something that would not be too hard to support. Not important here, but maybe something to look into if there are more cases like this.
Such an option is implemented in the APPEND_* variables, if I understood you correctly:https://github.com/bitcoin/bitcoin/blob/0e5cd608da5d8c3d9c758dbfa3fc36df4af4a100/CMakeLists.txt#L203-L206
23 | +export BITCOIN_CONFIG="\ 24 | + -DCMAKE_BUILD_TYPE=Debug \ 25 | + -DCMAKE_C_FLAGS_DEBUG='' \ 26 | + -DCMAKE_CXX_FLAGS_DEBUG='' \ 27 | + -DSANITIZERS=memory \ 28 | + -DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE' \
So given the Build type change, this line, and the equivalent flag in the other CI config, are redundant and should have been removed. However I don't actually think they should be removed, because it's instead CMake that should be fixed, to remove the assumption that builds using the Debug build type, will always have no optimisations, as this isn't actually the case, for example, in oss-fuzz, or doing a depends build with DEBUG=1. See further discussion in https://github.com/google/oss-fuzz/pull/12443.