Timm =?utf-8?q?Bäder?= <tbae...@redhat.com>, Timm =?utf-8?q?Bäder?= <tbae...@redhat.com>, Timm =?utf-8?q?Bäder?= <tbae...@redhat.com>, Timm =?utf-8?q?Bäder?= <tbae...@redhat.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/67...@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?= <tbae...@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?= <tbae...@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?= <tbae...@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?= <tbae...@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?= <tbae...@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}} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
