refactor: Remove negative lock annotations from globals

MarcoFalke commented at 5:59 pm on April 4, 2021: member

They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional.

DrahtBot added the label P2P on Apr 4, 2021

DrahtBot added the label Refactoring on Apr 4, 2021

DrahtBot added the label Wallet on Apr 4, 2021

MarcoFalke removed the label P2P on Apr 4, 2021

MarcoFalke removed the label Wallet on Apr 4, 2021

hebasto commented at 11:36 pm on April 4, 2021: member

Concept ACK (in the light of #20272 discussion).

fanquake deleted a comment on Apr 4, 2021

hebasto commented at 1:52 am on April 5, 2021: member

Tested faadb1b050dd37ea8d294def554f1da22029d3e6 on Ubuntu 21.10:

0$ clang --version
1Ubuntu clang version 12.0.0-++rc3-4ubuntu1
2Target: x86_64-pc-linux-gnu
3Thread model: posix
4InstalledDir: /usr/bin

With this PR massive TSA warnings are gone.

Could the global namespace be explicitly used for global mutexes in the touched lines?

hebasto changes_requested

hebasto commented at 3:04 am on April 5, 2021: member

May I suggest to:

add some missed annotations:

 0diff --git a/src/index/base.h b/src/index/base.h
 1index 8559e3cb6..ac3c429a5 100644
 2--- a/src/index/base.h
 3+++ b/src/index/base.h
 4@@ -109,7 +109,7 @@ public:
 5     /// sync once and only needs to process blocks in the ValidationInterface
 6     /// queue. If the index is catching up from far behind, this method does
 7     /// not block and immediately returns false.
 8-    bool BlockUntilSyncedToCurrentChain() const;
 9+    bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main);
10 
11     void Interrupt();
12 
13diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
14index 7932bd291..3a650923c 100644
15--- a/src/rpc/rawtransaction.cpp
16+++ b/src/rpc/rawtransaction.cpp
17@@ -815,7 +815,7 @@ static RPCHelpMan signrawtransactionwithkey()
18     };
19 }
20 
21-static RPCHelpMan sendrawtransaction()
22+static RPCHelpMan sendrawtransaction() LOCKS_EXCLUDED(::cs_main)
23 {
24     return RPCHelpMan{"sendrawtransaction",
25                 "\nSubmit a raw transaction (serialized, hex-encoded) to local node and network.\n"

fix EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) example in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

?

hebasto commented at 3:12 am on April 5, 2021: member

This change reverts the @ajtowns’s idea for AssertLockNotHeld but I cannot see the way to save it, unfortunately.

ajtowns commented at 6:33 am on April 5, 2021: member

Approach ACK, +1 on doc change as well

refactor: Remove negative lock annotations from globals fa5eabe721

MarcoFalke force-pushed on Apr 5, 2021

MarcoFalke commented at 6:43 am on April 5, 2021: member

This change reverts

Correct, but the revert is only temporary (until cs_main is removed or a private member)

MarcoFalke commented at 6:43 am on April 5, 2021: member

Force pushed to address feedback

hebasto approved

hebasto commented at 6:49 am on April 5, 2021: member

ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

vasild approved

vasild commented at 2:55 pm on April 5, 2021: member

ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

No warnings with Clang 13.

sipa commented at 10:11 pm on April 5, 2021: member

utACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

fanquake requested review from ajtowns on Apr 6, 2021

ajtowns commented at 5:52 am on April 6, 2021: member

ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

MarcoFalke merged this on Apr 6, 2021

MarcoFalke closed this on Apr 6, 2021

MarcoFalke deleted the branch on Apr 6, 2021

sidhujag referenced this in commit 9dfecc6d14 on Apr 6, 2021

Fabcien referenced this in commit 184dbf064f on Sep 23, 2021

vasild commented at 11:55 am on June 7, 2022: member

This silenced the compiler warnings at the time, so it was ok. I just wonder about the following reasoning, in the light of #24931:

Remove negative lock annotations from globals … They only make sense for mutexes that are private members.

Why is that? For example:

 0Mutex g_mutex;
 1
 2void f1()
 3{
 4    LOCK(g_mutex);
 5}
 6
 7void f2()
 8{
 9    LOCK(g_mutex);
10    f1();
11}

This is undefined behavior that would be detected if f1() is annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex).

To me it looks like that negative lock annotations “make sense” regardless of the scope of the mutex, no?

MarcoFalke commented at 12:11 pm on June 7, 2022: member

This pull was done in light of #20272 (review) . If #24931 fixes the mentioned issues, then that is fine.

ajtowns commented at 2:21 am on June 13, 2022: member

@vasild The problem is when you call f1 – the negative annotation needs to keep getting propogated up the call stack:

 0
 1Mutex g_mutex;
 2void f1() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
 3{
 4    LOCK(g_mutex);
 5}
 6
 7class X {
 8private:
 9    Mutex m_mut;
10public:
11    void f1() EXCLUSIVE_LOCKS_REQUIRED(!m_mut)
12    {
13        LOCK(m_mut);
14    }
15};
16X g_x;
17
18void caller() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
19{
20    f1();
21    g_x.f1();
22}

vasild commented at 1:30 pm on June 13, 2022: member

Yes, it has to be propagated, but I wouldn’t call it a “problem” though because it works exactly as intended. True that it creates inconvenience with code isolation (like this code should not know about that mutex), but after all a global mutex is not isolated, so IMO it is ok and even desirable to propagate its negative annotation.

DrahtBot locked this on Jun 13, 2023

refactor: Remove negative lock annotations from globals #21598