[scudo] Add missing thread-safety analysis annotations.

[legrosbuffle]

This avoids new warnings from -Wthread-safety after #67776, see #67795.

Note that this are not really useful since thread safety analysis is disabled anyway here: llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Changes

This avoids new warnings from -Wthread-safety after #67776, see #67795.

Note that this are not really useful since thread safety analysis is disabled anyway here: llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172

---

Full diff: https://github.com/llvm/llvm-project/pull/68072.diff

1 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/tsd.h (+3-3)

diff --git a/compiler-rt/lib/scudo/standalone/tsd.h b/compiler-rt/lib/scudo/standalone/tsd.h
index f4fa545de5e0468..d41142a2cf014e6 100644
--- a/compiler-rt/lib/scudo/standalone/tsd.h
+++ b/compiler-rt/lib/scudo/standalone/tsd.h
@@ -53,7 +53,7 @@ template <class Allocator> struct alignas(SCUDO_CACHE_LINE_SIZE) TSD {
   inline void unlock() NO_THREAD_SAFETY_ANALYSIS { Mutex.unlock(); }
   inline uptr getPrecedence() { return atomic_load_relaxed(&Precedence); }
 
-  void commitBack(Allocator *Instance) ASSERT_CAPABILITY(Mutex) {
+  void commitBack(Allocator *Instance) ASSERT_CAPABILITY(Mutex) REQUIRES(Mutex) {
     Instance->commitBack(this);
   }
 
@@ -66,11 +66,11 @@ template <class Allocator> struct alignas(SCUDO_CACHE_LINE_SIZE) TSD {
   // TODO(chiahungduan): Ideally, we want to do `Mutex.assertHeld` but acquiring
   // TSD doesn't always require holding the lock. Add this assertion while the
   // lock is always acquired.
-  typename Allocator::CacheT &getCache() ASSERT_CAPABILITY(Mutex) {
+  typename Allocator::CacheT &getCache() ASSERT_CAPABILITY(Mutex) REQUIRES(Mutex) {
     return Cache;
   }
   typename Allocator::QuarantineCacheT &getQuarantineCache()
-      ASSERT_CAPABILITY(Mutex) {
+      ASSERT_CAPABILITY(Mutex) REQUIRES(Mutex) {
     return QuarantineCache;
   }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ChiaHungDuan]

One quick question, why do we need to annotate REQUIRES capability here if we already mark it as ASSERT_CAPABILITY?

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability

[legrosbuffle]

One quick question, why do we need to annotate REQUIRES capability here if we already mark it as ASSERT_CAPABILITY?

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability

My understanding is that ASSERT_CAPABILITY is a purely dynamic check, and does not imply REQUIRES. Adding @aaronpuchert for confirmation.

[ChiaHungDuan]

One quick question, why do we need to annotate REQUIRES capability here if we already mark it as ASSERT_CAPABILITY?
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability

My understanding is that ASSERT_CAPABILITY is a purely dynamic check, and does not imply REQUIRES. Adding @aaronpuchert for confirmation.

Just read the description again and I found that I misunderstood its meaning.

... Presence of this annotation causes the analysis to assume the capability is held after calls to the annotated function ...

I thought it asserted before the calling of the function.

I have prepared a fix #68273 and verified it with #67776. If you would like to submit a workaround first, I would suggest marking them as NO_THREAD_SAFETY_ANALYSIS and I will submit the fix later soon. Let me know which way you prefer

Thanks!

[aaronpuchert]

From the looks of it, ASSERT_CAPABILITY should be dropped in all of these functions. It should perhaps have been called ASSERTS_CAPABILITY to avoid misunderstanding.

My understanding is that ASSERT_CAPABILITY is a purely dynamic check, and does not imply REQUIRES. Adding @aaronpuchert for confirmation.

Correct, it's for cases where we can't statically prove that the lock is held, so you can add a runtime check as “escape hatch“. The compiler will assume that the lock is held starting from the call to an assert-annotated function.

Just read the description again and I found that I misunderstood its meaning.

... Presence of this annotation causes the analysis to assume the capability is held after calls to the annotated function ...

The documentation is tricky, we already had some discussions in D87629.

I thought it asserted before the calling of the function.

Either way, we currently consider the attribute only when such a function is called, not within its own implementation. That's because we can't generally see assertions (e.g. with -DNDEBUG), and generally such a function doesn't do more than asserting.

[ChiaHungDuan]

Yes, we have removed those misused ASSERT_CAPABILITY in Scudo. There are other restriction s in Scudo's code structure. I'm thinking to help with some known thread-safety analysis limitations too. It's off topic here anyway.

Thanks for the explanation!

Chia-hung

[legrosbuffle]

Closing this as the issue has been fixed by the code owners.