Add missing thread safety annotations #20272

Fix a pile compilation warnings due to missing thread safety annotations.

I don't know why I started seeing these now. Maybe because I upgraded my compiler: clang version 12.0.0 (git@github.com:freebsd/freebsd-ports.git 65a9ee72e496521f839dcf5ac36a833c27a88022)

Not sure if we want this. Approach NACK on this diff.

If those warnings are not fixed, then the thread safety annotations are pretty useless. Why? Because right now:

1. The build is broken with --enable-werror, so people with recent clang cannot use that.

2. Without --enable-werror it will compile but will produce 33 thread safety warnings. If a new one is introduced and they become 34 it will almost surely remain unnoticed.

Here is a list of all warnings:

/home/vd/gh/bitcoin/bitcoin/src/net_processing.cpp:2724:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
            ProcessGetData(pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
            ^
/home/vd/gh/bitcoin/bitcoin/src/net_processing.cpp:3798:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
            ProcessGetData(*pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
            ^
2 warnings generated.
/home/vd/gh/bitcoin/bitcoin/src/validationinterface.cpp:162:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
/home/vd/gh/bitcoin/bitcoin/src/validation.cpp:2861:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
/home/vd/gh/bitcoin/bitcoin/src/validation.cpp:2873:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
/home/vd/gh/bitcoin/bitcoin/src/validation.cpp:3714:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
/home/vd/gh/bitcoin/bitcoin/src/validation.cpp:3843:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
1 warning generated.
/home/vd/gh/bitcoin/bitcoin/src/wallet/bdb.cpp:428:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_db' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_db);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
1 warning generated.
4 warnings generated.
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcdump.cpp:746:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    wallet.BlockUntilSyncedToCurrentChain();
           ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:478:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:557:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:718:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:757:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:798:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:838:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:908:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1224:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1267:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1448:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1566:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    wallet.BlockUntilSyncedToCurrentChain();
           ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1708:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1782:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1821:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2172:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2382:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    wallet.BlockUntilSyncedToCurrentChain();
           ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2458:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2953:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:3051:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:3486:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    pwallet->BlockUntilSyncedToCurrentChain();
             ^
1 warning generated.
20 warnings generated.
/home/vd/gh/bitcoin/bitcoin/src/index/base.cpp:273:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
1 warning generated.
/home/vd/gh/bitcoin/bitcoin/src/rpc/blockchain.cpp:121:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main); // For performance reasons
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
/home/vd/gh/bitcoin/bitcoin/src/rpc/blockchain.cpp:150:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main); // For performance reasons
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
2 warnings generated.
/home/vd/gh/bitcoin/bitcoin/src/rpc/rawtransaction.cpp:864:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
1 warning generated.

Clang 12 documentation:

Negative requirements are an experimental feature which is off by default, because it will produce many warnings in existing code. It can be enabled by passing -Wthread-safety-negative.

Is -Wthread-safety-negative on by default in Clang 12 for you?

My assumption when I merged the negative annotations was that -Wthread-safety-negative is turned off. If clang 12 enables that, it should be disabled explicitly.

The warnings above end in ... [-Wthread-safety-analysis] which means that they are due to -Wthread-safety-analysis, not due to -Wthread-safety-negative. -Wno-thread-safety-negative does not silence them.

The current clang version I am using is llvm-devel-12.0.d20201027 (that is the name of the FreeBSD package). The previous one llvm-devel-12.0.d20200925 does not produce the warnings. So they are indeed due to some recent change in Clang 12.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20250 (Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr)
• #20220 (wallet, rpc: explicit fee rate follow-ups for 0.21 by jonatack)
• #20017 (rpc: Add RPCContext by promag)
• #19983 (Drop some TSan suppressions by hebasto)
• #19982 (test: Fix inconsistent lock order in wallet_tests/CreateWalletFromFile by hebasto)
• #19064 (refactor: Cleanup thread ctor calls by hebasto)
• #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
• #15719 (Wallet passive startup 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.

The build is broken with --enable-werror, so people with recent clang cannot use that.

You mean anyone using an in-development version of Clang right? I'd consider anything from Clang 8 onwards to be pretty "recent" (some would probably argue even older than that), especially Clang 11, given it only came out a few weeks ago.

The current clang version I am using is llvm-devel-12.0.d20201027 (that is the name of the FreeBSD package). The previous one llvm-devel-12.0.d20200925 does not produce the warnings. So they are indeed due to some recent change in Clang 12.

So recently something changed in the LLVM dev branch, and now there are some warnings. Maybe it'll change back / be adjusted / something else will happen prior to Clang 12 becoming stable & being released. It's not clear to me that we should be making any code changes here just yet.

I feel like this isn't the first time in recent months that we've either had PRs open to "fix" issues with unreleased compilers, or had people reporting issues that came and went because they were using an unreleased version of a compiler on their day-to-day workstation. If think if someones choosing to use an in-development compiler, they should accept that it may not work perfectly at all times & they're obviously much more likely to run into any issues in relation to changing defaults, new warnings etc.

I do think staying on top of what's on the horizon up in terms of compiler changes is useful. However I'm not sure that shotgunning EXCLUSIVE_LOCKS_REQUIRED annotations all over the codebase just to "quiet warnings" and make it possible for you to turn --enable-werror back on is the right solution here.

Edit: I'll also make the point that testing these changes will require someone to be running at least the same in-development version of Clang, which makes it a bit more cumbersome to review than most other PRs.

I agree with @fanquake. It can be good to test with unreleased compilers, but it will always be a moving target. Things might well change around again. There's the difficulty of checking—we can't expect reviewers to be compiler developers—as well.

It would be different if this testing found an actual issue in our code.

We can only consider this PRs that 'fix warnings' for released compilers, sorry. Feel free to keep this open until then.

FWIW I did try compiling using the Clang 12 shipped with Debian experimental:

clang-12 --version
Debian clang version 12.0.0-++20200929085817+962a247aebb-1~exp1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

which looks slightly newer than the version you were using previously, but must still be too old to produce the warnings.

So I built Clang trunk (d11710dae6c18e91ee7e58b2f833f4722cc8f78a):

/llvm-project/build/bin/clang++ --version
clang version 12.0.0 (https://github.com/llvm/llvm-project d11710dae6c18e91ee7e58b2f833f4722cc8f78a)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /llvm-project/build/bin

and used that to build master (42b66a6b814bca130a9ccf0a3f747cf33d628232). Now I do see some warnings, but definitely not as many as you've listed above.

net_processing.cpp:2724:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
            ProcessGetData(pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
            ^
net_processing.cpp:3798:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
            ProcessGetData(*pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
            ^
validationinterface.cpp:162:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
  CXX      libbitcoin_common_a-bech32.o
validation.cpp:2861:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
validation.cpp:2873:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
validation.cpp:3714:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
validation.cpp:3843:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
index/base.cpp:273:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
rpc/blockchain.cpp:121:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main); // For performance reasons
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
rpc/blockchain.cpp:150:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main); // For performance reasons
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^
  CXX      rpc/libbitcoin_server_a-server.o
  CXX      script/libbitcoin_server_a-sigcache.o
rpc/rawtransaction.cpp:864:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
    AssertLockNotHeld(cs_main);
    ^
./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
                              ^

I had a bit of a look and there has definitely been some Thread safety analysis: work going on in Clang recently. For example it's possible the new warnings are due to this commit: Thread safety analysis: Consider global variables in scope. However that was reverted shortly after Temporairly revert "Thread safety analysis: Consider global variables… and then re-commited a few weeks later in Thread safety analysis: Consider global variables in scope.

This is just one example (out of many thread safety commits from the past month or two), but obviously depending on where the commit your compiler is built from lands, it could have "new but broken", "not new" or "new and now working better" behaviour from -Wthread-safety-analysis.

It all makes sense and I agree with the above, thanks everybody for the attention! I am closing this for now so that it does not create unnecessary "conflict warnings" with other PRs. @aaronpuchert are the above warnings intentional behavior of Clang under -Wthread-safety-analysis and not under -Wthread-safety-negative, that is likely to be released in the final 12?

@aaronpuchert are the above warnings intentional behavior of Clang under -Wthread-safety-analysis and not under -Wthread-safety-negative, that is likely to be released in the final 12?

Yes, that's intentional. I don't want to go into to much detail, but -Wthread-safety-negative only affects whether negative capabilities are required for acquiring a mutex. Here however ProcessGetData has an annotation EXCLUSIVE_LOCKS_REQUIRED(!cs_main, [...]) and these requirements were always propagated, even before my change. Before #19911 the function had LOCKS_EXCLUDED(cs_main) which doesn't behave that way.

What I changed is the following: normally callers have to provide a negative capability only when it's considered visible, and before my change only class members were considered visible to member functions of the same class. With the change we also consider global variables to be visible to everybody, and cs_main is global. I have another change cooking that also considers access specifiers. (https://reviews.llvm.org/D87194)

Now if you don't want to deal with negative capabilities (which are about preventing double locking), then you should be using LOCKS_EXCLUDED(x) (or no annotation at all) instead of EXCLUSIVE_LOCKS_REQUIRED(!x). Consider removing operator! completely, because that's only needed for negative capabilities.

Thanks! That is very valuable input!

Just an update: as expected, clang 12.0.0 rc2 emits warnings with -Wthread-safety-analysis.

I disabled locally -Wthread-safety-analysis in my dev environment if clang 12 or 13 is used :disappointed:

Just an update: as expected, clang 12.0.0 rc2 emits warnings with -Wthread-safety-analysis. @vasild I'm happy for you to reopen this PR (or a new one), to address issues, when Clang 12 is released. What we want to avoid is playing whack-a-mole with unreleased compilers, while code is being added, removed, fixed up etc.

I think we should just remove the annotations that include a !. Adding missing ones (as this pull tries to achieve) is not only impossible, but also a step in the wrong direction (as explained in my NACK above).

Right. Will come back to this and discuss approaches once clang 12 is released.

It is highly unlikely that clang is going to change this late in the release cycle, so I think we should be starting to create and review the needed changes.

This is superseded by #21598 which removes the warnings with a smaller amount of changes.