Add missing thread safety annotations #20272
Conversation
Fix the following:
```
src/net_processing.cpp:2724:13: error: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
ProcessGetData(pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
^
src/net_processing.cpp:3798:13: error: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
ProcessGetData(*pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
^
```
Clang 12
Fix the following:
```
src/index/base.cpp:273:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
```
Clang 12
Fix the following:
```
src/rpc/blockchain.cpp:121:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main); // For performance reasons
^
```
Clang 12
Fix the following:
```
rpc/blockchain.cpp:150:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main); // For performance reasons
^
```
Clang 12
Fix the following:
```
src/rpc/rawtransaction.cpp:864:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
```
Clang 12
Fix the following:
```
src/validation.cpp:2861:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
src/validation.cpp:2873:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
src/validation.cpp:3714:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
src/validation.cpp:3843:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
```
Clang 12
Fix the following:
```
src/validationinterface.cpp:162:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_main' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_main);
^
```
Clang 12
Fix the following:
```
src/wallet/bdb.cpp:428:5: error: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::__1::recursive_mutex>>' requires negative capability '!cs_db' [-Werror,-Wthread-safety-analysis]
AssertLockNotHeld(cs_db);
^
```
Clang 12
| @@ -2878,7 +2895,8 @@ static RPCHelpMan listunspent() | |||
| + HelpExampleCli("listunspent", "6 9999999 '[]' true '{ \"minimumAmount\": 0.005 }'") | |||
| + HelpExampleRpc("listunspent", "6, 9999999, [] , true, { \"minimumAmount\": 0.005 } ") | |||
| }, | |||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | |||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) | |||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_main) -> UniValue | |||
There was a problem hiding this comment.
The wallet shouldn't know and care about validation locks. Also, this only silences a warning, it won't and can't ever be checked upstream in the call graph
There was a problem hiding this comment.
Would NO_THREAD_SAFETY_ANALYSIS be better?
The purpose of this PR is to only silence the warnings.
|
If those warnings are not fixed, then the thread safety annotations are pretty useless. Why? Because right now:
Here is a list of all warnings: |
Is |
|
My assumption when I merged the negative annotations was that |
|
The warnings above end in The current clang version I am using is |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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.
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 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/binwhich 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/binand used that to build master (42b66a6). 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 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 |
|
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 |
Yes, that's intentional. I don't want to go into to much detail, but 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 Now if you don't want to deal with negative capabilities (which are about preventing double locking), then you should be using |
|
Thanks! That is very valuable input! |
|
Just an update: as expected, clang 12.0.0 rc2 emits warnings with I disabled locally |
@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 |
|
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. |
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)