build: enable libc++ and libstdc++ hardening

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

When ENABLE_HARDENING is ON (which is the default), then enable light or full hardening in libc++ or libstdc++, depending on the library used and depending on the build type:

library	Bitcoin Core compiled in debug mode	Bitcoin Core compiled in non-debug mode
libc++	`_LIBCPP_HARDENING_MODE_DEBUG`1	`_LIBCPP_HARDENING_MODE_FAST`1
libstdc++	`_GLIBCXX_ASSERTIONS`	`_GLIBCXX_ASSERTIONS`

1 this will override the hardening mode with which libc++ is compiled, for the header-based parts of libc++ (most of it). The pre-built components of libc++ will still use whatever mode is configured when libc++ was compiled. For more info see the docs.

Remove hardening flags from depends/hosts/linux.mk in favor of the more generic way to enable them via CMake. The same would be achieved after this change by cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_HARDENING=ON.

Inspired by #31272 (comment)

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

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

build: enable libc++ and libstdc++ hardening

When `ENABLE_HARDENING` is `ON` (which is the default), then enable
light or full hardening in libc++ or libstdc++, depending on the
library used and depending on the build type.

Remove hardening flags from `depends/hosts/linux.mk` in favor of the
more generic way to enable them via CMake. The same would be achieved
after this change by
`cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_HARDENING=ON`.

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

1ef9f7ca4a

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

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…

in CMakeLists.txt:542 in 1ef9f7ca4a

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

build: enable libc++ and libstdc++ hardening #31424

Code Coverage & Benchmarks

Reviews

Conflicts