From 79be4874209f71ba6428a80c40c9f028ac936c41 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 28 May 2020 09:32:21 +0300 Subject: [PATCH 1/5] Add thread safety annotated wrapper for std::mutex Co-authored-by: Anthony Towns --- src/logging.h | 2 +- src/threadsafety.h | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/logging.h b/src/logging.h index c55f5819163..2bd8f2683cb 100644 --- a/src/logging.h +++ b/src/logging.h @@ -62,7 +62,7 @@ namespace BCLog { class Logger { 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 + mutable StdMutex 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 GUARDED_BY(m_cs) = nullptr; std::list m_msgs_before_open GUARDED_BY(m_cs); diff --git a/src/threadsafety.h b/src/threadsafety.h index 81f86eac3a6..404ecafebb4 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -56,12 +56,18 @@ #define ASSERT_EXCLUSIVE_LOCK(...) #endif // __GNUC__ +// StdMutex provides an annotated version of std::mutex for us, +// and should only be used when sync.h Mutex/LOCK/etc are not usable. +class LOCKABLE StdMutex : public std::mutex +{ +}; + // 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 +class SCOPED_LOCKABLE LockGuard : public std::lock_guard { public: - explicit LockGuard(std::mutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) { } + explicit LockGuard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) {} ~LockGuard() UNLOCK_FUNCTION() {}; }; From dfb75ae49d4d617ec02188a6f449e8b8015ad467 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 28 May 2020 09:42:26 +0300 Subject: [PATCH 2/5] refactor: Rename LockGuard to StdLockGuard for consistency with StdMutex --- src/logging.cpp | 6 +++--- src/logging.h | 6 +++--- src/threadsafety.h | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 41c6f5c9321..fe58ae9e73e 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() { - LockGuard scoped_lock(m_cs); + StdLockGuard scoped_lock(m_cs); assert(m_buffering); assert(m_fileout == nullptr); @@ -80,7 +80,7 @@ bool BCLog::Logger::StartLogging() void BCLog::Logger::DisconnectTestLogger() { - LockGuard scoped_lock(m_cs); + StdLockGuard 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) { - LockGuard scoped_lock(m_cs); + StdLockGuard 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 2bd8f2683cb..7e646ef67a6 100644 --- a/src/logging.h +++ b/src/logging.h @@ -100,14 +100,14 @@ namespace BCLog { /** Returns whether logs will be written to any output */ bool Enabled() const { - LockGuard scoped_lock(m_cs); + StdLockGuard 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) { - LockGuard scoped_lock(m_cs); + StdLockGuard scoped_lock(m_cs); m_print_callbacks.push_back(std::move(fun)); return --m_print_callbacks.end(); } @@ -115,7 +115,7 @@ namespace BCLog { /** Delete a connection */ void DeleteCallback(std::list>::iterator it) { - LockGuard scoped_lock(m_cs); + StdLockGuard scoped_lock(m_cs); m_print_callbacks.erase(it); } diff --git a/src/threadsafety.h b/src/threadsafety.h index 404ecafebb4..942aa3fdcdb 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -62,13 +62,13 @@ class LOCKABLE StdMutex : public std::mutex { }; -// 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 +// StdLockGuard provides an annotated version of std::lock_guard for us, +// and should only be used when sync.h Mutex/LOCK/etc are not usable. +class SCOPED_LOCKABLE StdLockGuard : public std::lock_guard { public: - explicit LockGuard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) {} - ~LockGuard() UNLOCK_FUNCTION() {}; + explicit StdLockGuard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) {} + ~StdLockGuard() UNLOCK_FUNCTION() {} }; #endif // BITCOIN_THREADSAFETY_H From 971a468ccf0474ca00fa7d20278569b8fb11f0fb Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 28 May 2020 09:55:04 +0300 Subject: [PATCH 3/5] Use template function instead of void* parameter This change gets rid of -Wthread-safety-attributes warning spam. --- src/sync.cpp | 5 ++++- src/sync.h | 6 ++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index c3312b5a00e..9abdedbed42 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -219,12 +219,15 @@ static bool LockHeld(void* mutex) return false; } -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) +template +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) { if (LockHeld(cs)) return; tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); } +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) { diff --git a/src/sync.h b/src/sync.h index 0c6f0ef0a7f..60e5a87aec1 100644 --- a/src/sync.h +++ b/src/sync.h @@ -52,7 +52,8 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs); +template +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 DeleteLock(void* cs); @@ -66,7 +67,8 @@ extern bool g_debug_lockorder_abort; void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline LeaveCritical() {} void static inline CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} -void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {} +template +void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {} void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} void static inline DeleteLock(void* cs) {} #endif From 9cc6eb3c9e0eb1d5be26fb81cc5595c131fec8f4 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 28 May 2020 09:55:39 +0300 Subject: [PATCH 4/5] Get rid of -Wthread-safety-precise warnings --- src/blockencodings.cpp | 7 +++---- src/blockencodings.h | 2 +- src/wallet/rpcdump.cpp | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 263d863cfa0..a47709cd82c 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -105,13 +105,12 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c std::vector have_txn(txn_available.size()); { LOCK(pool->cs); - const std::vector >& vTxHashes = pool->vTxHashes; - for (size_t i = 0; i < vTxHashes.size(); i++) { - uint64_t shortid = cmpctblock.GetShortID(vTxHashes[i].first); + for (size_t i = 0; i < pool->vTxHashes.size(); i++) { + uint64_t shortid = cmpctblock.GetShortID(pool->vTxHashes[i].first); std::unordered_map::iterator idit = shorttxids.find(shortid); if (idit != shorttxids.end()) { if (!have_txn[idit->second]) { - txn_available[idit->second] = vTxHashes[i].second->GetSharedTx(); + txn_available[idit->second] = pool->vTxHashes[i].second->GetSharedTx(); have_txn[idit->second] = true; mempool_count++; } else { diff --git a/src/blockencodings.h b/src/blockencodings.h index 9ec1beeaf7a..326db1b4a7c 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -126,7 +126,7 @@ class PartiallyDownloadedBlock { protected: std::vector txn_available; size_t prefilled_count = 0, mempool_count = 0, extra_count = 0; - CTxMemPool* pool; + const CTxMemPool* pool; public: CBlockHeader header; explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {} diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 7bf3d169c3b..d5f6d63a46e 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -746,7 +746,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); + LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore); EnsureWalletIsUnlocked(&wallet); @@ -769,7 +769,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) std::map mapKeyBirth; const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); - pwallet->GetKeyBirthTimes(mapKeyBirth); + wallet.GetKeyBirthTimes(mapKeyBirth); std::set scripts = spk_man.GetCScripts(); From 87766b355c47fcb0f0dcf3f6fe359eb00227d50c Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 28 May 2020 09:56:44 +0300 Subject: [PATCH 5/5] build: Replace -Wthread-safety-analysis with broader -Wthread-safety --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 7f0c5dbd7ac..28015d005de 100644 --- a/configure.ac +++ b/configure.ac @@ -381,7 +381,7 @@ if test "x$enable_werror" = "xyes"; then AX_CHECK_COMPILE_FLAG([-Werror=gnu],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=gnu"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Werror=switch],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=switch"],,[[$CXXFLAG_WERROR]]) - AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]]) + AX_CHECK_COMPILE_FLAG([-Werror=thread-safety],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]]) @@ -401,7 +401,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]]) - AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]]) + AX_CHECK_COMPILE_FLAG([-Wthread-safety],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])