thread-safety: fix annotations with REVERSE_LOCK #32465

1. 
   theuni commented at 0:49 am on May 10, 2025: member

   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 (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, but either way it makes sense to me to modernize.

   The second commit just fixes an unrelated logging bug that I noticed while hacking around in here.

   The third 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 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. Though the docs don’t describe how to accomplish it, the functionality was added in this commit. 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_LOCKs have been updated. To ensure that the mutexes actually match, a runtime assertion is added.

2. thread-safety: modernize thread safety macros

   Clang added new "capability"-based thread-safety attributes years ago, but the
   old ones remain supported for backwards-compatibility.
   
   However, while adding annotations for our reverse_lock, I noticed that there
   is a difference between the unlock_function and release_capability attributes.
   
   unlock_function actually maps to release_generic_capability, which does not
   work properly when implementing a scoped unlocker.
   
   To be consistent, the other capability-based attributes are updated here as
   well. To avoid having to update our macro usage throughout the codebase, I
   reused our existing ones.
   
   Additionally, SHARED_UNLOCK_FUNCTION is added here, as a subsequent PR will
   introduce annotations for shared_mutex and shared_lock.

   832c57a534
3. thread-safety: add missing lock annotation

   No warning is currently emitted because our reverse_lock does not enforce our
   thread-safety annotations. Once it is fixed, the unlock would cause a warning.

   aeea5f0ec1
4. thread-safety: add missing lockname for reverse_lock

   Noticed while adding annotations to reverse_lock.

   0065b9673d
5. 
   DrahtBot commented at 0:49 am on May 10, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32465.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan
   Concept ACK	hebasto

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

6. thread-safety: fix annotations with REVERSE_LOCK

   Without proper annotations, clang thinks that mutexes are still held for the
   duration of a reverse_lock. This could lead to subtle bugs as
   EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't.
   
   As mentioned in the docs [0], 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.
   
   [0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

   96a341d406
7. theuni force-pushed on May 10, 2025
8. 
   hebasto commented at 7:43 am on May 10, 2025: member

   Concept ACK.
9. laanwj added the label Utils/log/libs on May 10, 2025
10. laanwj added the label Tests on May 10, 2025
11. in src/threadsafety.h:19 in 832c57a534 outdated

    14@@ -15,23 +15,24 @@
    15 // See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
    16 // for documentation.  The clang compiler can do advanced static analysis
    17 // of locking when given the -Wthread-safety option.
    18-#define LOCKABLE __attribute__((lockable))
    19+#define LOCKABLE __attribute__((capability("")))
    20 #define SCOPED_LOCKABLE __attribute__((scoped_lockable))
    

    ---

    ---

    

    TheCharlatan commented at 11:10 am on May 10, 2025:

    This threw me off for a bit, but while some attributes have been renamed, some have been kept the same, and their macros renamed instead. See: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h
12. TheCharlatan approved
13. 
    TheCharlatan commented at 12:07 pm on May 10, 2025: contributor

    ACK 96a341d4064132292621af404de908f01fbe3e2f
14. DrahtBot requested review from hebasto on May 10, 2025

Contributors
theuni DrahtBot hebasto TheCharlatan

Review Requested
hebasto

Labels
Tests Utils/log/libs