[clang] Thread Safety Analysis: Allow guarded_by/pt_guarded_by to take multiple capabilities (PR #185172)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Mar 7 03:52:03 PST 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: Marco Elver (melver)
<details>
<summary>Changes</summary>
Previously, GUARDED_BY and PT_GUARDED_BY only accepted a single
capability argument. This meant that requiring multiple locks to protect
a variable required chaining separate attributes:
int x GUARDED_BY(mu1) GUARDED_BY(mu2);
Allow listing all required capabilities in a single attribute, consistent with
other thread safety attributes:
int x GUARDED_BY(mu1, mu2);
Both forms remain supported and are semantically equivalent.
---
Full diff: https://github.com/llvm/llvm-project/pull/185172.diff
12 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+6)
- (modified) clang/docs/ThreadSafetyAnalysis.rst (+31-7)
- (modified) clang/include/clang/Basic/Attr.td (+2-2)
- (modified) clang/lib/AST/ASTImporter.cpp (+6-2)
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+5-3)
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+12-15)
- (modified) clang/lib/Sema/SemaDeclCXX.cpp (+2-2)
- (modified) clang/test/Sema/warn-thread-safety-analysis.c (+4-4)
- (modified) clang/test/SemaCXX/thread-safety-annotations.h (+2-2)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+33)
- (modified) clang/test/SemaCXX/warn-thread-safety-parsing.cpp (+12-12)
- (modified) clang/unittests/AST/ASTImporterTest.cpp (+2-2)
``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5d07bfc210e05..eb4cc616897b4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -207,6 +207,12 @@ Attribute Changes in Clang
type-level control over overflow behavior. There is also an accompanying type
specifier for each behavior kind via `__ob_wrap` and `__ob_trap`.
+- The :doc:`ThreadSafetyAnalysis` attributes ``guarded_by`` and
+ ``pt_guarded_by`` now accept multiple capability arguments.
+ ``GUARDED_BY(mu1, mu2)`` is equivalent to ``GUARDED_BY(mu1) GUARDED_BY(mu2)``
+ and requires all listed capabilities to be held when accessing the guarded
+ variable.
+
Improvements to Clang's diagnostics
-----------------------------------
- Added ``-Wlifetime-safety`` to enable lifetime safety analysis,
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index d0f96f58dac17..0628206918f7f 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -153,16 +153,18 @@ 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; all of them must be held to access
+the data member.
``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 +183,28 @@ it points to* is protected by the given capability.
p3.reset(new int); // OK.
}
+When multiple capabilities are listed, all of them must be held:
+
+.. code-block:: c++
+
+ Mutex mu1, mu2;
+ int a GUARDED_BY(mu1, mu2); // Requires both mu1 and mu2.
+ int *p PT_GUARDED_BY(mu1, mu2);
+
+ void test() {
+ mu1.Lock();
+ a = 0; // Warning! mu2 is not held.
+ *p = 0; // Warning! mu2 is not held.
+ mu1.Unlock();
+
+ mu1.Lock();
+ mu2.Lock();
+ a = 0; // OK.
+ *p = 0; // OK.
+ mu2.Unlock();
+ mu1.Unlock();
+ }
+
REQUIRES(...), REQUIRES_SHARED(...)
-----------------------------------
@@ -860,11 +884,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__))
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index befd671393c7c..c3068ce9e69d9 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/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;
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 340a0fb4fb5aa..da2bda1b43526 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/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: {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 2c5976af9f61d..91017178fea25 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1935,7 +1935,8 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
}
for (const auto *I : D->specific_attrs<GuardedByAttr>())
- warnIfMutexNotHeld(FSet, D, Exp, AK, I->getArg(), POK, nullptr, Loc);
+ for (auto *Arg : I->args())
+ warnIfMutexNotHeld(FSet, D, Exp, AK, Arg, POK, nullptr, Loc);
}
/// Checks pt_guarded_by and pt_guarded_var attributes.
@@ -1999,8 +2000,9 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
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 *Arg : I->args())
+ warnIfMutexNotHeld(FSet, D, Exp, AK, Arg, PtPOK, nullptr,
+ Exp->getExprLoc());
}
/// Process a function call, method call, constructor call,
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 1b7f41061061d..7380f0057f2fa 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -459,36 +459,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,
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 2ae6e5de0e3ee..cfb78ef5f5897 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -19450,9 +19450,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))
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 69ec109e1523c..0fd382eb02b78 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -286,12 +286,12 @@ struct TestInit test_init(void) {
// attribute in a single __attribute__.
void run(void) __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}}
-int value_with_wrong_number_of_args GUARDED_BY(mu1, mu2); // expected-error{{'guarded_by' attribute takes one argument}}
+int value_with_multiple_guarded_args GUARDED_BY(mu1, mu2);
-int *ptr_with_wrong_number_of_args PT_GUARDED_BY(mu1, mu2); // expected-error{{'pt_guarded_by' attribute takes one argument}}
+int *ptr_with_multiple_guarded_args PT_GUARDED_BY(mu1, mu2);
-int value_with_no_open_brace __attribute__((guarded_by)); // expected-error{{'guarded_by' attribute takes one argument}}
-int *ptr_with_no_open_brace __attribute__((pt_guarded_by)); // expected-error{{'pt_guarded_by' attribute takes one argument}}
+int value_with_no_open_brace __attribute__((guarded_by)); // expected-error{{'guarded_by' attribute takes at least 1 argument}}
+int *ptr_with_no_open_brace __attribute__((pt_guarded_by)); // expected-error{{'pt_guarded_by' attribute takes at least 1 argument}}
int value_with_no_open_brace_on_acquire_after __attribute__((acquired_after)); // expected-error{{'acquired_after' attribute takes at least 1 argument}}
int value_with_no_open_brace_on_acquire_before __attribute__((acquired_before)); // expected-error{{'acquired_before' attribute takes at least 1 argument}}
diff --git a/clang/test/SemaCXX/thread-safety-annotations.h b/clang/test/SemaCXX/thread-safety-annotations.h
index 00d432da4b6f0..bcda81cc69049 100644
--- a/clang/test/SemaCXX/thread-safety-annotations.h
+++ b/clang/test/SemaCXX/thread-safety-annotations.h
@@ -31,8 +31,8 @@
// Capabilities only
#define EXCLUSIVE_UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__)))
#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__)))
-#define GUARDED_BY(x) __attribute__((guarded_by(x)))
-#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
+#define GUARDED_BY(...) __attribute__((guarded_by(__VA_ARGS__)))
+#define PT_GUARDED_BY(...) __attribute__((pt_guarded_by(__VA_ARGS__)))
// Common
#define REENTRANT_CAPABILITY __attribute__((reentrant_capability))
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index a6c9a2236fc45..9ef6d7d3349d2 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1526,6 +1526,39 @@ class Foo {
f2(); // expected-warning {{cannot call function 'f2' while mutex 'mu1' is held}} \
// expected-warning {{cannot call function 'f2' while mutex 'mu2' is held}}
}
+
+ // Holding only mu1 (but not mu2) is insufficient to access x.
+ void f3() EXCLUSIVE_LOCKS_REQUIRED(mu1) {
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu2' exclusively}}
+ }
+
+ // Holding only mu2 (but not mu1) is insufficient to access x.
+ void f4() EXCLUSIVE_LOCKS_REQUIRED(mu2) {
+ x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu1' exclusively}}
+ }
+};
+
+// Test multi-arg guarded_by: GUARDED_BY(mu1, mu2, mu3) requires all three locks.
+class Bar {
+ Mutex mu1, mu2, mu3;
+ int z GUARDED_BY(mu1, mu2, mu3);
+
+ // All three locks held: no warning.
+ void g1() EXCLUSIVE_LOCKS_REQUIRED(mu1, mu2, mu3) {
+ z = 1;
+ }
+
+ // Only mu1 and mu2 held: warn about mu3.
+ void g2() EXCLUSIVE_LOCKS_REQUIRED(mu1, mu2) {
+ z = 1; // expected-warning {{writing variable 'z' requires holding mutex 'mu3' exclusively}}
+ }
+
+ // No locks held: warn about all three.
+ void g3() {
+ z = 1; // expected-warning {{writing variable 'z' requires holding mutex 'mu1' exclusively}} \
+ // expected-warning {{writing variable 'z' requires holding mutex 'mu2' exclusively}} \
+ // expected-warning {{writing variable 'z' requires holding mutex 'mu3' exclusively}}
+ }
};
Foo *foo;
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 570586ab2f532..86d998711a1a0 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -5,9 +5,9 @@
#define LOCKABLE __attribute__ ((lockable))
#define REENTRANT_CAPABILITY __attribute__ ((reentrant_capability))
#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x) __attribute__ ((guarded_by(x)))
+#define GUARDED_BY(...) __attribute__ ((guarded_by(__VA_ARGS__)))
#define GUARDED_VAR __attribute__ ((guarded_var))
-#define PT_GUARDED_BY(x) __attribute__ ((pt_guarded_by(x)))
+#define PT_GUARDED_BY(...) __attribute__ ((pt_guarded_by(__VA_ARGS__)))
#define PT_GUARDED_VAR __attribute__ ((pt_guarded_var))
#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__)))
#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__)))
@@ -317,8 +317,6 @@ void sl_function_params(int lvar SCOPED_LOCKABLE); // \
// Guarded By Attribute (gb)
//-----------------------------------------//
-// FIXME: Eventually, would we like this attribute to take more than 1 arg?
-
#if !__has_attribute(guarded_by)
#error "Should support guarded_by attribute"
#endif
@@ -329,17 +327,18 @@ int gb_var_arg GUARDED_BY(mu1);
int gb_non_ascii GUARDED_BY(L"wide"); // expected-warning {{ignoring 'guarded_by' attribute because its argument is invalid}}
-int gb_var_args __attribute__((guarded_by(mu1, mu2))); // \
- // expected-error {{'guarded_by' attribute takes one argument}}
+// Multi-arg form: all listed mutexes must be held.
+int gb_var_args __attribute__((guarded_by(mu1, mu2)));
int gb_var_noargs __attribute__((guarded_by)); // \
- // expected-error {{'guarded_by' attribute takes one argument}}
+ // expected-error {{'guarded_by' attribute takes at least 1 argument}}
class GBFoo {
private:
int gb_field_noargs __attribute__((guarded_by)); // \
- // expected-error {{'guarded_by' attribute takes one argument}}
+ // expected-error {{'guarded_by' attribute takes at least 1 argument}}
int gb_field_args GUARDED_BY(mu1);
+ int gb_field_multi GUARDED_BY(mu1, mu2);
};
class GUARDED_BY(mu1) GB { // \
@@ -397,12 +396,12 @@ int gb_var_arg_bad_4 GUARDED_BY(umu); // \
//1. Check applied to the right types & argument number
int *pgb_var_noargs __attribute__((pt_guarded_by)); // \
- // expected-error {{'pt_guarded_by' attribute takes one argument}}
+ // expected-error {{'pt_guarded_by' attribute takes at least 1 argument}}
int *pgb_ptr_var_arg PT_GUARDED_BY(mu1);
-int *pgb_ptr_var_args __attribute__((pt_guarded_by(mu1, mu2))); // \
- // expected-error {{'pt_guarded_by' attribute takes one argument}}
+// Multi-arg form: all listed mutexes must be held.
+int *pgb_ptr_var_args __attribute__((pt_guarded_by(mu1, mu2)));
int pgb_var_args PT_GUARDED_BY(mu1); // \
// expected-warning {{'pt_guarded_by' only applies to pointer types; type here is 'int'}}
@@ -410,8 +409,9 @@ int pgb_var_args PT_GUARDED_BY(mu1); // \
class PGBFoo {
private:
int *pgb_field_noargs __attribute__((pt_guarded_by)); // \
- // expected-error {{'pt_guarded_by' attribute takes one argument}}
+ // expected-error {{'pt_guarded_by' attribute takes at least 1 argument}}
int *pgb_field_args PT_GUARDED_BY(mu1);
+ int *pgb_field_multi PT_GUARDED_BY(mu1, mu2);
};
class PT_GUARDED_BY(mu1) PGB { // \
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 164790606ea0b..d1d6ea94d3154 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -8142,7 +8142,7 @@ TEST_P(ImportAttributes, ImportGuardedBy) {
int test __attribute__((guarded_by(G)));
)",
FromAttr, ToAttr);
- checkImported(FromAttr->getArg(), ToAttr->getArg());
+ checkImportVariadicArg(FromAttr->args(), ToAttr->args());
}
TEST_P(ImportAttributes, ImportPtGuardedBy) {
@@ -8153,7 +8153,7 @@ TEST_P(ImportAttributes, ImportPtGuardedBy) {
int *test __attribute__((pt_guarded_by(G)));
)",
FromAttr, ToAttr);
- checkImported(FromAttr->getArg(), ToAttr->getArg());
+ checkImportVariadicArg(FromAttr->args(), ToAttr->args());
}
TEST_P(ImportAttributes, ImportAcquiredAfter) {
``````````
</details>
https://github.com/llvm/llvm-project/pull/185172
More information about the cfe-commits
mailing list