Thread Safety Analysis: Support guarded_by/pt_guarded_by with multiple capabilities (llvm#186838)

Previously, `guarded_by` and `pt_guarded_by` only accepted a single
capability argument. Introduce support for declaring that a variable is
guarded by multiple capabilities, which exploits the following property:
any writer must hold all capabilities, so holding any one of them
(exclusive or shared) guarantees at least shared (read) access.
Therefore, writing requires all listed capabilities to be held
exclusively, while reading only requires at least one to be held.

This synchronization pattern is frequently used where the underlying
lock implementation does not support real reader locking, and instead
several lock "shards" are used to reduce contention for readers. For
example, the Linux kernel makes frequent use of this pattern [1].

Backwards compatibility is not affected by this change: for the time
being we deliberately do not change the semantics of multiple stacked
attributes (this retains existing semantics precisely, while giving a
way to choose the "stricter" semantics if needed).

Previous Version: llvm#185173
Link: https://lore.kernel.org/all/20250307085204.GJ16878@noisy.programming.kicks-ass.net/ [1]

Author: melver

clang/docs/ReleaseNotes.rst

@@ -221,6 +221,13 @@ Attribute Changes in Clang
   foreign language personality with a given function. Note that this does not
   perform any ABI validation for the personality routine.
 
+- The :doc:`ThreadSafetyAnalysis` attributes ``guarded_by`` and
+  ``pt_guarded_by`` now accept multiple capability arguments with refined
+  access semantics: *writing* requires all listed capabilities to be held
+  exclusively, while *reading* requires at least one to be held.  This is
+  sound because any writer must hold all capabilities, so holding any one
+  prevents concurrent writes.
+
 Improvements to Clang's diagnostics
 -----------------------------------
 - Added ``-Wlifetime-safety`` to enable lifetime safety analysis,

clang/docs/ThreadSafetyAnalysis.rst

@@ -153,16 +153,20 @@ general capability model.  The prior names are still in use, and will be
 mentioned under the tag *previously* where appropriate.
 
 
-GUARDED_BY(c) and PT_GUARDED_BY(c)
-----------------------------------
+GUARDED_BY(...) and PT_GUARDED_BY(...)
+--------------------------------------
 
 ``GUARDED_BY`` is an attribute on data members, which declares that the data
 member is protected by the given capability.  Read operations on the data
 require shared access, while write operations require exclusive access.
 
+Multiple capabilities may be specified, subject to the following rules:
+a writer must hold *all* listed capabilities exclusively, so holding *any one*
+of them is sufficient to guarantee at least shared (read) access.
+
 ``PT_GUARDED_BY`` is similar, but is intended for use on pointers and smart
 pointers. There is no constraint on the data member itself, but the *data that
-it points to* is protected by the given capability.
+it points to* is protected by the given capabilities.
 
 .. code-block:: c++
 
@@ -181,6 +185,25 @@ it points to* is protected by the given capability.
     p3.reset(new int);  // OK.
   }
 
+When multiple capabilities are listed:
+
+* **Write** access requires all listed capabilities to be held exclusively.
+* **Read** access requires at least one of them to be held (shared or exclusive).
+
+.. code-block:: c++
+
+  Mutex mu1, mu2;
+  int a GUARDED_BY(mu1, mu2);
+
+  void reader() REQUIRES_SHARED(mu1) {
+    int x = a;   // OK: at least one capability is held.
+    a = 0;       // Warning!  Writing requires both mu1 and mu2.
+  }
+
+  void writer() REQUIRES(mu1, mu2) {
+    a = 0;       // OK: both capabilities are held exclusively.
+  }
+
 
 REQUIRES(...), REQUIRES_SHARED(...)
 -----------------------------------
@@ -860,11 +883,11 @@ implementation.
   #define SCOPED_CAPABILITY \
     THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
 
-  #define GUARDED_BY(x) \
-    THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
+  #define GUARDED_BY(...) \
+    THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(__VA_ARGS__))
 
-  #define PT_GUARDED_BY(x) \
-    THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))
+  #define PT_GUARDED_BY(...) \
+    THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(__VA_ARGS__))
 
   #define ACQUIRED_BEFORE(...) \
     THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__))

clang/include/clang/Analysis/Analyses/ThreadSafety.h

@@ -19,6 +19,7 @@
 #define LLVM_CLANG_ANALYSIS_ANALYSES_THREADSAFETY_H
 
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
@@ -193,6 +194,17 @@ class ThreadSafetyHandler {
   virtual void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,
                                  AccessKind AK, SourceLocation Loc) {}
 
+  /// Warn when a read of a multi-capability guarded_by variable occurs while
+  /// none of the listed capabilities are held.
+  /// \param D -- The decl for the protected variable
+  /// \param POK -- The kind of protected operation (e.g. variable access)
+  /// \param LockNames -- Names of the capabilities that were not held
+  /// \param Loc -- The location of the read
+  virtual void handleGuardedByAnyReadNotHeld(const NamedDecl *D,
+                                             ProtectedOperationKind POK,
+                                             ArrayRef<StringRef> LockNames,
+                                             SourceLocation Loc) {}
+
   /// Warn when a protected operation occurs while the specific mutex protecting
   /// the operation is not locked.
   /// \param Kind -- the capability's name parameter (role, mutex, etc).

clang/include/clang/Basic/Attr.td

@@ -4155,7 +4155,7 @@ def NoThreadSafetyAnalysis : InheritableAttr {
 
 def GuardedBy : InheritableAttr {
   let Spellings = [GNU<"guarded_by">];
-  let Args = [ExprArgument<"Arg">];
+  let Args = [VariadicExprArgument<"Args">];
   let LateParsed = LateAttrParseExperimentalExt;
   let TemplateDependent = 1;
   let ParseArgumentsAsUnevaluated = 1;
@@ -4166,7 +4166,7 @@ def GuardedBy : InheritableAttr {
 
 def PtGuardedBy : InheritableAttr {
   let Spellings = [GNU<"pt_guarded_by">];
-  let Args = [ExprArgument<"Arg">];
+  let Args = [VariadicExprArgument<"Args">];
   let LateParsed = LateAttrParseExperimentalExt;
   let TemplateDependent = 1;
   let ParseArgumentsAsUnevaluated = 1;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -4327,6 +4327,10 @@ def warn_var_deref_requires_any_lock : Warning<
   "%select{reading|writing}1 the value pointed to by %0 requires holding "
   "%select{any mutex|any mutex exclusively}1">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
+def warn_requires_any_of_locks : Warning<
+  "reading %select{variable|the value pointed to by}1 %0 requires holding "
+  "at least one of %2">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def warn_fun_excludes_mutex : Warning<
   "cannot call function '%1' while %0 '%2' is held">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;

clang/lib/AST/ASTImporter.cpp

@@ -9749,12 +9749,16 @@ Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) {
   }
   case attr::GuardedBy: {
     const auto *From = cast<GuardedByAttr>(FromAttr);
-    AI.importAttr(From, AI.importArg(From->getArg()).value());
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
+                  From->args_size());
     break;
   }
   case attr::PtGuardedBy: {
     const auto *From = cast<PtGuardedByAttr>(FromAttr);
-    AI.importAttr(From, AI.importArg(From->getArg()).value());
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
+                  From->args_size());
     break;
   }
   case attr::AcquiredAfter: {

clang/lib/Analysis/ThreadSafety.cpp

@@ -1279,6 +1279,11 @@ class ThreadSafetyAnalyzer {
                           const Expr *Exp, AccessKind AK, Expr *MutexExp,
                           ProtectedOperationKind POK, til::SExpr *Self,
                           SourceLocation Loc);
+  void warnIfAnyMutexNotHeldForRead(const FactSet &FSet, const NamedDecl *D,
+                                    const Expr *Exp,
+                                    llvm::ArrayRef<Expr *> Args,
+                                    ProtectedOperationKind POK,
+                                    SourceLocation Loc);
   void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp,
                        Expr *MutexExp, til::SExpr *Self, SourceLocation Loc);
 
@@ -1848,6 +1853,38 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
   }
 }
 
+void ThreadSafetyAnalyzer::warnIfAnyMutexNotHeldForRead(
+    const FactSet &FSet, const NamedDecl *D, const Expr *Exp,
+    llvm::ArrayRef<Expr *> Args, ProtectedOperationKind POK,
+    SourceLocation Loc) {
+  SmallVector<CapabilityExpr, 2> Caps;
+  for (auto *Arg : Args) {
+    CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, nullptr);
+    if (Cp.isInvalid()) {
+      warnInvalidLock(Handler, Arg, D, Exp, Cp.getKind());
+      continue;
+    }
+    if (Cp.shouldIgnore())
+      continue;
+    const FactEntry *LDat = FSet.findLockUniv(FactMan, Cp);
+    if (LDat && LDat->isAtLeast(LK_Shared))
+      return; // At least one held — read access is safe.
+    // FIXME: try findPartialMatch as a fallback to support
+    //        -Wno-thread-safety-precise, as warnIfMutexNotHeld does.
+    Caps.push_back(Cp);
+  }
+  if (Caps.empty())
+    return;
+  // Materialize names only now that we know we are going to warn.
+  SmallVector<std::string, 2> NameStorage;
+  SmallVector<StringRef, 2> Names;
+  for (const auto &Cp : Caps) {
+    NameStorage.push_back(Cp.toString());
+    Names.push_back(NameStorage.back());
+  }
+  Handler.handleGuardedByAnyReadNotHeld(D, POK, Names, Loc);
+}
+
 /// Warn if the LSet contains the given lock.
 void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
                                            const NamedDecl *D, const Expr *Exp,
@@ -1934,8 +1971,18 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
     Handler.handleNoMutexHeld(D, POK, AK, Loc);
   }
 
-  for (const auto *I : D->specific_attrs<GuardedByAttr>())
-    warnIfMutexNotHeld(FSet, D, Exp, AK, I->getArg(), POK, nullptr, Loc);
+  for (const auto *I : D->specific_attrs<GuardedByAttr>()) {
+    if (AK == AK_Written || I->args_size() == 1) {
+      // Write requires all capabilities; single-arg read uses the normal
+      // per-lock warning path.
+      for (auto *Arg : I->args())
+        warnIfMutexNotHeld(FSet, D, Exp, AK, Arg, POK, nullptr, Loc);
+    } else {
+      // Multi-arg read: holding any one of the listed capabilities is
+      // sufficient (a writer must hold all, so any one prevents writes).
+      warnIfAnyMutexNotHeldForRead(FSet, D, Exp, I->args(), POK, Loc);
+    }
+  }
 }
 
 /// Checks pt_guarded_by and pt_guarded_var attributes.
@@ -1998,9 +2045,20 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
   if (D->hasAttr<PtGuardedVarAttr>() && FSet.isEmpty(FactMan))
     Handler.handleNoMutexHeld(D, PtPOK, AK, Exp->getExprLoc());
 
-  for (auto const *I : D->specific_attrs<PtGuardedByAttr>())
-    warnIfMutexNotHeld(FSet, D, Exp, AK, I->getArg(), PtPOK, nullptr,
-                       Exp->getExprLoc());
+  for (auto const *I : D->specific_attrs<PtGuardedByAttr>()) {
+    if (AK == AK_Written || I->args_size() == 1) {
+      // Write requires all capabilities; single-arg read uses the normal
+      // per-lock warning path.
+      for (auto *Arg : I->args())
+        warnIfMutexNotHeld(FSet, D, Exp, AK, Arg, PtPOK, nullptr,
+                           Exp->getExprLoc());
+    } else {
+      // Multi-arg read: holding any one of the listed capabilities is
+      // sufficient (a writer must hold all, so any one prevents writes).
+      warnIfAnyMutexNotHeldForRead(FSet, D, Exp, I->args(), PtPOK,
+                                   Exp->getExprLoc());
+    }
+  }
 }
 
 /// Process a function call, method call, constructor call,

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -55,6 +55,7 @@
 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/TimeProfiler.h"
@@ -2141,6 +2142,39 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
     Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
+  void handleGuardedByAnyReadNotHeld(const NamedDecl *D,
+                                     ProtectedOperationKind POK,
+                                     ArrayRef<StringRef> LockNames,
+                                     SourceLocation Loc) override {
+    bool IsDeref;
+    switch (POK) {
+    case POK_VarAccess:
+    case POK_PassByRef:
+    case POK_ReturnByRef:
+    case POK_PassPointer:
+    case POK_ReturnPointer:
+      IsDeref = false;
+      break;
+    case POK_VarDereference:
+    case POK_PtPassByRef:
+    case POK_PtReturnByRef:
+    case POK_PtPassPointer:
+    case POK_PtReturnPointer:
+      IsDeref = true;
+      break;
+    case POK_FunctionCall:
+      llvm_unreachable("POK_FunctionCall not applicable here");
+    }
+    std::string Quoted;
+    llvm::raw_string_ostream OS(Quoted);
+    llvm::ListSeparator LS;
+    for (StringRef Name : LockNames)
+      OS << LS << "'" << Name << "'";
+    PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_requires_any_of_locks)
+                                         << D << IsDeref << Quoted);
+    Warnings.emplace_back(std::move(Warning), getNotes());
+  }
+
   void handleMutexNotHeld(StringRef Kind, const NamedDecl *D,
                           ProtectedOperationKind POK, Name LockName,
                           LockKind LK, SourceLocation Loc,

clang/lib/Sema/SemaDeclAttr.cpp

@@ -460,36 +460,33 @@ static void handlePtGuardedVarAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 }
 
 static bool checkGuardedByAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
-                                     Expr *&Arg) {
-  SmallVector<Expr *, 1> Args;
-  // check that all arguments are lockable objects
-  checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
-  unsigned Size = Args.size();
-  if (Size != 1)
+                                     SmallVectorImpl<Expr *> &Args) {
+  if (!AL.checkAtLeastNumArgs(S, 1))
     return false;
 
-  Arg = Args[0];
-
-  return true;
+  checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
+  return !Args.empty();
 }
 
 static void handleGuardedByAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  Expr *Arg = nullptr;
-  if (!checkGuardedByAttrCommon(S, D, AL, Arg))
+  SmallVector<Expr *, 1> Args;
+  if (!checkGuardedByAttrCommon(S, D, AL, Args))
     return;
 
-  D->addAttr(::new (S.Context) GuardedByAttr(S.Context, AL, Arg));
+  D->addAttr(::new (S.Context)
+                 GuardedByAttr(S.Context, AL, Args.data(), Args.size()));
 }
 
 static void handlePtGuardedByAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  Expr *Arg = nullptr;
-  if (!checkGuardedByAttrCommon(S, D, AL, Arg))
+  SmallVector<Expr *, 1> Args;
+  if (!checkGuardedByAttrCommon(S, D, AL, Args))
     return;
 
   if (!threadSafetyCheckIsPointer(S, D, AL))
     return;
 
-  D->addAttr(::new (S.Context) PtGuardedByAttr(S.Context, AL, Arg));
+  D->addAttr(::new (S.Context)
+                 PtGuardedByAttr(S.Context, AL, Args.data(), Args.size()));
 }
 
 static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,

clang/lib/Sema/SemaDeclCXX.cpp

@@ -19476,9 +19476,9 @@ bool Sema::checkThisInStaticMemberFunctionAttributes(CXXMethodDecl *Method) {
     Expr *Arg = nullptr;
     ArrayRef<Expr *> Args;
     if (const auto *G = dyn_cast<GuardedByAttr>(A))
-      Arg = G->getArg();
+      Args = llvm::ArrayRef(G->args_begin(), G->args_size());
     else if (const auto *G = dyn_cast<PtGuardedByAttr>(A))
-      Arg = G->getArg();
+      Args = llvm::ArrayRef(G->args_begin(), G->args_size());
     else if (const auto *AA = dyn_cast<AcquiredAfterAttr>(A))
       Args = llvm::ArrayRef(AA->args_begin(), AA->args_size());
     else if (const auto *AB = dyn_cast<AcquiredBeforeAttr>(A))