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

Reviews

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

Conflicts

No conflicts as of last run.

7daae2cf96...39a1326488: drop some changes and move them into a separate PR: #31612, suggested by @maflcko

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/35202310502}

39a1326488...1ef9f7ca4a: drop the additions of _LIBCPP_ABI_BOUNDED* when compiling Bitcoin Core. See #31612 (comment) for reasoning.

Further considerations:

• Consider enabling libstdc++s _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC in debug builds. The problem is that "Note that this flag changes the sizes and behavior of standard class templates such as std::vector, and therefore you can only link code compiled with debug mode and code compiled without debug mode if no instantiation of a container is passed between the two translation units.". So maybe that would make sense in some controlled environment like in a CI job.

This should already be done via depends. For example:

ci/test/00_setup_env_native_previous_releases.sh:export DEP_OPTS="DEBUG=1 CC=gcc-11 CXX=g++-11"

Excellent! Removed from OP.

Would be nice to have steps to test this with gcc and clang (maybe with an example bug from the past or a synthetic one). Otherwise, it will be harder to test and the real benefits are harder to see and touch. Also the pull title could be adjusted to mention the Debug restriction.

@maflcko:

--- i/src/test/random_tests.cpp
+++ w/src/test/random_tests.cpp
@@ -17,12 +17,18 @@ BOOST_FIXTURE_TEST_SUITE(random_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(osrandom_tests)
 {
     BOOST_CHECK(Random_SanityCheck());
 }
 
+BOOST_AUTO_TEST_CASE(t)
+{
+    std::vector<int> v{1, 2, 3};
+    (void)v[10];
+}
+
 BOOST_AUTO_TEST_CASE(fastrandom_tests_deterministic)
 {
     // Check that deterministic FastRandomContexts are deterministic
     SeedRandomForTest(SeedRand::ZEROS);
     FastRandomContext ctx1{true};
     FastRandomContext ctx2{true};

Compile with clang and run test_bitcoin --run_test="random_tests/t".

Without this PR:

unknown location(0): fatal error: in "random_tests/t": signal: privileged opcode; address of failing instruction: 0xe58e1f59fc5

With this PR:

/usr/include/c++/v1/vector:1436: assertion __n < size() failed: vector[] index out of bounds
unknown location(0): fatal error: in "random_tests/t": signal: SIGABRT (application abort requested)

I tried this with GCC with -DCMAKE_BUILD_TYPE=Debug, but it passed without error. What are the exact steps to reproduce your claim?

Note that libstdc++'s _GLIBCXX_ASSERTIONS (aka light debug mode) is enabled by default when compiling without optimizations), that is in our debug builds.

The documentation that I linked is explicit:

https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html

_GLIBCXX_ASSERTIONS
Defined by default when compiling with no optimization, undefined by default when compiling with optimization. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.

If you suspect that there is a bug in GCC or the documentation is wrong, you can try the above random_tests/t compiled on master (this PR does not change behavior when GCC is used):

-DCMAKE_BUILD_TYPE=Debug (uses -O0):

/usr/local/lib/gcc15/include/c++/bits/stl_vector.h:1262: constexpr std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = int; _Alloc = std::allocator<int>; reference = int&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.

-DCMAKE_BUILD_TYPE=Release (uses -O2):

*** No errors detected

There is no GCC release with this feature yet (gcc-mirror/gcc@361d230), so simply claiming that it is enabled by default is a bit misleading. I tried with the latest GCC release (14) and it didn't work.

Also, for libc++ you are enabling the strongest non-abi breaking one, so it would be better to do the same for gcc, which probably is _GLIBCXX_DEBUG_PEDANTIC? Edit: _GLIBCXX_ASSERTIONS.

Can't enable _GLIBCXX_DEBUG_PEDANTIC out in the wild (ie for everybody via CMake). But it is already enabled in a controlled environment (the CI): #31424 (comment)

libc++ hardening is also enabled in the CI.

I find it extremely hard to review this pull request, because for months, on every push it is changing what hardening setting to apply in what severity to what part of the system.

My recommendation would be to just state your goal, so that reviewers can easily decide what is the best way to achieve your goal.

libc++ hardening is also enabled in the CI.

Yes. That is not ABI breaking, so it can be enabled out in the wild (ie for everybody via CMake) in addition to the CI.

I find it extremely hard to review this pull request, because for months, on every push it is changing what hardening setting to apply in what severity to what part of the system.

I changed it based on reviewers' feedback, including yours. Thanks for that! Also reduced the scope and dropped parts of it because of possible performance issues. Right now it is +8 -0 lines.

My recommendation would be to just state your goal, so that reviewers can easily decide what is the best way to achieve your goal.

It is stated in the PR title and description.

I'd just find it odd to enable hardening for libc++, but not for GCC in debug builds. GCC is probably used more widely by devs. I know it may be enabled by default for the GCC std lib in a future release, but it may take some time until devs are updating to that. libc++ will also do the same: https://www.github.com/llvm/llvm-project/issues/122925

So if upstream is a reason to not do it for gcc, then it could also be skipped for libc++?

However, you are enabling the strongest debug mode for libc++ here, so I also wonder about the performance impact? I guess performance expectations are non-existing in debug builds, but it would still be good to know, as it would be impossible to disable, and would disallow hacks such as https://www.github.com/bitcoin/bitcoin/pull/32113#issuecomment-2747808769

I'd just find it odd to enable hardening for libc++, but not for GCC in debug builds.

This conversation is going in circles: can't enable full GCC debug like for Clang because the GCC one is ABI breaking: #31424 (comment)

GCC is probably used more widely by devs. I know it may be enabled by default for the GCC std lib in a future release, but it may take some time until devs are updating to that. libc++ will also do the same: https://www.github.com/llvm/llvm-project/issues/122925

This is about the "fast/light" debug which is not in the scope of this PR (anymore). GCC is not in the scope of this PR either. If you want to do GCC and/or "fast" debug, feel free to open another PR, I will review. That was here and was dropped.

So if upstream is a reason to not do it for gcc, then it could also be skipped for libc++?

GCC is ABI breaking and Clang is not. So, only do Clang in this PR.

However, you are enabling the strongest debug mode for libc++ here, so I also wonder about the performance impact? I guess performance expectations are non-existing in debug builds, but it would still be good to know, as it would be impossible to disable, and would disallow hacks such as https://www.github.com/bitcoin/bitcoin/pull/32113#issuecomment-2747808769

Can be disabled with:

cmake ... -DAPPEND_CPPFLAGS="-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE"
Edit:
cmake ... -DAPPEND_CPPFLAGS="-U_LIBCPP_HARDENING_MODE -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE"

The fast GCC debug mode should not be abi-breaking (_GLIBCXX_ASSERTIONS), but if the fast one and GCC is out-of-scope here, then it is fine to ignore it.

However, before merge, it would be good to know the performance impact of the slow one. This makes it easier to judge the impact on CI (c.f. #29796 (comment)) and easier to judge when it should be disabled, or if it can just be left turned on all the time.

Note that this will also appear broken in the summary:

cmake -B build -DCMAKE_BUILD_TYPE=Debug
<snip>
Preprocessor defined macros ........... <snip> RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME $<IF:$<CONFIG:Debug>,_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG,>

I guess performance expectations are non-existing in debug builds, but it would still be good to know ...

Agree. Debug-performance of the unit tests seems to be the same as in master, or within noise:

master:

$ for i in {1..5} ; do ctest --test-dir build-master 2>&1 |grep 'Total Test time' ; done
Total Test time (real) = 407.20 sec
Total Test time (real) = 397.09 sec
Total Test time (real) = 422.87 sec
Total Test time (real) = 410.20 sec
Total Test time (real) = 415.94 sec

PR:

$ for i in {1..5} ; do ctest --test-dir build-pr 2>&1 |grep 'Total Test time' ; done
Total Test time (real) = 403.39 sec
Total Test time (real) = 403.48 sec
Total Test time (real) = 404.27 sec
Total Test time (real) = 394.76 sec
Total Test time (real) = 407.62 sec

clang 21.0.0

Further considerations:

* Consider enabling `libstdc++`s `_GLIBCXX_ASSERTIONS` (aka light debug mode) also in non-debug builds. Should assess [performance impact](https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1910658664).

* Consider enabling `libc++`s `_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST` in non-debug builds. Should assess peformance impact as well.

Turned this into a brainstorming issue, see #32265

ENABLE_HARDENING and the hardening_interface have been removed.

ENABLE_HARDENING and the hardening_interface have been removed.

Thanks for the note. There was a silent merge conflict.

a3a799c77c...e83494d75f: rebase

e83494d75f...63a14e293a: rebase due to conflicts

Seems fine. I tested not this pull, but rm -rf ./bld-cmake && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-std=c++23 -O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' and found that some benchmarks are slower by 10x (--filter=EvictionProtection3Networks100Candidates), however this should be fine, as it can be disabled. Funnily, some are even faster, like (--filter=Bech32Encode).

Not sure how many devs would benefit from this. I can trivially set this myself, and I presume the majority isn't using libc++, except for macOS devs? So it may be better to ask macOS devs for review/testing?