Replace -Wthread-safety-analysis with broader -Wthread-safety by hebasto · Pull Request #18635 · bitcoin/bitcoin

hebasto · 2020-04-14T17:17:41Z

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.

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.

src/sync.cpp

src/sync.h

hebasto · 2020-05-07T07:47:54Z

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

vasild · 2020-05-07T10:09:06Z

Concept ACK

I confirm that if master (3b1e289) 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:

patch to use MutexType* argument, on top of this PR (b6e44c3)

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)

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

patch to extend warnings in configure.ac

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

hebasto · 2020-05-09T09:13:23Z

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.

maflcko

Concept ACK

src/blockencodings.cpp

DrahtBot · 2020-05-09T19:21:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: fix -Wformat-security check when compiling with GCC #18882 (build: fix -Wformat-security check when compiling with GCC by fanquake)
build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
Use shared pointers only in validation interface #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.

hebasto · 2020-05-10T05:29:38Z

Updated 6bc97d0 -> 7a54927 (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?

vasild

ACK 7a54927

It compiles locally with Clang 11 using the new flags without thread-safety related warnings.

ajtowns · 2020-05-12T06:46:33Z

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

ajtowns · 2020-05-19T07:37:37Z

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.

hebasto · 2020-05-19T07:50:25Z

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.

hebasto · 2020-05-28T07:05:38Z

Rebased 908c6c3 -> 87766b3 (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.

vasild

ACK 87766b3

practicalswift · 2020-05-28T10:04:36Z

ACK 87766b3 -- patch looks correct

ajtowns · 2020-05-28T11:26:21Z

ACK 87766b3

maflcko · 2020-05-28T14:47:34Z

ACK 87766b3 👍

Show signature and timestamp

Signature:

Timestamp of file with hash 55f3dcd9f7f76a0aa36c4f8d7721a03cda9bfb278201a4ebc4c064d229aba0a4 -

Summary: Co-authored-by: Anthony Towns <[email protected]> This is part [1/5] of Core [[bitcoin/bitcoin#18635 | PR18635]] : bitcoin/bitcoin@79be487 Test Plan: ninja all check Using clang. Check that the thread safety analysis is happy with it. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7877

Summary: This is part [2/5] of Core [[bitcoin/bitcoin#18635 | PR18635]] : bitcoin/bitcoin@dfb75ae Depends on D7877 Test Plan: ninja all check-all Check that clang's thread safety analysis is happy. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7878

Summary: This change gets rid of -Wthread-safety-attributes warning spam. This is part [3/5] of Core [[bitcoin/bitcoin#18635 | PR18635]] : bitcoin/bitcoin@971a468 Depends on D7878 Test Plan: ninja all check Checks that clang thread safety analysis is happy. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7879

Summary: This is part [4/5] of Core [[bitcoin/bitcoin#18635 | PR18635]] : bitcoin/bitcoin@9cc6eb3 Depends on D7879, D7876 and D7875 Test Plan: ninja all check Check that clang thread safety analysis is happy. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7880

Summary: This is part [5/5] of Core [[bitcoin/bitcoin#18635 | PR18635]] : bitcoin/bitcoin@87766b3 Depends on D7882 Test Plan: ninja all check Make sure that clang is happy. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7883

…afety

merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements

hebasto mentioned this pull request Apr 14, 2020

Thread Safety Analysis warnings #18636

Closed

hebasto marked this pull request as draft April 14, 2020 19:58

hebasto changed the title ~~Revert "Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis"~~ [WIP] Revert "Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis" Apr 14, 2020

hebasto force-pushed the 200414-threads branch from f412666 to b6e44c3 Compare April 14, 2020 22:01

hebasto changed the title ~~[WIP] Revert "Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis"~~ Suppress -Wthread-safety-attributes warning spam Apr 14, 2020

hebasto marked this pull request as ready for review April 14, 2020 22:36

fanquake requested a review from ajtowns May 7, 2020 00:45

ajtowns reviewed May 7, 2020

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

src/sync.h Outdated Show resolved Hide resolved

hebasto force-pushed the 200414-threads branch from b6e44c3 to 6bc97d0 Compare May 9, 2020 08:47

hebasto changed the title ~~Suppress -Wthread-safety-attributes warning spam~~ Replace -Wthread-safety-analysis with broader -Wthread-safety May 9, 2020

DrahtBot added Build system RPC/REST/ZMQ Wallet labels May 9, 2020

maflcko reviewed May 9, 2020

View reviewed changes

src/blockencodings.cpp Show resolved Hide resolved

This was referenced May 9, 2020

build: fix -Wformat-security check when compiling with GCC #18882

Merged

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

DrahtBot mentioned this pull request May 9, 2020

Use shared pointers only in validation interface #18354

Closed

hebasto force-pushed the 200414-threads branch from 6bc97d0 to 7a54927 Compare May 10, 2020 05:26

vasild approved these changes May 11, 2020

View reviewed changes

DrahtBot mentioned this pull request May 19, 2020

Prevent UB in DeleteLock() function #18881

Merged

hebasto mentioned this pull request May 24, 2020

txmempool: split epoch logic into class #18017

Merged

DrahtBot added the Needs rebase label May 26, 2020

build: Replace -Wthread-safety-analysis with broader -Wthread-safety

87766b3

hebasto force-pushed the 200414-threads branch from 908c6c3 to 87766b3 Compare May 28, 2020 07:02

vasild approved these changes May 28, 2020

View reviewed changes

maflcko merged commit 082a417 into bitcoin:master May 28, 2020

hebasto deleted the 200414-threads branch May 28, 2020 15:53

ryanofsky mentioned this pull request Jun 1, 2020

Wallet passive startup #15719

Closed

LarryRuane mentioned this pull request Mar 26, 2021

Allow zcashd to be built cleanly with -Wthread-safety zcash/zcash#5035

Open

kwvg mentioned this pull request Jun 5, 2021

merge #11640, #11599, #16112, #16127, #18635, #19249: thread safety and locking improvements dashpay/dash#4190

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2021

merge bitcoin#18635: Replace -Wthread-safety-analysis with -Wthread-s…

51fabab

…afety

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 11, 2021

Merge pull request #4190 from kittywhiskers/tlocks

a8aee57

merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

hebasto commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 7, 2020

Uh oh!

vasild commented May 7, 2020

Uh oh!

hebasto commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented May 10, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented May 12, 2020

Uh oh!

ajtowns commented May 19, 2020

Uh oh!

hebasto commented May 19, 2020

Uh oh!

hebasto commented May 28, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 28, 2020

Uh oh!

ajtowns commented May 28, 2020

Uh oh!

maflcko commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Apr 14, 2020 •

edited

Loading

hebasto commented May 9, 2020 •

edited

Loading

DrahtBot commented May 9, 2020 •

edited

Loading