Skip to content

Commit 55114a6

Browse files
MarcoFalkeknst
MarcoFalke
authored and
knst
committed
Merge bitcoin#19668: Do not hide compile-time thread safety warnings
ea74e10 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov) 2ee7743 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns) 23d71d1 Do not hide compile-time thread safety warnings (Hennadii Stepanov) 3ddc150 Add missed thread safety annotations (Hennadii Stepanov) af9ea55 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov) Pull request description: On the way of transit from `RecursiveMutex` to `Mutex` (see bitcoin#19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings. On master (65e4eca) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied: ```diff --- a/src/txmempool.h +++ b/src/txmempool.h @@ -607,7 +607,7 @@ public: void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); ``` Clang compiles the code without any thread safety warnings. See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR. ACKs for top commit: MarcoFalke: ACK ea74e10 🎙 jnewbery: ACK ea74e10 ajtowns: ACK ea74e10 Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
1 parent 898282d commit 55114a6

File tree

5 files changed

+80
-8
lines changed

5 files changed

+80
-8
lines changed

‎doc/developer-notes.md‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -899,6 +899,72 @@ the upper cycle, etc.
899899
Threads and synchronization
900900
----------------------------
901901
902+
- Prefer `Mutex` type to `RecursiveMutex` one
903+
904+
- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to
905+
get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
906+
run-time asserts in function definitions:
907+
908+
```C++
909+
// txmempool.h
910+
class CTxMemPool
911+
{
912+
public:
913+
...
914+
mutable RecursiveMutex cs;
915+
...
916+
void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs);
917+
...
918+
}
919+
920+
// txmempool.cpp
921+
void CTxMemPool::UpdateTransactionsFromBlock(...)
922+
{
923+
AssertLockHeld(::cs_main);
924+
AssertLockHeld(cs);
925+
...
926+
}
927+
```
928+
929+
```C++
930+
// validation.h
931+
class ChainstateManager
932+
{
933+
public:
934+
...
935+
bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
936+
...
937+
}
938+
939+
// validation.cpp
940+
bool ChainstateManager::ProcessNewBlock(...)
941+
{
942+
AssertLockNotHeld(::cs_main);
943+
...
944+
LOCK(::cs_main);
945+
...
946+
}
947+
```
948+
949+
- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:
950+
951+
```C++
952+
// net_processing.h
953+
void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
954+
955+
// net_processing.cpp
956+
void RelayTransaction(...)
957+
{
958+
AssertLockHeld(::cs_main);
959+
960+
connman.ForEachNode([&txid, &wtxid](CNode* pnode) {
961+
LockAssertion lock(::cs_main);
962+
...
963+
});
964+
}
965+
966+
```
967+
902968
- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
903969
deadlocks are introduced.
904970

‎src/sync.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,12 +285,16 @@ template void AssertLockHeldInternal(const char*, const char*, int, Mutex*);
285285
template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*);
286286
template void AssertLockHeldInternal(const char*, const char*, int, SharedMutex*);
287287

288-
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
288+
template <typename MutexType>
289+
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs)
289290
{
290291
if (!LockHeld(cs)) return;
291292
tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
292293
abort();
293294
}
295+
template void AssertLockNotHeldInternal(const char*, const char*, int, Mutex*);
296+
template void AssertLockNotHeldInternal(const char*, const char*, int, RecursiveMutex*);
297+
template void AssertLockNotHeldInternal(const char*, const char*, int, SharedMutex*);
294298

295299
void DeleteLock(void* cs)
296300
{

‎src/sync.h‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,9 @@ void LeaveCritical();
5757
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
5858
std::string LocksHeld();
5959
template <typename MutexType>
60-
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs);
61-
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
60+
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
61+
template <typename MutexType>
62+
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs);
6263
void DeleteLock(void* cs);
6364
bool LockStackEmpty();
6465

@@ -74,8 +75,9 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, M
7475
inline void LeaveCritical() {}
7576
inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
7677
template <typename MutexType>
77-
inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
78-
inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
78+
inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {}
79+
template <typename MutexType>
80+
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {}
7981
inline void DeleteLock(void* cs) {}
8082
inline bool LockStackEmpty() { return true; }
8183
#endif

‎src/wallet/scriptpubkeyman.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
528528
//! keeps track of whether Unlock has run a thorough check before
529529
bool m_decryption_thoroughly_checked = false;
530530

531-
bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey);
531+
bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
532532

533533
KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
534534

‎src/wallet/wallet.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1402,7 +1402,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
14021402
* Obviously holding cs_main/cs_wallet when going into this call may cause
14031403
* deadlock
14041404
*/
1405-
void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(cs_main, cs_wallet);
1405+
void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet);
14061406

14071407
/** set a single wallet flag */
14081408
void SetWalletFlag(uint64_t flags);
@@ -1515,7 +1515,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
15151515
void DeactivateScriptPubKeyMan(uint256 id, bool internal);
15161516

15171517
//! Create new DescriptorScriptPubKeyMans and add them to the wallet
1518-
void SetupDescriptorScriptPubKeyMans();
1518+
void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
15191519

15201520
//! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet
15211521
DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const;

0 commit comments

Comments
 (0)