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

Code before:
    {
        return Base::owns_lock();
    }

protected:
    // needed for reverse_lock
    UniqueLock() = default;

public:
    /**
     * An RAII-style reverse lock. Unlocks on construction and locks on destruction.
     */
    class reverse_lock {
    public:
        explicit reverse_lock(UniqueLock& _lock, const char* _guardname, const char* _file, int _line) : lock(_lock), file(_file), line(_line) {
            CheckLastCritical((void*)lock.mutex(), lockname, _guardname, _file, _line);
            lock.unlock();
            LeaveCritical();
            lock.swap(templock);
        }

        ~reverse_lock() {
            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;
};

#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; }

Review comment (bug):
I was pretty puzzled by the annotations I think in part because of the deprecated "lock/unlock" terminology instead of "acquire/release", but also because the meaning of `UNLOCK_FUNCTION`/`RELEASE` appears to shift from the constructor to the destructor. 

A reasonable explanation of these semantics is provided by their author: https://bugs.llvm.org/show_bug.cgi?id=36162#c7
> ```cpp
> class SCOPED_CAPABILITY AutoUnlock {
> public:
>   // Release mu, implicitly acquire *this and connect it to mu.
>   AutoUnlock(Mutex &mu) RELEASE(mu);
> 
>   // Reacquire all underlying capabilities and warn on double lock.
>   void lock() RELEASE();
> 
>   // Release all underlying capabilities, warn on double unlock.
>   void unlock() ACQUIRE();
> 
>   // Release *this and acquire all underlying capabilities, if they are not held.
>   // There is no warning on double lock.
>   ~AutoUnlock() RELEASE();
> };
>``` 
>
> Note that the destructor says RELEASE(), which seems a bit counter-intuitive. We release the scoped capability, which acquires the underlying capabilities.

More discussion here: https://bugs.llvm.org/show_bug.cgi?id=36162 and here: https://reviews.llvm.org/D52578

----

I wonder how persuaded one should be by the arguments *not* to use this pattern (RAII unlocks/reverse-locks) from [above](https://bugs.llvm.org/show_bug.cgi?id=36162#c3) 

> * Destructor calls are usually hidden behind return/break/continue statements and the regular exit from a block, which is why they are supposed to do only clean-up. Locking as part of cleaning up seems weird. As a consequence, there are potentially a bunch of implicit lock operations that are hard to see.
> * The destructor will also be called on stack unwinding after an exception was thrown. This is an important reason for using AutoLock, but for AutoUnlock it means that we might acquire a mutex when unwinding the stack.
> * Destructors are not supposed to throw exceptions, but locking can (generally) do that. Chromium's mutexes might not, but std::mutex::lock "throws std::system_error when errors occur, including errors from the underlying operating system that would prevent lock from meeting its specifications." [1] Note that std::mutex::unlock is specified as noexcept.

I think in the case of `ReadBlock()` a reverse lock could be avoided by moving locking from `ReadBlock()` to its callers, but, this topic is probably out of scope for this PR.

Code after:
    operator bool()
    {
        return Base::owns_lock();
    }

protected:
    // needed for reverse_lock
    UniqueLock() = default;

public:
    /**
     * An RAII-style reverse lock. Unlocks on construction and locks on destruction.
     */
    class SCOPED_LOCKABLE reverse_lock {
    public:
        explicit reverse_lock(UniqueLock& _lock, const MutexType& mutex, const char* _guardname, const char* _file, int _line) UNLOCK_FUNCTION(mutex) : lock(_lock), file(_file), line(_line) {
            // Ensure that mutex passed back for thread-safety analysis is indeed the original
            assert(std::addressof(mutex) == lock.mutex());

            CheckLastCritical((void*)lock.mutex(), lockname, _guardname, _file, _line);
            lock.unlock();
            LeaveCritical();
            lock.swap(templock);
        }

        ~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