Add missing thread safety annotations #20272

pull vasild wants to merge 8 commits into bitcoin:master from vasild:add_missing_tsa changing 27 files +156 −73
  1. vasild commented at 3:45 pm on October 30, 2020: member

    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)

  2. net: fix build by adding missing thread safety annotations 
    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
    711f73fe44
  3. index: fix build by adding missing thread safety annotation 
    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
    300e0103ba
  4. rpc: add missing negative capability on blockheaderToJSON() 
    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
    91038740ac
  5. rpc: add missing negative capability on blockToJSON() 
    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
    a9c1ab780c
  6. rpc: add missing annotation to sendrawtransaction() 
    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
    7057de918c
  7. init: add missing annotations to init.cpp and validation.cpp 
    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
    f13350e9ce
  8. rpc: annotate SyncWithValidationInterfaceQueue() 
    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
    a6b05418c3
  9. wallet: add missing annotations 
    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
    7def9855a9
  10. in src/wallet/rpcwallet.cpp:2899 in 7def9855a9 
    2894@@ -2878,7 +2895,8 @@ static RPCHelpMan listunspent()
2895             + HelpExampleCli("listunspent", "6 9999999 '[]' true '{ \"minimumAmount\": 0.005 }'")
2896             + HelpExampleRpc("listunspent", "6, 9999999, [] , true, { \"minimumAmount\": 0.005 } ")
2897                 },
2898-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2899+        [&](const RPCHelpMan& self, const JSONRPCRequest& request)
2900+            EXCLUSIVE_LOCKS_REQUIRED(!cs_main) -> UniValue
    MarcoFalke commented at 4:15 pm on October 30, 2020:
    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
    vasild commented at 4:52 pm on October 30, 2020:

    Would NO_THREAD_SAFETY_ANALYSIS be better?

    The purpose of this PR is to only silence the warnings.

  11. MarcoFalke commented at 4:16 pm on October 30, 2020: member
    Not sure if we want this. Approach NACK on this diff.
  12. vasild commented at 4:50 pm on October 30, 2020: member

    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:

      0/home/vd/gh/bitcoin/bitcoin/src/net_processing.cpp:2724:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
  1            ProcessGetData(pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
  2            ^
  3/home/vd/gh/bitcoin/bitcoin/src/net_processing.cpp:3798:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
  4            ProcessGetData(*pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
  5            ^
  62 warnings generated.
  7/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]
  8    AssertLockNotHeld(cs_main);
  9    ^
 10/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 11#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 12                              ^
 13/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]
 14    AssertLockNotHeld(cs_main);
 15    ^
 16/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 17#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 18                              ^
 19/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]
 20    AssertLockNotHeld(cs_main);
 21    ^
 22/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 23#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 24                              ^
 25/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]
 26    AssertLockNotHeld(cs_main);
 27    ^
 28/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 29#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 30                              ^
 31/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]
 32    AssertLockNotHeld(cs_main);
 33    ^
 34/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 35#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 36                              ^
 371 warning generated.
 38/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]
 39    AssertLockNotHeld(cs_db);
 40    ^
 41/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
 42#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 43                              ^
 441 warning generated.
 454 warnings generated.
 46/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcdump.cpp:746:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 47    wallet.BlockUntilSyncedToCurrentChain();
 48           ^
 49/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:478:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 50    pwallet->BlockUntilSyncedToCurrentChain();
 51             ^
 52/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:557:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 53    pwallet->BlockUntilSyncedToCurrentChain();
 54             ^
 55/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:718:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 56    pwallet->BlockUntilSyncedToCurrentChain();
 57             ^
 58/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:757:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 59    pwallet->BlockUntilSyncedToCurrentChain();
 60             ^
 61/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:798:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 62    pwallet->BlockUntilSyncedToCurrentChain();
 63             ^
 64/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:838:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 65    pwallet->BlockUntilSyncedToCurrentChain();
 66             ^
 67/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:908:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 68    pwallet->BlockUntilSyncedToCurrentChain();
 69             ^
 70/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1224:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 71    pwallet->BlockUntilSyncedToCurrentChain();
 72             ^
 73/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1267:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 74    pwallet->BlockUntilSyncedToCurrentChain();
 75             ^
 76/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1448:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 77    pwallet->BlockUntilSyncedToCurrentChain();
 78             ^
 79/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1566:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 80    wallet.BlockUntilSyncedToCurrentChain();
 81           ^
 82/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1708:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 83    pwallet->BlockUntilSyncedToCurrentChain();
 84             ^
 85/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1782:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 86    pwallet->BlockUntilSyncedToCurrentChain();
 87             ^
 88/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:1821:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 89    pwallet->BlockUntilSyncedToCurrentChain();
 90             ^
 91/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2172:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 92    pwallet->BlockUntilSyncedToCurrentChain();
 93             ^
 94/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2382:12: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 95    wallet.BlockUntilSyncedToCurrentChain();
 96           ^
 97/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2458:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 98    pwallet->BlockUntilSyncedToCurrentChain();
 99             ^
100/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:2953:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
101    pwallet->BlockUntilSyncedToCurrentChain();
102             ^
103/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:3051:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
104    pwallet->BlockUntilSyncedToCurrentChain();
105             ^
106/home/vd/gh/bitcoin/bitcoin/src/wallet/rpcwallet.cpp:3486:14: warning: calling function 'BlockUntilSyncedToCurrentChain' requires negative capability '!cs_main' [-Wthread-safety-analysis]
107    pwallet->BlockUntilSyncedToCurrentChain();
108             ^
1091 warning generated.
11020 warnings generated.
111/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]
112    AssertLockNotHeld(cs_main);
113    ^
114/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
115#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
116                              ^
1171 warning generated.
118/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]
119    AssertLockNotHeld(cs_main); // For performance reasons
120    ^
121/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
122#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
123                              ^
124/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]
125    AssertLockNotHeld(cs_main); // For performance reasons
126    ^
127/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
128#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
129                              ^
1302 warnings generated.
131/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]
132    AssertLockNotHeld(cs_main);
133    ^
134/home/vd/gh/bitcoin/bitcoin/src/sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
135#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
136                              ^
1371 warning generated.
  13. hebasto commented at 4:52 pm on October 30, 2020: member

    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?

  14. MarcoFalke commented at 4:58 pm on October 30, 2020: member
    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.
  15. vasild commented at 5:23 pm on October 30, 2020: member

    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.

  16. DrahtBot added the label GUI on Oct 30, 2020
  17. DrahtBot added the label P2P on Oct 30, 2020
  18. DrahtBot added the label RPC/REST/ZMQ on Oct 30, 2020
  19. DrahtBot added the label UTXO Db and Indexes on Oct 30, 2020
  20. DrahtBot added the label Validation on Oct 30, 2020
  21. DrahtBot added the label Wallet on Oct 30, 2020
  22. DrahtBot commented at 0:04 am on October 31, 2020: member

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

    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.

  23. fanquake commented at 3:27 am on October 31, 2020: member

    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.

  24. laanwj commented at 4:21 am on October 31, 2020: member

    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.

  25. fanquake commented at 6:32 am on October 31, 2020: member

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

    0clang-12 --version
1Debian clang version 12.0.0-++20200929085817+962a247aebb-1~exp1
2Target: x86_64-pc-linux-gnu
3Thread model: posix
4InstalledDir: /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):

    0/llvm-project/build/bin/clang++ --version
1clang version 12.0.0 (https://github.com/llvm/llvm-project d11710dae6c18e91ee7e58b2f833f4722cc8f78a)
2Target: x86_64-unknown-linux-gnu
3Thread model: posix
4InstalledDir: /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.

     0net_processing.cpp:2724:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 1            ProcessGetData(pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
 2            ^
 3net_processing.cpp:3798:13: warning: calling function 'ProcessGetData' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 4            ProcessGetData(*pfrom, *peer, m_chainparams, m_connman, m_mempool, interruptMsgProc);
 5            ^
 6validationinterface.cpp:162:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
 7    AssertLockNotHeld(cs_main);
 8    ^
 9./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
10#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
11                              ^
12  CXX      libbitcoin_common_a-bech32.o
13validation.cpp:2861:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
14    AssertLockNotHeld(cs_main);
15    ^
16./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
17#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
18                              ^
19validation.cpp:2873:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
20    AssertLockNotHeld(cs_main);
21    ^
22./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
23#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
24                              ^
25validation.cpp:3714:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
26    AssertLockNotHeld(cs_main);
27    ^
28./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
29#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
30                              ^
31validation.cpp:3843:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
32    AssertLockNotHeld(cs_main);
33    ^
34./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
35#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
36                              ^
37index/base.cpp:273:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
38    AssertLockNotHeld(cs_main);
39    ^
40./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
41#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
42                              ^
43rpc/blockchain.cpp:121:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
44    AssertLockNotHeld(cs_main); // For performance reasons
45    ^
46./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
47#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
48                              ^
49rpc/blockchain.cpp:150:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
50    AssertLockNotHeld(cs_main); // For performance reasons
51    ^
52./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
53#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
54                              ^
55  CXX      rpc/libbitcoin_server_a-server.o
56  CXX      script/libbitcoin_server_a-sigcache.o
57rpc/rawtransaction.cpp:864:5: warning: calling function 'AssertLockNotHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires negative capability '!cs_main' [-Wthread-safety-analysis]
58    AssertLockNotHeld(cs_main);
59    ^
60./sync.h:80:31: note: expanded from macro 'AssertLockNotHeld'
61#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
62                              ^

    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.

  26. vasild commented at 1:48 pm on October 31, 2020: member
    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?
  27. vasild closed this on Oct 31, 2020
  28. aaronpuchert commented at 10:17 pm on November 2, 2020: none

    @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.

  29. vasild commented at 7:56 am on November 3, 2020: member
    Thanks! That is very valuable input!
  30. vasild commented at 10:05 am on March 30, 2021: member

    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:

  31. fanquake commented at 11:18 am on March 30, 2021: member

    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.

  32. MarcoFalke commented at 11:32 am on March 30, 2021: member
    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).
  33. vasild commented at 2:38 pm on March 30, 2021: member
    Right. Will come back to this and discuss approaches once clang 12 is released.
  34. MarcoFalke commented at 10:09 am on April 4, 2021: member
    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.
  35. vasild commented at 2:56 pm on April 5, 2021: member
    This is superseded by #21598 which removes the warnings with a smaller amount of changes.
  36. DrahtBot locked this on Aug 18, 2022


vasild MarcoFalke hebasto DrahtBot fanquake laanwj aaronpuchert

Labels
GUI Wallet UTXO Db and Indexes RPC/REST/ZMQ P2P Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me