Strengthen thread safety assertions #24931

1. 
   ajtowns commented at 9:25 am on April 20, 2022: member

   This changes LOCK(mutex) for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

   This can’t reasonably be used for globals, because clang would require every function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex) for each global mutex; so this introduces a trivial GlobalMutex subclass of Mutex, and reduces the annotations for both GlobalMutex to LOCKS_EXCLUDED which only catches trivial errors (eg (LOCK(x); LOCK(x);).

2. 
   ajtowns commented at 9:30 am on April 20, 2022: member

   This hopefully avoids bugs like #24157 (review) and gets most of the benefits of -Wthread-safety-negative without introducing massive amounts of unhelpful warnings about cs_main and the like as per #20272. See also #21598.
3. 
   jonatack commented at 9:37 am on April 20, 2022: member

   Concept ACK
4. ajtowns force-pushed on Apr 20, 2022
5. DrahtBot added the label Consensus on Apr 20, 2022
6. DrahtBot added the label GUI on Apr 20, 2022
7. DrahtBot added the label P2P on Apr 20, 2022
8. DrahtBot added the label RPC/REST/ZMQ on Apr 20, 2022
9. DrahtBot added the label Utils/log/libs on Apr 20, 2022
10. DrahtBot added the label UTXO Db and Indexes on Apr 20, 2022
11. DrahtBot added the label Validation on Apr 20, 2022
12. DrahtBot added the label Wallet on Apr 20, 2022
13. 
    DrahtBot commented at 3:03 pm on April 20, 2022: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)

    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.

14. 
    jamesob commented at 3:24 pm on April 20, 2022: member

    Concept ACK
15. 
    hebasto commented at 3:44 pm on April 20, 2022: member

    Concept ACK. I had a similar perspective view since f8213c05f087e5fbb5d92a291f766b0baebc798f :)

    Maybe split off changes related to GlobalMutex as they looks non-controversial and pretty straightforward to review?

16. MarcoFalke removed the label GUI on Apr 20, 2022
17. MarcoFalke removed the label Wallet on Apr 20, 2022
18. MarcoFalke removed the label UTXO Db and Indexes on Apr 20, 2022
19. MarcoFalke removed the label RPC/REST/ZMQ on Apr 20, 2022
20. MarcoFalke removed the label P2P on Apr 20, 2022
21. MarcoFalke removed the label Validation on Apr 20, 2022
22. MarcoFalke removed the label Consensus on Apr 20, 2022
23. MarcoFalke removed the label Utils/log/libs on Apr 20, 2022
24. MarcoFalke added the label Refactoring on Apr 20, 2022
25. in src/checkqueue.h:191 in b6914e6007 outdated

    187@@ -188,7 +188,7 @@ class CCheckQueue
    188     }
    189 
    190     //! Stop all of the worker threads.
    191-    void StopWorkerThreads()
    192+    void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
    

    ---

    ---

    

    jonatack commented at 6:52 pm on April 20, 2022:

    Should each of the annotations added in the first commit also have AssertLockNotHeld?

    ---

    

    ajtowns commented at 1:12 am on April 21, 2022:

    That feels like overkill, but I’m not sure where to draw the line.

    ---

    

    MarcoFalke commented at 2:24 pm on June 10, 2022:

    Closing as resolved?
26. in src/sync.h:136 in b6914e6007 outdated

    132+
    133+/** Different type to mark Mutex's outside of a class
    134+ *
    135+ * Thread safety analysis can't handle negative assertions about mutex's
    136+ * with global scope well, so mark them with a separate type, and
    137+ * eventually move all the mutexes into classes so they are not globally
    

    ---

    ---

    

    jonatack commented at 6:55 pm on April 20, 2022:

    nit, “mutexes” here is correct, but s/Mutex’s/Mutexes/ on line 130 and s/mutex’s/mutexes/ on line 134
27. 
    jonatack commented at 7:00 pm on April 20, 2022: member

    Is the goal of this change to allow building with -Wthread-safety-negative? Still seeing many warnings on this branch with ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' && make clean && make. A regular debug build on this branch is clean, however.
28. 
    ajtowns commented at 1:25 am on April 21, 2022: member

    Is the goal of this change to allow building with -Wthread-safety-negative?

    No; as far as I can see that option prevents you from holding a RecursiveMutex when reacquiring the same RecursiveMutex, so is pointless until we don’t have any of those left.

29. 
    w0xlt commented at 8:47 am on April 22, 2022: contributor

    Concept ACK. Will review soon.
30. ajtowns force-pushed on Apr 26, 2022
31. 
    ajtowns commented at 9:37 am on April 26, 2022: member

    Rebased past #24157, fixed bad apostrophes
32. 
    w0xlt commented at 10:44 am on April 26, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/24931/commits/e735d93ce456eb37898ae11e623702e46970b80f

    I changed a RecursiveMutex to Mutex and the compilation failed with warnings requires negative capability for AssertLockNotHeld, LOCK and LOCK2.

33. in src/net.h:958 in e735d93ce4 outdated

    954@@ -954,11 +955,11 @@ class CConnman
    955     bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
    956     bool InitBinds(const Options& options);
    957 
    958-    void ThreadOpenAddedConnections();
    959-    void AddAddrFetch(const std::string& strDest);
    960-    void ProcessAddrFetch();
    961-    void ThreadOpenConnections(std::vector<std::string> connect);
    962-    void ThreadMessageHandler();
    963+    void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex);
    

    ---

    ---

    

    vasild commented at 1:31 pm on April 26, 2022:

    !m_addr_fetches_mutex and !m_nodes_mutex seem to be unnecessary here.

    0    void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
    

34. in src/net.h:961 in e735d93ce4 outdated

    961-    void ThreadOpenConnections(std::vector<std::string> connect);
    962-    void ThreadMessageHandler();
    963+    void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex);
    964+    void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex);
    965+    void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex);
    966+    void ThreadOpenConnections(std::vector<std::string> connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex);
    

    ---

    ---

    

    vasild commented at 1:41 pm on April 26, 2022:

    ThreadOpenConnections() acquires m_nodes_mutex directly.

    0    void ThreadOpenConnections(std::vector<std::string> connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex);
    

    ---

    

    ajtowns commented at 3:43 pm on April 26, 2022:

    This isn’t caught because m_nodes_mutex is a RecursiveMutex so it’s okay if it’s already held
35. in src/net.h:1036 in e735d93ce4 outdated

    1034     void SocketHandlerListening(const std::set<SOCKET>& recv_set);
    1035 
    1036-    void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
    1037-    void ThreadDNSAddressSeed();
    1038+    void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
    1039+    void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex);
    

    ---

    ---

    

    vasild commented at 1:54 pm on April 26, 2022:

    ThreadDNSAddressSeed() acquires m_nodes_mutex directly.

    0    void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);
    

36. in src/versionbits.h:90 in e735d93ce4 outdated

    86@@ -87,21 +87,21 @@ class VersionBitsCache
    87     /** Get the numerical statistics for a given deployment for the signalling period that includes pindex.
    88      * If provided, signalling_blocks is set to true/false based on whether each block in the period signalled
    89      */
    90-    static BIP9Stats Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks = nullptr);
    91+    static BIP9Stats Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks = nullptr) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
    

    ---

    ---

    

    vasild commented at 2:17 pm on April 26, 2022:

    This change seems unnecessary because this method does not lock any mutex or call a method that requires !m_mutex:

    0BIP9Stats VersionBitsCache::Statistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos, std::vector<bool>* signalling_blocks)
    1{
    2    return VersionBitsConditionChecker(pos).GetStateStatisticsFor(pindex, params, signalling_blocks);
    3}   
    

    ---

    

    ajtowns commented at 3:45 pm on April 26, 2022:

    It’s also static, so seems like it would have a hard time finding an m_mutex to reference…
37. in src/sync.h:145 in e735d93ce4 outdated

    141+ */
    142+class GlobalMutex : public Mutex { };
    143+
    144+#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
    145+
    146+inline void AssertLockNotHeldInline(const char* pszName, const char* pszFile, int nLine, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(pszName, pszFile, nLine, cs); }
    

    ---

    ---

    

    vasild commented at 2:35 pm on April 26, 2022:

    style: AssertLockNotHeldInline() is newly added function, so should abide to the naming convention for its arguments:

    0inline void AssertLockNotHeldInline(const char* mutex_name, const char* file_name, int line, Mutex* mutex) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(mutex_name, file_name, line, mutex); }
    

38. 
    vasild commented at 2:54 pm on April 26, 2022: member

    ACK 314d2d4879 - just the first two commits, modulo the inline comments below.

    The 3rd commit adds GlobalMutex type and having two types: Mutex and GlobalMutex seems like one more “WTF” moment to me.

39. ajtowns force-pushed on Apr 26, 2022
40. w0xlt approved
41. 
    w0xlt commented at 6:16 pm on April 26, 2022: contributor

    reACK https://github.com/bitcoin/bitcoin/pull/24931/commits/c7a011c0d18206ca79e54b9cfef3f6abce456889
42. 
    vasild commented at 9:28 am on April 27, 2022: member

    ACK a55ee93f5aa6c94eb333f16db112bb834807acd4 - just the first two commits, in case you decide to split them in a separate PR. Thanks!
43. DrahtBot added the label Needs rebase on Apr 30, 2022
44. 
    ajtowns commented at 5:01 pm on May 3, 2022: member

    Rebased past #24543
45. ajtowns force-pushed on May 3, 2022
46. DrahtBot removed the label Needs rebase on May 3, 2022
47. DrahtBot added the label Needs rebase on May 11, 2022
48. ajtowns force-pushed on May 11, 2022
49. 
    ajtowns commented at 5:16 pm on May 11, 2022: member

    Rebased past #25100 and split the first two commits out into #25109; please review that one first, it should be easy.
50. DrahtBot removed the label Needs rebase on May 11, 2022
51. ajtowns force-pushed on May 16, 2022
52. 
    ajtowns commented at 1:11 pm on May 16, 2022: member

    Rebased over #25109 merge
53. 
    hebasto commented at 3:38 pm on May 16, 2022: member

    It seems the g_shutdown_mutex has been missed to be converted from Mutex into GlobalMutex, no?
54. in src/sync.h:252 in e6f05cc7df outdated

    248+// When locking a Mutex, require negative capability to ensure the lock
    249+// is not already held
    250+inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
    251+inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
    252+
    253+// When locking a GlobalMutex, just check we don't know it's locked
    

    ---

    ---

    

    hebasto commented at 3:44 pm on May 16, 2022:

    Sorry for bothering, but it’s hard for me to parse this comment (sure I need to improve my English).

    Maybe, re-word it a bit?

    ---

    

    MarcoFalke commented at 3:48 pm on May 16, 2022:

    0// When locking a GlobalMutex, just check it is not locked in the surrounding scope.
    

    ---

    

    ajtowns commented at 5:09 pm on May 16, 2022:

    Done; thanks @MarcoFalke for the suggestion so I didn’t have to ask for one :)
55. in doc/developer-notes.md:905 in e6f05cc7df outdated

    901@@ -902,14 +902,19 @@ Threads and synchronization
    902 - Prefer `Mutex` type to `RecursiveMutex` one.
    903 
    904 - Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to
    905-  get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
    906-  run-time asserts in function definitions:
    907+  get compile-time warnings about potential race conditions in code.
    

    ---

    ---

    

    hebasto commented at 3:50 pm on May 16, 2022:

    Using of EXCLUSIVE_LOCKS_REQUIRED(!m_some_mutex) also could prevent deadlocks.

    ---

    

    ajtowns commented at 6:59 pm on May 16, 2022:

    “get compile-time warnings about potential race conditions and deadlocks in code” ?

    ---

    

    ajtowns commented at 10:46 pm on May 19, 2022:

    Done
56. 
    hebasto commented at 3:51 pm on May 16, 2022: member

    Approach ACK e6f05cc7dfa021a0aef03d1ae904aa3b9555b1aa.
57. 
    ajtowns commented at 3:57 pm on May 16, 2022: member

    The g_shutdown_mutex is scoped within the Shutdown() function rather than at the module level, so while it’s a global in the sense of “there’s only one of them”, it’s not global in the sense of “every function can directly reference it, without need it to be passed in”, so there’s no need to annotate it differently to other normal mutexes.
58. ajtowns force-pushed on May 16, 2022
59. ajtowns force-pushed on May 19, 2022
60. refactor: Propagate negative `!m_most_recent_block_mutex` capability

    Could be verified with
    $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
    $ make clean
    $ make 2>&1 | grep m_most_recent_block_mutex

    5a6e3c1db3
61. refactor: Propagate negative `!m_tx_relay_mutex` capability

    Could be verified with
    $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
    $ make clean
    $ make 2>&1 | grep m_tx_relay_mutex

    2b3373c152
62. 
    hebasto commented at 11:40 am on May 20, 2022: member

    Looks like this PR has a silent merge conflict because of #22778 and #24062.

    See #25175.

63. 
    hebasto commented at 11:44 am on May 20, 2022: member

    This changes LOCK(mutex) for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

    How about WITH_LOCK macro?

64. ajtowns force-pushed on May 20, 2022
65. 
    ajtowns commented at 2:24 pm on May 20, 2022: member

    Rebased onto #25175. The WITH_LOCK macro is based on LOCK, so also enforces these checks. LOCK2, TRY_LOCK and WAIT_LOCK are also updated.
66. 
    hebasto commented at 2:30 pm on May 20, 2022: member

    Rebased onto #25175. The WITH_LOCK macro is based on LOCK, so also enforces these checks. LOCK2, TRY_LOCK and WAIT_LOCK are also updated.

    https://github.com/bitcoin/bitcoin/blob/cd6ae6906602090a1f3537e9abf97e25e4a6421a/src/net_processing.cpp#L294-L297

    does not emit a warning about missed EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) annotation.

67. 
    ajtowns commented at 2:31 pm on May 20, 2022: member

    Though, on the other hand, WITH_LOCK is within a lambda (in order to be able to return the result of the expression) and the annotations aren’t propogated to the lambda. I think this is probably okay: RecursiveMutex and GlobalMutex don’t require propogation anyway, and Mutex are all within a class so the new lambda will be implicitly tagged with the negative capability anyway?

    However, tagging the WITH_LOCK lambda with LOCKS_EXCLUDED(cs) could be done, just as a double check that it’s not being used when the lock is already held?

68. net_processing: thread safety annotation for m_tx_relay_mutex f24bd45b37
69. qt/clientmodel: thread safety annotation for m_cached_tip_mutex be6aa72f9f
70. sync.h: Add GlobalMutex type a559509a0b
71. scripted-diff: Convert global Mutexes to GlobalMutexes

    -BEGIN VERIFY SCRIPT-
    sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]')
    -END VERIFY SCRIPT-

    bba87c0553
72. sync.h: Imply negative assertions when calling LOCK d2852917ee
73. doc: Update developer notes ce893c0497
74. ajtowns force-pushed on May 20, 2022
75. 
    ajtowns commented at 3:37 pm on May 20, 2022: member

    https://github.com/bitcoin/bitcoin/blob/cd6ae6906602090a1f3537e9abf97e25e4a6421a/src/net_processing.cpp#L294-L297

    does not emit a warning about missed EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) annotation.

    Updated with negative assertions propogated when WITH_LOCK is used, which caught the above case and another. Couldn’t see a way to avoid having WITH_LOCK expand cs twice though.

76. MarcoFalke referenced this in commit aac99faa66 on May 20, 2022
77. in src/sync.h:141 in ce893c0497

    136+ * eventually move all the mutexes into classes so they are not globally
    137+ * visible.
    138+ *
    139+ * See: https://github.com/bitcoin/bitcoin/pull/20272#issuecomment-720755781
    140+ */
    141+class GlobalMutex : public Mutex { };
    

    ---

    ---

    

    hebasto commented at 6:20 pm on May 20, 2022:

    a559509a0b8cade27199740212d7b589f71a0e3b

    nit, clang-format-diff.py complains:

    0class GlobalMutex : public Mutex
    1{
    2};
    

78. hebasto approved
79. 
    hebasto commented at 6:21 pm on May 20, 2022: member

    ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e
80. ajtowns requested review from jonatack on Jun 1, 2022
81. ajtowns requested review from jamesob on Jun 1, 2022
82. in src/sync.h:305 in ce893c0497

    301@@ -276,7 +302,7 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
    302 //!
    303 //! The above is detectable at compile-time with the -Wreturn-local-addr flag in
    304 //! gcc and the -Wreturn-stack-address flag in clang, both enabled by default.
    305-#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
    306+#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())
    

    ---

    ---

    

    vasild commented at 7:51 am on June 2, 2022:

    While playing with this I observed that it is possible to circumvent the thread safety warnings mechanism by wrapping the lock call in a lambda and calling this lambda immediately. That circumvention only works for mutexes that are a member of a class:

     0Mutex mutex_global;
     1
     2// Commented annotations would silence the warnings, clang 15
     3class C
     4{
     5public:
     6    Mutex mutex_member;
     7
     8    void f1() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_member)
     9    {
    10        // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_member' [-Wthread-safety-analysis]
    11        LOCK(mutex_member);
    12    }
    13
    14    void f2()
    15    {
    16        [&]() { LOCK(mutex_member); }(); // no need for annotations
    17    }
    18
    19    void f3() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_global)
    20    {
    21        // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_global' [-Wthread-safety-analysis]
    22        LOCK(mutex_global);
    23    }
    24
    25    void f4() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_global)
    26    {
    27        // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_global' [-Wthread-safety-analysis]
    28        [&]() /* EXCLUSIVE_LOCKS_REQUIRED(!mutex_global) */ { LOCK(mutex_global); }();
    29    }
    30};
    

    Looks like the construct used for WITH_LOCK() in this PR relies on the f2() behavior from the example above. Is that behavior documented somewhere in the clang docs? If not, then it may be wiser to not depend on undocumented behavior.

    ---

    

    ajtowns commented at 6:39 pm on June 2, 2022:

    You can avoid the thread safety warnings in a lot of ways if you create aliases for either the locks or the variables being guarded; eg:

    0    int i GUARDED_BY(mutex_member) = 0;
    1    void f5()
    2    {
    3        int* i_alias = &i;
    4        *i_alias = 5;
    5    }
    

    (Using [&] here is creating an implicit alias for all the member variables, though clang does a better job of analysing past the aliases there than in the above example…)

    I think the best long term approach is to do away with any uses of both GlobalMutex and RecursiveMutex, at which point we can annotate the lambda in WITH_LOCK with EXCLUSIVE_LOCKS_REQUIRED(!cs) and we’re done. Having it work in the meantime allows us to be more confident that we don’t have any potential recursive cases when switching a RecursiveMutex to a regular Mutex, so seems like it aids that goal to me.

    ---

    

    vasild commented at 12:58 pm on June 6, 2022:

    Ok, this explains why f2() needs no annotations. I guess at some point the compiler may become smarter, detect the reference and issue a warning.

    I think the best long term approach is to do away with any uses of both GlobalMutex and RecursiveMutex, at which point we can annotate the lambda in WITH_LOCK with EXCLUSIVE_LOCKS_REQUIRED(!cs) and we’re done.

    I agree. Do you envision that we do not have mutexes in global scope, or we have ones that are of type Mutex?

    ---

    

    ajtowns commented at 11:12 pm on June 6, 2022:

    Anytime you currently have:

    0GlobalMutex g_mut;
    1int g_importantnum GUARDED_BY(g_mut) = 0;
    2std::string g_importantstr GUARDED_BY(g_mut);
    3
    4void bump_importantint(int offset)
    5{
    6    LOCK(g_mut);
    7    g_importantnum += offset;
    8}
    

    you can rewrite it as

    0class important_stuff
    1{
    2private:
    3    Mutex m_mut;
    4    int m_num GUARDED_BY(m_mut) = 0;
    5    std::string m_str GUARDED_BY(m_mut);
    6public:
    7    void bump(int offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mut);
    8};
    9important_stuff g_important;
    

    or similar (depending on whether you can move all the code that uses the mutex into the class, or if you have to expose the mutex), which doesn’t require very large changes to the code, but does make for a better fit with the thread safety analysis. Might also make it easy to later change g_important to not need to be a global too.

    ---

    

    vasild commented at 9:00 am on June 7, 2022:

    Do you envision that we do not have mutexes in global scope, or we have ones that are of type Mutex

    I guess your reply above favors the “we do not have mutexes in global scope” case, right?

    ---

    

    ajtowns commented at 9:54 am on June 8, 2022:

    g_important.m_mut is still technically in global scope. Just works better with clang’s thread safety logic
83. 
    MarcoFalke commented at 2:29 pm on June 10, 2022: member

    Maybe drop the merged commits from the broken GitHub web view?

    0git commit --allow-empty -m empty
    1git push ...
    2git reset --hard HEAD~
    3git push ... --force
    

84. 
    MarcoFalke commented at 2:42 pm on June 10, 2022: member

    Nvm, the merge commit will have the correct commit list.

    review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjrDgwAuuYRw/pMLEzZ0bmQYj83qnnVrTospYbV67FoYgA4pfE9zHVJ6eN25SPP
     8akqhk9h1mhKVESBkBYr/7mrgvJjCqA7+TUBR5RWuQKrvu54JKxMTgEMyJ9nETT4a
     9iQKqbVU8TAcAR84A9nYFgQdwGF4OP1xm4SthwIiAEFw9aNeUPWRMuPQc7OoB+3/i
    10J6yjD6pI+TBLIYCSqK232QLw5kSR+m17GY4lg5akpdZi61o8jJ4ieGJKDZf8G54O
    11XHT2UR0+dJVpkWQFDuseYbXUsVVe8lhXPNwKCbhsvzhVsJueAeq0LVhHfDtydlQC
    12OKA4qQ2hxPr5O2PMJANv52QXyDs8InWj0+0bqOr/MDvtiZd+hdLhv9lS+PLznnLx
    13r4pLcQv8esvPya/erbcM0TCp442MjwMadD1V91eEl3L7VWDQ5gFxozimGncaIL2u
    14h5WW7VchbrYfJVna7KtOP6Hpey3aSfKP6Extba4jnz0mEYnXHut1Kc3S8K0+TwB+
    15PG/M6vik
    16=Pc8D
    17-----END PGP SIGNATURE-----
    

85. MarcoFalke merged this on Jun 10, 2022
86. MarcoFalke closed this on Jun 10, 2022

    ---

87. sidhujag referenced this in commit d8f5a94a6d on Jun 13, 2022
88. DrahtBot locked this on Jun 10, 2023

Contributors
ajtowns jonatack DrahtBot jamesob hebasto MarcoFalke w0xlt vasild

Review Requested
jamesob jonatack

Labels
Refactoring