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/67520 >From e89c66ced10f0d785d41d3b11c8447388c41e16e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Thu, 20 Jun 2024 07:39:20 +0200 Subject: [PATCH 1/2] Warn on RequiresCapability attribute mismatch --- .../clang/Analysis/Analyses/ThreadSafety.h | 3 +++ .../clang/Basic/DiagnosticSemaKinds.td | 4 ++- clang/lib/AST/ByteCode/Function.cpp | 1 + clang/lib/Analysis/ThreadSafety.cpp | 25 +++++++++++++++++++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 10 ++++++++ .../SemaCXX/warn-thread-safety-analysis.cpp | 12 ++++----- 6 files changed, 48 insertions(+), 7 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 0866b09bab2995e..8211f55b088906b 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -230,6 +230,9 @@ class ThreadSafetyHandler { /// Warn that there is a cycle in acquired_before/after dependencies. virtual void handleBeforeAfterCycle(Name L1Name, SourceLocation Loc) {} + virtual void handleAttributeMismatch(const NamedDecl *D1, + const NamedDecl *D2) {} + /// Called by the analysis when starting analysis of a function. /// Used to issue suggestions for changes to annotations. virtual void enterFunction(const FunctionDecl *FD) {} diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 157d77b38b354eb..b6e45bb9655ac33 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4004,6 +4004,9 @@ def err_attribute_argument_out_of_bounds_extra_info : Error< "%plural{0:no parameters to index into|" "1:can only be 1, since there is one parameter|" ":must be between 1 and %2}2">; +def warn_attribute_mismatch : Warning< + "attribute mismatch between function declarations of %0">, + InGroup<ThreadSafetyAttributes>, DefaultIgnore; // Thread Safety Analysis def warn_unlock_but_no_lock : Warning<"releasing %0 '%1' that was not held">, @@ -4055,7 +4058,6 @@ def warn_acquired_before_after_cycle : Warning< "cycle in acquired_before/after dependencies, starting with '%0'">, InGroup<ThreadSafetyAnalysis>, DefaultIgnore; - // Thread safety warnings negative capabilities def warn_acquire_requires_negative_cap : Warning< "acquiring %0 '%1' requires negative capability '%2'">, diff --git a/clang/lib/AST/ByteCode/Function.cpp b/clang/lib/AST/ByteCode/Function.cpp index 896a4fb3f9469ae..3b3a488bbcfcb3b 100644 --- a/clang/lib/AST/ByteCode/Function.cpp +++ b/clang/lib/AST/ByteCode/Function.cpp @@ -34,6 +34,7 @@ Function::ParamDescriptor Function::getParamDescriptor(unsigned Offset) const { } SourceInfo Function::getSource(CodePtr PC) const { + llvm::errs() << __PRETTY_FUNCTION__ << '\n'; assert(PC >= getCodeBegin() && "PC does not belong to this function"); assert(PC <= getCodeEnd() && "PC Does not belong to this function"); assert(hasBody() && "Function has no body"); diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 5577f45aa5217f7..9104058a6fe8601 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1073,6 +1073,8 @@ class ThreadSafetyAnalyzer { ProtectedOperationKind POK); void checkPtAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, ProtectedOperationKind POK); + + void checkMismatchedFunctionAttrs(const NamedDecl *ND); }; } // namespace @@ -2263,6 +2265,27 @@ static bool neverReturns(const CFGBlock *B) { return false; } +void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) { + auto collectCapabilities = [&](const Decl *D) { + CapExprSet Caps; + for (const auto *A : D->specific_attrs<RequiresCapabilityAttr>()) { + for (const Expr *E : A->args()) + Caps.push_back_nodup(SxBuilder.translateAttrExpr(E, nullptr)); + } + return Caps; + }; + + CapExprSet NDArgs = collectCapabilities(ND); + for (const Decl *D = ND->getPreviousDecl(); D; D = D->getPreviousDecl()) { + CapExprSet DArgs = collectCapabilities(D); + + for (const auto &[A, B] : zip_longest(NDArgs, DArgs)) { + if (!A || !B || !A->equals(*B)) + Handler.handleAttributeMismatch(ND, cast<NamedDecl>(D)); + } + } +} + /// Check a function's CFG for thread-safety violations. /// /// We traverse the blocks in the CFG, compute the set of mutexes that are held @@ -2282,6 +2305,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const NamedDecl *D = walker.getDecl(); CurrentFunction = dyn_cast<FunctionDecl>(D); + checkMismatchedFunctionAttrs(D); + if (D->hasAttr<NoThreadSafetyAnalysisAttr>()) return; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 37d966a5a046381..b0deb766ec496c5 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2065,6 +2065,16 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { Warnings.emplace_back(std::move(Warning), getNotes()); } + void handleAttributeMismatch(const NamedDecl *ThisDecl, + const NamedDecl *PrevDecl) override { + PartialDiagnosticAt Warning(ThisDecl->getLocation(), + S.PDiag(diag::warn_attribute_mismatch) + << ThisDecl); + PartialDiagnosticAt Note(PrevDecl->getLocation(), + S.PDiag(diag::note_previous_decl) << PrevDecl); + Warnings.emplace_back(std::move(Warning), getNotes(Note)); + } + void enterFunction(const FunctionDecl* FD) override { CurrentFunction = FD; } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 8477200456d985d..d6c5e03647fb8ad 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2198,8 +2198,8 @@ namespace FunctionDefinitionTest { class Foo { public: void foo1(); - void foo2(); - void foo3(Foo *other); + void foo2() EXCLUSIVE_LOCKS_REQUIRED(mu_); + void foo3(Foo *other) EXCLUSIVE_LOCKS_REQUIRED(other->mu_); template<class T> void fooT1(const T& dummy1); @@ -2249,8 +2249,8 @@ void fooF1(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { f->a = 1; } -void fooF2(Foo *f); -void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { +void fooF2(Foo *f); // expected-note {{declared here}} +void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { // expected-warning {{attribute mismatch between function declarations of 'fooF2'}} f->a = 2; } @@ -2269,9 +2269,9 @@ void test() { Foo myFoo; myFoo.foo2(); // \ - // expected-warning {{calling function 'foo2' requires holding mutex 'myFoo.mu_' exclusively}} + // expected-warning 2{{calling function 'foo2' requires holding mutex 'myFoo.mu_' exclusively}} myFoo.foo3(&myFoo); // \ - // expected-warning {{calling function 'foo3' requires holding mutex 'myFoo.mu_' exclusively}} + // expected-warning 2{{calling function 'foo3' requires holding mutex 'myFoo.mu_' exclusively}} myFoo.fooT1(dummy); // \ // expected-warning {{calling function 'fooT1<int>' requires holding mutex 'myFoo.mu_' exclusively}} >From a29f2403b3caba49c01237150c2f8635a1a08afc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Mon, 25 Nov 2024 11:16:32 +0100 Subject: [PATCH 2/2] Check ReleaseCapabilityAttr as well --- clang/lib/Analysis/ThreadSafety.cpp | 40 ++++++++++++++++++----------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 9104058a6fe8601..5784c1c470eb444 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2265,27 +2265,37 @@ static bool neverReturns(const CFGBlock *B) { return false; } -void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) { - auto collectCapabilities = [&](const Decl *D) { - CapExprSet Caps; - for (const auto *A : D->specific_attrs<RequiresCapabilityAttr>()) { - for (const Expr *E : A->args()) - Caps.push_back_nodup(SxBuilder.translateAttrExpr(E, nullptr)); - } - return Caps; - }; +template <typename AttrT> +static CapExprSet collectAttrArgs(SExprBuilder &SxBuilder, const Decl *D) { + CapExprSet Caps; + for (const auto *A : D->specific_attrs<AttrT>()) { + for (const Expr *E : A->args()) + Caps.push_back_nodup(SxBuilder.translateAttrExpr(E, nullptr)); + } + return Caps; +} + +template <typename AttrT> +static void maybeDiagnoseFunctionAttrs(const NamedDecl *ND, + SExprBuilder &SxBuilder, + ThreadSafetyHandler &Handler) { - CapExprSet NDArgs = collectCapabilities(ND); + // FIXME: The diagnostic here is suboptimal. It would be better to print + // what attributes are missing in the first declaration. + CapExprSet NDArgs = collectAttrArgs<AttrT>(SxBuilder, ND); for (const Decl *D = ND->getPreviousDecl(); D; D = D->getPreviousDecl()) { - CapExprSet DArgs = collectCapabilities(D); + CapExprSet DArgs = collectAttrArgs<AttrT>(SxBuilder, D); - for (const auto &[A, B] : zip_longest(NDArgs, DArgs)) { - if (!A || !B || !A->equals(*B)) - Handler.handleAttributeMismatch(ND, cast<NamedDecl>(D)); - } + if (NDArgs.size() != DArgs.size()) + Handler.handleAttributeMismatch(ND, cast<NamedDecl>(D)); } } +void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) { + maybeDiagnoseFunctionAttrs<RequiresCapabilityAttr>(ND, SxBuilder, Handler); + maybeDiagnoseFunctionAttrs<ReleaseCapabilityAttr>(ND, SxBuilder, Handler); +} + /// Check a function's CFG for thread-safety violations. /// /// We traverse the blocks in the CFG, compute the set of mutexes that are held _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
