ci: allow for any libc++ intrumentation & use it for TSAN

fanquake commented at 12:03 pm on July 30, 2025: member

Allow for instrumenting libc++ with a sanitizer other than MemoryWithOrigins.

Would also close #33087, as with the extra instrumentation, the issue from #32862 (comment) is avoided (also see #33081), and we can drop DEBUG_LOCKORDER.

DrahtBot added the label Tests on Jul 30, 2025

DrahtBot commented at 12:03 pm on July 30, 2025: 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/33099.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32989 (ci: Migrate CI to hosted Cirrus Runners by willcl-ark)

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.

fanquake marked this as ready for review on Jul 30, 2025

in ci/test/00_setup_env_native_tsan.sh:22 in 943a7e8a4f outdated

20 export CI_LIMIT_STACK_SIZE=1
21 export BITCOIN_CONFIG="-DWITH_ZMQ=ON -DSANITIZERS=thread \
22--DAPPEND_CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES'"
23+-DAPPEND_CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES'"
24+export USE_INSTRUMENTED_LIBCPP="true"
25+export INSTRUMENTATION="Thread"

maflcko commented at 1:15 pm on July 30, 2025:

0export USE_INSTRUMENTED_LIBCPP="Thread"

Can flatten the option into a single one?

Also, adjust the check below to

0  if [ -n "${USE_INSTRUMENTED_LIBCPP}" ]; then

fanquake commented at 1:40 pm on July 30, 2025:

Squashed this down.

maflcko commented at 1:19 pm on July 30, 2025: member

lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?

fanquake commented at 1:20 pm on July 30, 2025: member

lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?

Yea, the QT build has started taking a ridiculous amount of time. Maybe we can just disable it in this job?

maflcko commented at 1:29 pm on July 30, 2025: member

Yeah, I guess it may be best to disable it for now, if the instrumented libc++ turns the qt depends build into a time sink. For reference, the task never in history took that long, looking at https://0xb10c.github.io/bitcoin-core-ci-stats/graph/build/#TSan,%20depends,%20gui.

Obviously, it would be good to retain depends-qt-tsan as a nightly CI task, but I can open a general brainstorming issue about it.

fanquake force-pushed on Jul 30, 2025

ci: allow libc++ instrumentation other than msan 6653cafd0b

fanquake force-pushed on Jul 30, 2025

DrahtBot added the label CI failed on Jul 30, 2025

DrahtBot commented at 1:40 pm on July 30, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/47028451291 LLM reason (✨ experimental): The CI failure is caused by errors detected in the lint check step.

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.

fanquake commented at 1:41 pm on July 30, 2025: member

Squashed into a single option. Should have fixed the linter. Dropped Qt.

in ci/test/00_setup_env_native_tsan.sh:13 in 45915fd214 outdated

 8@@ -9,9 +9,13 @@ export LC_ALL=C.UTF-8
 9 export CONTAINER_NAME=ci_native_tsan
10 export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:24.04"
11 export APT_LLVM_V="20"
12-export PACKAGES="clang-${APT_LLVM_V} llvm-${APT_LLVM_V} libclang-rt-${APT_LLVM_V}-dev libc++abi-${APT_LLVM_V}-dev libc++-${APT_LLVM_V}-dev python3-zmq"
13-export DEP_OPTS="CC=clang CXX='clang++ -stdlib=libc++'"
14+LIBCXX_DIR="/cxx_build/"
15+export LIBCXX_FLAGS=" -fsanitize=thread -nostdinc++ -nostdlib++ -isystem ${LIBCXX_DIR}include/c++/v1 -L${LIBCXX_DIR}lib -Wl,-rpath,${LIBCXX_DIR}lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument"

maflcko commented at 2:07 pm on July 30, 2025:

nit in 45915fd2145e3f593e00a3ee8f0961303dcf32c8: Does it need to be exported (seems an internal-only var)? Also, stray space in the beginning?

fanquake commented at 2:30 pm on July 30, 2025:

Fixed the space and export.

maflcko approved

maflcko commented at 2:12 pm on July 30, 2025: member

Haven’t checked the ci log for the correct flags, but the change looks good.

review ACK dcc6c2cc1d07d4123fdea14ca835a94a352a2c9a 🌉

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK dcc6c2cc1d07d4123fdea14ca835a94a352a2c9a 🌉
3XNrUEJCI8z8wGDdOo6qiSqBcxwhcWa3yeMZoJMs1O6BVnbBAG9P4RRgye+hx0LQIY54z+9B6hS16m+9qFoFyAQ==

ci: instrument libc++ in TSAN job

Qt is disabled, as the build is now taking a very long time.

b09af2ce50

ci: remove DEBUG_LOCKORDER from TSAN job 7aa5b67132

fanquake force-pushed on Jul 30, 2025

maflcko commented at 4:23 pm on July 30, 2025: member

Looks like the flags are correctly picked up: https://cirrus-ci.com/task/6536414633918464?logs=ci#L4866

[10:46:11.059] C++ compiler flags .................... -fsanitize=thread -nostdinc++ -nostdlib++ -isystem /cxx_build/include/c++/v1 -L/cxx_build/lib -Wl,-rpath,/cxx_build/lib -lc++ -lc++abi -lpthread -Wno-unused-command-line-argument -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base/src=. -fmacro-prefix-map=/ci_container_base/src=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Werror -fsanitize=thread -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 -DARENA_DEBUG -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES

re-ACK 7aa5b67132dfb71e915675a3dbcb806284e08197 🦀

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 7aa5b67132dfb71e915675a3dbcb806284e08197 🦀
3OyyQPnNeYeoIGyqxI2eGBLJtsektDZ5KnIzt+KvErGx+bi0/DbkCdDqPmIikup+yrN8NOjrjECe+kKIFjdHvAg==

DrahtBot removed the label CI failed on Jul 30, 2025

dergoegge approved

dergoegge commented at 12:32 pm on July 31, 2025: member

utACK 7aa5b67132dfb71e915675a3dbcb806284e08197

fanquake merged this on Jul 31, 2025

fanquake closed this on Jul 31, 2025

fanquake deleted the branch on Jul 31, 2025

ci: allow for any libc++ intrumentation & use it for TSAN #33099

Code Coverage & Benchmarks

Reviews

Conflicts