[clang] [clang][TSA] Make RequiresCapability a DeclOrType attribute (PR #67095)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 3 01:45:02 PDT 2023
Timm =?utf-8?q?Bäder?= <tbaeder at redhat.com>,
Timm =?utf-8?q?Bäder?= <tbaeder at redhat.com>,
Timm =?utf-8?q?Bäder?= <tbaeder at redhat.com>,
Timm =?utf-8?q?Bäder?= <tbaeder at redhat.com>,
Timm =?utf-8?q?Bäder?= <tbaeder at redhat.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/67095 at github.com>
https://github.com/tbaederr updated https://github.com/llvm/llvm-project/pull/67095
>From c0708670eac0a079c878e94093897a708ece044d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 22 Sep 2023 08:42:05 +0200
Subject: [PATCH 1/6] [clang][TSA] Make RequiresCapability a DeclOrType
attribute
---
clang/include/clang/Basic/Attr.td | 6 +++---
clang/test/Sema/warn-thread-safety-analysis.c | 11 +++++++++++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 60b549999c155e5..8963c2d3c660768 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3331,7 +3331,7 @@ def ReleaseCapability : InheritableAttr {
let Documentation = [ReleaseCapabilityDocs];
}
-def RequiresCapability : InheritableAttr {
+def RequiresCapability : DeclOrTypeAttr {
let Spellings = [Clang<"requires_capability", 0>,
Clang<"exclusive_locks_required", 0>,
Clang<"requires_shared_capability", 0>,
@@ -3340,8 +3340,8 @@ def RequiresCapability : InheritableAttr {
let LateParsed = 1;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
- let InheritEvenIfAlreadyPresent = 1;
- let Subjects = SubjectList<[Function]>;
+ /*let InheritEvenIfAlreadyPresent = 1;*/
+ /*let Subjects = SubjectList<[Function]>;*/
let Accessors = [Accessor<"isShared", [Clang<"requires_shared_capability", 0>,
Clang<"shared_locks_required", 0>]>];
let Documentation = [Undocumented];
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 642ea88ec3c96f7..cd852efac21cb81 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -136,6 +136,17 @@ int main(void) {
// Cleanup happens automatically -> no warning.
}
+ /// Function pointers
+ {
+ int __attribute__((requires_capability(&mu1))) (*function_ptr)(int) = Foo_fun1;
+
+ function_ptr(5); // expected-warning {{calling function 'function_ptr' requires holding mutex 'mu1'}}
+
+ mutex_exclusive_lock(&mu1);
+ int five = function_ptr(5);
+ mutex_exclusive_unlock(&mu1);
+ }
+
return 0;
}
>From a4be29c2a872cfaa1738bf1fee329a2c46473486 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Tue, 26 Sep 2023 09:57:41 +0200
Subject: [PATCH 2/6] Restrict to functions and function pointer decls
---
clang/include/clang/Basic/Attr.td | 2 +-
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 8963c2d3c660768..3f0c39e982017fa 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3340,7 +3340,7 @@ def RequiresCapability : DeclOrTypeAttr {
let LateParsed = 1;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
- /*let InheritEvenIfAlreadyPresent = 1;*/
+ let InheritEvenIfAlreadyPresent = 1;
/*let Subjects = SubjectList<[Function]>;*/
let Accessors = [Accessor<"isShared", [Clang<"requires_shared_capability", 0>,
Clang<"shared_locks_required", 0>]>];
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 842a01a88cd3c6d..dd053b73be635bb 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8187,6 +8187,16 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl *D,
if (!AL.checkAtLeastNumArgs(S, 1))
return;
+ // We allow this on function declaration as well as
+ // variable declarations of function pointer type.
+ if (!D->isFunctionPointerType() && !isa<FunctionDecl>(D)) {
+ // FIXME: Diagnostic should say "functions and function pointer decls" now I
+ // guess.
+ S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+ << AL << AL.isRegularKeywordAttribute() << ExpectedFunction;
+ return;
+ }
+
// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
>From 5032a3d25ee404884965a8cc8e20685ccf20b51a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 13 Oct 2023 10:23:43 +0200
Subject: [PATCH 3/6] Add processing to SemaType.cpp
---
clang/include/clang/Basic/Attr.td | 2 +-
clang/lib/Sema/SemaDeclAttr.cpp | 10 ----------
clang/lib/Sema/SemaType.cpp | 30 ++++++++++++++++++++++++++++++
3 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 3f0c39e982017fa..bce0e2f2ac9f167 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3341,7 +3341,7 @@ def RequiresCapability : DeclOrTypeAttr {
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
- /*let Subjects = SubjectList<[Function]>;*/
+ let Subjects = SubjectList<[Function, FunctionPointer]>;
let Accessors = [Accessor<"isShared", [Clang<"requires_shared_capability", 0>,
Clang<"shared_locks_required", 0>]>];
let Documentation = [Undocumented];
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index dd053b73be635bb..842a01a88cd3c6d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8187,16 +8187,6 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl *D,
if (!AL.checkAtLeastNumArgs(S, 1))
return;
- // We allow this on function declaration as well as
- // variable declarations of function pointer type.
- if (!D->isFunctionPointerType() && !isa<FunctionDecl>(D)) {
- // FIXME: Diagnostic should say "functions and function pointer decls" now I
- // guess.
- S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
- << AL << AL.isRegularKeywordAttribute() << ExpectedFunction;
- return;
- }
-
// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index a46deed8e7c58b4..eea5bd208ec3166 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8642,6 +8642,31 @@ static void HandleAnnotateTypeAttr(TypeProcessingState &State,
CurType = State.getAttributedType(AnnotateTypeAttr, CurType, CurType);
}
+static void HandleRequiresCapabilityAttr(TypeProcessingState &State,
+ QualType &CurType,
+ const ParsedAttr &PA) {
+ Sema &S = State.getSema();
+
+ if (PA.getNumArgs() < 1) {
+ S.Diag(PA.getLoc(), diag::err_attribute_too_few_arguments) << PA << 1;
+ return;
+ }
+
+ // FIXME: We need to sanity check the arguments here I think? Like we do in
+ // SemaDeclAtr.cpp.
+
+ llvm::SmallVector<Expr *, 4> Args;
+ Args.reserve(PA.getNumArgs() - 1);
+ for (unsigned Idx = 1; Idx < PA.getNumArgs(); Idx++) {
+ assert(!PA.isArgIdent(Idx));
+ Args.push_back(PA.getArgAsExpr(Idx));
+ }
+
+ auto *RCAttr =
+ RequiresCapabilityAttr::Create(S.Context, Args.data(), Args.size(), PA);
+ CurType = State.getAttributedType(RCAttr, CurType, CurType);
+}
+
static void HandleLifetimeBoundAttr(TypeProcessingState &State,
QualType &CurType,
ParsedAttr &Attr) {
@@ -8938,6 +8963,11 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type,
attr.setUsedAsTypeAttr();
break;
}
+ case ParsedAttr::AT_RequiresCapability: {
+ HandleRequiresCapabilityAttr(state, type, attr);
+ attr.setUsedAsTypeAttr();
+ break;
+ }
}
// Handle attributes that are defined in a macro. We do not want this to be
>From 59b558d8e94be3b6c7256eb53b21cda6a87fbbb9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 3 Nov 2023 06:08:31 +0100
Subject: [PATCH 4/6] Remove double diagnostics
---
clang/lib/Sema/SemaType.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index eea5bd208ec3166..839e6b878d223cf 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8648,7 +8648,7 @@ static void HandleRequiresCapabilityAttr(TypeProcessingState &State,
Sema &S = State.getSema();
if (PA.getNumArgs() < 1) {
- S.Diag(PA.getLoc(), diag::err_attribute_too_few_arguments) << PA << 1;
+ // Already diganosed elsewhere, just ignore.
return;
}
>From 8b52d2318262d0064f2addbb87a1ead3c9fe1049 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 3 Nov 2023 09:06:13 +0100
Subject: [PATCH 5/6] Add a function pointer test case
---
clang/test/SemaCXX/warn-thread-safety-parsing.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
index 0c5b0cc85897bee..bc8586b4a9586b2 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -1093,6 +1093,8 @@ void elr_function_args() EXCLUSIVE_LOCKS_REQUIRED(mu1, mu2);
int elr_testfn(int y) EXCLUSIVE_LOCKS_REQUIRED(mu1);
+int EXCLUSIVE_LOCKS_REQUIRED(mu1) (*function_ptr)(int);
+
int elr_testfn(int y) {
int x EXCLUSIVE_LOCKS_REQUIRED(mu1) = y; // \
// expected-warning {{'exclusive_locks_required' attribute only applies to functions}}
>From 4d0bc2c0b08f505b475d342946fd23080e928ac3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 3 Nov 2023 09:44:37 +0100
Subject: [PATCH 6/6] check type attribute arguments as well
---
clang/include/clang/Sema/Sema.h | 5 ++++
clang/lib/AST/TypePrinter.cpp | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 43 +++++++++++++++------------------
clang/lib/Sema/SemaType.cpp | 8 +-----
4 files changed, 28 insertions(+), 30 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index daed24be0a86d11..a5b315145e3fc04 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13868,6 +13868,11 @@ class Sema final {
SourceLocation BuiltinLoc,
SourceLocation RParenLoc);
+ void checkAttrArgsAreCapabilityObjs(Decl *D, const ParsedAttr &AL,
+ SmallVectorImpl<Expr *> &Args,
+ unsigned Sidx = 0,
+ bool ParamIdxOk = false);
+
private:
bool SemaBuiltinPrefetch(CallExpr *TheCall);
bool SemaBuiltinAllocaWithAlign(CallExpr *TheCall);
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index e4f5f40cd625996..2d62b6a5bd85133 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1894,6 +1894,8 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::ArmMveStrictPolymorphism:
OS << "__clang_arm_mve_strict_polymorphism";
break;
+ case attr::RequiresCapability:
+ OS << "requires_capability(blah)";
}
OS << "))";
}
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 842a01a88cd3c6d..e45b0f20d745827 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -600,12 +600,10 @@ static bool isCapabilityExpr(Sema &S, const Expr *Ex) {
/// \param Sidx The attribute argument index to start checking with.
/// \param ParamIdxOk Whether an argument can be indexing into a function
/// parameter list.
-static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
- const ParsedAttr &AL,
- SmallVectorImpl<Expr *> &Args,
- unsigned Sidx = 0,
- bool ParamIdxOk = false) {
- if (Sidx == AL.getNumArgs()) {
+void Sema::checkAttrArgsAreCapabilityObjs(Decl *D, const ParsedAttr &AL,
+ SmallVectorImpl<Expr *> &Args,
+ unsigned Sidx, bool ParamIdxOk) {
+ if (D && Sidx == AL.getNumArgs()) {
// If we don't have any capability arguments, the attribute implicitly
// refers to 'this'. So we need to make sure that 'this' exists, i.e. we're
// a non-static method, and that the class is a (scoped) capability.
@@ -615,11 +613,10 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
// FIXME -- need to check this again on template instantiation
if (!checkRecordDeclForAttr<CapabilityAttr>(RD) &&
!checkRecordDeclForAttr<ScopedLockableAttr>(RD))
- S.Diag(AL.getLoc(),
- diag::warn_thread_attribute_not_on_capability_member)
+ Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_capability_member)
<< AL << MD->getParent();
} else {
- S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
+ Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
<< AL;
}
}
@@ -644,7 +641,7 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
// We allow constant strings to be used as a placeholder for expressions
// that are not valid C++ syntax, but warn that they are ignored.
- S.Diag(AL.getLoc(), diag::warn_thread_attribute_ignored) << AL;
+ Diag(AL.getLoc(), diag::warn_thread_attribute_ignored) << AL;
Args.push_back(ArgExp);
continue;
}
@@ -663,7 +660,7 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
const RecordType *RT = getRecordType(ArgTy);
// Now check if we index into a record type function param.
- if(!RT && ParamIdxOk) {
+ if (D && !RT && ParamIdxOk) {
const auto *FD = dyn_cast<FunctionDecl>(D);
const auto *IL = dyn_cast<IntegerLiteral>(ArgExp);
if(FD && IL) {
@@ -672,8 +669,8 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
uint64_t ParamIdxFromOne = ArgValue.getZExtValue();
uint64_t ParamIdxFromZero = ParamIdxFromOne - 1;
if (!ArgValue.isStrictlyPositive() || ParamIdxFromOne > NumParams) {
- S.Diag(AL.getLoc(),
- diag::err_attribute_argument_out_of_bounds_extra_info)
+ Diag(AL.getLoc(),
+ diag::err_attribute_argument_out_of_bounds_extra_info)
<< AL << Idx + 1 << NumParams;
continue;
}
@@ -685,8 +682,8 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
// expression have capabilities. This allows for writing C code where the
// capability may be on the type, and the expression is a capability
// boolean logic expression. Eg) requires_capability(A || B && !C)
- if (!typeHasCapability(S, ArgTy) && !isCapabilityExpr(S, ArgExp))
- S.Diag(AL.getLoc(), diag::warn_thread_attribute_argument_not_lockable)
+ if (!typeHasCapability(*this, ArgTy) && !isCapabilityExpr(*this, ArgExp))
+ Diag(AL.getLoc(), diag::warn_thread_attribute_argument_not_lockable)
<< AL << ArgTy;
Args.push_back(ArgExp);
@@ -708,7 +705,7 @@ 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);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size != 1)
return false;
@@ -750,7 +747,7 @@ static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
}
// Check that all arguments are lockable objects.
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
if (Args.empty())
return false;
@@ -781,7 +778,7 @@ static bool checkLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args) {
// zero or more arguments ok
// check that all arguments are lockable objects
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 0, /*ParamIdxOk=*/true);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args, 0, /*ParamIdxOk=*/true);
return true;
}
@@ -883,7 +880,7 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
}
// check that all arguments are lockable objects
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args, 1);
return true;
}
@@ -911,7 +908,7 @@ static void handleExclusiveTrylockFunctionAttr(Sema &S, Decl *D,
static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
// check that the argument is lockable object
SmallVector<Expr*, 1> Args;
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size == 0)
return;
@@ -925,7 +922,7 @@ static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
unsigned Size = Args.size();
if (Size == 0)
return;
@@ -8176,7 +8173,7 @@ static void handleReleaseCapabilityAttr(Sema &S, Decl *D,
const ParsedAttr &AL) {
// Check that all arguments are lockable objects.
SmallVector<Expr *, 1> Args;
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 0, true);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args, 0, true);
D->addAttr(::new (S.Context) ReleaseCapabilityAttr(S.Context, AL, Args.data(),
Args.size()));
@@ -8189,7 +8186,7 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl *D,
// check that all arguments are lockable objects
SmallVector<Expr*, 1> Args;
- checkAttrArgsAreCapabilityObjs(S, D, AL, Args);
+ S.checkAttrArgsAreCapabilityObjs(D, AL, Args);
if (Args.empty())
return;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 839e6b878d223cf..577f99da6adc86a 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8652,15 +8652,9 @@ static void HandleRequiresCapabilityAttr(TypeProcessingState &State,
return;
}
- // FIXME: We need to sanity check the arguments here I think? Like we do in
- // SemaDeclAtr.cpp.
-
llvm::SmallVector<Expr *, 4> Args;
Args.reserve(PA.getNumArgs() - 1);
- for (unsigned Idx = 1; Idx < PA.getNumArgs(); Idx++) {
- assert(!PA.isArgIdent(Idx));
- Args.push_back(PA.getArgAsExpr(Idx));
- }
+ State.getSema().checkAttrArgsAreCapabilityObjs(/*Decl=*/nullptr, PA, Args);
auto *RCAttr =
RequiresCapabilityAttr::Create(S.Context, Args.data(), Args.size(), PA);
More information about the cfe-commits
mailing list