Skip to content

test, ci: Fix threadsanitizer errors in mptest #222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanofsky merged 5 commits into from
Oct 7, 2025
Merged

test, ci: Fix threadsanitizer errors in mptest #222

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7eb1da1
ci: Use tsan-instrumented libcxx in sanitizers job
ryanofsky Oct 2, 2025
ca3c05d
test: Use KJ_LOG instead of std::cout for logging
ryanofsky Oct 2, 2025
c332774
test: Fix failing exception check in new thread busy test
ryanofsky Oct 2, 2025
b74e1bb
ci: Use tsan-instrumented cap'n proto in sanitizers job
ryanofsky Oct 2, 2025
73d22ba
test: Fix tsan race in thread busy test
ryanofsky Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/configs/sanitize.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
CI_DESC="CI job running ThreadSanitizer"
CI_DIR=build-sanitize
NIX_ARGS=(--arg enableLibcxx true --argstr libcxxSanitizers "Thread")
NIX_ARGS=(--arg enableLibcxx true --argstr libcxxSanitizers "Thread" --argstr capnprotoSanitizers "thread")
export CXX=clang++
export CXXFLAGS="-ggdb -Werror -Wall -Wextra -Wpedantic -Wthread-safety -Wno-unused-parameter -fsanitize=thread"
CMAKE_ARGS=()
Expand Down
13 changes: 12 additions & 1 deletion shell.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
, enableLibcxx ? false # Whether to use libc++ toolchain and libraries instead of libstdc++
, minimal ? false # Whether to create minimal shell without extra tools (faster when cross compiling)
, capnprotoVersion ? null
, capnprotoSanitizers ? null # Optional sanitizers to build cap'n proto with
, cmakeVersion ? null
, libcxxSanitizers ? null # Optional LLVM_USE_SANITIZER value to use for libc++, see https://llvm.org/docs/CMake.html
}:
Expand Down Expand Up @@ -41,7 +42,17 @@ let
} // (lib.optionalAttrs (lib.versionOlder capnprotoVersion "0.10") {
env = { }; # Drop -std=c++20 flag forced by nixpkgs
}));
capnproto = capnprotoBase.override (lib.optionalAttrs enableLibcxx { clangStdenv = llvm.libcxxStdenv; });
capnproto = (capnprotoBase.overrideAttrs (old: lib.optionalAttrs (capnprotoSanitizers != null) {
Copy link
Contributor

@Eunovo Eunovo Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b74e1bb:

Adds capnprotoSanitizers if specified. This apply the "Thread" sanitizer specified in sanitize.bash.

The flags -fno-omit-frame-pointer and -g help with producing better stack traces and showing file names and line numbers in warning messages. For example see https://clang.llvm.org/docs/ThreadSanitizer.html#usage

env = (old.env or { }) // {
CXXFLAGS =
lib.concatStringsSep " " [
(old.env.CXXFLAGS or "")
"-fsanitize=${capnprotoSanitizers}"
"-fno-omit-frame-pointer"
Copy link
Member

@Sjors Sjors Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In b74e1bb ci: Use tsan-instrumented cap'n proto in sanitizers job: in Bitcoin Core -fno-omit-frame-pointer is only used with MSan, but I guess it's fine to use with TSan too?

Copy link
Contributor

@Eunovo Eunovo Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddressSanitizer also uses it, https://clang.llvm.org/docs/AddressSanitizer.html#usage. It should be fine to leave it in, since it's good for sanitizers

Copy link
Collaborator Author

@ryanofsky ryanofsky Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In b74e1bb ci: Use tsan-instrumented cap'n proto in sanitizers job: in Bitcoin Core -fno-omit-frame-pointer is only used with MSan, but I guess it's fine to use with TSan too?

Yes my understanding is it just helps provide better debug information in general, so maybe also be useful with GDB. This suggestion just came from chatgpt though, so it is good to know different sanitizer docs also recommend it. Thanks for the links!

Copy link
Contributor

@theuni theuni Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is one of the first things I do when debugging. Also necessary for anything that needs traces, like generating flamegraphs. It forces the compiler to use the frame-pointer register as-intended, rather than as a general purpose register. Abuse of that extra register is necessary in certain places (like our sha2 impl, IIRC), and useful in others to avoid stack spilling, but is generally just an optimization that's in conflict with debugging.

"-g"
];
};
})).override (lib.optionalAttrs enableLibcxx { clangStdenv = llvm.libcxxStdenv; });
clang = if enableLibcxx then llvm.libcxxClang else llvm.clang;
clang-tools = llvm.clang-tools.override { inherit enableLibcxx; };
cmakeHashes = {
Expand Down