-Wthread-safety-attributes
and -Wthread-safety-precise
warnings, and replaces -Wthread-safety-analysis
compiler option with the broader -Wthread-safety
one.
-Wthread-safety-attributes
and -Wthread-safety-precise
warnings, and replaces -Wthread-safety-analysis
compiler option with the broader -Wthread-safety
one.
193+ return;
194+ tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
195+ abort();
196+}
197+
198+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs)
Rather than duplicating the function, you can use a template:
0template<typename PARENT>
1void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, AnnotatedMixin<PARENT>* cs)
2{ ... }
3
4template void AssertLockHeldInternal(const char*, const char*, int, Mutex*);
5template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*);
That means changing the header to:
0template<typename PARENT>
1void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, AnnotatedMixin<PARENT>* cs) ASSERT_EXCLUSIVE_LOCK(cs);
to match.
Or alternatively, don’t touch sync.cpp file at all, drop the ASSERT_EXCLUSIVE_LOCK(cs)
from AssertLockHeldInternal
and change AssertLockHeld
to:
0template<typename PARENT>
1void static inline AssertLockHeldInternalTyped(const char* pszName, const char* pszFile, int nLine, AnnotatedMixin<PARENT>* cs) ASSERT_EXCLUSIVE_LOCK(cs) { AssertLockHeldInternal(pszName, pszFile, nLine, (void*)cs); };
2
3#define AssertLockHeld(cs) AssertLockHeldInternalTyped(#cs, __FILE__, __LINE__, &cs)
Right, in C
the “any type” is void*
, whereas in C++
we have templates.
That void*
typecast at the bottom of the comment above is like sweeping it under the carpet and it still leaves the possibility to use AssertLockHeldInternal()
in a type unsafe way (because it takes void*
argument).
116@@ -110,7 +117,7 @@ class LOCKABLE AnnotatedMixin : public PARENT
117 using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
118
119 /** Wrapped mutex: supports waiting but not recursive locking */
120-typedef AnnotatedMixin<std::mutex> Mutex;
121+using Mutex = AnnotatedMixin<std::mutex>;
Mutex
and RecursiveMutex
after you added the definitions above.
-Wthread-safety-attributes
is, but better typing makes sense in general to me.
Concept ACK
I confirm that if master
(3b1e28924) is compiled with -Wthread-safety-attributes
then some warnings are shown. This patch fixes all such warnings (Clang 11, FreeBSD 12).
I agree with @ajtowns that it would be better to use a template than duplicating the function body. Further, instead of template<typename PARENT>
/ AnnotatedMixin<PARENT>* cs
we can use template <typename MutexType>
/ MutexType* cs
- that is simpler and removes the need to move the Mutex
and RecursiveMutex
definitions higher in sync.h
. Just to be explicit, this is what I mean:
0diff --git i/src/sync.cpp w/src/sync.cpp
1index 67779b33a..8fc7f5d97 100644
2--- i/src/sync.cpp
3+++ w/src/sync.cpp
4@@ -182,29 +182,25 @@ std::string LocksHeld()
5 std::string result;
6 for (const std::pair<void*, CLockLocation>& i : g_lockstack)
7 result += i.second.ToString() + std::string("\n");
8 return result;
9 }
10
11-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, RecursiveMutex* cs)
12+template <typename MutexType>
13+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs)
14 {
15 for (const std::pair<void*, CLockLocation>& i : g_lockstack)
16 if (i.first == cs)
17 return;
18 tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
19 abort();
20 }
21
22-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs)
23-{
24- for (const std::pair<void*, CLockLocation>& i : g_lockstack)
25- if (i.first == cs)
26- return;
27- tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
28- abort();
29-}
30+// Explicitly instantiate for Mutex and RecursiveMutex.
31+template void AssertLockHeldInternal(const char*, const char*, int, Mutex*);
32+template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*);
33
34 void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
35 {
36 for (const std::pair<void*, CLockLocation>& i : g_lockstack) {
37 if (i.first == cs) {
38 tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
39diff --git i/src/sync.h w/src/sync.h
40index 78adf29c0..60e5a87ae 100644
41--- i/src/sync.h
42+++ w/src/sync.h
43@@ -11,17 +11,12 @@
44
45 #include <condition_variable>
46 #include <mutex>
47 #include <string>
48 #include <thread>
49
50-template <typename PARENT>
51-class AnnotatedMixin;
52-using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
53-using Mutex = AnnotatedMixin<std::mutex>;
54-
55 ////////////////////////////////////////////////
56 // //
57 // THE SIMPLE DEFINITION, EXCLUDING DEBUG CODE //
58 // //
59 ////////////////////////////////////////////////
60
61@@ -54,14 +49,14 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
62
63 #ifdef DEBUG_LOCKORDER
64 void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
65 void LeaveCritical();
66 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
67 std::string LocksHeld();
68-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, RecursiveMutex* cs) ASSERT_EXCLUSIVE_LOCK(cs);
69-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs) ASSERT_EXCLUSIVE_LOCK(cs);
70+template <typename MutexType>
71+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs);
72 void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
73 void DeleteLock(void* cs);
74
75 /**
76 * Call abort() if a potential lock order deadlock bug is detected, instead of
77 * just logging information and throwing a logic_error. Defaults to true, and
78@@ -69,14 +64,14 @@ void DeleteLock(void* cs);
79 */
80 extern bool g_debug_lockorder_abort;
81 #else
82 void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
83 void static inline LeaveCritical() {}
84 void static inline CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
85-void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, RecursiveMutex* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
86-void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
87+template <typename MutexType>
88+void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
89 void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
90 void static inline DeleteLock(void* cs) {}
91 #endif
92 #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
93 #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
94
Maybe add thread-safety-attributes
to the warnings flags in configure.ac
to make sure future changes do not introduce such warnings:
0diff --git i/configure.ac w/configure.ac
1index 4c9902efc..e19346e59 100644
2--- i/configure.ac
3+++ w/configure.ac
4@@ -326,12 +326,13 @@ if test "x$enable_werror" = "xyes"; then
5 if test "x$CXXFLAG_WERROR" = "x"; then
6 AC_MSG_ERROR("enable-werror set but -Werror is not usable")
7 fi
8 AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]])
9 AX_CHECK_COMPILE_FLAG([-Werror=switch],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=switch"],,[[$CXXFLAG_WERROR]])
10 AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]])
11+ AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-attributes],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-attributes"],,[[$CXXFLAG_WERROR]])
12 AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]])
13 AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]])
14 AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
15 fi
16
17 if test "x$CXXFLAGS_overridden" = "xno"; then
18@@ -339,12 +340,13 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
19 AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
20 AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
21 AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
22 AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]])
23 AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
24 AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
25+ AX_CHECK_COMPILE_FLAG([-Wthread-safety-attributes],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-attributes"],,[[$CXXFLAG_WERROR]])
26 AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
27 AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])
28 AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])
29 AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
30
31 dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
112+ uint64_t shortid = cmpctblock.GetShortID(pool->vTxHashes[i].first);
113 std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
114 if (idit != shorttxids.end()) {
115 if (!have_txn[idit->second]) {
116- txn_available[idit->second] = vTxHashes[i].second->GetSharedTx();
117+ txn_available[idit->second] = pool->vTxHashes[i].second->GetSharedTx();
Since you are touching all the lines where pool
is used anyway, mind to make it const, since the access is read-only?
0diff --git a/src/blockencodings.h b/src/blockencodings.h
1index 377ac3a1a6..6efcc8c9c9 100644
2--- a/src/blockencodings.h
3+++ b/src/blockencodings.h
4@@ -125,7 +125,8 @@ class PartiallyDownloadedBlock {
5 protected:
6 std::vector<CTransactionRef> txn_available;
7 size_t prefilled_count = 0, mempool_count = 0, extra_count = 0;
8- CTxMemPool* pool;
9+ const CTxMemPool* pool;
10+
11 public:
12 CBlockHeader header;
13 explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Updated 6bc97d0f860b52933b5664386576876d61ae228c -> 7a54927d2f068fef1638b3f6e3f5d7b52be22edf (pr18635.03 -> pr18635.04, diff):
Since you are touching all the lines where
pool
is used anyway, mind to make it const, since the access is read-only?
ACK 7a54927d2
It compiles locally with Clang 11 using the new flags without thread-safety
related warnings.
-Wthread-safety-attributes
means you need to add a wrapper around any plain std::mutex
instances in order to do thread safety analysis on them. Adding class LOCKABLE StdMutex : public std::mutex { };
and having #16127’s LockGuard
refer to that instead of std::mutex
seems to be sufficient to resolve the conflict, I think.
Note that this conflicts with #16127 in that
-Wthread-safety-attributes
means you need to add a wrapper around any plainstd::mutex
instances in order to do thread safety analysis on them. Addingclass LOCKABLE StdMutex : public std::mutex { };
and having #16127’sLockGuard
refer to that instead ofstd::mutex
seems to be sufficient to resolve the conflict, I think.
Agree. Hope #16127 will be merged soon.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
This change gets rid of -Wthread-safety-attributes warning spam.
Rebased 908c6c3cea5ffe3b9a9612f2327227cf3f54e5fc -> 87766b355c47fcb0f0dcf3f6fe359eb00227d50c (pr18635.05 -> pr18635.06) due to the conflict with #16127:
Note that this conflicts with #16127 in that
-Wthread-safety-attributes
means you need to add a wrapper around any plainstd::mutex
instances in order to do thread safety analysis on them. Addingclass LOCKABLE StdMutex : public std::mutex { };
and having #16127’sLockGuard
refer to that instead ofstd::mutex
seems to be sufficient to resolve the conflict, I think.
ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c 👍
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c 👍
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUi4iAwAg0uiwOcah5mJaVzZrOWF2rtKNZTQjr+KEZxWCBRuq1Q+GLJ7Dsf0cqwl
8CWtPoyfI524dIxAEANrMOqlitZ0fS7PYfgWAHDKVgS4mXaYRUEH7YKs1aShz/0u8
9ES1GiLCtAM7WCcvjeVTwsvQ7D8hXtRDyN86oXJnJg+WWJPtp0yd2dMuWhZNvLTiF
10hRbv03wBr632i8dSlBG5y0libBVyPBozVMYAnVKmK3ZMlrLlUwYPtFPBwOmN+4cf
114MrST70Ks90f+6Tlo6VbSoFbIvpNLYfM1g8saQDseA0y04q0h8y0og6WnUMzNYX5
12QwJVr3hR9t50wbiKmAsdIPmFiOTlVu1pqbckAkgNCNmrHaASgNIuRfPR9WyP8fMq
13CWYLw0727ZsUNKxCwEp18vIWkq9wZYVbXTM8GnfpfiA8FX8OH9FH+q7uroNH784N
14Shf9kU2DPmOEHu8MG+7htpWRjHuEN/jVWCrxsoU66g1G7jREddIKNfoLDAzmB0p9
15PlykKQ8f
16=ufrW
17-----END PGP SIGNATURE-----
Timestamp of file with hash 55f3dcd9f7f76a0aa36c4f8d7721a03cda9bfb278201a4ebc4c064d229aba0a4 -