thread-safety: Support the new capability-based names for all related attributes. #99919
+65
−53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Pierre d'Herbemont (pdherbemont) ChangesFor some reason some attributes weren't renamed in code but were Those are:
This patch makes it so that we support the new name as documented. And Full diff: https://github.com/llvm/llvm-project/pull/99919.diff 12 Files Affected:
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b492..995d29ffb6ef0 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -801,7 +801,7 @@ implementation.
THREAD_ANNOTATION_ATTRIBUTE__(capability(x))
#define SCOPED_CAPABILITY \
- THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
+ THREAD_ANNOTATION_ATTRIBUTE__(scoped_capability)
#define GUARDED_BY(x) \
THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
@@ -843,7 +843,7 @@ implementation.
THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__))
#define EXCLUDES(...) \
- THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))
+ THREAD_ANNOTATION_ATTRIBUTE__(capabilities_excluded(__VA_ARGS__))
#define ASSERT_CAPABILITY(x) \
THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x))
@@ -852,7 +852,7 @@ implementation.
THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x))
#define RETURN_CAPABILITY(x) \
- THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x))
+ THREAD_ANNOTATION_ATTRIBUTE__(capability_returned(x))
#define NO_THREAD_SAFETY_ANALYSIS \
THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..b76188cbe7b74 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -346,7 +346,7 @@ class SExprBuilder {
/// the appropriate mutex expression in the lexical context where the function
/// is called. PrevCtx holds the context in which the arguments themselves
/// should be evaluated; multiple calling contexts can be chained together
- /// by the lock_returned attribute.
+ /// by the capability_returned attribute.
struct CallingContext {
// The previous context; or 0 if none.
CallingContext *Prev;
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 4825979a974d2..cfa51d3ebca3d 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3583,8 +3583,9 @@ def Lockable : InheritableAttr {
let ASTNode = 0; // Replaced by Capability
}
-def ScopedLockable : InheritableAttr {
- let Spellings = [Clang<"scoped_lockable", 0>];
+def ScopedCapability : InheritableAttr {
+ let Spellings = [Clang<"scoped_capability", 0>,
+ Clang<"scoped_lockable", 0>];
let Subjects = SubjectList<[Record]>;
let Documentation = [Undocumented];
let SimpleHandler = 1;
@@ -3779,8 +3780,9 @@ def SharedTrylockFunction : InheritableAttr {
let Documentation = [Undocumented];
}
-def LockReturned : InheritableAttr {
- let Spellings = [GNU<"lock_returned">];
+def CapabilityReturned : InheritableAttr {
+ let Spellings = [GNU<"capability_returned">,
+ GNU<"lock_returned">];
let Args = [ExprArgument<"Arg">];
let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
@@ -3789,8 +3791,9 @@ def LockReturned : InheritableAttr {
let Documentation = [Undocumented];
}
-def LocksExcluded : InheritableAttr {
- let Spellings = [GNU<"locks_excluded">];
+def CapabilitiesExcluded : InheritableAttr {
+ let Spellings = [GNU<"capabilities_excluded">,
+ GNU<"locks_excluded">];
let Args = [VariadicExprArgument<"Args">];
let LateParsed = LateAttrParseStandard;
let TemplateDependent = 1;
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 0c27f6f5df2da..51cf6ea427eba 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -9359,13 +9359,13 @@ Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) {
From->args_size());
break;
}
- case attr::LockReturned: {
- const auto *From = cast<LockReturnedAttr>(FromAttr);
+ case attr::CapabilityReturned: {
+ const auto *From = cast<CapabilityReturnedAttr>(FromAttr);
AI.importAttr(From, AI.importArg(From->getArg()).value());
break;
}
- case attr::LocksExcluded: {
- const auto *From = cast<LocksExcludedAttr>(FromAttr);
+ case attr::CapabilitiesExcluded: {
+ const auto *From = cast<CapabilitiesExcludedAttr>(FromAttr);
AI.importAttr(From,
AI.importArrayArg(From->args(), From->args_size()).value(),
From->args_size());
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843c9bf83..46b58c415a795 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -887,7 +887,7 @@ class LockableFactEntry : public FactEntry {
}
};
-class ScopedLockableFactEntry : public FactEntry {
+class ScopedCapabilityFactEntry : public FactEntry {
private:
enum UnderlyingCapabilityKind {
UCK_Acquired, ///< Any kind of acquired capability.
@@ -903,7 +903,7 @@ class ScopedLockableFactEntry : public FactEntry {
SmallVector<UnderlyingCapability, 2> UnderlyingMutexes;
public:
- ScopedLockableFactEntry(const CapabilityExpr &CE, SourceLocation Loc)
+ ScopedCapabilityFactEntry(const CapabilityExpr &CE, SourceLocation Loc)
: FactEntry(CE, LK_Exclusive, Loc, Acquired) {}
void addLock(const CapabilityExpr &M) {
@@ -1812,7 +1812,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
assert(inserted.second && "Are we visiting the same expression again?");
if (isa<CXXConstructExpr>(Exp))
Self = Placeholder.first;
- if (TagT->getDecl()->hasAttr<ScopedLockableAttr>())
+ if (TagT->getDecl()->hasAttr<ScopedCapabilityAttr>())
Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
}
@@ -1896,8 +1896,8 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
break;
}
- case attr::LocksExcluded: {
- const auto *A = cast<LocksExcludedAttr>(At);
+ case attr::CapabilitiesExcluded: {
+ const auto *A = cast<CapabilitiesExcludedAttr>(At);
for (auto *Arg : A->args()) {
Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
// use for deferring a lock
@@ -1935,7 +1935,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
if (!Scp.shouldIgnore()) {
// Add the managing object as a dummy mutex, mapped to the underlying mutex.
- auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, Loc);
+ auto ScopedEntry = std::make_unique<ScopedCapabilityFactEntry>(Scp, Loc);
for (const auto &M : ExclusiveLocksToAdd)
ScopedEntry->addLock(M);
for (const auto &M : SharedLocksToAdd)
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 3e8c959ccee4f..bd7bca0916345 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -420,10 +420,10 @@ til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE,
CallingContext *Ctx,
const Expr *SelfE) {
if (CapabilityExprMode) {
- // Handle LOCK_RETURNED
+ // Handle CAPABILITY_RETURNED
if (const FunctionDecl *FD = CE->getDirectCallee()) {
FD = FD->getMostRecentDecl();
- if (LockReturnedAttr *At = FD->getAttr<LockReturnedAttr>()) {
+ if (CapabilityReturnedAttr *At = FD->getAttr<CapabilityReturnedAttr>()) {
CallingContext LRCallCtx(Ctx);
LRCallCtx.AttrDecl = CE->getDirectCallee();
LRCallCtx.SelfArg = SelfE;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 5fd8622c90dd8..4f9df1ed019be 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -339,7 +339,7 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
const CXXRecordDecl *RD = MD->getParent();
// FIXME -- need to check this again on template instantiation
if (!checkRecordDeclForAttr<CapabilityAttr>(RD) &&
- !checkRecordDeclForAttr<ScopedLockableAttr>(RD))
+ !checkRecordDeclForAttr<ScopedCapabilityAttr>(RD))
S.Diag(AL.getLoc(),
diag::warn_thread_attribute_not_on_capability_member)
<< AL << MD->getParent();
@@ -633,7 +633,8 @@ static void handleExclusiveTrylockFunctionAttr(Sema &S, Decl *D,
S.Context, AL, AL.getArgAsExpr(0), Args.data(), Args.size()));
}
-static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleCapabilityReturnedAttr(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
// check that the argument is lockable object
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
@@ -641,10 +642,11 @@ static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
if (Size == 0)
return;
- D->addAttr(::new (S.Context) LockReturnedAttr(S.Context, AL, Args[0]));
+ D->addAttr(::new (S.Context) CapabilityReturnedAttr(S.Context, AL, Args[0]));
}
-static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleCapabilitiesExcludedAttr(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
if (!AL.checkAtLeastNumArgs(S, 1))
return;
@@ -657,7 +659,7 @@ static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
Expr **StartArg = &Args[0];
D->addAttr(::new (S.Context)
- LocksExcludedAttr(S.Context, AL, StartArg, Size));
+ CapabilitiesExcludedAttr(S.Context, AL, StartArg, Size));
}
static bool checkFunctionConditionAttr(Sema &S, Decl *D, const ParsedAttr &AL,
@@ -6927,11 +6929,11 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
case ParsedAttr::AT_ExclusiveTrylockFunction:
handleExclusiveTrylockFunctionAttr(S, D, AL);
break;
- case ParsedAttr::AT_LockReturned:
- handleLockReturnedAttr(S, D, AL);
+ case ParsedAttr::AT_CapabilityReturned:
+ handleCapabilityReturnedAttr(S, D, AL);
break;
- case ParsedAttr::AT_LocksExcluded:
- handleLocksExcludedAttr(S, D, AL);
+ case ParsedAttr::AT_CapabilitiesExcluded:
+ handleCapabilitiesExcludedAttr(S, D, AL);
break;
case ParsedAttr::AT_SharedTrylockFunction:
handleSharedTrylockFunctionAttr(S, D, AL);
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 04b8d88cae217..8d059037f592c 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18792,9 +18792,9 @@ bool Sema::checkThisInStaticMemberFunctionAttributes(CXXMethodDecl *Method) {
} 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<CapabilityReturnedAttr>(A))
Arg = LR->getArg();
- else if (const auto *LE = dyn_cast<LocksExcludedAttr>(A))
+ else if (const auto *LE = dyn_cast<CapabilitiesExcludedAttr>(A))
Args = llvm::ArrayRef(LE->args_begin(), LE->args_size());
else if (const auto *RC = dyn_cast<RequiresCapabilityAttr>(A))
Args = llvm::ArrayRef(RC->args_begin(), RC->args_size());
diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
index 33f9c2f51363c..c5752a34a344c 100644
--- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -176,7 +176,7 @@
// CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
// CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
// CHECK-NEXT: SYCLSpecialClass (SubjectMatchRule_record)
-// CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
+// CHECK-NEXT: ScopedCapability (SubjectMatchRule_record)
// CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
// CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
// CHECK-NEXT: SpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
diff --git a/clang/test/Parser/cxx11-stmt-attributes.cpp b/clang/test/Parser/cxx11-stmt-attributes.cpp
index 75fb37ea9fb44..46454889c423b 100644
--- a/clang/test/Parser/cxx11-stmt-attributes.cpp
+++ b/clang/test/Parser/cxx11-stmt-attributes.cpp
@@ -52,6 +52,10 @@ void foo(int i) {
} catch (...) {
}
+ [[capability_returned]] try { // expected-warning {{unknown attribute 'capability_returned' ignored}}
+ } catch (...) {
+ }
+
[[weakref]] return; // expected-warning {{unknown attribute 'weakref' ignored}}
[[carries_dependency]] ; // expected-error {{'carries_dependency' attribute cannot be applied to a statement}}
diff --git a/clang/test/SemaCXX/thread-safety-annotations.h b/clang/test/SemaCXX/thread-safety-annotations.h
index d89bcf8ff4706..8073398cdaaac 100644
--- a/clang/test/SemaCXX/thread-safety-annotations.h
+++ b/clang/test/SemaCXX/thread-safety-annotations.h
@@ -11,6 +11,9 @@
#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_shared_capability(__VA_ARGS__)))
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((requires_capability(__VA_ARGS__)))
#define SHARED_LOCKS_REQUIRED(...) __attribute__((requires_shared_capability(__VA_ARGS__)))
+#define SCOPED_LOCKABLE __attribute__((scoped_capability))
+#define LOCK_RETURNED(x) __attribute__((capability_returned(x)))
+#define LOCKS_EXCLUDED(...) __attribute__((capabilities_excluded(__VA_ARGS__)))
#else
#define LOCKABLE __attribute__((lockable))
#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_exclusive_lock(__VA_ARGS__)))
@@ -22,6 +25,9 @@
#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((shared_trylock_function(__VA_ARGS__)))
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
#define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__)))
+#define SCOPED_LOCKABLE __attribute__((scoped_lockable))
+#define LOCK_RETURNED(x) __attribute__((lock_returned(x)))
+#define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
#endif
// Lock semantics only
@@ -35,9 +41,6 @@
#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
// Common
-#define SCOPED_LOCKABLE __attribute__((scoped_lockable))
#define ACQUIRED_AFTER(...) __attribute__((acquired_after(__VA_ARGS__)))
#define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
-#define LOCK_RETURNED(x) __attribute__((lock_returned(x)))
-#define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 92f9bae6cb064..bfa558777c798 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -7819,10 +7819,10 @@ TEST_P(ImportAttributes, ImportPtGuardedVar) {
ToAttr);
}
-TEST_P(ImportAttributes, ImportScopedLockable) {
- ScopedLockableAttr *FromAttr, *ToAttr;
- importAttr<CXXRecordDecl>("struct __attribute__((scoped_lockable)) test {};",
- FromAttr, ToAttr);
+TEST_P(ImportAttributes, ImportScopedCapability) {
+ ScopedCapabilityAttr *FromAttr, *ToAttr;
+ importAttr<CXXRecordDecl>(
+ "struct __attribute__((scoped_capability)) test {};", FromAttr, ToAttr);
}
TEST_P(ImportAttributes, ImportCapability) {
@@ -7965,19 +7965,19 @@ TEST_P(ImportAttributes, ImportSharedTrylockFunction) {
checkImportVariadicArg(FromAttr->args(), ToAttr->args());
}
-TEST_P(ImportAttributes, ImportLockReturned) {
- LockReturnedAttr *FromAttr, *ToAttr;
+TEST_P(ImportAttributes, ImportCapabilityReturned) {
+ CapabilityReturnedAttr *FromAttr, *ToAttr;
importAttr<FunctionDecl>(
- "void test(int A1) __attribute__((lock_returned(A1)));", FromAttr,
+ "void test(int A1) __attribute__((capability_returned(A1)));", FromAttr,
ToAttr);
checkImported(FromAttr->getArg(), ToAttr->getArg());
}
-TEST_P(ImportAttributes, ImportLocksExcluded) {
- LocksExcludedAttr *FromAttr, *ToAttr;
- importAttr<FunctionDecl>(
- "void test(int A1, int A2) __attribute__((locks_excluded(A1, A2)));",
- FromAttr, ToAttr);
+TEST_P(ImportAttributes, ImportCapabilitiesExcluded) {
+ CapabilitiesExcludedAttr *FromAttr, *ToAttr;
+ importAttr<FunctionDecl>("void test(int A1, int A2) "
+ "__attribute__((capabilities_excluded(A1, A2)));",
+ FromAttr, ToAttr);
checkImportVariadicArg(FromAttr->args(), ToAttr->args());
}
|
… attributes. For some reason some attributes weren't renamed in code but were documented with the new names. I am not sure why this was the case and maybe I am missing something. Those are: - scoped_lockable -> scoped_capability - lock_returned -> capability_returned - locks_excluded -> capabilities_excluded This patch makes it so that we support the new name as documented. And change the test to support the old and new name like for other attributes.
| .bind("scoped-lockable"); | ||
| auto ScopedCapability = varDecl(hasType(hasCanonicalType(hasDeclaration( | ||
| hasAttr(attr::Kind::ScopedCapability))))) | ||
| .bind("scoped-lockable"); |
There was a problem hiding this comment.
Should we rename the binding name as well (and update its uses)?
They are already sort of documented as such – but will update the ReleaseNotes.rst. Does anyone know why the new names were documented but not actually implemented? |
I think this was my oversight -- I can't find any discussion where we intentionally made a decision to do this. CC @delesley who also worked on this at the same time. |
| @@ -887,7 +887,7 @@ class LockableFactEntry : public FactEntry { | |||
| } | |||
| }; | |||
|
|
|||
| class ScopedLockableFactEntry : public FactEntry { | |||
| class ScopedCapabilityFactEntry : public FactEntry { | |||
There was a problem hiding this comment.
The other derived class is still called LockableFactEntry. We should either leave this or change both.
|
You should also adapt the warning message for |
|
Was this dropped? It'd be nice to see this change land. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some reason some attributes weren't renamed in code but were
documented with the new names. I am not sure why this was the case and
maybe I am missing something.
Those are:
This patch makes it so that we support the new name as documented. And
change the test to support the old and new name like for other
attributes.