mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2024-11-19 18:09:47 +01:00
Code Issues Projects Releases Wiki Activity

Merge #19668: Do not hide compile-time thread safety warnings

Browse Source 
ea74e10acf doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe7 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)

Pull request description:

  On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.

  On master (65e4ecabd5) 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 ea74e10acf 🎙
  jnewbery:
    ACK ea74e10acf
  ajtowns:
    ACK ea74e10acf

Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
This commit is contained in:
MarcoFalke 2020-09-01 08:18:20 +02:00
commit bab4cce1b0
No known key found for this signature in database
GPG Key ID: CE2B75697E69A548
7 changed files with 86 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
doc/developer-notes.md
View File

@ -746,6 +746,72 @@ the upper cycle, etc.
Threads and synchronization
----------------------------
- Prefer `Mutex` type to `RecursiveMutex` one
- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to
get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
run-time asserts in function definitions:
```C++
// txmempool.h
class CTxMemPool
{
public:
...
mutable RecursiveMutex cs;
...
void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs);
...
}
// txmempool.cpp
void CTxMemPool::UpdateTransactionsFromBlock(...)
{
AssertLockHeld(::cs_main);
AssertLockHeld(cs);
...
}
```
```C++
// validation.h
class ChainstateManager
{
public:
...
bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
...
}
// validation.cpp
bool ChainstateManager::ProcessNewBlock(...)
{
AssertLockNotHeld(::cs_main);
...
LOCK(::cs_main);
...
}
```
- 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`.

12
src/net_processing.cpp
View File

@ -663,13 +663,12 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
}
}
connman.ForNode(nodeid, [&connman](CNode* pfrom){
AssertLockHeld(cs_main);
LockAssertion lock(::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
// blocks using compact encodings.
connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
AssertLockHeld(cs_main);
connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
return true;
});
@ -1371,7 +1370,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
}
m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
AssertLockHeld(cs_main);
LockAssertion lock(::cs_main);
// TODO: Avoid the repeated-serialization here
if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
@ -1513,7 +1512,8 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
{
connman.ForEachNode([&txid, &wtxid](CNode* pnode)
{
AssertLockHeld(cs_main);
LockAssertion lock(::cs_main);
CNodeState &state = *State(pnode->GetId());
if (state.m_wtxid_relay) {
pnode->PushTxInventory(wtxid);
@ -3993,7 +3993,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) {
AssertLockHeld(cs_main);
LockAssertion lock(::cs_main);
// Ignore non-outbound peers, or nodes marked for disconnect already
if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return;
@ -4010,7 +4010,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
});
if (worst_peer != -1) {
bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) {
AssertLockHeld(cs_main);
LockAssertion lock(::cs_main);
// Only disconnect a peer that has been connected to us for
// some reasonable fraction of our check-frequency, to give

5
src/sync.cpp
View File

@ -238,12 +238,15 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine,
template void AssertLockHeldInternal(const char*, const char*, int, Mutex*);
template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*);
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
template <typename MutexType>
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs)
{
if (!LockHeld(cs)) return;
tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
abort();
}
template void AssertLockNotHeldInternal(const char*, const char*, int, Mutex*);
template void AssertLockNotHeldInternal(const char*, const char*, int, RecursiveMutex*);
void DeleteLock(void* cs)
{

10
src/sync.h
View File

@ -53,8 +53,9 @@ 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) ASSERT_EXCLUSIVE_LOCK(cs);
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
template <typename MutexType>
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs);
void DeleteLock(void* cs);
bool LockStackEmpty();
@ -69,8 +70,9 @@ 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) ASSERT_EXCLUSIVE_LOCK(cs) {}
inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(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

2
src/validation.h
View File

@ -245,7 +245,7 @@ bool TestLockPointValidity(const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_mai
*
* See consensus/consensus.h for flag definitions.
*/
bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs);
/**
* Closure representing one script verification

2
src/wallet/scriptpubkeyman.h
View File

@ -535,7 +535,7 @@ private:
//! keeps track of whether Unlock has run a thorough check before
bool m_decryption_thoroughly_checked = false;
bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey);
bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);

4
src/wallet/wallet.h
View File

@ -1178,7 +1178,7 @@ public:
* Obviously holding cs_main/cs_wallet when going into this call may cause
* deadlock
*/
void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(cs_main, cs_wallet);
void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet);
/** set a single wallet flag */
void SetWalletFlag(uint64_t flags);
@ -1284,7 +1284,7 @@ public:
void LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal);
//! Create new DescriptorScriptPubKeyMans and add them to the wallet
void SetupDescriptorScriptPubKeyMans();
void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet
DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const;