From cc2588579c3fbc133a7ca762fa213ca18b736551 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Thu, 16 May 2019 11:25:31 +0200 Subject: [PATCH 1/4] Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) --- src/sync.h | 11 +++++++++++ src/threadsafety.h | 11 ----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/sync.h b/src/sync.h index 2667fb52f..a4c22f67c 100644 --- a/src/sync.h +++ b/src/sync.h @@ -304,4 +304,15 @@ public: } }; +// Utility class for indicating to compiler thread analysis that a mutex is +// locked (when it couldn't be determined otherwise). +struct SCOPED_LOCKABLE LockAnnotation +{ + template + explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) + { + } + ~LockAnnotation() UNLOCK_FUNCTION() {} +}; + #endif // BITCOIN_SYNC_H diff --git a/src/threadsafety.h b/src/threadsafety.h index 33acddc65..47e6b2ea3 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -54,15 +54,4 @@ #define ASSERT_EXCLUSIVE_LOCK(...) #endif // __GNUC__ -// Utility class for indicating to compiler thread analysis that a mutex is -// locked (when it couldn't be determined otherwise). -struct SCOPED_LOCKABLE LockAnnotation -{ - template - explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) - { - } - ~LockAnnotation() UNLOCK_FUNCTION() {} -}; - #endif // BITCOIN_THREADSAFETY_H From 3a809446b3881e1a6853da78cccf42643c9a5927 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Thu, 16 May 2019 11:45:32 +0200 Subject: [PATCH 2/4] Move LockAnnotation to make it reflect the truth --- src/wallet/test/wallet_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 62630c011..b230000a9 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -272,8 +272,8 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 SetMockTime(mockTime); CBlockIndex* block = nullptr; if (blockTime > 0) { - LockAnnotation lock(::cs_main); auto locked_chain = wallet.chain().lock(); + LockAnnotation lock(::cs_main); auto inserted = mapBlockIndex.emplace(GetRandHash(), new CBlockIndex); assert(inserted.second); const uint256& hash = inserted.first->first; From de9b5dbca34230b8213ce612036fccabdb6ee53b Mon Sep 17 00:00:00 2001 From: practicalswift Date: Thu, 16 May 2019 11:36:01 +0200 Subject: [PATCH 3/4] Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) --- src/sync.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sync.h b/src/sync.h index a4c22f67c..43473534d 100644 --- a/src/sync.h +++ b/src/sync.h @@ -311,6 +311,9 @@ struct SCOPED_LOCKABLE LockAnnotation template explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) { +#ifdef DEBUG_LOCKORDER + AssertLockHeld(mutex); +#endif } ~LockAnnotation() UNLOCK_FUNCTION() {} }; From 9f85e9cb3d687862128ddf464d2bc2462b8627f0 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Thu, 16 May 2019 15:10:38 +0200 Subject: [PATCH 4/4] scripted-diff: Rename LockAnnotation to LockAssertion -BEGIN VERIFY SCRIPT- git grep -l LockAnnotation | xargs sed -i 's/LockAnnotation/LockAssertion/' -END VERIFY SCRIPT- --- src/interfaces/chain.cpp | 26 +++++++++++++------------- src/qt/test/wallettests.cpp | 2 +- src/sync.h | 6 +++--- src/wallet/test/wallet_tests.cpp | 10 +++++----- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 6097d8093..a864f21f0 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -41,7 +41,7 @@ class LockImpl : public Chain::Lock, public UniqueLock { Optional getHeight() override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); int height = ::ChainActive().Height(); if (height >= 0) { return height; @@ -50,7 +50,7 @@ class LockImpl : public Chain::Lock, public UniqueLock } Optional getBlockHeight(const uint256& hash) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = LookupBlockIndex(hash); if (block && ::ChainActive().Contains(block)) { return block->nHeight; @@ -65,34 +65,34 @@ class LockImpl : public Chain::Lock, public UniqueLock } uint256 getBlockHash(int height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = ::ChainActive()[height]; assert(block != nullptr); return block->GetBlockHash(); } int64_t getBlockTime(int height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = ::ChainActive()[height]; assert(block != nullptr); return block->GetBlockTime(); } int64_t getBlockMedianTimePast(int height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = ::ChainActive()[height]; assert(block != nullptr); return block->GetMedianTimePast(); } bool haveBlockOnDisk(int height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = ::ChainActive()[height]; return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; } Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height); if (block) { if (hash) *hash = block->GetBlockHash(); @@ -102,7 +102,7 @@ class LockImpl : public Chain::Lock, public UniqueLock } Optional findPruned(int start_height, Optional stop_height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); if (::fPruneMode) { CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip(); while (block && block->nHeight >= start_height) { @@ -116,7 +116,7 @@ class LockImpl : public Chain::Lock, public UniqueLock } Optional findFork(const uint256& hash, Optional* height) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); const CBlockIndex* block = LookupBlockIndex(hash); const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr; if (height) { @@ -133,12 +133,12 @@ class LockImpl : public Chain::Lock, public UniqueLock } CBlockLocator getTipLocator() override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); return ::ChainActive().GetLocator(); } Optional findLocatorFork(const CBlockLocator& locator) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) { return fork->nHeight; } @@ -146,12 +146,12 @@ class LockImpl : public Chain::Lock, public UniqueLock } bool checkFinalTx(const CTransaction& tx) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); return CheckFinalTx(tx); } bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state) override { - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */, false /* bypass limits */, absurd_fee); } diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 21209d499..ab40e9962 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -145,7 +145,7 @@ void TestGUI() } { auto locked_chain = wallet->chain().lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); WalletRescanReserver reserver(wallet.get()); reserver.reserve(); diff --git a/src/sync.h b/src/sync.h index 43473534d..bdbdde1a2 100644 --- a/src/sync.h +++ b/src/sync.h @@ -306,16 +306,16 @@ public: // Utility class for indicating to compiler thread analysis that a mutex is // locked (when it couldn't be determined otherwise). -struct SCOPED_LOCKABLE LockAnnotation +struct SCOPED_LOCKABLE LockAssertion { template - explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) + explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) { #ifdef DEBUG_LOCKORDER AssertLockHeld(mutex); #endif } - ~LockAnnotation() UNLOCK_FUNCTION() {} + ~LockAssertion() UNLOCK_FUNCTION() {} }; #endif // BITCOIN_SYNC_H diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b230000a9..922bb0fe6 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -44,7 +44,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) auto chain = interfaces::MakeChain(); auto locked_chain = chain->lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); // Verify ScanForWalletTransactions accommodates a null start block. { @@ -123,7 +123,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) auto chain = interfaces::MakeChain(); auto locked_chain = chain->lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); // Prune the older block file. PruneOneBlockFile(oldTip->GetBlockPos().nFile); @@ -190,7 +190,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) auto chain = interfaces::MakeChain(); auto locked_chain = chain->lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); std::string backup_file = (SetDataDir("importwallet_rescan") / "wallet.backup").string(); @@ -248,7 +248,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) CWalletTx wtx(&wallet, m_coinbase_txns.back()); auto locked_chain = chain->lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); LOCK(wallet.cs_wallet); wtx.hashBlock = ::ChainActive().Tip()->GetBlockHash(); @@ -273,7 +273,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 CBlockIndex* block = nullptr; if (blockTime > 0) { auto locked_chain = wallet.chain().lock(); - LockAnnotation lock(::cs_main); + LockAssertion lock(::cs_main); auto inserted = mapBlockIndex.emplace(GetRandHash(), new CBlockIndex); assert(inserted.second); const uint256& hash = inserted.first->first;