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/67520 >From 5f411735a5366499481c09a317aa170af44796f3 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/3] Warn on RequiresCapability attribute mismatch --- .../clang/Analysis/Analyses/ThreadSafety.h | 4 +++ .../clang/Basic/DiagnosticSemaKinds.td | 4 ++- clang/lib/Analysis/ThreadSafety.cpp | 34 +++++++++++++++++++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 ++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 0866b09bab2995..169eef811f5793 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -26,6 +26,7 @@ namespace clang { class AnalysisDeclContext; class FunctionDecl; class NamedDecl; +class Attr; namespace threadSafety { @@ -230,6 +231,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 FunctionDecl *FD1, + const FunctionDecl *FD2) {} + /// 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 9e8f152852fd1e..6c5a4a6499edba 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4033,7 +4033,9 @@ def warn_acquired_before : Warning< def warn_acquired_before_after_cycle : Warning< "cycle in acquired_before/after dependencies, starting with '%0'">, InGroup<ThreadSafetyAnalysis>, DefaultIgnore; - +def warn_attribute_mismatch : Warning< + "attribute mismatch between function definition and declaration of %0">, + InGroup<ThreadSafetyAnalysis>, DefaultIgnore; // Thread safety warnings negative capabilities def warn_acquire_requires_negative_cap : Warning< diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 5577f45aa5217f..25c63f130ca75e 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2263,6 +2263,37 @@ static bool neverReturns(const CFGBlock *B) { return false; } +template <typename AttrT> +static SmallVector<const Expr *> collectAttrArgs(const FunctionDecl *FD) { + SmallVector<const Expr *> Args; + for (const AttrT *A : FD->specific_attrs<AttrT>()) { + for (const Expr *E : A->args()) + Args.push_back(E); + } + + return Args; +} + +static void diagnoseMismatchedFunctionAttrs(const FunctionDecl *FD, + ThreadSafetyHandler &Handler) { + assert(FD); + FD = FD->getDefinition(); + assert(FD); + auto FDArgs = collectAttrArgs<RequiresCapabilityAttr>(FD); + + for (const FunctionDecl *D = FD->getPreviousDecl(); D; + D = D->getPreviousDecl()) { + auto DArgs = collectAttrArgs<RequiresCapabilityAttr>(D); + + for (const Expr *E : FDArgs) { + if (!llvm::is_contained(DArgs, E)) { + // FD requires E, but D doesn't. + Handler.handleAttributeMismatch(FD, 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 +2313,9 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const NamedDecl *D = walker.getDecl(); CurrentFunction = dyn_cast<FunctionDecl>(D); + if (CurrentFunction) + diagnoseMismatchedFunctionAttrs(CurrentFunction, Handler); + if (D->hasAttr<NoThreadSafetyAnalysisAttr>()) return; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 6496a33b8f5a50..fb4f97096fb962 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2073,6 +2073,13 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { Warnings.emplace_back(std::move(Warning), getNotes()); } + void handleAttributeMismatch(const FunctionDecl *FD1, + const FunctionDecl *FD2) override { + PartialDiagnosticAt Warning(FD2->getLocation(), + S.PDiag(diag::warn_attribute_mismatch) << FD1); + Warnings.emplace_back(std::move(Warning), getNotes()); + } + void enterFunction(const FunctionDecl* FD) override { CurrentFunction = FD; } >From 7b583e1eae3c9620a33827784e453e2ee252792c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Mon, 30 Sep 2024 13:13:06 +0200 Subject: [PATCH 2/3] Revert "Warn on RequiresCapability attribute mismatch" This reverts commit 5f411735a5366499481c09a317aa170af44796f3. --- .../clang/Analysis/Analyses/ThreadSafety.h | 4 --- .../clang/Basic/DiagnosticSemaKinds.td | 4 +-- clang/lib/Analysis/ThreadSafety.cpp | 34 ------------------- clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 ---- 4 files changed, 1 insertion(+), 48 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 169eef811f5793..0866b09bab2995 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -26,7 +26,6 @@ namespace clang { class AnalysisDeclContext; class FunctionDecl; class NamedDecl; -class Attr; namespace threadSafety { @@ -231,9 +230,6 @@ class ThreadSafetyHandler { /// Warn that there is a cycle in acquired_before/after dependencies. virtual void handleBeforeAfterCycle(Name L1Name, SourceLocation Loc) {} - virtual void handleAttributeMismatch(const FunctionDecl *FD1, - const FunctionDecl *FD2) {} - /// 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 6c5a4a6499edba..9e8f152852fd1e 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4033,9 +4033,7 @@ def warn_acquired_before : Warning< def warn_acquired_before_after_cycle : Warning< "cycle in acquired_before/after dependencies, starting with '%0'">, InGroup<ThreadSafetyAnalysis>, DefaultIgnore; -def warn_attribute_mismatch : Warning< - "attribute mismatch between function definition and declaration of %0">, - InGroup<ThreadSafetyAnalysis>, DefaultIgnore; + // Thread safety warnings negative capabilities def warn_acquire_requires_negative_cap : Warning< diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 25c63f130ca75e..5577f45aa5217f 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2263,37 +2263,6 @@ static bool neverReturns(const CFGBlock *B) { return false; } -template <typename AttrT> -static SmallVector<const Expr *> collectAttrArgs(const FunctionDecl *FD) { - SmallVector<const Expr *> Args; - for (const AttrT *A : FD->specific_attrs<AttrT>()) { - for (const Expr *E : A->args()) - Args.push_back(E); - } - - return Args; -} - -static void diagnoseMismatchedFunctionAttrs(const FunctionDecl *FD, - ThreadSafetyHandler &Handler) { - assert(FD); - FD = FD->getDefinition(); - assert(FD); - auto FDArgs = collectAttrArgs<RequiresCapabilityAttr>(FD); - - for (const FunctionDecl *D = FD->getPreviousDecl(); D; - D = D->getPreviousDecl()) { - auto DArgs = collectAttrArgs<RequiresCapabilityAttr>(D); - - for (const Expr *E : FDArgs) { - if (!llvm::is_contained(DArgs, E)) { - // FD requires E, but D doesn't. - Handler.handleAttributeMismatch(FD, 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 @@ -2313,9 +2282,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const NamedDecl *D = walker.getDecl(); CurrentFunction = dyn_cast<FunctionDecl>(D); - if (CurrentFunction) - diagnoseMismatchedFunctionAttrs(CurrentFunction, Handler); - if (D->hasAttr<NoThreadSafetyAnalysisAttr>()) return; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index fb4f97096fb962..6496a33b8f5a50 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2073,13 +2073,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { Warnings.emplace_back(std::move(Warning), getNotes()); } - void handleAttributeMismatch(const FunctionDecl *FD1, - const FunctionDecl *FD2) override { - PartialDiagnosticAt Warning(FD2->getLocation(), - S.PDiag(diag::warn_attribute_mismatch) << FD1); - Warnings.emplace_back(std::move(Warning), getNotes()); - } - void enterFunction(const FunctionDecl* FD) override { CurrentFunction = FD; } >From 96f854242577f3e7f83cb0ea8bd8cde4605459fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Mon, 30 Sep 2024 14:48:58 +0200 Subject: [PATCH 3/3] Warn on RequiresCapability attribute mismatches between declarations --- .../clang/Basic/DiagnosticSemaKinds.td | 3 +++ clang/lib/Sema/SemaDeclAttr.cpp | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 9e8f152852fd1e..f15a5fc8d7067d 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4033,6 +4033,9 @@ def warn_acquired_before : Warning< def warn_acquired_before_after_cycle : Warning< "cycle in acquired_before/after dependencies, starting with '%0'">, InGroup<ThreadSafetyAnalysis>, DefaultIgnore; +def warn_attribute_mismatch : Warning< + "attribute mismatch between function declarations of %0">, + InGroup<ThreadSafetyAttributes>, DefaultIgnore; // Thread safety warnings negative capabilities diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index c9b9f3a0007daa..98147101487ec4 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -5794,6 +5794,30 @@ static void handleRequiresCapabilityAttr(Sema &S, Decl *D, RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size()); D->addAttr(RCA); + + if (const auto *FD = dyn_cast<FunctionDecl>(D)) { + + auto collectExprs = [](const FunctionDecl *FuncDecl) { + std::set<const ValueDecl*> Args; + for (const auto *A : FuncDecl->specific_attrs<RequiresCapabilityAttr>()) { + for (const Expr *E : A->args()) { + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + Args.insert(DRE->getDecl()); + } + } + return Args; + }; + auto ThisDecl = collectExprs(FD); + for (const FunctionDecl *P = FD->getPreviousDecl(); P; + P = P->getPreviousDecl()) { + auto PrevDecl = collectExprs(P); + // FIXME: It would be nice to mention _what_ attribute isn't matched and maybe + // even where the previous declaration was? + if (ThisDecl.size() != PrevDecl.size()) + S.Diag(D->getLocation(), diag::warn_attribute_mismatch) << FD; + } + + } } static void handleDeprecatedAttr(Sema &S, Decl *D, const ParsedAttr &AL) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
