refactor: Remove negative lock annotations from globals #21598

1. 
   MarcoFalke commented at 5:59 PM on April 4, 2021: member

   They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional.

2. DrahtBot added the label P2P on Apr 4, 2021
3. DrahtBot added the label Refactoring on Apr 4, 2021
4. DrahtBot added the label Wallet on Apr 4, 2021
5. MarcoFalke removed the label P2P on Apr 4, 2021
6. MarcoFalke removed the label Wallet on Apr 4, 2021
7. 
   hebasto commented at 11:36 PM on April 4, 2021: member

   Concept ACK (in the light of #20272 discussion).

8. fanquake deleted a comment on Apr 4, 2021
9. 
   hebasto commented at 1:52 AM on April 5, 2021: member

   Tested faadb1b050dd37ea8d294def554f1da22029d3e6 on Ubuntu 21.10:

   $ clang --version
   Ubuntu clang version 12.0.0-++rc3-4ubuntu1
   Target: x86_64-pc-linux-gnu
   Thread model: posix
   InstalledDir: /usr/bin
   

   With this PR massive TSA warnings are gone.

   Could the global namespace be explicitly used for global mutexes in the touched lines?

10. hebasto changes_requested
11. 
    hebasto commented at 3:04 AM on April 5, 2021: member

    May I suggest to:

    • add some missed annotations:

    diff --git a/src/index/base.h b/src/index/base.h
    index 8559e3cb6..ac3c429a5 100644
    --- a/src/index/base.h
    +++ b/src/index/base.h
    @@ -109,7 +109,7 @@ public:
         /// sync once and only needs to process blocks in the ValidationInterface
         /// queue. If the index is catching up from far behind, this method does
         /// not block and immediately returns false.
    -    bool BlockUntilSyncedToCurrentChain() const;
    +    bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main);
     
         void Interrupt();
     
    diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
    index 7932bd291..3a650923c 100644
    --- a/src/rpc/rawtransaction.cpp
    +++ b/src/rpc/rawtransaction.cpp
    @@ -815,7 +815,7 @@ static RPCHelpMan signrawtransactionwithkey()
         };
     }
     
    -static RPCHelpMan sendrawtransaction()
    +static RPCHelpMan sendrawtransaction() LOCKS_EXCLUDED(::cs_main)
     {
         return RPCHelpMan{"sendrawtransaction",
                     "\nSubmit a raw transaction (serialized, hex-encoded) to local node and network.\n"
    

    • fix EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) example in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

    ?

12. 
    hebasto commented at 3:12 AM on April 5, 2021: member

    This change reverts the @ajtowns's idea for AssertLockNotHeld but I cannot see the way to save it, unfortunately.

13. 
    ajtowns commented at 6:33 AM on April 5, 2021: member

    Approach ACK, +1 on doc change as well

14. refactor: Remove negative lock annotations from globals fa5eabe721
15. MarcoFalke force-pushed on Apr 5, 2021
16. 
    MarcoFalke commented at 6:43 AM on April 5, 2021: member

    This change reverts

    Correct, but the revert is only temporary (until cs_main is removed or a private member)

17. 
    MarcoFalke commented at 6:43 AM on April 5, 2021: member

    Force pushed to address feedback

18. hebasto approved
19. 
    hebasto commented at 6:49 AM on April 5, 2021: member

    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

20. vasild approved
21. 
    vasild commented at 2:55 PM on April 5, 2021: member

    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

    No warnings with Clang 13.

22. 
    sipa commented at 10:11 PM on April 5, 2021: member

    utACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

23. fanquake requested review from ajtowns on Apr 6, 2021
24. 
    ajtowns commented at 5:52 AM on April 6, 2021: member

    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

25. MarcoFalke merged this on Apr 6, 2021
26. MarcoFalke closed this on Apr 6, 2021

    ---

27. MarcoFalke deleted the branch on Apr 6, 2021
28. sidhujag referenced this in commit 9dfecc6d14 on Apr 6, 2021
29. Fabcien referenced this in commit 184dbf064f on Sep 23, 2021
30. 
    vasild commented at 11:55 AM on June 7, 2022: member

    This silenced the compiler warnings at the time, so it was ok. I just wonder about the following reasoning, in the light of #24931:

    Remove negative lock annotations from globals ... They only make sense for mutexes that are private members.

    Why is that? For example:

    Mutex g_mutex;
    
    void f1()
    {
        LOCK(g_mutex);
    }
    
    void f2()
    {
        LOCK(g_mutex);
        f1();
    }
    

    This is undefined behavior that would be detected if f1() is annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex).

    To me it looks like that negative lock annotations "make sense" regardless of the scope of the mutex, no?

31. 
    MarcoFalke commented at 12:11 PM on June 7, 2022: member

    This pull was done in light of #20272 (review) . If #24931 fixes the mentioned issues, then that is fine.

32. 
    ajtowns commented at 2:21 AM on June 13, 2022: member

    @vasild The problem is when you call f1 -- the negative annotation needs to keep getting propogated up the call stack:

    
    Mutex g_mutex;
    void f1() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
    {
        LOCK(g_mutex);
    }
    
    class X {
    private:
        Mutex m_mut;
    public:
        void f1() EXCLUSIVE_LOCKS_REQUIRED(!m_mut)
        {
            LOCK(m_mut);
        }
    };
    X g_x;
    
    void caller() EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)
    {
        f1();
        g_x.f1();
    }
    

33. 
    vasild commented at 1:30 PM on June 13, 2022: member

    Yes, it has to be propagated, but I wouldn't call it a "problem" though because it works exactly as intended. True that it creates inconvenience with code isolation (like this code should not know about that mutex), but after all a global mutex is not isolated, so IMO it is ok and even desirable to propagate its negative annotation.

34. DrahtBot locked this on Jun 13, 2023

Review Requested
ajtowns