build: enable libc++ hardening in debug builds

vasild commented at 6:18 am on December 5, 2024: contributor

When ENABLE_HARDENING is ON (which is the default) and compiling with libc++ in debug mode, then enable full libc++ hardening.

Inspired by #31272 (comment)

Note that libstdc++’s _GLIBCXX_ASSERTIONS (aka light debug mode) is enabled by default when compiling without optimizations), that is in our debug builds. But see #31424 (comment).

Further considerations:

Consider enabling libstdc++s _GLIBCXX_ASSERTIONS (aka light debug mode) also in non-debug builds. Should assess performance impact.
Consider enabling libc++s _LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST in non-debug builds. Should assess peformance impact as well.

DrahtBot commented at 6:18 am on December 5, 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/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.

DrahtBot added the label Build system on Dec 5, 2024

in CMakeLists.txt:466 in 0398eb63a3 outdated

458@@ -459,6 +459,12 @@ try_append_cxx_flags("-fstack-reuse=none" TARGET core_interface)
459 if(ENABLE_HARDENING)
460   add_library(hardening_interface INTERFACE)
461   target_link_libraries(core_interface INTERFACE hardening_interface)
462+
463+  target_compile_options(hardening_interface INTERFACE
464+    -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG
465+    -D_GLIBCXX_ASSERTIONS
466+  )

vasild commented at 6:19 am on December 5, 2024:

Should this be set to _LIBCPP_HARDENING_MODE_FAST or _LIBCPP_HARDENING_MODE_DEBUG based on the build type?

vasild commented at 12:10 pm on December 9, 2024:

I think so, “yes”. Done.

in CMakeLists.txt:467 in 0398eb63a3 outdated

458@@ -459,6 +459,12 @@ try_append_cxx_flags("-fstack-reuse=none" TARGET core_interface)
459 if(ENABLE_HARDENING)
460   add_library(hardening_interface INTERFACE)
461   target_link_libraries(core_interface INTERFACE hardening_interface)
462+
463+  target_compile_options(hardening_interface INTERFACE
464+    -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG
465+    -D_GLIBCXX_ASSERTIONS
466+  )
467+

vasild commented at 6:20 am on December 5, 2024:

How is this related to the following?

https://github.com/bitcoin/bitcoin/blob/e8cc790fe2a223cfaa6dedfba88a6dc4f336f1d9/depends/hosts/linux.mk#L16-L20

dergoegge commented at 9:50 am on December 5, 2024:

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

maflcko commented at 10:04 am on December 5, 2024:

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

vasild commented at 12:22 pm on December 9, 2024:

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.

fanquake commented at 12:32 pm on December 9, 2024:

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.

vasild commented at 5:50 am on December 10, 2024:

-Werror would flip that warning into an error. But anyway, in the latest incarnation of this PR _LIBCPP_HARDENING_MODE is set from one place. So there is no way that one part of the build system setting it to one value and another part of the build system setting it to another value, overriding the first one.

vasild commented at 1:36 pm on December 17, 2024:

The libc hardening is now removed from depends/hosts/linux.mk in favor of more universal hardening from CMakeLists.txt (which is enabled when building via depends).

maflcko commented at 2:36 pm on December 17, 2024:

So there is no way that one part of the build system setting it to one value and another part of the build system setting it to another value, overriding the first one.

I don’t think this is true. Any vendor may start to enable the value any time in the future, if one does not yet exist.

vasild commented at 5:24 pm on December 17, 2024:

How would a vendor enable _LIBCPP_HARDENING_MODE? Some way other than cmake -DENABLE_HARDENING=ON? If they supply flags somehow externally, then the same argument can be made for any flag - our build system may override a flag set externally (e.g. -O1 vs -O2 or whatever).

Thinking about this, the proper way to supply flags externally is to use cmake -DAPPEND_CPPFLAGS="-DFOO=1".

I think this is safe:

The proper way to enable _LIBCPP_HARDENING_MODE and other hardening options depending on compiler and environment is via cmake -DENABLE_HARDENING=ON.
- If somebody uses cmake -DENABLE_HARDENING=ON and supplies _LIBCPP_HARDENING_MODE externally, then they are shooting themselves in the foot and will get compilation warning or error depending on -Werror about redefined macro. The warnings will be all over the place, for every invocation of the compiler for every file.
It would be possible to fiddle manually with this by using cmake -DENABLE_HARDENING=OFF -DAPPEND_CPPFLAGS="-D_LIBCPP_HARDENING_MODE=XYZ"

fanquake commented at 5:28 pm on December 17, 2024:

Thinking about this, the proper way to supply flags externally is to use cmake -DAPPEND_CPPFLAGS="-DFOO=1".

No? The append_* flags are a hack we had to add to bypass various other CMake behaviours and they are not intended for general use (hence “special builds”).

maflcko commented at 5:32 pm on December 17, 2024:

I meant a libc++ vendor, not a Bitcoin Core vendor. There is a good chance this is fine, but it would be good to double-check.

vasild commented at 5:45 pm on December 17, 2024:

I meant a libc++ vendor

Oh, sorry for misunderstanding.

So, according to https://libcxx.llvm.org/Hardening.html#notes-for-users the libc++ vendor can set a default value and users that wish to use a different value can do that by passing -D_LIBCPP_HARDENING_MODE=... to the compiler which is exactly what Bitcoin Core will do. Sounds good?

maflcko commented at 5:53 pm on December 17, 2024:

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)

vasild commented at 8:37 am on December 18, 2024:

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.

maflcko commented at 8:44 am on December 18, 2024:

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.

vasild commented at 8:24 am on December 20, 2024:

Extended the PR description, see the 1 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”.

maflcko commented at 8:46 am on December 20, 2024:

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.

vasild commented at 12:53 pm on March 14, 2025:

Dropped changes to depends/hosts/linux.mk.

vasild commented at 6:20 am on December 5, 2024: contributor

cc @hebasto

DrahtBot added the label CI failed on Dec 5, 2024

in CMakeLists.txt:463 in 0398eb63a3 outdated

458@@ -459,6 +459,12 @@ try_append_cxx_flags("-fstack-reuse=none" TARGET core_interface)
459 if(ENABLE_HARDENING)
460   add_library(hardening_interface INTERFACE)
461   target_link_libraries(core_interface INTERFACE hardening_interface)
462+
463+  target_compile_options(hardening_interface INTERFACE

dergoegge commented at 9:53 am on December 5, 2024:

Maybe we should add this to depends instead?

vasild commented at 12:27 pm on December 9, 2024:

By default hardening is enabled in the CMake config and the depends build will benefit from that. Only if depends build uses make NO_HARDEN=1 then it will disable the hardening in CMake.

vasild force-pushed on Dec 6, 2024

in CMakeLists.txt:525 in 5523b84bdf outdated

520+          -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG
521+          -D_LIBCPP_ABI_BOUNDED_ITERATORS
522+          -D_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
523+          -D_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
524+          -D_LIBCPP_ABI_BOUNDED_UNIQUE_PTR
525+          -D_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY

maflcko commented at 12:38 pm on December 7, 2024:

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

vasild commented at 12:28 pm on December 9, 2024:

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

maflcko commented at 7:28 am on December 10, 2024:

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

vasild commented at 8:01 am on December 10, 2024:

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

maflcko commented at 8:50 am on December 10, 2024:

It would be good to have them in a CI task

vasild commented at 8:28 am on December 20, 2024:

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.

maflcko commented at 8:37 am on December 20, 2024:

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.

vasild commented at 12:08 pm on December 20, 2024:

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.

maflcko commented at 12:23 pm on December 20, 2024:

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.

vasild commented at 5:19 pm on January 7, 2025:

All _LIBCPP_ABI_BOUNDED* additions have been removed from this PR.

vasild force-pushed on Dec 9, 2024

vasild force-pushed on Dec 10, 2024

maflcko changes_requested

maflcko commented at 9:00 am on December 12, 2024: member

There will need to be a workaround for gcc12-14, see #31436

Edit: Fixed in https://github.com/bitcoin/bitcoin/pull/31493

in CMakeLists.txt:527 in 0c9e6143e5 outdated

511@@ -512,6 +512,38 @@ if(ENABLE_HARDENING)
512     if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
513       try_append_linker_flag("-Wl,-fixup_chains" TARGET hardening_interface)
514     endif()
515+
516+    if(HAVE_LIBCPP)
517+      # https://libcxx.llvm.org/Hardening.html
518+      if(CMAKE_BUILD_TYPE STREQUAL "Debug")

theuni commented at 4:54 pm on December 13, 2024:

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

vasild commented at 2:20 pm on December 17, 2024:

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

0if debug then
1    add compile-definitions-A to hardening_interface
2else
3    add compile-definitions-B to hardening_interface

because

the surrounding code adds stuff to hardening_interface and I wanted to do the same, not add to core_interface_debug and
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.

vasild commented at 2:47 pm on January 27, 2025:

I ended up appending to hardening_interface and using the CMake generator expression $<IF:condition,true_string,false_string>. Resolved?

in CMakeLists.txt:530 in 3d3f9b3982 outdated

523+          _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
524+          _LIBCPP_ABI_BOUNDED_UNIQUE_PTR
525+          _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY
526+        )
527+      else()
528+        target_compile_definitions(hardening_interface INTERFACE _LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST)

maflcko commented at 1:47 pm on December 17, 2024:

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.

Not saying a vendor build exists that sets it, but it seems plausible.

vasild commented at 2:29 pm on December 17, 2024:

Seems like you copied your comment from #31424 (review) to a new discussion thread here. I think that is confusing and will make it difficult to follow the conversation flow. I have already replied to your comment in the original thread.

I am marking this as resolved. If you think the original thread was wrongly marked as resolved I think it would be better to comment there and I will reopen it (or feel free to reopen it yourself if github allows that).

maflcko commented at 2:38 pm on December 17, 2024:

I don’t have permission to re-open a thread, but I left a reply there

vasild force-pushed on Dec 18, 2024

vasild force-pushed on Dec 19, 2024

DrahtBot removed the label CI failed on Dec 19, 2024

vasild force-pushed on Dec 20, 2024

maflcko commented at 1:08 pm on December 20, 2024: member

concept ack, but it would be good to get the (CI) changes right

vasild force-pushed on Jan 6, 2025

vasild commented at 2:40 pm on January 6, 2025: contributor

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

DrahtBot commented at 6:53 pm on January 6, 2025: contributor

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

Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

DrahtBot added the label CI failed on Jan 6, 2025

vasild force-pushed on Jan 7, 2025

vasild commented at 5:21 pm on January 7, 2025: contributor

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

DrahtBot removed the label CI failed on Jan 7, 2025

in depends/hosts/linux.mk:17 in 1ef9f7ca4a outdated

12@@ -13,12 +13,6 @@ linux_release_CXXFLAGS=$(linux_release_CFLAGS)
13 linux_debug_CFLAGS=-O1 -g
14 linux_debug_CXXFLAGS=$(linux_debug_CFLAGS)
15 
16-# https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode.html
17-linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC

fanquake commented at 4:54 pm on January 10, 2025:

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

vasild commented at 1:04 pm on January 14, 2025:

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…

fanquake commented at 12:49 pm on January 23, 2025:

so this became redundant.

How? Options set in depends are applied to the libraries built in depends (this list might also expand in future), and then passed through. The same doesn’t happen if you only use them when using CMakes Debug mode for the Core build.

vasild commented at 12:55 pm on March 14, 2025:

I didn’t realize that these flags are used also for the libraries. Dropped these changes.

in CMakeLists.txt:539 in 1ef9f7ca4a outdated

537+      #
538+      # libstdc++ has _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC but they are not ABI compatible with
539+      # normal mode which could cause linkage failures like:
540+      #   undefined reference to `mp::Connection::removeSyncCleanup(__gnu_debug::_Safe_iterator...`
541+      # when an external library is not compiled with _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC,
542+      # so we use _GLIBCXX_ASSERTIONS in both Debug and non-Debug builds.

fanquake commented at 4:55 pm on January 10, 2025:

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

0./master/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11
1|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
2|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
3|      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:

0./build_assertions/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11
1|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
2|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
3|      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.

vasild commented at 1:05 pm on January 14, 2025:

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

fanquake commented at 1:41 pm on January 17, 2025:

libstdc++

vasild commented at 12:56 pm on March 14, 2025:

Given the above I left non-debug builds alone.

vasild force-pushed on Jan 27, 2025

vasild commented at 2:45 pm on January 27, 2025: contributor

1ef9f7ca4a...8cf75a4a18: use generator expressions instead of if(CMAKE_BUILD_TYPE STREQUAL "Debug") because they also work for the Ninja Multi-Config generator.

fanquake commented at 1:24 pm on February 5, 2025: member

What is the status of this? Are you investigating the performance regressions?

build: enable libc++ hardening

When `ENABLE_HARDENING` is `ON` (which is the default) and compiling
with libc++ in debug mode, then enable full libc++ hardening.

Inspired by
https://github.com/bitcoin/bitcoin/issues/31272#issuecomment-2518700939

a3a799c77c

vasild force-pushed on Mar 14, 2025

vasild renamed this:
~~build: enable libc++ and libstdc++ hardening~~
build: enable libc++ hardening
on Mar 14, 2025

vasild commented at 12:58 pm on March 14, 2025: contributor

8cf75a4...a3a799c: reduce the scope of this to only add hardening on libc++ when in debug mode, updated the OP.

vasild marked this as ready for review on Mar 14, 2025

maflcko commented at 1:06 pm on March 14, 2025: member

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:

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

vasild commented at 1:11 pm on March 14, 2025: contributor

Excellent! Removed from OP.

maflcko commented at 2:46 pm on March 14, 2025: member

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.

vasild renamed this:
~~build: enable libc++ hardening~~
build: enable libc++ hardening in debug builds
on Mar 14, 2025

vasild commented at 4:07 pm on March 14, 2025: contributor

@maflcko:

 0--- i/src/test/random_tests.cpp
 1+++ w/src/test/random_tests.cpp
 2@@ -17,12 +17,18 @@ BOOST_FIXTURE_TEST_SUITE(random_tests, BasicTestingSetup)
 3 
 4 BOOST_AUTO_TEST_CASE(osrandom_tests)
 5 {
 6     BOOST_CHECK(Random_SanityCheck());
 7 }
 8 
 9+BOOST_AUTO_TEST_CASE(t)
10+{
11+    std::vector<int> v{1, 2, 3};
12+    (void)v[10];
13+}
14+
15 BOOST_AUTO_TEST_CASE(fastrandom_tests_deterministic)
16 {
17     // Check that deterministic FastRandomContexts are deterministic
18     SeedRandomForTest(SeedRand::ZEROS);
19     FastRandomContext ctx1{true};
20     FastRandomContext ctx2{true};

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

Without this PR:

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

With this PR:

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

maflcko commented at 6:34 pm on March 14, 2025: member

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.

vasild commented at 8:14 am on March 18, 2025: contributor

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

0/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):

0*** No errors detected

maflcko commented at 8:30 am on March 18, 2025: member

There is no GCC release with this feature yet (https://github.com/gcc-mirror/gcc/commit/361d230fd7800a7e749aba8ed020f54f5c26d504), 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.

maflcko commented at 9:04 am on March 25, 2025: member

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.

vasild commented at 2:43 pm on April 2, 2025: contributor

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)

maflcko commented at 3:26 pm on April 2, 2025: member

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.

vasild commented at 4:09 pm on April 2, 2025: contributor

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.

maflcko commented at 4:28 pm on April 2, 2025: member

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

vasild commented at 4:54 pm on April 2, 2025: contributor

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"

maflcko commented at 9:35 am on April 3, 2025: member

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.

fanquake commented at 7:06 am on April 4, 2025: member

Note that this will also appear broken in the summary:

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

vasild commented at 12:01 pm on April 7, 2025: contributor

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:

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

PR:

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

clang 21.0.0

maflcko commented at 11:17 am on April 14, 2025: member

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 https://github.com/bitcoin/bitcoin/issues/32265

build: enable libc++ hardening in debug builds #31424

Code Coverage & Benchmarks

Reviews

Conflicts