sync: Replace LockAssertion with WeaklyAssertLockHeld

ryanofsky · ryanofsky · commit 070803dc7e74 · 2020-09-08T11:40:06.000-04:00
WeaklyAssertLockHeld does the same thing LockAssertion except it:

- Correctly reports file and line number where assertion fails instead of
  location in sync.h.

- Has a simpler syntax that doesn't require declaring an unused variable name.

- Should be harder to confuse with AssertLockHeld. Name should indicate it's a
  weaker assertion not to be preferred when stronger compile time checks are
  available.

This also adds doxygen comments describing AssertLockHeld,
WeaklyAssertLockHeld, and AssertLockNotHeld macros.
diff --git a/doc/developer-notes.md b/doc/developer-notes.md
@@ -793,7 +793,7 @@ bool ChainstateManager::ProcessNewBlock(...)
 }
 ```
 
-- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:
+- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `WeaklyAssertLockHeld`:
 
 ```C++
 // net_processing.h
@@ -805,7 +805,7 @@ void RelayTransaction(...)
     AssertLockHeld(::cs_main);
 
     connman.ForEachNode([&txid, &wtxid](CNode* pnode) {
-        LockAssertion lock(::cs_main);
+        WeaklyAssertLockHeld(::cs_main);
         ...
     });
 }
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -663,7 +663,7 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
             }
         }
         connman.ForNode(nodeid, [&connman](CNode* pfrom){
-            LockAssertion lock(::cs_main);
+            WeaklyAssertLockHeld(::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
@@ -1371,7 +1371,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
     }
 
     m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
-        LockAssertion lock(::cs_main);
+        WeaklyAssertLockHeld(::cs_main);
 
         // TODO: Avoid the repeated-serialization here
         if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
@@ -1506,7 +1506,7 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
 {
     connman.ForEachNode([&txid, &wtxid](CNode* pnode)
     {
-        LockAssertion lock(::cs_main);
+        WeaklyAssertLockHeld(::cs_main);
 
         CNodeState &state = *State(pnode->GetId());
         if (state.m_wtxid_relay) {
@@ -3999,7 +3999,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
         int64_t oldest_block_announcement = std::numeric_limits<int64_t>::max();
 
         m_connman.ForEachNode([&](CNode* pnode) {
-            LockAssertion lock(::cs_main);
+            WeaklyAssertLockHeld(::cs_main);
 
             // Ignore non-outbound peers, or nodes marked for disconnect already
             if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return;
@@ -4016,7 +4016,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
         });
         if (worst_peer != -1) {
             bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) {
-                LockAssertion lock(::cs_main);
+                WeaklyAssertLockHeld(::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
@@ -53,7 +53,7 @@ void LeaveCritical();
 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
 std::string LocksHeld();
 template <typename MutexType>
-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs);
 template <typename MutexType>
 void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs);
 void DeleteLock(void* cs);
@@ -70,13 +70,34 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, v
 inline void LeaveCritical() {}
 inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
 template <typename MutexType>
-inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {}
+inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) {}
 template <typename MutexType>
 void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {}
 inline void DeleteLock(void* cs) {}
 inline bool LockStackEmpty() { return true; }
 #endif
-#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
+
+/**
+ * Assert at compile time and run time that a mutex is locked. Has no effect
+ * when EXCLUSIVE_LOCKS_REQUIRED annotations are enforced because the runtime
+ * assertions will never trigger (analogous to asserting an unsigned int is
+ * greator or equal to 0).
+ *
+ * Use of this macro is neither encouraged nor discouraged in new code. It was
+ * historically used before compile time checks were available, but it may
+ * still be be useful for virtual methods or std::function callbacks where
+ * EXCLUSIVE_LOCKS_REQUIRED annotations are not enforced.
+ */
+#define AssertLockHeld(mutex) [&]() EXCLUSIVE_LOCKS_REQUIRED(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()
+
+/**
+ * Assert at runtime that a mutex is locked. May be necessary to work around
+ * EXCLUSIVE_LOCKS_REQUIRED errors when the compiler can't determine that a
+ * mutex is locked.
+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()
+
+/** Assert at compile time and run time that a mutex is not locked. */
 #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 
 /**
@@ -352,18 +373,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
`@@ -793,7 +793,7 @@ bool ChainstateManager::ProcessNewBlock(...)`
`793`	`793`	`}`
`794`	`794`	```
`795`	`795`
`796`		-- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:
	`796`	+- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `WeaklyAssertLockHeld`:
`797`	`797`
`798`	`798`	```C++
`799`	`799`	`// net_processing.h`
`@@ -805,7 +805,7 @@ void RelayTransaction(...)`
`805`	`805`	`AssertLockHeld(::cs_main);`
`806`	`806`
`807`	`807`	`connman.ForEachNode([&txid, &wtxid](CNode* pnode) {`
`808`		`- LockAssertion lock(::cs_main);`
	`808`	`+ WeaklyAssertLockHeld(::cs_main);`
`809`	`809`	`...`
`810`	`810`	`});`
`811`	`811`	`}`