threadsafety: Add STDLOCK() macro for StdMutex #34809

ajtowns commented at 7:00 PM on March 11, 2026: contributor

Using STDLOCK(mutex) instead of StdLockGuard guard(mutex) allows clang to propagate missing lock annotations backwards (for global locks or locks in the same class, anyway), and also avoids declaring a dummy name. Use this in logging.h, and also use it in sync.cpp, adding annotations around the internal structure.

DrahtBot commented at 7:01 PM on March 11, 2026: contributor

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	theuni, sedited
Concept ACK	w0xlt, hebasto

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34865 (logging: better use of log::Entry internally by stickies-v)
#34514 (refactor: remove unnecessary std::move for trivially copyable types by l0rinc)
#34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
#33646 (log: check fclose() results and report safely in logging.cpp by cedwies)

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.

ajtowns renamed this:
~~Add STDLOCK() macro for StdMutex~~
threadsafety: Add STDLOCK() macro for StdMutex
on Mar 11, 2026

w0xlt commented at 7:39 PM on March 11, 2026: contributor

Concept ACK

sedited commented at 11:30 AM on March 19, 2026: contributor

LGTM, but I think the TODO line should be dropped in either the last, or a separate commit.

fanquake commented at 11:32 AM on March 19, 2026: member

cc @theuni

in src/threadsafety.h:83 in 6d8bad26fa

  86 | +
  87 | +    static inline StdMutex& CheckNotHeld(StdMutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
  88 |  };
  89 |  
  90 | +// Provide STDLOCK(..) wrapper around StdMutex::Guard that checks the lock is not already held
  91 | +#define STDLOCK(cs) StdMutex::Guard UNIQUE_NAME(criticalblock){StdMutex::CheckNotHeld(cs)}

theuni commented at 4:32 PM on March 19, 2026:

It's worth noting that this is only required because we don't use -Wthread-safety-negative. When using that, this warning is emitted without the need for CheckNotHeld().

Unfortunately, it also warns about all of our global locks. Arguably we want those warnings on, but it would require a ton of additional annotations. g_maplocalhost_mutex is especially annoying.

ajtowns commented at 6:47 AM on March 20, 2026:

I don't think it's feasible without removing all global locks -- every function that calls a function that takes a lock has to get an annotation (iirc including stdlib functions). eg:

StdMutex x;
int y GUARDED_BY(x);

template<typename Fn>
void g(Fn&& f) { f(9); }

int main(void) EXCLUSIVE_LOCKS_REQUIRED(!x) {
    auto f = [&](int i) EXCLUSIVE_LOCKS_REQUIRED(!x) { StdLockGuard xg{x}; ::y = i; };
    g(f);
    return 0;
}

negsafe.cpp:59:18: warning: calling function 'operator()' requires negative capability '!x' [-Wthread-safety-analysis]
   59 | void g(Fn&& f) { f(9); }
      |                  ^
negsafe.cpp:63:5: note: in instantiation of function template specialization 'g<(lambda at negsafe.cpp:62:14) &>' requested here
   63 |     g(f);
      |     ^
1 warning generated.

ajtowns commented at 7:12 AM on March 20, 2026:

I suppose you could "technically" remove global locks by having a class GM { Mutex mutex; } ? Would be a lot of churn when the macros we have already get us pretty much the same behaviour though.

theuni commented at 2:55 PM on March 20, 2026:

Yeah, it wouldn't be trivial. But FWIW, we're currently vulnerable to the pattern you described, we just don't have any checks for it.

Many of our "global" locks are actually contained to a single compilation unit. As you mentioned, those can safely be easily fixed by hiding them in a class and instantiating a static instance of the class instead.

Also, the negative warnings don't apply to mutexes marked as re-entrant, so the nastiest ones (looking at you, cs_main) would still be exempt.

I have a patch for clang that adds an ASSUMES annotation for capabilities that means "assume I was unlocked at this point (trust me, bro), but enforce checks from here down". The intended use-case was helping with migration away from nasty recursive mutexes (ala cs_main), but I'm realizing it'd be helpful in this case too. I should really get that upstreamed.

Several of the globals get in the way and are part of my todo list for my net refactor anyway. I didn't mean to imply that we could turn on -Wthread-safety-negative any time soon, only that it would be a nice-to-have in the future.

ajtowns commented at 12:45 AM on March 21, 2026:

But FWIW, we're currently vulnerable to the pattern you described, we just don't have any checks for it.

There's no bug/vulnerability in the pattern I described, it's just a limitation of clang's thread safety pattern that it can't sensibly track lock requirements when lock-sensitive functions are passed as arguments to generics, so you have to do it yourself. The same is true when you use std::function, which throws away annotations for the functions it wraps. Moving from global mutexes to non-private member mutexes (or mutexes exposed through GetMutex() helpers) just makes the checks weaker, so you get fewer false compile errors, but also get fewer checks.

Having a way to do "hierarchial mutexes" (every mutex gets assigned a level, attempting to acquire a mutex with level n fails if you have already acquired any other mutex with level m >= n) would be nice, particularly if we could derive the levels automatically from ACQUIRED_AFTER / ACQUIRED_BEFORE annotations or similar. I couldn't figure out a way of doing that super conveniently, and we also have some cases where that's not actually true currently (something like LOCK(A), LOCK(B) sometimes, LOCK(B), LOCK(C) at other times, and LOCK(C); LOCK(A) at other times, iirc; might have been cs_main, mempool.cs and m_tx_relay_mutex) which I think is a deficiency in our lock-order checking.

theuni approved

theuni commented at 4:36 PM on March 19, 2026: member

Nice improvements. crACK 913d28407b7d467bd4b74ccf232f926b259df74b.

It'd be nice to get #34639 in first though, so these changes are happening in a new header.

Also, I think it's worth investigating doing the work to turn on Wthread-safety-negative and dropping our wrappers.

hebasto commented at 5:18 PM on March 19, 2026: member

Concept ACK.

hebasto commented at 5:57 PM on March 19, 2026: member

6d8bad26fa91b130efc1013b20867a8fb096abb5

StdLockGuard and clang's thread safety annotations did not ensure that the lock it was taking was not already held.

How can I reproduce the failure described above? I made a naive attempt to trigger a double-lock on master using the following patch::

--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -110,6 +110,7 @@ void BCLog::Logger::DisconnectTestLogger()
 
 void BCLog::Logger::DisableLogging()
 {
+    StdLockGuard scoped_lock_0(m_cs);
     {
         StdLockGuard scoped_lock(m_cs);
         assert(m_buffering);

However, this successfully triggers a -Wthread-safety-analysis warning:

[52/134] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/__/logging.cpp.o
/home/hebasto/dev/bitcoin/src/logging.cpp:115:22: warning: acquiring  'm_cs' that is already held [-Wthread-safety-analysis]
  115 |         StdLockGuard scoped_lock(m_cs);
      |                      ^
/home/hebasto/dev/bitcoin/src/logging.cpp:113:18: note:  acquired here
  113 |     StdLockGuard scoped_lock_0(m_cs);
      |                  ^
1 warning generated.

If Clang is already catching this, what specific scenario was the commit message referring to?

theuni commented at 6:16 PM on March 19, 2026: member

@hebasto Removing the EXCLUSIVE_LOCKS_REQUIRED(!m_cs) from NumConnections() should now cause a warning.

So I think an undetected double-lock bug would've looked like:

void BCLog::Logger::DisableLogging()
{
    StdLockGuard scoped_lock_0(m_cs);
    auto foo = NumConnections();
}

fanquake commented at 3:17 AM on March 23, 2026: member

It'd be nice to get #34639 in first though, so these changes are happening in a new header.

That has now gone in.

DrahtBot added the label Needs rebase on Mar 23, 2026

util/stdmutex.h: Add STDLOCK() and improve annotation checking for StdMutex

StdLockGuard and clang's thread safety annotations did not ensure that
the lock it was taking was not already held. Add a STDLOCK() macro which
uses an annotated StdMutex::CheckNotHeld() function to correct that.

e196cf26e0

logging: Add missing thread safety annotations f808786f48

scripted-diff: logging: Switch from StdLockGuard to STDLOCK

-BEGIN VERIFY SCRIPT-
sed -i 's/StdLockGuard scoped_lock(\(.*\));/STDLOCK(\1);/' src/logging.h src/logging.cpp
-END VERIFY SCRIPT-

cbc231ed8e

sync: Use StdMutex for thread safety annotations 8d2f06853a

ajtowns force-pushed on Mar 23, 2026

DrahtBot removed the label Needs rebase on Mar 23, 2026

theuni approved

theuni commented at 10:08 PM on March 23, 2026: member

ACK 8d2f06853acabdff0b5a1a99531f2b432daf3883

DrahtBot requested review from hebasto on Mar 23, 2026

ajtowns commented at 5:35 AM on March 24, 2026: contributor

LGTM, but I think the TODO line should be dropped in either the last, or a separate commit.

It's left in to minimise interference / silent merge-conflicts with other PRs that might add uses of StdLockGuard, eg #34806.

sedited commented at 8:17 AM on March 24, 2026: contributor

It's left in to minimise interference / silent merge-conflicts with other PRs that might add uses of StdLockGuard, eg #34806.

That doesn't make sense to me. When are we ever going to remove it then? In general, I don't like people delaying because of potential merge conflicts. If multiple conflicting PRs are close to being merged, maintainers can coordinate to prioritize, but outside of that my experience is that it leads to weird dynamics

ajtowns commented at 8:32 AM on March 24, 2026: contributor

It's left in to minimise interference / silent merge-conflicts with other PRs that might add uses of StdLockGuard, eg #34806.

That doesn't make sense to me. When are we ever going to remove it then?

Are you kidding? It's a one removal, so it is completely trivial to do at any time.

The time to do it is when there are no open PRs that would be broken by removing that line. If both #34809 and #34806 are merged as-is, then the PR removing that line would re-do the scripted-diff to change StdLockGuard to STDLOCK(). If this is merged first, and 34806 doesn't have ACKs, I'd probably rebase it, and it could be merged in parallel with a PR removing that line. If 34806 is merged first, then I'd probably rebase this to update the scripted-diff, and, if all the other PRs using StdLockGuard are also rebased (or unmergable due to conflicts) would update this to also remove the code.

In general, I don't like people delaying because of potential merge conflicts.

What are you doing now if not delaying?

sedited closed this on Mar 24, 2026

sedited reopened this on Mar 24, 2026

sedited commented at 8:44 AM on March 24, 2026: contributor

What are you doing now if not delaying?

I'm trying to figure out why you are leaving a TODO and how I should judge these things in the future. I don't think introducing a TODO because another PR of yours has a trivial conflict is a good thing. Besides I'd also like people to avoid TODO comments, because the LLM bots pick up on them and give us more work to deal with.

If this is merged first, and 34806 doesn't have ACKs, I'd probably rebase it, and it could be merged in parallel with a PR removing that line.

Or we just do it here? I don't like one-off follow ups. They command more time from reviewers too.

ajtowns commented at 9:01 AM on March 24, 2026: contributor

What are you doing now if not delaying?

I'm trying to figure out why you are leaving a TODO and how I should judge these things in the future. I don't think introducing a TODO because another PR of yours has a trivial conflict is a good thing. Besides I'd also like people to avoid TODO comments, because the LLM bots pick up on them and give us more work to deal with.

If this is merged first, and 34806 doesn't have ACKs, I'd probably rebase it, and it could be merged in parallel with a PR removing that line.

Or we just do it here? I don't like one-off follow ups. They command more time from reviewers too.

I had these two sets of PRs sequenced in #34038, and was specifically asked to split them up. Now you're complaining in the split up PR that they're not sequenced, even though there is no need for them to be sequenced and doing so adds work, and artificially delays whichever PR is sequenced second, after this PR has already been arbitrary delayed for sequencing. Having this argument wastes time and energy as well, and I would expect substantially more than reviewing a one-line "delete deprecated declaration" PR.

sedited approved

sedited commented at 9:13 AM on March 24, 2026: contributor

ACK 8d2f06853acabdff0b5a1a99531f2b432daf3883

Having this argument wastes time and energy as well.

Agreed, thanks for elaborating.

sedited merged this on Mar 24, 2026

sedited closed this on Mar 24, 2026