How is this related to the following?

These are flags set for DEBUG=1 when building depends. Iirc -D_GLIBCXX_DEBUG includes -D_GLIBCXX_ASSERTIONS as well as other slower checks.

How do they interact? According to https://libcxx.llvm.org/Hardening.html#notes-for-users only one is allowed to be passed.

I don't think it is useful to possibly silently downgrade DEBUG into FAST.

Also, it can be considered to enable the ABI checks in debug depends builds, see https://libcxx.llvm.org/Hardening.html#abi-options

How is this related to the following?

I understand that the flags in bitcoin/depends/hosts/linux.mk are (obviously) only used when building depends and only on Linux. In comparison, if added to CMakeLists.txt under ENABLE_HARDENING then the flags will be used for depends and for non-depends builds on all platforms that use libc++ or libstdc++, not only Linux. Thus I dropped the flags from bitcoin/depends/hosts/linux.mk and added (a more extensive) hardening to CMakeLists.txt.

Yes, -D_GLIBCXX_DEBUG includes -D_GLIBCXX_ASSERTIONS.

How do they interact?

I guess it is harmless to specify both _GLIBCXX_DEBUG* and _LIBCPP_HARDENING_MODE because libstdc++ will honor the first and libc++ the second.

I don't think it is useful to possibly silently downgrade DEBUG into FAST.

Indeed that would be bad. If _LIBCPP_HARDENING_MODE is provided more than once on the command line, then a warning will be raised: "macro is redefined". E.g. -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST will produce a warning.

Also, it can be considered to enable the ABI checks

Done.

I don't think it is useful to possibly silently downgrade DEBUG into FAST.

If _LIBCPP_HARDENING_MODE is provided more than once on the command line, then a warning will be raised

Just printing a warning will more than likely still result in silent downgrading.

Yes, this is what I was referring to. But:

• It would be good to double check the behavior instead of only relying on the docs (docs can be wrong sometimes).
• I don't think it is useful to possibly silently downgrade DEBUG into FAST. (It is now the third time I copy-pasted this, sorry, but I am not sure how to make it any more clear)

Using an example would help. Are you worried about the following?

A libc++ vendor, lets say "Ubuntu 123.45 LTS", compiles the libc++ they ship with LIBCXX_HARDENING_MODE=debug (see https://libcxx.llvm.org/Hardening.html#notes-for-vendors). Then a user of "Ubuntu 123.45 LTS" compiles Bitcoin Core with cmake -DCMAKE_BUILD_TYPE=Release. This results in Bitcoin Core being compiled with fast hardening mode (debug silently downgraded to fast?).

The above is how it should be - the OS default is supposed to be overridable and in Bitcoin Core -DCMAKE_BUILD_TYPE=Release is intended to use the fast mode.

There is also this note in the docs:

Since the static and shared library components of libc++ are built by the vendor, setting this macro will have no impact on the hardening mode for the pre-built components. Most libc++ code is header-based, so a user-provided value for _LIBCPP_HARDENING_MODE will be mostly respected.

I think it is no-brainer that an application (e.g. Bitcoin Core) should pass -D_LIBCPP_HARDENING_MODE=... to the compiler and that is the correct and the best thing an app can do and this PR does that.

The above is how it should be - the OS default is supposed to be overridable and in Bitcoin Core -DCMAKE_BUILD_TYPE=Release is intended to use the fast mode.

It would be good to mention this in the pull description, that this is your goal. I'd say the system vendor default should not be silently downgraded, but if your goal is to ignore the vendor default, then it should be mentioned.

Extended the PR description, see the ¹ specifically. I wouldn't call it a "downgrade" - it is less checks, but an "upgrade" from performance point of view. All these settings provide varying tradeoffs between performance and amount of checks. Downgrade for one is upgrade for the other.

This is analogous to the -O flag. Assuming the default is -O1 (if no -O... is provided). If the user explicitly provides -O2 that will use -O2 instead of the default -O1. I wouldn't call this (silent) "downgrade" or "upgrade", but rather "intentional override of the default".

I don't expect the downgrade will ever affect users. So instead of "Ubuntu 123.45 LTS" I can only imagine a test-only build of a distro that has debug hardening enabled in the system toolchain. This way, they can simply compile all system (and third-party) packages and run the tests with maximum hardening to spot any mistakes before they hit real users.

This was possible before your changes, but won't be possible after your changes (unless they pick the build type debug). Maybe it is expected that they need to specifically select the debug cmake build type in this case. Either way is fine, and the pull should be an overall improvement in any case.

Have you tested this? I expect it to break the ABI for at least bdb?

Not manually, but I assume CI will fail if that happens. If it fails, then I will remove it.

I don't think using a passing CI as an excuse to break debug builds for people using vendor libc++ is a good approach.

Do you think it would be better to omit those _LIBCPP_ABI_BOUNDED*?

It would be good to have them in a CI task

I removed the _LIBCPP_ABI_BOUNDED* options from CMake when ENABLE_HARDENING=ON because they caused linking failures for the multiprocess task because libmultiprocess was compiled without them edit: tsan task, i.e. ABI incompatibility issues. So, don't enable them on users when ENABLE_HARDENING=ON.

It would be good to have them in a CI task

Right, I changed a few CI tasks to use them, enabling them manually, outside of what CMake does by default, see the second commit and the PR description.

Why would the other tasks not break the ABI? Just because CI is green doesn't mean the change is correct. Instead of repeating what the changes are doing in the description, it would be more helpful to reviewers to explain why the changes are correct.

No, wait, it was the tsan task, not the multiprocess task! Edited the above comment of mine. Anyway, it does not matter that much - it is an ABI incompatibility, so the conclusion is the same: don't enable _LIBCPP_ABI_BOUNDED* on users when ENABLE_HARDENING=ON.

Why would the other tasks not break the ABI?

I am not sure exactly, but it has probably something to do with the way libc++ is compiled or _LIBCPP_HARDENING_MODE_FAST not playing well with _LIBCPP_ABI_BOUNDED*. Here is the exact failure: https://api.cirrus-ci.com/v1/task/5304194619408384/logs/ci.log, see undefined reference to ... std::__1::basic_string<...>::insert(std::__1::__bounded_iter<.... This error just does not happen on the other tasks.

Just because CI is green doesn't mean the change is correct.

If there are ABI incompatibilities, then it will fail to compile with undefined reference because it cannot find __bounded_iter or std::__debug::whatever in the standard library.

This error just does not happen on the other tasks.

Why would it not happen? I am happy to review and test this myself, but I'd appreciate if you could explain, or at least test the changes yourself. The link error will absolutely happen on the tasks, if you tested it.

The only reason why the CI is passing is that the ABI checks haven't been implemented in clang-16, so if you tested your change, you could see that the addition to the CI task is a no-op (as in: It can't find any more bugs than before). Even worse, once the CI task is bumped to use a clang version that implements the ABI-breaking checks, it will fail the link step.

If I wanted to look at code, where the author simply claims that it is working, without an explanation or tests, I could just ask an LLM. However, I am thinking that the changes in this pull request are important enough to get right. Simply dismissing my comments and resolving the discussion threads doesn't seem helpful nor productive.

All _LIBCPP_ABI_BOUNDED* additions have been removed from this PR.

See here for a more robust approach to the if(debug) test.

Yeah, I considered core_interface_debug, but I couldn't fit it into this pattern:

if debug then
    add compile-definitions-A to hardening_interface
else
    add compile-definitions-B to hardening_interface

because

1. the surrounding code adds stuff to hardening_interface and I wanted to do the same, not add to core_interface_debug and

2. if I add to core_interface_debug then in the non-debug case (the else branch), where should I add compile-definitions-B to? core_interface will be used for both release and debug. I think we don't have a symmetric core_interface_release which will only be used in release, like core_interface_debug is used only in debug.

Not sure. This just seems to be removing an option (that otherwise doesn't conflict with the other changes)?

I removed this because the same options were enabled by CMake's HARDERNING option when in Debug mode, so this became redundant. However, later I changed HARDENING+Debug to only use _GLIBCXX_ASSERTIONS instead of -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC because the latter produced: undefined reference to mp::Connection::removeSyncCleanup(__gnu_debug::_Safe_iterator....

Maybe this should stay. I wonder why using -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC from here does not produce an error...

so we use _GLIBCXX_ASSERTIONS in both Debug and non-Debug builds.

How did you test the performance impacts of this? For example master (non-Debug build, using GCC 14.2.0):

./master/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11
|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|      495,996,518.00 |                2.02 |    0.3% |5,431,375,728.00 |1,800,295,241.00 |  3.017 | 551,650,338.00 |    0.2% |      5.44 | `LinearizeOptimallyExample11`

vs this PR:

./build_assertions/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11
|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|      678,578,986.00 |                1.47 |    0.2% |8,178,898,994.00 |2,465,218,869.00 |  3.318 |1,114,065,676.00 |    0.1% |      7.47 | `LinearizeOptimallyExample11`

Also see: https://corecheck.dev/bitcoin/bitcoin/pulls/31424.

Interesting. Is that using libstdc++ or libc++?

libstdc++