test, ci: Fix threadsanitizer errors in mptest

[ryanofsky]

This PR fixes threadsanitizer errors that were found in bitcoin CI and reported by maflcko in bitcoin/bitcoin#33518 (comment). It also extends the libmultiprocess sanitize CI job to report the same errors.

Before this PR, libmultiprocess was built with threadsanitzer in CI, but cap'n proto and libc++ dependencies were built without it, so races in these libraries could be undetected.

Building libc++ with threadsanitzer triggered threadsanitzer errors about accessing std::cout from multiple threads. Those errors are suppressed in bitcoin CI, but there is not really a reason to suppress there here, so they are just fixed by replacing std::cout with KJ_LOG in the test. This change also cleans up mptest output, which is nice. Full output can be seen by running mptest --verbose.

Building cap'nproto with threadsanitzer triggered threadsanitzer errors in the new "thread busy" test added in #214. These were just caused by accessing some test variables from two different threads and are fixed in the last commit here.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Eunovo, Sjors

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:

• #218 (Better error and log messages by ryanofsky)

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.

[ryanofsky]

Updated 1cfe541 -> 73d22ba (pr/san.1 -> pr/san.2, compare) fixing IWYU errors

[theuni]

The errors just happen because callback_thread and request_thread objects in the test are accessed by two different threads: the main test thread, and the EventLoop thread, and it is only actually safe to reference them from the EventLoop thread.

Would it be possible to add threading annotations to enforce this contract? For ex, adding a private dummy mutex, guarding the vulnerable variables with it, and holding that mutex for the duration of the EventLoop thread? We use that trick (g_msgproc_mutex) in Core to enforce a similar contract.

(Technically clang doesn't require a real mutex, only a capability, so the dummy could just be an int. But as there's no actual contention, using a real mutex seems fine imo).

[theuni]

Thanks for fixing this. This was a placeholder that I forgot to revisit.

[ryanofsky]

Would it be possible to add threading annotations to enforce this contract? For ex, adding a private dummy mutex, guarding the vulnerable variables with it, and holding that mutex for the duration of the EventLoop thread? We use that trick (g_msgproc_mutex) in Core to enforce a similar contract.

I think that trick might not apply exactly because the state here is internal to cap'nproto, and cap'nproto is single threaded and doesn't use any thread synchronization or mutexes itself. The requirement that needs to be enforced is that any capnproto function which might trigger IO (including some capnproto object destructors in this case) need to be called from the EventLoop thread with the EventLoop::sync method.

I imagine there could be compile time checks to enforce this (maybe using a pointer-like object overloading * and -> to wrap capnproto objects) but I don't have a clear idea of what it would look like. Does seem worth thinking about.

[ryanofsky]

re: #222 (comment)

Would it be possible to add threading annotations to enforce this contract?

Was thinking about this a little more, and I think an approach that could work could be to have a LoopObj<T> struct with T and EventLoop& members and * and -> operators returning T& and requiring requiring a TSA capability to dereference. The EventLoop object could be the capability object, or it could have a dummy member that's is marked as a capability. Then there could be a dummy locking object to acquire the capability, just asserting the current thread is the event loop thread when constructed.

Concretely then if the ProxyClientBase::m_client variable were declared with LoopObj<Interface::Client> type instead of Interface::Client type, the race in the #214 test wouldn't have happened because the callback_thread and request_thread variables would be protected.

[ryanofsky]

@Sjors @theuni @Eunovo, or others it would be helpful to have at least one ACK to be able to merge this PR.

This PR is directly blocking bitcoin/bitcoin#33518, bitcoin/bitcoin#33519, and bitcoin/bitcoin#33517, and indirectly blocking backport of bitcoin/bitcoin#33229 according to bitcoin/bitcoin#33473 (comment).

Except for a new stringify function, this PR is just test and CI changes and I think even a partial review (for example excluding CI changes) would be helpful.

[Sjors]

Will try to review this today or tomorrow.

[Eunovo]

Tested ACK 73d22ba

I first reproduced the tsan errors by running ci_native_tsan locally, then I pulled this branch into my local Bitcoin repo with the git subtree pull command, and ran ci_native_tsan locally to see that the errors have been resolved.

I left comments as I was reviewing, mostly explaining the code as I understood it.

[Eunovo]

7eb1da1:

LLVM version 21 correctly matches bitcoin-core's tsan ci llvm version https://github.com/bitcoin/bitcoin/blob/master/ci/test/00_setup_env_native_tsan.sh#L11-L14

[Eunovo]

7eb1da1:

Applies LLVM_USE_SANITIZER if specified. This will apply the "Thread" sanitizer specified in NIX_ARGS above.

[Eunovo]

ca3c05d:

This function is required by KJ for it to stringify Log objects https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#stringification

[Eunovo]

c332774:

Not important, but I think the following is worth mentioning:
Maybe we don't need the KJ_EXPECT here at all? The signal itself is a prop for testing, and the catch block already catches the error we expect. I don't think it will be detrimental to the test to remove the KJ_EXPECT call.

[ryanofsky]

Yes I KJ_EXPECT checking for the right error code is pretty inessential to the test and would be ok to drop. I don't think I see any downsides of keeping it though, assuming it still passes and correctly describes what's expected to happen during the test.

[Sjors]

Let's keep the error code check unless it causes problems later. It makes the test easier to understand too.

[Eunovo]

SGTM

[Eunovo]

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

[Sjors]

ACK 73d22ba

I'm not very familiar with Nix so only glossed over those changes.

I reproduced locally that b74e1bb fails TSan (didn't check the error in detail) and the next commit fixes it.

[Sjors]

In 7eb1da1 ci: Use tsan-instrumented libcxx in sanitizers job: not sure if the llvm version bump is worth mentioning, but 21 is also the version we use in the Bitcoin Core TSan job.

[Sjors]

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?

[Eunovo]

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

[ryanofsky]

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!

[theuni]

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.

[ryanofsky]

Thanks for the reviews!

[ryanofsky]

Yes I KJ_EXPECT checking for the right error code is pretty inessential to the test and would be ok to drop. I don't think I see any downsides of keeping it though, assuming it still passes and correctly describes what's expected to happen during the test.

[ryanofsky]

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!

[theuni]

Post-merge ACK 73d22ba. Reviewed the test changes, and the c-i changes make sense to me, but I didn't repro before or after.