https://github.com/melver created https://github.com/llvm/llvm-project/pull/185172
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); This commit allows listing all required capabilities in a single attribute, consistent with REQUIRES and other thread safety annotations: int x GUARDED_BY(mu1, mu2); Both forms remain supported and are semantically equivalent. >From 3eda9da0df660057c69786389f70847fd886ed5a Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Sat, 7 Mar 2026 12:09:39 +0100 Subject: [PATCH] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20initia?= =?UTF-8?q?l=20version?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.8-beta.1 --- clang/docs/ReleaseNotes.rst | 6 +++ clang/docs/ThreadSafetyAnalysis.rst | 38 +++++++++++++++---- clang/include/clang/Basic/Attr.td | 4 +- clang/lib/AST/ASTImporter.cpp | 8 +++- clang/lib/Analysis/ThreadSafety.cpp | 8 ++-- clang/lib/Sema/SemaDeclAttr.cpp | 27 ++++++------- clang/lib/Sema/SemaDeclCXX.cpp | 4 +- clang/test/Sema/warn-thread-safety-analysis.c | 8 ++-- .../test/SemaCXX/thread-safety-annotations.h | 4 +- .../SemaCXX/warn-thread-safety-analysis.cpp | 33 ++++++++++++++++ .../SemaCXX/warn-thread-safety-parsing.cpp | 24 ++++++------ clang/unittests/AST/ASTImporterTest.cpp | 4 +- 12 files changed, 117 insertions(+), 51 deletions(-) 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) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
