diff --git a/src/logging.cpp b/src/logging.cpp index 56c44ae1ea4..41c6f5c9321 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -41,7 +41,7 @@ static int FileWriteStr(const std::string &str, FILE *fp) bool BCLog::Logger::StartLogging() { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); assert(m_buffering); assert(m_fileout == nullptr); @@ -80,7 +80,7 @@ bool BCLog::Logger::StartLogging() void BCLog::Logger::DisconnectTestLogger() { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_buffering = true; if (m_fileout != nullptr) fclose(m_fileout); m_fileout = nullptr; @@ -246,7 +246,7 @@ namespace BCLog { void BCLog::Logger::LogPrintStr(const std::string& str) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); std::string str_prefixed = LogEscapeMessage(str); if (m_log_threadnames && m_started_new_line) { diff --git a/src/logging.h b/src/logging.h index ab07010316b..c55f5819163 100644 --- a/src/logging.h +++ b/src/logging.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -62,9 +63,10 @@ namespace BCLog { { private: mutable std::mutex m_cs; // Can not use Mutex from sync.h because in debug mode it would cause a deadlock when a potential deadlock was detected - FILE* m_fileout = nullptr; // GUARDED_BY(m_cs) - std::list m_msgs_before_open; // GUARDED_BY(m_cs) - bool m_buffering{true}; //!< Buffer messages before logging can be started. GUARDED_BY(m_cs) + + FILE* m_fileout GUARDED_BY(m_cs) = nullptr; + std::list m_msgs_before_open GUARDED_BY(m_cs); + bool m_buffering GUARDED_BY(m_cs) = true; //!< Buffer messages before logging can be started. /** * m_started_new_line is a state variable that will suppress printing of @@ -79,7 +81,7 @@ namespace BCLog { std::string LogTimestampStr(const std::string& str); /** Slots that connect to the print signal */ - std::list> m_print_callbacks /* GUARDED_BY(m_cs) */ {}; + std::list> m_print_callbacks GUARDED_BY(m_cs) {}; public: bool m_print_to_console = false; @@ -98,14 +100,14 @@ namespace BCLog { /** Returns whether logs will be written to any output */ bool Enabled() const { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); return m_buffering || m_print_to_console || m_print_to_file || !m_print_callbacks.empty(); } /** Connect a slot to the print signal and return the connection */ std::list>::iterator PushBackCallback(std::function fun) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_print_callbacks.push_back(std::move(fun)); return --m_print_callbacks.end(); } @@ -113,7 +115,7 @@ namespace BCLog { /** Delete a connection */ void DeleteCallback(std::list>::iterator it) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_print_callbacks.erase(it); } diff --git a/src/net.cpp b/src/net.cpp index 9950b9aea45..c9cfb67ec85 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1455,7 +1455,7 @@ void CConnman::ThreadSocketHandler() void CConnman::WakeMessageHandler() { { - std::lock_guard lock(mutexMsgProc); + LOCK(mutexMsgProc); fMsgProcWake = true; } condMsgProc.notify_one(); @@ -2058,7 +2058,7 @@ void CConnman::ThreadMessageHandler() WAIT_LOCK(mutexMsgProc, lock); if (!fMoreWork) { - condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this] { return fMsgProcWake; }); + condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this]() EXCLUSIVE_LOCKS_REQUIRED(mutexMsgProc) { return fMsgProcWake; }); } fMsgProcWake = false; } @@ -2366,7 +2366,7 @@ static CNetCleanup instance_of_cnetcleanup; void CConnman::Interrupt() { { - std::lock_guard lock(mutexMsgProc); + LOCK(mutexMsgProc); flagInterruptMsgProc = true; } condMsgProc.notify_all(); diff --git a/src/net.h b/src/net.h index 66aac6f0845..517445e8f45 100644 --- a/src/net.h +++ b/src/net.h @@ -454,7 +454,7 @@ private: const uint64_t nSeed0, nSeed1; /** flag for waking the message processor. */ - bool fMsgProcWake; + bool fMsgProcWake GUARDED_BY(mutexMsgProc); std::condition_variable condMsgProc; Mutex mutexMsgProc; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7d43de6646c..4eb47d7b15b 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -52,7 +52,7 @@ struct CUpdatedBlock static Mutex cs_blockchange; static std::condition_variable cond_blockchange; -static CUpdatedBlock latestblock; +static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); NodeContext& EnsureNodeContext(const util::Ref& context) { @@ -223,7 +223,7 @@ static UniValue getbestblockhash(const JSONRPCRequest& request) void RPCNotifyBlockChange(const CBlockIndex* pindex) { if(pindex) { - std::lock_guard lock(cs_blockchange); + LOCK(cs_blockchange); latestblock.hash = pindex->GetBlockHash(); latestblock.height = pindex->nHeight; } @@ -258,9 +258,9 @@ static UniValue waitfornewblock(const JSONRPCRequest& request) WAIT_LOCK(cs_blockchange, lock); block = latestblock; if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]{return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); else - cond_blockchange.wait(lock, [&block]{return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); block = latestblock; } UniValue ret(UniValue::VOBJ); @@ -300,9 +300,9 @@ static UniValue waitforblock(const JSONRPCRequest& request) { WAIT_LOCK(cs_blockchange, lock); if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]{return latestblock.hash == hash || !IsRPCRunning();}); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning();}); else - cond_blockchange.wait(lock, [&hash]{return latestblock.hash == hash || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning(); }); block = latestblock; } @@ -344,9 +344,9 @@ static UniValue waitforblockheight(const JSONRPCRequest& request) { WAIT_LOCK(cs_blockchange, lock); if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]{return latestblock.height >= height || !IsRPCRunning();}); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning();}); else - cond_blockchange.wait(lock, [&height]{return latestblock.height >= height || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning(); }); block = latestblock; } UniValue ret(UniValue::VOBJ); @@ -1995,7 +1995,6 @@ bool FindScriptPubKey(std::atomic& scan_progress, const std::atomic& } /** RAII object to prevent concurrency issue when scanning the txout set */ -static std::mutex g_utxosetscan; static std::atomic g_scan_progress; static std::atomic g_scan_in_progress; static std::atomic g_should_abort_scan; @@ -2008,18 +2007,15 @@ public: bool reserve() { CHECK_NONFATAL(!m_could_reserve); - std::lock_guard lock(g_utxosetscan); - if (g_scan_in_progress) { + if (g_scan_in_progress.exchange(true)) { return false; } - g_scan_in_progress = true; m_could_reserve = true; return true; } ~CoinsViewScanReserver() { if (m_could_reserve) { - std::lock_guard lock(g_utxosetscan); g_scan_in_progress = false; } } diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 05659822156..35750b2ebc5 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -57,14 +58,14 @@ struct FailingCheck { }; struct UniqueCheck { - static std::mutex m; - static std::unordered_multiset results; + static Mutex m; + static std::unordered_multiset results GUARDED_BY(m); size_t check_id; UniqueCheck(size_t check_id_in) : check_id(check_id_in){}; UniqueCheck() : check_id(0){}; bool operator()() { - std::lock_guard l(m); + LOCK(m); results.insert(check_id); return true; } @@ -127,7 +128,7 @@ struct FrozenCleanupCheck { std::mutex FrozenCleanupCheck::m{}; std::atomic FrozenCleanupCheck::nFrozen{0}; std::condition_variable FrozenCleanupCheck::cv{}; -std::mutex UniqueCheck::m; +Mutex UniqueCheck::m; std::unordered_multiset UniqueCheck::results; std::atomic FakeCheckCheckCompletion::n_calls{0}; std::atomic MemoryCheck::fake_allocated_memory{0}; @@ -290,11 +291,15 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) control.Add(vChecks); } } - bool r = true; - BOOST_REQUIRE_EQUAL(UniqueCheck::results.size(), COUNT); - for (size_t i = 0; i < COUNT; ++i) - r = r && UniqueCheck::results.count(i) == 1; - BOOST_REQUIRE(r); + { + LOCK(UniqueCheck::m); + bool r = true; + BOOST_REQUIRE_EQUAL(UniqueCheck::results.size(), COUNT); + for (size_t i = 0; i < COUNT; ++i) { + r = r && UniqueCheck::results.count(i) == 1; + } + BOOST_REQUIRE(r); + } tg.interrupt_all(); tg.join_all(); } diff --git a/src/threadsafety.h b/src/threadsafety.h index bb988dfdfd8..81f86eac3a6 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -6,6 +6,8 @@ #ifndef BITCOIN_THREADSAFETY_H #define BITCOIN_THREADSAFETY_H +#include + #ifdef __clang__ // TL;DR Add GUARDED_BY(mutex) to member variables. The others are // rarely necessary. Ex: int nFoo GUARDED_BY(cs_foo); @@ -54,4 +56,13 @@ #define ASSERT_EXCLUSIVE_LOCK(...) #endif // __GNUC__ +// LockGuard provides an annotated version of lock_guard for us +// should only be used when sync.h Mutex/LOCK/etc aren't usable +class SCOPED_LOCKABLE LockGuard : public std::lock_guard +{ +public: + explicit LockGuard(std::mutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) { } + ~LockGuard() UNLOCK_FUNCTION() {}; +}; + #endif // BITCOIN_THREADSAFETY_H diff --git a/src/util/system.cpp b/src/util/system.cpp index 2013b416dbb..bde0f097be6 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -3,6 +3,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include @@ -75,18 +76,18 @@ const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf"; ArgsManager gArgs; +/** Mutex to protect dir_locks. */ +static Mutex cs_dir_locks; /** A map that contains all the currently held directory locks. After * successful locking, these will be held here until the global destructor * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks * is called. */ -static std::map> dir_locks; -/** Mutex to protect dir_locks. */ -static std::mutex cs_dir_locks; +static std::map> dir_locks GUARDED_BY(cs_dir_locks); bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only) { - std::lock_guard ulock(cs_dir_locks); + LOCK(cs_dir_locks); fs::path pathLockFile = directory / lockfile_name; // If a lock for this directory already exists in the map, don't try to re-lock it @@ -110,13 +111,13 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name) { - std::lock_guard lock(cs_dir_locks); + LOCK(cs_dir_locks); dir_locks.erase((directory / lockfile_name).string()); } void ReleaseDirectoryLocks() { - std::lock_guard ulock(cs_dir_locks); + LOCK(cs_dir_locks); dir_locks.clear(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 29d04a0cbad..e3141baef04 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -631,7 +631,6 @@ private: std::atomic fScanningWallet{false}; // controlled by WalletRescanReserver std::atomic m_scanning_start{0}; std::atomic m_scanning_progress{0}; - std::mutex mutexScanning; friend class WalletRescanReserver; //! the current wallet version: clients below this version are not able to load the wallet @@ -1287,13 +1286,11 @@ public: bool reserve() { assert(!m_could_reserve); - std::lock_guard lock(m_wallet.mutexScanning); - if (m_wallet.fScanningWallet) { + if (m_wallet.fScanningWallet.exchange(true)) { return false; } m_wallet.m_scanning_start = GetTimeMillis(); m_wallet.m_scanning_progress = 0; - m_wallet.fScanningWallet = true; m_could_reserve = true; return true; } @@ -1305,7 +1302,6 @@ public: ~WalletRescanReserver() { - std::lock_guard lock(m_wallet.mutexScanning); if (m_could_reserve) { m_wallet.fScanningWallet = false; }