Replace -Wthread-safety-analysis with broader -Wthread-safety #18635

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.

It's not clear to me what the value of adding -Wthread-safety-attributes is, but better typing makes sense in general to me.

@ajtowns

It's not clear to me what the value of adding -Wthread-safety-attributes is, but better typing makes sense in general to me.

From the Thread Safety Analysis docs:

-Wthread-safety-attributes: Sanity checks on attribute syntax.

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:

<details> <summary>patch to use MutexType* argument, on top of this PR (b6e44c3fd)</summary>

diff --git i/src/sync.cpp w/src/sync.cpp
index 67779b33a..8fc7f5d97 100644
--- i/src/sync.cpp
+++ w/src/sync.cpp
@@ -182,29 +182,25 @@ std::string LocksHeld()
     std::string result;
     for (const std::pair<void*, CLockLocation>& i : g_lockstack)
         result += i.second.ToString() + std::string("\n");
     return result;
 }
 
-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, RecursiveMutex* cs)
+template <typename MutexType>
+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs)
 {
     for (const std::pair<void*, CLockLocation>& i : g_lockstack)
         if (i.first == cs)
             return;
     tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
     abort();
 }
 
-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs)
-{
-    for (const std::pair<void*, CLockLocation>& i : g_lockstack)
-        if (i.first == cs)
-            return;
-    tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
-    abort();
-}
+// Explicitly instantiate for Mutex and RecursiveMutex.
+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)
 {
     for (const std::pair<void*, CLockLocation>& i : g_lockstack) {
         if (i.first == cs) {
             tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
diff --git i/src/sync.h w/src/sync.h
index 78adf29c0..60e5a87ae 100644
--- i/src/sync.h
+++ w/src/sync.h
@@ -11,17 +11,12 @@
 
 #include <condition_variable>
 #include <mutex>
 #include <string>
 #include <thread>
 
-template <typename PARENT>
-class AnnotatedMixin;
-using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
-using Mutex = AnnotatedMixin<std::mutex>;
-
 ////////////////////////////////////////////////
 //                                            //
 // THE SIMPLE DEFINITION, EXCLUDING DEBUG CODE //
 //                                            //
 ////////////////////////////////////////////////
 
@@ -54,14 +49,14 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
 
 #ifdef DEBUG_LOCKORDER
 void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
 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, RecursiveMutex* cs) ASSERT_EXCLUSIVE_LOCK(cs);
-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs) ASSERT_EXCLUSIVE_LOCK(cs);
+template <typename MutexType>
+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);
 
 /**
  * Call abort() if a potential lock order deadlock bug is detected, instead of
  * just logging information and throwing a logic_error. Defaults to true, and
@@ -69,14 +64,14 @@ void DeleteLock(void* cs);
  */
 extern bool g_debug_lockorder_abort;
 #else
 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, RecursiveMutex* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
-void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, Mutex* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
+template <typename MutexType>
+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
 #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
 #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 

</details>

Maybe add thread-safety-attributes to the warnings flags in configure.ac to make sure future changes do not introduce such warnings:

<details> <summary>patch to extend warnings in configure.ac</summary>

diff --git i/configure.ac w/configure.ac
index 4c9902efc..e19346e59 100644
--- i/configure.ac
+++ w/configure.ac
@@ -326,12 +326,13 @@ if test "x$enable_werror" = "xyes"; then
   if test "x$CXXFLAG_WERROR" = "x"; then
     AC_MSG_ERROR("enable-werror set but -Werror is not usable")
   fi
   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-attributes],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-attributes"],,[[$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]])
 fi

 if test "x$CXXFLAGS_overridden" = "xno"; then
@@ -339,12 +340,13 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
   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-attributes],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-attributes"],,[[$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]])
   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])

   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all

</details>

Updated:

• addressed @ajtowns's and @vasild's comments
• rebased
• added two commits that allow to replace -Wthread-safety-analysis with broader -Wthread-safety

The OP has been updated.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18882 (build: fix -Wformat-security check when compiling with GCC by fanquake)
• #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
• #18354 (Use shared pointers only in validation interface by bvbfan)

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):

• addressed @MarcoFalke's comment:

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.

@hebasto Yeah, I saw the "sanity check" description, but it's still not clear to me what that actually means. Changes for precise look good to me.

Note that this conflicts with #16127 in that -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 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.

Agree. Hope #16127 will be merged soon.

Rebased 7a54927d2f068fef1638b3f6e3f5d7b52be22edf -> 908c6c3cea5ffe3b9a9612f2327227cf3f54e5fc (pr18635.04 -> pr18635.05) due to the conflict with #18881.

Code review ACK 908c6c3cea5ffe3b9a9612f2327227cf3f54e5fc. It seems good to enable more static checking here, and the code changes are simple and straightforward

ACK 908c6c3cea5ffe3b9a9612f2327227cf3f54e5fc: patch looks correct, and more compile-time checking is better! :)

Needs update now that #16127 is merged

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 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.

ACK 87766b3

ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c -- patch looks correct

ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c

ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c 👍

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 87766b355c47fcb0f0dcf3f6fe359eb00227d50c 👍
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi4iAwAg0uiwOcah5mJaVzZrOWF2rtKNZTQjr+KEZxWCBRuq1Q+GLJ7Dsf0cqwl
CWtPoyfI524dIxAEANrMOqlitZ0fS7PYfgWAHDKVgS4mXaYRUEH7YKs1aShz/0u8
ES1GiLCtAM7WCcvjeVTwsvQ7D8hXtRDyN86oXJnJg+WWJPtp0yd2dMuWhZNvLTiF
hRbv03wBr632i8dSlBG5y0libBVyPBozVMYAnVKmK3ZMlrLlUwYPtFPBwOmN+4cf
4MrST70Ks90f+6Tlo6VbSoFbIvpNLYfM1g8saQDseA0y04q0h8y0og6WnUMzNYX5
QwJVr3hR9t50wbiKmAsdIPmFiOTlVu1pqbckAkgNCNmrHaASgNIuRfPR9WyP8fMq
CWYLw0727ZsUNKxCwEp18vIWkq9wZYVbXTM8GnfpfiA8FX8OH9FH+q7uroNH784N
Shf9kU2DPmOEHu8MG+7htpWRjHuEN/jVWCrxsoU66g1G7jREddIKNfoLDAzmB0p9
PlykKQ8f
=ufrW
-----END PGP SIGNATURE-----

Timestamp of file with hash 55f3dcd9f7f76a0aa36c4f8d7721a03cda9bfb278201a4ebc4c064d229aba0a4 -

</details>