Skip to content

Commit 082a417

Browse files
author
MarcoFalke
committed
Merge bitcoin#18635: Replace -Wthread-safety-analysis with broader -Wthread-safety
87766b3 build: Replace -Wthread-safety-analysis with broader -Wthread-safety (Hennadii Stepanov) 9cc6eb3 Get rid of -Wthread-safety-precise warnings (Hennadii Stepanov) 971a468 Use template function instead of void* parameter (Hennadii Stepanov) dfb75ae refactor: Rename LockGuard to StdLockGuard for consistency with StdMutex (Hennadii Stepanov) 79be487 Add thread safety annotated wrapper for std::mutex (Hennadii Stepanov) Pull request description: This PR gets rid of `-Wthread-safety-attributes` and `-Wthread-safety-precise` warnings, and replaces `-Wthread-safety-analysis` compiler option with the broader `-Wthread-safety` one. ACKs for top commit: practicalswift: ACK 87766b3 -- patch looks correct ajtowns: ACK 87766b3 MarcoFalke: ACK 87766b3 👍 vasild: ACK 87766b3 Tree-SHA512: b1fe29f2568c954c612f964f9022a1f02333fae4a62c8c16c45e83f5f50a27231fc60b6bd369ccd3bbdb42ef4a0f19defde350c31a62613082ffbc9d7e383a5f
2 parents 55b4c65 + 87766b3 commit 082a417

9 files changed

+34
-24
lines changed

‎configure.ac

+2-2
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,7 @@ if test "x$enable_werror" = "xyes"; then
381381
AX_CHECK_COMPILE_FLAG([-Werror=gnu],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=gnu"],,[[$CXXFLAG_WERROR]])
382382
AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]])
383383
AX_CHECK_COMPILE_FLAG([-Werror=switch],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=switch"],,[[$CXXFLAG_WERROR]])
384-
AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]])
384+
AX_CHECK_COMPILE_FLAG([-Werror=thread-safety],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety"],,[[$CXXFLAG_WERROR]])
385385
AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]])
386386
AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]])
387387
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
401401
AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
402402
AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]])
403403
AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
404-
AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
404+
AX_CHECK_COMPILE_FLAG([-Wthread-safety],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety"],,[[$CXXFLAG_WERROR]])
405405
AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
406406
AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])
407407
AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])

‎src/blockencodings.cpp

+3-4
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,12 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
105105
std::vector<bool> have_txn(txn_available.size());
106106
{
107107
LOCK(pool->cs);
108-
const std::vector<std::pair<uint256, CTxMemPool::txiter> >& vTxHashes = pool->vTxHashes;
109-
for (size_t i = 0; i < vTxHashes.size(); i++) {
110-
uint64_t shortid = cmpctblock.GetShortID(vTxHashes[i].first);
108+
for (size_t i = 0; i < pool->vTxHashes.size(); i++) {
109+
uint64_t shortid = cmpctblock.GetShortID(pool->vTxHashes[i].first);
111110
std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
112111
if (idit != shorttxids.end()) {
113112
if (!have_txn[idit->second]) {
114-
txn_available[idit->second] = vTxHashes[i].second->GetSharedTx();
113+
txn_available[idit->second] = pool->vTxHashes[i].second->GetSharedTx();
115114
have_txn[idit->second] = true;
116115
mempool_count++;
117116
} else {

‎src/blockencodings.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ class PartiallyDownloadedBlock {
126126
protected:
127127
std::vector<CTransactionRef> txn_available;
128128
size_t prefilled_count = 0, mempool_count = 0, extra_count = 0;
129-
CTxMemPool* pool;
129+
const CTxMemPool* pool;
130130
public:
131131
CBlockHeader header;
132132
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}

‎src/logging.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ static int FileWriteStr(const std::string &str, FILE *fp)
4141

4242
bool BCLog::Logger::StartLogging()
4343
{
44-
LockGuard scoped_lock(m_cs);
44+
StdLockGuard scoped_lock(m_cs);
4545

4646
assert(m_buffering);
4747
assert(m_fileout == nullptr);
@@ -80,7 +80,7 @@ bool BCLog::Logger::StartLogging()
8080

8181
void BCLog::Logger::DisconnectTestLogger()
8282
{
83-
LockGuard scoped_lock(m_cs);
83+
StdLockGuard scoped_lock(m_cs);
8484
m_buffering = true;
8585
if (m_fileout != nullptr) fclose(m_fileout);
8686
m_fileout = nullptr;
@@ -246,7 +246,7 @@ namespace BCLog {
246246

247247
void BCLog::Logger::LogPrintStr(const std::string& str)
248248
{
249-
LockGuard scoped_lock(m_cs);
249+
StdLockGuard scoped_lock(m_cs);
250250
std::string str_prefixed = LogEscapeMessage(str);
251251

252252
if (m_log_threadnames && m_started_new_line) {

‎src/logging.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ namespace BCLog {
6262
class Logger
6363
{
6464
private:
65-
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
65+
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
6666

6767
FILE* m_fileout GUARDED_BY(m_cs) = nullptr;
6868
std::list<std::string> m_msgs_before_open GUARDED_BY(m_cs);
@@ -100,22 +100,22 @@ namespace BCLog {
100100
/** Returns whether logs will be written to any output */
101101
bool Enabled() const
102102
{
103-
LockGuard scoped_lock(m_cs);
103+
StdLockGuard scoped_lock(m_cs);
104104
return m_buffering || m_print_to_console || m_print_to_file || !m_print_callbacks.empty();
105105
}
106106

107107
/** Connect a slot to the print signal and return the connection */
108108
std::list<std::function<void(const std::string&)>>::iterator PushBackCallback(std::function<void(const std::string&)> fun)
109109
{
110-
LockGuard scoped_lock(m_cs);
110+
StdLockGuard scoped_lock(m_cs);
111111
m_print_callbacks.push_back(std::move(fun));
112112
return --m_print_callbacks.end();
113113
}
114114

115115
/** Delete a connection */
116116
void DeleteCallback(std::list<std::function<void(const std::string&)>>::iterator it)
117117
{
118-
LockGuard scoped_lock(m_cs);
118+
StdLockGuard scoped_lock(m_cs);
119119
m_print_callbacks.erase(it);
120120
}
121121

‎src/sync.cpp

+4-1
Original file line numberDiff line numberDiff line change
@@ -219,12 +219,15 @@ static bool LockHeld(void* mutex)
219219
return false;
220220
}
221221

222-
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
222+
template <typename MutexType>
223+
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs)
223224
{
224225
if (LockHeld(cs)) return;
225226
tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
226227
abort();
227228
}
229+
template void AssertLockHeldInternal(const char*, const char*, int, Mutex*);
230+
template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*);
228231

229232
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
230233
{

‎src/sync.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs
5252
void LeaveCritical();
5353
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
5454
std::string LocksHeld();
55-
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
55+
template <typename MutexType>
56+
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs);
5657
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
5758
void DeleteLock(void* cs);
5859

@@ -66,7 +67,8 @@ extern bool g_debug_lockorder_abort;
6667
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
6768
void static inline LeaveCritical() {}
6869
void static inline CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
69-
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
70+
template <typename MutexType>
71+
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
7072
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
7173
void static inline DeleteLock(void* cs) {}
7274
#endif

‎src/threadsafety.h

+11-5
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,19 @@
5656
#define ASSERT_EXCLUSIVE_LOCK(...)
5757
#endif // __GNUC__
5858

59-
// LockGuard provides an annotated version of lock_guard for us
60-
// should only be used when sync.h Mutex/LOCK/etc aren't usable
61-
class SCOPED_LOCKABLE LockGuard : public std::lock_guard<std::mutex>
59+
// StdMutex provides an annotated version of std::mutex for us,
60+
// and should only be used when sync.h Mutex/LOCK/etc are not usable.
61+
class LOCKABLE StdMutex : public std::mutex
62+
{
63+
};
64+
65+
// StdLockGuard provides an annotated version of std::lock_guard for us,
66+
// and should only be used when sync.h Mutex/LOCK/etc are not usable.
67+
class SCOPED_LOCKABLE StdLockGuard : public std::lock_guard<StdMutex>
6268
{
6369
public:
64-
explicit LockGuard(std::mutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard<std::mutex>(cs) { }
65-
~LockGuard() UNLOCK_FUNCTION() {};
70+
explicit StdLockGuard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard<StdMutex>(cs) {}
71+
~StdLockGuard() UNLOCK_FUNCTION() {}
6672
};
6773

6874
#endif // BITCOIN_THREADSAFETY_H

‎src/wallet/rpcdump.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -746,7 +746,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
746746
// the user could have gotten from another RPC command prior to now
747747
wallet.BlockUntilSyncedToCurrentChain();
748748

749-
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
749+
LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore);
750750

751751
EnsureWalletIsUnlocked(&wallet);
752752

@@ -769,7 +769,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
769769

770770
std::map<CKeyID, int64_t> mapKeyBirth;
771771
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
772-
pwallet->GetKeyBirthTimes(mapKeyBirth);
772+
wallet.GetKeyBirthTimes(mapKeyBirth);
773773

774774
std::set<CScriptID> scripts = spk_man.GetCScripts();
775775

0 commit comments

Comments
 (0)