Author: Aaron Puchert Date: 2025-04-15T23:21:34+02:00 New Revision: 9c73eba8aa17cb7ca4248ab1c7f67ea7ec9b50b1
URL: https://github.com/llvm/llvm-project/commit/9c73eba8aa17cb7ca4248ab1c7f67ea7ec9b50b1 DIFF: https://github.com/llvm/llvm-project/commit/9c73eba8aa17cb7ca4248ab1c7f67ea7ec9b50b1.diff LOG: Merge similar Clang Thread Safety attributes (#135561) Some of the old lock-based and new capability-based spellings behave basically in the same way, so merging them simplifies the code significantly. There are two minor functional changes: we only warn (instead of an error) when the try_acquire_capability attribute is used on something else than a function. The alternative would have been to produce an error for the old spelling, but we seem to only warn for all function attributes, so this is arguably more consistent. The second change is that we also check the first argument (which is the value returned for a successful try-acquire) for `this`. But from what I can tell, this code is defunct anyway at the moment (see #31414). Added: Modified: clang/include/clang/Basic/Attr.td clang/lib/AST/ASTImporter.cpp clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/SemaDeclAttr.cpp clang/lib/Sema/SemaDeclCXX.cpp clang/test/Sema/attr-capabilities.c clang/unittests/AST/ASTImporterTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 9d4900f3029c8..9465451cbfe1f 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -3818,7 +3818,9 @@ def Capability : InheritableAttr { def AssertCapability : InheritableAttr { let Spellings = [Clang<"assert_capability", 0>, - Clang<"assert_shared_capability", 0>]; + Clang<"assert_shared_capability", 0>, + GNU<"assert_exclusive_lock">, + GNU<"assert_shared_lock">]; let Subjects = SubjectList<[Function]>; let LateParsed = LateAttrParseStandard; let TemplateDependent = 1; @@ -3826,7 +3828,8 @@ def AssertCapability : InheritableAttr { let InheritEvenIfAlreadyPresent = 1; let Args = [VariadicExprArgument<"Args">]; let Accessors = [Accessor<"isShared", - [Clang<"assert_shared_capability", 0>]>]; + [Clang<"assert_shared_capability", 0>, + GNU<"assert_shared_lock">]>]; let Documentation = [AssertCapabilityDocs]; } @@ -3849,16 +3852,18 @@ def AcquireCapability : InheritableAttr { def TryAcquireCapability : InheritableAttr { let Spellings = [Clang<"try_acquire_capability", 0>, - Clang<"try_acquire_shared_capability", 0>]; - let Subjects = SubjectList<[Function], - ErrorDiag>; + Clang<"try_acquire_shared_capability", 0>, + GNU<"exclusive_trylock_function">, + GNU<"shared_trylock_function">]; + let Subjects = SubjectList<[Function]>; let LateParsed = LateAttrParseStandard; let TemplateDependent = 1; let ParseArgumentsAsUnevaluated = 1; let InheritEvenIfAlreadyPresent = 1; let Args = [ExprArgument<"SuccessValue">, VariadicExprArgument<"Args">]; let Accessors = [Accessor<"isShared", - [Clang<"try_acquire_shared_capability", 0>]>]; + [Clang<"try_acquire_shared_capability", 0>, + GNU<"shared_trylock_function">]>]; let Documentation = [TryAcquireCapabilityDocs]; } @@ -3948,54 +3953,6 @@ def AcquiredBefore : InheritableAttr { let Documentation = [Undocumented]; } -def AssertExclusiveLock : InheritableAttr { - let Spellings = [GNU<"assert_exclusive_lock">]; - let Args = [VariadicExprArgument<"Args">]; - let LateParsed = LateAttrParseStandard; - let TemplateDependent = 1; - let ParseArgumentsAsUnevaluated = 1; - let InheritEvenIfAlreadyPresent = 1; - let Subjects = SubjectList<[Function]>; - let Documentation = [Undocumented]; -} - -def AssertSharedLock : InheritableAttr { - let Spellings = [GNU<"assert_shared_lock">]; - let Args = [VariadicExprArgument<"Args">]; - let LateParsed = LateAttrParseStandard; - let TemplateDependent = 1; - let ParseArgumentsAsUnevaluated = 1; - let InheritEvenIfAlreadyPresent = 1; - let Subjects = SubjectList<[Function]>; - let Documentation = [Undocumented]; -} - -// The first argument is an integer or boolean value specifying the return value -// of a successful lock acquisition. -def ExclusiveTrylockFunction : InheritableAttr { - let Spellings = [GNU<"exclusive_trylock_function">]; - let Args = [ExprArgument<"SuccessValue">, VariadicExprArgument<"Args">]; - let LateParsed = LateAttrParseStandard; - let TemplateDependent = 1; - let ParseArgumentsAsUnevaluated = 1; - let InheritEvenIfAlreadyPresent = 1; - let Subjects = SubjectList<[Function]>; - let Documentation = [Undocumented]; -} - -// The first argument is an integer or boolean value specifying the return value -// of a successful lock acquisition. -def SharedTrylockFunction : InheritableAttr { - let Spellings = [GNU<"shared_trylock_function">]; - let Args = [ExprArgument<"SuccessValue">, VariadicExprArgument<"Args">]; - let LateParsed = LateAttrParseStandard; - let TemplateDependent = 1; - let ParseArgumentsAsUnevaluated = 1; - let InheritEvenIfAlreadyPresent = 1; - let Subjects = SubjectList<[Function]>; - let Documentation = [Undocumented]; -} - def LockReturned : InheritableAttr { let Spellings = [GNU<"lock_returned">]; let Args = [ExprArgument<"Arg">]; diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index b55b8f2c14147..00628602e61fa 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -9425,34 +9425,6 @@ Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) { From->args_size()); break; } - case attr::AssertExclusiveLock: { - const auto *From = cast<AssertExclusiveLockAttr>(FromAttr); - AI.importAttr(From, - AI.importArrayArg(From->args(), From->args_size()).value(), - From->args_size()); - break; - } - case attr::AssertSharedLock: { - const auto *From = cast<AssertSharedLockAttr>(FromAttr); - AI.importAttr(From, - AI.importArrayArg(From->args(), From->args_size()).value(), - From->args_size()); - break; - } - case attr::ExclusiveTrylockFunction: { - const auto *From = cast<ExclusiveTrylockFunctionAttr>(FromAttr); - AI.importAttr(From, AI.importArg(From->getSuccessValue()).value(), - AI.importArrayArg(From->args(), From->args_size()).value(), - From->args_size()); - break; - } - case attr::SharedTrylockFunction: { - const auto *From = cast<SharedTrylockFunctionAttr>(FromAttr); - AI.importAttr(From, AI.importArg(From->getSuccessValue()).value(), - AI.importArrayArg(From->args(), From->args_size()).value(), - From->args_size()); - break; - } case attr::LockReturned: { const auto *From = cast<LockReturnedAttr>(FromAttr); AI.importAttr(From, AI.importArg(From->getArg()).value()); diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 6b5b49377fa08..42fb0fe7dcdaa 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1511,38 +1511,17 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result, return; auto *FunDecl = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl()); - if(!FunDecl || !FunDecl->hasAttrs()) + if (!FunDecl || !FunDecl->hasAttr<TryAcquireCapabilityAttr>()) return; CapExprSet ExclusiveLocksToAdd; CapExprSet SharedLocksToAdd; // If the condition is a call to a Trylock function, then grab the attributes - for (const auto *Attr : FunDecl->attrs()) { - switch (Attr->getKind()) { - case attr::TryAcquireCapability: { - auto *A = cast<TryAcquireCapabilityAttr>(Attr); - getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, - Exp, FunDecl, PredBlock, CurrBlock, A->getSuccessValue(), - Negate); - break; - }; - case attr::ExclusiveTrylockFunction: { - const auto *A = cast<ExclusiveTrylockFunctionAttr>(Attr); - getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, - A->getSuccessValue(), Negate); - break; - } - case attr::SharedTrylockFunction: { - const auto *A = cast<SharedTrylockFunctionAttr>(Attr); - getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, - A->getSuccessValue(), Negate); - break; - } - default: - break; - } - } + for (const auto *Attr : FunDecl->specific_attrs<TryAcquireCapabilityAttr>()) + getMutexIDs(Attr->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, Attr, + Exp, FunDecl, PredBlock, CurrBlock, Attr->getSuccessValue(), + Negate); // Add and remove locks. SourceLocation Loc = Exp->getExprLoc(); @@ -1882,29 +1861,6 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, // An assert will add a lock to the lockset, but will not generate // a warning if it is already there, and will not generate a warning // if it is not removed. - case attr::AssertExclusiveLock: { - const auto *A = cast<AssertExclusiveLockAttr>(At); - - CapExprSet AssertLocks; - Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self); - for (const auto &AssertLock : AssertLocks) - Analyzer->addLock( - FSet, std::make_unique<LockableFactEntry>( - AssertLock, LK_Exclusive, Loc, FactEntry::Asserted)); - break; - } - case attr::AssertSharedLock: { - const auto *A = cast<AssertSharedLockAttr>(At); - - CapExprSet AssertLocks; - Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self); - for (const auto &AssertLock : AssertLocks) - Analyzer->addLock( - FSet, std::make_unique<LockableFactEntry>( - AssertLock, LK_Shared, Loc, FactEntry::Asserted)); - break; - } - case attr::AssertCapability: { const auto *A = cast<AssertCapabilityAttr>(At); CapExprSet AssertLocks; @@ -2499,12 +2455,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { getMutexIDs(A->isShared() ? SharedLocksAcquired : ExclusiveLocksAcquired, A, nullptr, D); - } else if (isa<ExclusiveTrylockFunctionAttr>(Attr)) { - // Don't try to check trylock functions for now. - return; - } else if (isa<SharedTrylockFunctionAttr>(Attr)) { - // Don't try to check trylock functions for now. - return; } else if (isa<TryAcquireCapabilityAttr>(Attr)) { // Don't try to check trylock functions for now. return; diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 20ea38b7e05db..bc891fb009410 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -538,29 +538,6 @@ static bool checkLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL, return true; } -static void handleAssertSharedLockAttr(Sema &S, Decl *D, const ParsedAttr &AL) { - SmallVector<Expr *, 1> Args; - if (!checkLockFunAttrCommon(S, D, AL, Args)) - return; - - unsigned Size = Args.size(); - Expr **StartArg = Size == 0 ? nullptr : &Args[0]; - D->addAttr(::new (S.Context) - AssertSharedLockAttr(S.Context, AL, StartArg, Size)); -} - -static void handleAssertExclusiveLockAttr(Sema &S, Decl *D, - const ParsedAttr &AL) { - SmallVector<Expr *, 1> Args; - if (!checkLockFunAttrCommon(S, D, AL, Args)) - return; - - unsigned Size = Args.size(); - Expr **StartArg = Size == 0 ? nullptr : &Args[0]; - D->addAttr(::new (S.Context) - AssertExclusiveLockAttr(S.Context, AL, StartArg, Size)); -} - /// Checks to be sure that the given parameter number is in bounds, and /// is an integral type. Will emit appropriate diagnostics if this returns /// false. @@ -640,26 +617,6 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL, return true; } -static void handleSharedTrylockFunctionAttr(Sema &S, Decl *D, - const ParsedAttr &AL) { - SmallVector<Expr*, 2> Args; - if (!checkTryLockFunAttrCommon(S, D, AL, Args)) - return; - - D->addAttr(::new (S.Context) SharedTrylockFunctionAttr( - S.Context, AL, AL.getArgAsExpr(0), Args.data(), Args.size())); -} - -static void handleExclusiveTrylockFunctionAttr(Sema &S, Decl *D, - const ParsedAttr &AL) { - SmallVector<Expr*, 2> Args; - if (!checkTryLockFunAttrCommon(S, D, AL, Args)) - return; - - D->addAttr(::new (S.Context) ExclusiveTrylockFunctionAttr( - S.Context, AL, AL.getArgAsExpr(0), Args.data(), Args.size())); -} - static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) { // check that the argument is lockable object SmallVector<Expr*, 1> Args; @@ -7528,12 +7485,6 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL, break; // Thread safety attributes: - case ParsedAttr::AT_AssertExclusiveLock: - handleAssertExclusiveLockAttr(S, D, AL); - break; - case ParsedAttr::AT_AssertSharedLock: - handleAssertSharedLockAttr(S, D, AL); - break; case ParsedAttr::AT_PtGuardedVar: handlePtGuardedVarAttr(S, D, AL); break; @@ -7549,18 +7500,12 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL, case ParsedAttr::AT_PtGuardedBy: handlePtGuardedByAttr(S, D, AL); break; - case ParsedAttr::AT_ExclusiveTrylockFunction: - handleExclusiveTrylockFunctionAttr(S, D, AL); - break; case ParsedAttr::AT_LockReturned: handleLockReturnedAttr(S, D, AL); break; case ParsedAttr::AT_LocksExcluded: handleLocksExcludedAttr(S, D, AL); break; - case ParsedAttr::AT_SharedTrylockFunction: - handleSharedTrylockFunctionAttr(S, D, AL); - break; case ParsedAttr::AT_AcquiredBefore: handleAcquiredBeforeAttr(S, D, AL); break; diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 2247aded9384a..05991228dbfc2 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -19401,13 +19401,7 @@ bool Sema::checkThisInStaticMemberFunctionAttributes(CXXMethodDecl *Method) { Args = llvm::ArrayRef(AA->args_begin(), AA->args_size()); else if (const auto *AB = dyn_cast<AcquiredBeforeAttr>(A)) Args = llvm::ArrayRef(AB->args_begin(), AB->args_size()); - else if (const auto *ETLF = dyn_cast<ExclusiveTrylockFunctionAttr>(A)) { - Arg = ETLF->getSuccessValue(); - Args = llvm::ArrayRef(ETLF->args_begin(), ETLF->args_size()); - } else if (const auto *STLF = dyn_cast<SharedTrylockFunctionAttr>(A)) { - Arg = STLF->getSuccessValue(); - Args = llvm::ArrayRef(STLF->args_begin(), STLF->args_size()); - } else if (const auto *LR = dyn_cast<LockReturnedAttr>(A)) + else if (const auto *LR = dyn_cast<LockReturnedAttr>(A)) Arg = LR->getArg(); else if (const auto *LE = dyn_cast<LocksExcludedAttr>(A)) Args = llvm::ArrayRef(LE->args_begin(), LE->args_size()); @@ -19415,9 +19409,10 @@ bool Sema::checkThisInStaticMemberFunctionAttributes(CXXMethodDecl *Method) { Args = llvm::ArrayRef(RC->args_begin(), RC->args_size()); else if (const auto *AC = dyn_cast<AcquireCapabilityAttr>(A)) Args = llvm::ArrayRef(AC->args_begin(), AC->args_size()); - else if (const auto *AC = dyn_cast<TryAcquireCapabilityAttr>(A)) + else if (const auto *AC = dyn_cast<TryAcquireCapabilityAttr>(A)) { + Arg = AC->getSuccessValue(); Args = llvm::ArrayRef(AC->args_begin(), AC->args_size()); - else if (const auto *RC = dyn_cast<ReleaseCapabilityAttr>(A)) + } else if (const auto *RC = dyn_cast<ReleaseCapabilityAttr>(A)) Args = llvm::ArrayRef(RC->args_begin(), RC->args_size()); if (Arg && !Finder.TraverseStmt(Arg)) diff --git a/clang/test/Sema/attr-capabilities.c b/clang/test/Sema/attr-capabilities.c index 5138803bd5eb7..12b18b687803e 100644 --- a/clang/test/Sema/attr-capabilities.c +++ b/clang/test/Sema/attr-capabilities.c @@ -14,7 +14,7 @@ struct __attribute__((capability("custom"))) CustomName {}; int Test1 __attribute__((capability("test1"))); // expected-error {{'capability' attribute only applies to structs, unions, classes, and typedefs}} int Test2 __attribute__((shared_capability("test2"))); // expected-error {{'shared_capability' attribute only applies to structs, unions, classes, and typedefs}} int Test3 __attribute__((acquire_capability("test3"))); // expected-warning {{'acquire_capability' attribute only applies to functions}} -int Test4 __attribute__((try_acquire_capability("test4"))); // expected-error {{'try_acquire_capability' attribute only applies to functions}} +int Test4 __attribute__((try_acquire_capability("test4"))); // expected-warning {{'try_acquire_capability' attribute only applies to functions}} int Test5 __attribute__((release_capability("test5"))); // expected-warning {{'release_capability' attribute only applies to functions}} struct __attribute__((capability(12))) Test3 {}; // expected-error {{expected string literal as argument of 'capability' attribute}} diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index 40e1197bc21f1..4192faee1af80 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -7977,42 +7977,6 @@ TEST_P(ImportAttributes, ImportAcquiredBefore) { checkImportVariadicArg(FromAttr->args(), ToAttr->args()); } -TEST_P(ImportAttributes, ImportAssertExclusiveLock) { - AssertExclusiveLockAttr *FromAttr, *ToAttr; - importAttr<FunctionDecl>("void test(int A1, int A2) " - "__attribute__((assert_exclusive_lock(A1, A2)));", - FromAttr, ToAttr); - checkImportVariadicArg(FromAttr->args(), ToAttr->args()); -} - -TEST_P(ImportAttributes, ImportAssertSharedLock) { - AssertSharedLockAttr *FromAttr, *ToAttr; - importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((assert_shared_lock(A1, A2)));", - FromAttr, ToAttr); - checkImportVariadicArg(FromAttr->args(), ToAttr->args()); -} - -TEST_P(ImportAttributes, ImportExclusiveTrylockFunction) { - ExclusiveTrylockFunctionAttr *FromAttr, *ToAttr; - importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((exclusive_trylock_function(1, " - "A1, A2)));", - FromAttr, ToAttr); - checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue()); - checkImportVariadicArg(FromAttr->args(), ToAttr->args()); -} - -TEST_P(ImportAttributes, ImportSharedTrylockFunction) { - SharedTrylockFunctionAttr *FromAttr, *ToAttr; - importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((shared_trylock_function(1, A1, " - "A2)));", - FromAttr, ToAttr); - checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue()); - checkImportVariadicArg(FromAttr->args(), ToAttr->args()); -} - TEST_P(ImportAttributes, ImportLockReturned) { LockReturnedAttr *FromAttr, *ToAttr; importAttr<FunctionDecl>( _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
