Merge bitcoin/bitcoin#32465: thread-safety: fix annotations with REVERSE_LOCK

a201a99f8c thread-safety: fix annotations with REVERSE_LOCK (Cory Fields)
aeea5f0ec1 thread-safety: add missing lock annotation (Cory Fields)
832c57a534 thread-safety: modernize thread safety macros (Cory Fields)

Pull request description:

  This is one of several PRs to cleanup/modernize our threading primitives.

  While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly.

  Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference between the old `unlock_function` and new `release_capability`, where our `reverse_lock` only works with the latter. I believe this is an upstream bug. I've [reported and attempted a fix here](https://github.com/llvm/llvm-project/pull/139343), but either way it makes sense to me to modernize.

  The second adds a missing annotation pointed out by a fixed `REVERSE_LOCK`. Because clang's thread-safety annotations aren't passed through a reference to `UniqueLock` as one may assume (see [here](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis) for more details), `cs_main` has to be listed explicitly as a requirement.

  The last commit actually fixes the `reverse_lock` by making it a `SCOPED_LOCK` and using the pattern [found in a clang test](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3126). Though the docs don't describe how to accomplish it, the functionality was added [in this commit](https://github.com/llvm/llvm-project/commit/6a68efc959bd1024506bf661a9e155de8a8440da). Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing `REVERSE_LOCK`s have been updated. To ensure that the mutexes actually match, a runtime assertion is added.

ACKs for top commit:
  fjahr:
    re-ACK a201a99f8c
  davidgumberg:
    reACK https://github.com/bitcoin/bitcoin/commit/a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7
  theuni:
    Ok, done. Those last pushes can be ignored. ACKs on a201a99 are still fresh.
  ryanofsky:
    Code review ACK a201a99f8c. Just dropping 0065b9673db5da2994b0b07c1d50ebfb19af39d0 and fixing incorrect `reverse_lock::lockname` initialization since last review.
  TheCharlatan:
    Re-ACK a201a99f8c

Tree-SHA512: 2755fae0c41021976a1a633014a86d927f104ccbc8014c01c06dae89af363f92e5bc5d4276ad6d759302ac4679fe02a543758124d48318074db1c370989af7a7

src/node/interfaces.cpp

@@ -431,7 +431,7 @@ public:
 };
 
 // NOLINTNEXTLINE(misc-no-recursion)
-bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active, const BlockManager& blockman)
+bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active, const BlockManager& blockman) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     if (!index) return false;
     if (block.m_hash) *block.m_hash = index->GetBlockHash();
@@ -443,7 +443,7 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
     if (block.m_locator) { *block.m_locator = GetLocator(index); }
     if (block.m_next_block) FillBlock(active[index->nHeight] == index ? active[index->nHeight + 1] : nullptr, *block.m_next_block, lock, active, blockman);
     if (block.m_data) {
-        REVERSE_LOCK(lock);
+        REVERSE_LOCK(lock, cs_main);
         if (!blockman.ReadBlock(*block.m_data, *index)) block.m_data->SetNull();
     }
     block.found = true;

src/scheduler.cpp

@@ -56,7 +56,7 @@ void CScheduler::serviceQueue()
             {
                 // Unlock before calling f, so it can reschedule itself or another task
                 // without deadlocking:
-                REVERSE_LOCK(lock);
+                REVERSE_LOCK(lock, newTaskMutex);
                 f();
             }
         } catch (...) {

src/sync.h

@@ -14,6 +14,7 @@
 #include <threadsafety.h> // IWYU pragma: export
 #include <util/macros.h>
 
+#include <cassert>
 #include <condition_variable>
 #include <mutex>
 #include <string>
@@ -212,16 +213,19 @@ public:
     /**
      * An RAII-style reverse lock. Unlocks on construction and locks on destruction.
      */
-    class reverse_lock {
+    class SCOPED_LOCKABLE reverse_lock {
     public:
-        explicit reverse_lock(UniqueLock& _lock, const char* _guardname, const char* _file, int _line) : lock(_lock), file(_file), line(_line) {
+        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() {
+        ~reverse_lock() UNLOCK_FUNCTION() {
             templock.swap(lock);
             EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex());
             lock.lock();
@@ -240,7 +244,11 @@ public:
      friend class reverse_lock;
 };
 
-#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::type::reverse_lock UNIQUE_NAME(revlock)(g, #g, __FILE__, __LINE__)
+// 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

src/test/reverselock_tests.cpp

@@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(reverselock_basics)
 
     BOOST_CHECK(lock.owns_lock());
     {
-        REVERSE_LOCK(lock);
+        REVERSE_LOCK(lock, mutex);
         BOOST_CHECK(!lock.owns_lock());
     }
     BOOST_CHECK(lock.owns_lock());
@@ -33,9 +33,9 @@ BOOST_AUTO_TEST_CASE(reverselock_multiple)
 
     // Make sure undoing two locks succeeds
     {
-        REVERSE_LOCK(lock);
+        REVERSE_LOCK(lock, mutex);
         BOOST_CHECK(!lock.owns_lock());
-        REVERSE_LOCK(lock2);
+        REVERSE_LOCK(lock2, mutex2);
         BOOST_CHECK(!lock2.owns_lock());
     }
     BOOST_CHECK(lock.owns_lock());
@@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
     g_debug_lockorder_abort = false;
 
     // Make sure trying to reverse lock a previous lock fails
-    BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2), std::logic_error, HasReason("lock2 was not most recent critical section locked"));
+    BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2, mutex2), std::logic_error, HasReason("lock2 was not most recent critical section locked"));
     BOOST_CHECK(lock2.owns_lock());
 
     g_debug_lockorder_abort = prev;
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
 
     bool failed = false;
     try {
-        REVERSE_LOCK(lock);
+        REVERSE_LOCK(lock, mutex);
     } catch(...) {
         failed = true;
     }
@@ -82,7 +82,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
     lock.lock();
     BOOST_CHECK(lock.owns_lock());
     {
-        REVERSE_LOCK(lock);
+        REVERSE_LOCK(lock, mutex);
         BOOST_CHECK(!lock.owns_lock());
     }
 

src/threadsafety.h

@@ -15,23 +15,24 @@
 // See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
 // for documentation.  The clang compiler can do advanced static analysis
 // of locking when given the -Wthread-safety option.
-#define LOCKABLE __attribute__((lockable))
+#define LOCKABLE __attribute__((capability("")))
 #define SCOPED_LOCKABLE __attribute__((scoped_lockable))
 #define GUARDED_BY(x) __attribute__((guarded_by(x)))
 #define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
 #define ACQUIRED_AFTER(...) __attribute__((acquired_after(__VA_ARGS__)))
 #define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
-#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
+#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((acquire_capability(__VA_ARGS__)))
-#define SHARED_LOCK_FUNCTION(...) __attribute__((shared_lock_function(__VA_ARGS__)))
+#define SHARED_LOCK_FUNCTION(...) __attribute__((acquire_shared_capability(__VA_ARGS__)))
-#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((exclusive_trylock_function(__VA_ARGS__)))
+#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_capability(__VA_ARGS__)))
-#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((shared_trylock_function(__VA_ARGS__)))
+#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_shared_capability(__VA_ARGS__)))
-#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
+#define UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__)))
+#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__)))
 #define LOCK_RETURNED(x) __attribute__((lock_returned(x)))
 #define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
-#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
+#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((requires_capability(__VA_ARGS__)))
-#define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__)))
+#define SHARED_LOCKS_REQUIRED(...) __attribute__((requires_shared_capability(__VA_ARGS__)))
 #define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
-#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_exclusive_lock(__VA_ARGS__)))
+#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_capability(__VA_ARGS__)))
 #else
 #define LOCKABLE
 #define SCOPED_LOCKABLE
@@ -44,6 +45,7 @@
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...)
 #define SHARED_TRYLOCK_FUNCTION(...)
 #define UNLOCK_FUNCTION(...)
+#define SHARED_UNLOCK_FUNCTION(...)
 #define LOCK_RETURNED(x)
 #define LOCKS_EXCLUDED(...)
 #define EXCLUSIVE_LOCKS_REQUIRED(...)

src/validationinterface.cpp

@@ -83,7 +83,7 @@ public:
         for (auto it = m_list.begin(); it != m_list.end();) {
             ++it->count;
             {
-                REVERSE_LOCK(lock);
+                REVERSE_LOCK(lock, m_mutex);
                 f(*it->callbacks);
             }
             it = --it->count ? std::next(it) : m_list.erase(it);