refactor: Remove negative lock annotations from globals #21598
Conversation
|
Concept ACK (in the light of #20272 discussion). |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Tested faadb1b050dd37ea8d294def554f1da22029d3e6 on Ubuntu 21.10:
$ clang --version
Ubuntu clang version 12.0.0-++rc3-4ubuntu1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
With this PR massive TSA warnings are gone.
Could the global namespace be explicitly used for global mutexes in the touched lines?
There was a problem hiding this comment.
May I suggest to:
- add some missed annotations:
diff --git a/src/index/base.h b/src/index/base.h
index 8559e3cb6..ac3c429a5 100644
--- a/src/index/base.h
+++ b/src/index/base.h
@@ -109,7 +109,7 @@ public:
/// sync once and only needs to process blocks in the ValidationInterface
/// queue. If the index is catching up from far behind, this method does
/// not block and immediately returns false.
- bool BlockUntilSyncedToCurrentChain() const;
+ bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main);
void Interrupt();
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 7932bd291..3a650923c 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -815,7 +815,7 @@ static RPCHelpMan signrawtransactionwithkey()
};
}
-static RPCHelpMan sendrawtransaction()
+static RPCHelpMan sendrawtransaction() LOCKS_EXCLUDED(::cs_main)
{
return RPCHelpMan{"sendrawtransaction",
"\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
?
|
Approach ACK, +1 on doc change as well |
Correct, but the revert is only temporary (until cs_main is removed or a private member) |
|
Force pushed to address feedback |
|
utACK fa5eabe |
|
ACK fa5eabe |
…globals fa5eabe refactor: Remove negative lock annotations from globals (MarcoFalke) Pull request description: 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. ACKs for top commit: sipa: utACK fa5eabe ajtowns: ACK fa5eabe hebasto: ACK fa5eabe vasild: ACK fa5eabe Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc
Summary: > 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. This is a backport of [[bitcoin/bitcoin#21598 | core#21598]] Test Plan: With TSAN: ``` $ git checkout <previous commit hash> $ ninja &> before.log $ git checkout pr21598 $ ninja &> after.log $ grep "negative capability" before.log | wc -l 33 $ grep "negative capability" after.log | wc -l 0 ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10187
|
This silenced the compiler warnings at the time, so it was ok. I just wonder about the following reasoning, in the light of #24931:
Why is that? For example: Mutex g_mutex;
void f1()
{
LOCK(g_mutex);
}
void f2()
{
LOCK(g_mutex);
f1();
}This is undefined behavior that would be detected if To me it looks like that negative lock annotations "make sense" regardless of the scope of the mutex, no? |
|
This pull was done in light of #20272 (comment) . If #24931 fixes the mentioned issues, then that is fine. |
|
@vasild The problem is when you call Mutex g_mutex;
void f1() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
{
LOCK(g_mutex);
}
class X {
private:
Mutex m_mut;
public:
void f1() EXCLUSIVE_LOCKS_REQUIRED(!m_mut)
{
LOCK(m_mut);
}
};
X g_x;
void caller() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
{
f1();
g_x.f1();
} |
|
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. |
6 participants
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.