[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:06:50 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>
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/5] [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/5] 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/5] 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/5] 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/5] 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}}
More information about the cfe-commits
mailing list