https://github.com/melver created https://github.com/llvm/llvm-project/pull/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: https://github.com/llvm/llvm-project/pull/185173 Link: https://lore.kernel.org/all/[email protected]/ [1] >From b8d3b269400bfc6f899b36fd3dd210d90dcb21b0 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Sat, 7 Mar 2026 02:34:36 +0100 Subject: [PATCH] Thread Safety Analysis: Support guarded_by/pt_guarded_by with multiple capabilities 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). Further Discussion: https://github.com/llvm/llvm-project/pull/185173 Link: https://lore.kernel.org/all/[email protected]/ [1] --- clang/docs/ReleaseNotes.rst | 7 +++ clang/docs/ThreadSafetyAnalysis.rst | 45 ++++++++++++--- .../clang/Analysis/Analyses/ThreadSafety.h | 11 ++++ clang/include/clang/Basic/Attr.td | 4 +- .../clang/Basic/DiagnosticSemaKinds.td | 6 ++ clang/lib/AST/ASTImporter.cpp | 8 ++- clang/lib/Analysis/ThreadSafety.cpp | 56 +++++++++++++++++-- clang/lib/Sema/AnalysisBasedWarnings.cpp | 27 +++++++++ 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 | 54 ++++++++++++++++++ .../SemaCXX/warn-thread-safety-parsing.cpp | 22 ++++---- clang/unittests/AST/ASTImporterTest.cpp | 4 +- 15 files changed, 234 insertions(+), 53 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 6f89cc9a30603..f8aad791b0645 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -213,6 +213,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, diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index d0f96f58dac17..0baaa8b3d1720 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/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, exploiting the following invariant: +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,33 @@ 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); + int *p PT_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. + *p = 0; // Warning! Writing requires both mu1 and mu2. + } + + void writer() REQUIRES(mu1, mu2) { + a = 0; // OK: both capabilities are held exclusively. + *p = 0; // OK. + } + + void unprotected() { + int x = a; // Warning! No capability held at all. + a = 0; // Warning! Both mu1 and mu2 must be held. + } + REQUIRES(...), REQUIRES_SHARED(...) ----------------------------------- @@ -860,11 +891,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/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 9fb23212aaf97..f05fa2219c4eb 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -193,6 +193,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 -- Comma-separated list of capability names, quoted + /// \param Loc -- The location of the read + virtual void handleGuardedByAnyReadNotHeld(const NamedDecl *D, + ProtectedOperationKind POK, + 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). diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 8ab4aaa6f5781..fc1f2b8e645ca 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/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index fae63cc0ba139..9a9e1672a5ab5 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4333,6 +4333,12 @@ 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_variable_requires_any_of_locks : Warning< + "reading variable %0 requires holding at least one of %1">, + InGroup<ThreadSafetyAnalysis>, DefaultIgnore; +def warn_var_deref_requires_any_of_locks : Warning< + "reading the value pointed to by %0 requires holding at least one of %1">, + InGroup<ThreadSafetyAnalysis>, DefaultIgnore; def warn_fun_excludes_mutex : Warning< "cannot call function '%1' while %0 '%2' is held">, InGroup<ThreadSafetyAnalysis>, DefaultIgnore; 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..d25e997396dc7 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -41,6 +41,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Allocator.h" #include "llvm/Support/Casting.h" @@ -1282,6 +1283,12 @@ class ThreadSafetyAnalyzer { void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp, Expr *MutexExp, 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 checkAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, ProtectedOperationKind POK); void checkPtAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, @@ -1868,6 +1875,24 @@ void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet, } } +void ThreadSafetyAnalyzer::warnIfAnyMutexNotHeldForRead( + const FactSet &FSet, const NamedDecl *D, const Expr *Exp, + llvm::ArrayRef<Expr *> Args, ProtectedOperationKind POK, + SourceLocation Loc) { + SmallVector<std::string, 2> Names; + for (auto *Arg : Args) { + CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, nullptr); + if (Cp.isInvalid() || 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. + Names.push_back("'" + Cp.toString() + "'"); + } + if (!Names.empty()) + Handler.handleGuardedByAnyReadNotHeld(D, POK, llvm::join(Names, ", "), Loc); +} + /// Checks guarded_by and pt_guarded_by attributes. /// Whenever we identify an access (read or write) to a DeclRefExpr that is /// marked with guarded_by, we must ensure the appropriate mutexes are held. @@ -1934,8 +1959,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 +2033,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, diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index e2bd8d7956561..116d46bf2f979 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2141,6 +2141,33 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { Warnings.emplace_back(std::move(Warning), getNotes()); } + void handleGuardedByAnyReadNotHeld(const NamedDecl *D, + ProtectedOperationKind POK, + StringRef LockNames, + SourceLocation Loc) override { + unsigned DiagID = 0; + switch (POK) { + case POK_VarAccess: + case POK_PassByRef: + case POK_ReturnByRef: + case POK_PassPointer: + case POK_ReturnPointer: + DiagID = diag::warn_variable_requires_any_of_locks; + break; + case POK_VarDereference: + case POK_PtPassByRef: + case POK_PtReturnByRef: + case POK_PtPassPointer: + case POK_PtReturnPointer: + DiagID = diag::warn_var_deref_requires_any_of_locks; + break; + case POK_FunctionCall: + llvm_unreachable("POK_FunctionCall not applicable here"); + } + PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << D << LockNames); + Warnings.emplace_back(std::move(Warning), getNotes()); + } + void handleMutexNotHeld(StringRef Kind, const NamedDecl *D, ProtectedOperationKind POK, Name LockName, LockKind LK, SourceLocation Loc, diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 6fc749464586d..cd6b975c12456 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 56f315e005320..45a58e677e094 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/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)) 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..079e068ca7811 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1526,6 +1526,16 @@ 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}} + } }; Foo *foo; @@ -1537,6 +1547,50 @@ void func() } } // end namespace thread_annot_lock_42 +namespace guarded_by_multi { +// Test multi-arg GUARDED_BY: any one capability suffices for read; all are +// required for write. +class Foo { + Mutex mu1, mu2; + int x GUARDED_BY(mu1, mu2); + int *p PT_GUARDED_BY(mu1, mu2); + + // All locks held exclusively: read and write both OK. + void f1() EXCLUSIVE_LOCKS_REQUIRED(mu1, mu2) { + x = 1; + *p = 1; + } + + // Only mu1 held exclusively: read OK, write warns about mu2. + void f2() EXCLUSIVE_LOCKS_REQUIRED(mu1) { + int y = x; + int z = *p; + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu2' exclusively}} + *p = 1; // expected-warning {{writing the value pointed to by 'p' requires holding mutex 'mu2' exclusively}} + } + + // One lock held shared: read OK, write warns about both. + void f3() SHARED_LOCKS_REQUIRED(mu1) { + int y = x; + int z = *p; + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu1' exclusively}} \ + // expected-warning {{writing variable 'x' requires holding mutex 'mu2' exclusively}} + *p = 1; // expected-warning {{writing the value pointed to by 'p' requires holding mutex 'mu1' exclusively}} \ + // expected-warning {{writing the value pointed to by 'p' requires holding mutex 'mu2' exclusively}} + } + + // No locks held: read and write both warn. + void f4() { + int y = x; // expected-warning {{reading variable 'x' requires holding at least one of 'mu1', 'mu2'}} + int z = *p; // expected-warning {{reading the value pointed to by 'p' requires holding at least one of 'mu1', 'mu2'}} + x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu1' exclusively}} \ + // expected-warning {{writing variable 'x' requires holding mutex 'mu2' exclusively}} + *p = 1; // expected-warning {{writing the value pointed to by 'p' requires holding mutex 'mu1' exclusively}} \ + // expected-warning {{writing the value pointed to by 'p' requires holding mutex 'mu2' exclusively}} + } +}; +} // end namespace guarded_by_multi + namespace thread_annot_lock_46 { // Test the support for annotations on virtual functions. class Base { diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp index 570586ab2f532..a1832d0190362 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,17 @@ 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}} +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 +395,11 @@ 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}} +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 +407,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
