Repository: bitcoin/bitcoin
File: src/sync.h (C/C++)

Code before:
            EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex());
            lock.lock();
        }

     private:
        reverse_lock(reverse_lock const&);
        reverse_lock& operator=(reverse_lock const&);

        UniqueLock& lock;
        UniqueLock templock;
        std::string lockname;
        const std::string file;
        const int line;
     };
     friend class reverse_lock;
};

#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::type::reverse_lock UNIQUE_NAME(revlock)(g, #g, __FILE__, __LINE__)

// When locking a Mutex, require negative capability to ensure the lock
// is not already held
inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }

// When locking a GlobalMutex or RecursiveMutex, just check it is not
// locked in the surrounding scope.
template <typename MutexType>
inline MutexType& MaybeCheckNotHeld(MutexType& m) LOCKS_EXCLUDED(m) LOCK_RETURNED(m) { return m; }
template <typename MutexType>
inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNED(m) { return m; }

#define LOCK(cs) UniqueLock UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)
#define LOCK2(cs1, cs2)                                               \
    UniqueLock criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \
    UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__)
#define LOCK_ARGS(cs) MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__
#define TRY_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs), true)
#define WAIT_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs))

#define ENTER_CRITICAL_SECTION(cs)                            \
    {                                                         \
        EnterCritical(#cs, __FILE__, __LINE__, &cs); \
        (cs).lock();                                          \
    }

#define LEAVE_CRITICAL_SECTION(cs)                                          \
    {                                                                       \
        std::string lockname;                                               \
        CheckLastCritical((void*)(&cs), lockname, #cs, __FILE__, __LINE__); \
        (cs).unlock();                                                      \
        LeaveCritical();                                                    \

Review comment (refactor):
In commit "thread-safety: fix annotations with REVERSE_LOCK" (a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7)

I think there is a minor mistake on this line and `#g` should be `#cs` here.

The only place this string is used is for ["%s was not most recent critical section locked"](https://github.com/bitcoin/bitcoin/blob/a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7/src/sync.cpp#L233-L237) messages inside the [CheckLastCritical call](https://github.com/bitcoin/bitcoin/blob/a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7/src/sync.h#L222) so it seems to be expecting the name of a critical section or mutex object not the name of an RAII lock object.

The string `#cs` is also usually more meaningful because it is something like "cs_main" or "cs_wallet" not just "lock". Also if you look at the [other CheckLastLiteral call](https://github.com/bitcoin/bitcoin/blob/a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7/src/sync.h#L281) it is passing `#cs` not the name of an RAII lock object (because there isn't one).

This would be nice to clean up, but it is just a preexisting inconsistency, so should not be necessary to change in this PR.

Code after:
        }

        ~reverse_lock() UNLOCK_FUNCTION() {
            templock.swap(lock);
            EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex());
            lock.lock();
        }

     private:
        reverse_lock(reverse_lock const&);
        reverse_lock& operator=(reverse_lock const&);

        UniqueLock& lock;
        UniqueLock templock;
        std::string lockname;
        const std::string file;
        const int line;
     };
     friend class reverse_lock;
};

// clang's thread-safety analyzer is unable to deal with aliases of mutexes, so
// it is not possible to use the lock's copy of the mutex for that purpose.
// Instead, the original mutex needs to be passed back to the reverse_lock for
// the sake of thread-safety analysis, but it is not actually used otherwise.
#define REVERSE_LOCK(g, cs) typename std::decay<decltype(g)>::type::reverse_lock UNIQUE_NAME(revlock)(g, cs, #g, __FILE__, __LINE__)

// When locking a Mutex, require negative capability to ensure the lock
// is not already held
inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }

// When locking a GlobalMutex or RecursiveMutex, just check it is not
// locked in the surrounding scope.
template <typename MutexType>
inline MutexType& MaybeCheckNotHeld(MutexType& m) LOCKS_EXCLUDED(m) LOCK_RETURNED(m) { return m; }
template <typename MutexType>
inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNED(m) { return m; }

#define LOCK(cs) UniqueLock UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)
#define LOCK2(cs1, cs2)                                               \
    UniqueLock criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \
    UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__)
#define TRY_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true)
#define WAIT_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__)

#define ENTER_CRITICAL_SECTION(cs)                            \
    {                                                         \
        EnterCritical(#cs, __FILE__, __LINE__, &cs); \
        (cs).lock();                                          \
    }