Merge #19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion

MarcoFalke · MarcoFalke · commit 8235dca6210d · 2020-09-23T16:37:07.000+02:00
0bd1184 Remove unused LockAssertion struct (Hennadii Stepanov) ab2a442 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov) 73f71e1 refactor: Use explicit function type instead of template (Hennadii Stepanov) Pull request description: This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`. This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs ACKs for top commit: MarcoFalke: ACK 0bd1184 ajtowns: ACK 0bd1184 vasild: ACK 0bd1184 Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41
diff --git a/doc/developer-notes.md b/doc/developer-notes.md
@@ -793,25 +793,6 @@ bool ChainstateManager::ProcessNewBlock(...)
 }
 ```
 
-- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:
-
-```C++
-// net_processing.h
-void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
-
-// net_processing.cpp
-void RelayTransaction(...)
-{
-    AssertLockHeld(::cs_main);
-
-    connman.ForEachNode([&txid, &wtxid](CNode* pnode) {
-        LockAssertion lock(::cs_main);
-        ...
-    });
-}
-
-```
-
 - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
   deadlocks are introduced. As of 0.12, this is defined by default when
   configuring with `--enable-debug`.
diff --git a/src/net.h b/src/net.h
@@ -258,8 +258,8 @@ class CConnman
 
     void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func)
+    using NodeFn = std::function<void(CNode*)>;
+    void ForEachNode(const NodeFn& func)
     {
         LOCK(cs_vNodes);
         for (auto&& node : vNodes) {
@@ -268,8 +268,7 @@ class CConnman
         }
     };
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func) const
+    void ForEachNode(const NodeFn& func) const
     {
         LOCK(cs_vNodes);
         for (auto&& node : vNodes) {
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -662,8 +662,8 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
                 return;
             }
         }
-        connman.ForNode(nodeid, [&connman](CNode* pfrom){
-            LockAssertion lock(::cs_main);
+        connman.ForNode(nodeid, [&connman](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+            AssertLockHeld(::cs_main);
             uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
             if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
                 // As per BIP152, we only get 3 of our peers to announce
@@ -1355,8 +1355,8 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
         fWitnessesPresentInMostRecentCompactBlock = fWitnessEnabled;
     }
 
-    m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
-        LockAssertion lock(::cs_main);
+    m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+        AssertLockHeld(::cs_main);
 
         // TODO: Avoid the repeated-serialization here
         if (pnode->GetCommonVersion() < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
@@ -1489,9 +1489,8 @@ bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED
 
 void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman)
 {
-    connman.ForEachNode([&txid, &wtxid](CNode* pnode)
-    {
-        LockAssertion lock(::cs_main);
+    connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+        AssertLockHeld(::cs_main);
 
         CNodeState* state = State(pnode->GetId());
         if (state == nullptr) return;
@@ -3975,8 +3974,8 @@ void PeerManager::EvictExtraOutboundPeers(int64_t time_in_seconds)
         NodeId worst_peer = -1;
         int64_t oldest_block_announcement = std::numeric_limits<int64_t>::max();
 
-        m_connman.ForEachNode([&](CNode* pnode) {
-            LockAssertion lock(::cs_main);
+        m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+            AssertLockHeld(::cs_main);
 
             // Ignore non-outbound peers, or nodes marked for disconnect already
             if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return;
@@ -3992,8 +3991,8 @@ void PeerManager::EvictExtraOutboundPeers(int64_t time_in_seconds)
             }
         });
         if (worst_peer != -1) {
-            bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) {
-                LockAssertion lock(::cs_main);
+            bool disconnected = m_connman.ForNode(worst_peer, [&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+                AssertLockHeld(::cs_main);
 
                 // Only disconnect a peer that has been connected to us for
                 // some reasonable fraction of our check-frequency, to give
diff --git a/src/sync.h b/src/sync.h
@@ -352,18 +352,4 @@ class CSemaphoreGrant
     }
 };
 
-// Utility class for indicating to compiler thread analysis that a mutex is
-// locked (when it couldn't be determined otherwise).
-struct SCOPED_LOCKABLE LockAssertion
-{
-    template <typename Mutex>
-    explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
-    {
-#ifdef DEBUG_LOCKORDER
-        AssertLockHeld(mutex);
-#endif
-    }
-    ~LockAssertion() UNLOCK_FUNCTION() {}
-};
-
 #endif // BITCOIN_SYNC_H

Original file line number	Diff line number	Diff line change
`@@ -258,8 +258,8 @@ class CConnman`
`258`	`258`
`259`	`259`	`void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);`
`260`	`260`
`261`		`- template<typename Callable>`
`262`		`- void ForEachNode(Callable&& func)`
	`261`	`+ using NodeFn = std::function<void(CNode*)>;`
	`262`	`+ void ForEachNode(const NodeFn& func)`
`263`	`263`	`{`
`264`	`264`	`LOCK(cs_vNodes);`
`265`	`265`	`for (auto&& node : vNodes) {`
`@@ -268,8 +268,7 @@ class CConnman`
`268`	`268`	`}`
`269`	`269`	`};`
`270`	`270`
`271`		`- template<typename Callable>`
`272`		`- void ForEachNode(Callable&& func) const`
	`271`	`+ void ForEachNode(const NodeFn& func) const`
`273`	`272`	`{`
`274`	`273`	`LOCK(cs_vNodes);`
`275`	`274`	`for (auto&& node : vNodes) {`