thread-safety: fix annotations with REVERSE

theuni commented at 0:49 am on May 10, 2025: member

This is one of several PRs to cleanup/modernize our threading primitives.

While replacing the old critical section locks in the mining code with a REVERSE_LOCK, I noticed that our thread-safety annotations weren’t hooked up to it. This PR gets REVERSE_LOCK working properly.

Firstly it modernizes the attributes as-recommended by the clang docs (ctrl+f for USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES). There’s a subtle difference between the old unlock_function and new release_capability, where our reverse_lock only works with the latter. I believe this is an upstream bug. I’ve reported and attempted a fix here, but either way it makes sense to me to modernize.

The second commit just fixes an unrelated logging bug that I noticed while hacking around in here.

The third adds a missing annotation pointed out by a fixed REVERSE_LOCK. Because clang’s thread-safety annotations aren’t passed through a reference to UniqueLock as one may assume (see here for more details), cs_main has to be listed explicitly as a requirement.

The last commit actually fixes the reverse_lock by making it a SCOPED_LOCK and using the pattern found in a clang test. Though the docs don’t describe how to accomplish it, the functionality was added in this commit. Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing REVERSE_LOCKs have been updated. To ensure that the mutexes actually match, a runtime assertion is added.

thread-safety: modernize thread safety macros

Clang added new "capability"-based thread-safety attributes years ago, but the
old ones remain supported for backwards-compatibility.

However, while adding annotations for our reverse_lock, I noticed that there
is a difference between the unlock_function and release_capability attributes.

unlock_function actually maps to release_generic_capability, which does not
work properly when implementing a scoped unlocker.

To be consistent, the other capability-based attributes are updated here as
well. To avoid having to update our macro usage throughout the codebase, I
reused our existing ones.

Additionally, SHARED_UNLOCK_FUNCTION is added here, as a subsequent PR will
introduce annotations for shared_mutex and shared_lock.

832c57a534

thread-safety: add missing lock annotation

No warning is currently emitted because our reverse_lock does not enforce our
thread-safety annotations. Once it is fixed, the unlock would cause a warning.

aeea5f0ec1

thread-safety: add missing lockname for reverse_lock

Noticed while adding annotations to reverse_lock.

0065b9673d

DrahtBot commented at 0:49 am on May 10, 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/32465.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	hebasto

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:

#32592 (threading: remove ancient CRITICAL_SECTION macros by theuni)

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.

thread-safety: fix annotations with REVERSE_LOCK

Without proper annotations, clang thinks that mutexes are still held for the
duration of a reverse_lock. This could lead to subtle bugs as
EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't.

As mentioned in the docs [0], clang's thread-safety analyzer is unable to deal
with aliases of mutexes, so it is not possible to use the lock's copy of the
mutex for that purpose. Instead, the original mutex needs to be passed back to
the reverse_lock for the sake of thread-safety analysis, but it is not actually
used otherwise.

[0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

96a341d406

theuni force-pushed on May 10, 2025

hebasto commented at 7:43 am on May 10, 2025: member

Concept ACK.

laanwj added the label Utils/log/libs on May 10, 2025

laanwj added the label Tests on May 10, 2025

in src/threadsafety.h:19 in 832c57a534 outdated

14@@ -15,23 +15,24 @@
15 // See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
16 // for documentation.  The clang compiler can do advanced static analysis
17 // of locking when given the -Wthread-safety option.
18-#define LOCKABLE __attribute__((lockable))
19+#define LOCKABLE __attribute__((capability("")))
20 #define SCOPED_LOCKABLE __attribute__((scoped_lockable))

TheCharlatan commented at 11:10 am on May 10, 2025:

This threw me off for a bit, but while some attributes have been renamed, some have been kept the same, and their macros renamed instead. See: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h

TheCharlatan approved

TheCharlatan commented at 12:07 pm on May 10, 2025: contributor

ACK 96a341d4064132292621af404de908f01fbe3e2f

DrahtBot requested review from hebasto on May 10, 2025

fanquake referenced this in commit 0a8ab55951 on May 22, 2025

in src/threadsafety.h:28 in 832c57a534 outdated

29-#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
30+#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((acquire_capability(__VA_ARGS__)))
31+#define SHARED_LOCK_FUNCTION(...) __attribute__((acquire_shared_capability(__VA_ARGS__)))
32+#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_capability(__VA_ARGS__)))
33+#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_shared_capability(__VA_ARGS__)))
34+#define UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__)))

davidgumberg commented at 11:13 pm on May 22, 2025:

In commit https://github.com/bitcoin/bitcoin/commit/832c57a53410870471a52647ce107454de3bc69c (thread-safety: modernize thread safety macros)

Feel free to disregard, I am just trying to understand the motivation of this commit:

unlock_function actually maps to release_generic_capability, which does not work properly when implementing a scoped unlocker.

Looking at the example code in clang’s documentation:

0  // [ s/mutex/capability ] 
1  // Release/unlock an exclusive mutex.
2  void Unlock() RELEASE();
3
4  // Release/unlock a shared mutex.
5  void ReaderUnlock() RELEASE_SHARED();
6
7  // Generic unlock, can unlock exclusive and shared mutexes.
8  void GenericUnlock() RELEASE_GENERIC();

So, my assumption here is that when the “unlock capability” that ReverseLock takes is released generically, the thread safety analyzer can’t be sure whether this was an exclusive capability or a shared one, consequently it is ambiguous whether the resource is locked again, because ReverseLock may have been one of many that took “unlock capability” on the lock, is that what the problem is? If release_generic can’t disambiguate these situations, what is it even useful for annotating?

I can see empirically why undoing just this part of the diff

0
1-#define UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__)))
2+#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))

this passage:

0    if (block.m_data) {
1        REVERSE_LOCK(lock, cs_main);
2        if (!blockman.ReadBlock(*block.m_data, *index)) block.m_data->SetNull();
3    }
4    block.found = true;

results in the following error:

0src/node/interfaces.cpp:449:19: warning: mutex 'cs_main' is not held on every path through here [-Wthread-safety-analysis]
1  449 |     block.found = true;

Which indicates that the thread safety analyzer does not understand the “unlock capability” was released and the lock was taken again when the reverse lock went out of scope, I’m just not sure I understand why.

thread-safety: fix annotations with REVERSE_LOCK #32465

Code Coverage & Benchmarks

Reviews

Conflicts