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>, 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 1c117257921fc1c246fbb9f51e3c95d6dc6d295e 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/7] 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 64e6d0407b0ce3..f218a82122c881 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4031,7 +4031,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 f3ccd5cc229ba4d6476efeb04701ade4c0d844e5 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/7] 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 f218a82122c881..64e6d0407b0ce3 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4031,9 +4031,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 804ed367fc75af0c2648e99335040151b206a051 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/7] 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 64e6d0407b0ce3..1bf13eb2d9b90e 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4031,6 +4031,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) { >From f59156611313793fba190f88262be52bcc88370c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Tue, 1 Oct 2024 15:44:19 +0200 Subject: [PATCH 4/7] Revert "Warn on RequiresCapability attribute mismatches between declarations" This reverts commit 804ed367fc75af0c2648e99335040151b206a051. --- .../clang/Basic/DiagnosticSemaKinds.td | 3 --- clang/lib/Sema/SemaDeclAttr.cpp | 24 ------------------- 2 files changed, 27 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 1bf13eb2d9b90e..64e6d0407b0ce3 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4031,9 +4031,6 @@ 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 98147101487ec4..c9b9f3a0007daa 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -5794,30 +5794,6 @@ 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) { >From cf9728c57e2ffec67a59d748a54f573d5cacac08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Tue, 1 Oct 2024 15:44:29 +0200 Subject: [PATCH 5/7] Reapply "Warn on RequiresCapability attribute mismatch" This reverts commit f3ccd5cc229ba4d6476efeb04701ade4c0d844e5. --- .../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 64e6d0407b0ce3..f218a82122c881 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4031,7 +4031,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 d78dbaba7274701c2a89cbf87565d0f380eb334a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Tue, 1 Oct 2024 17:18:42 +0200 Subject: [PATCH 6/7] - --- clang/lib/Analysis/ThreadSafety.cpp | 43 ++++++++++++----------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 25c63f130ca75e..01ef18f7eba373 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 FunctionDecl *FD); }; } // namespace @@ -2263,34 +2265,25 @@ 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); - } +void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs( + const FunctionDecl *FD) { + FD = FD->getMostRecentDecl(); - return Args; -} - -static void diagnoseMismatchedFunctionAttrs(const FunctionDecl *FD, - ThreadSafetyHandler &Handler) { - assert(FD); - FD = FD->getDefinition(); - assert(FD); - auto FDArgs = collectAttrArgs<RequiresCapabilityAttr>(FD); + auto collectCapabilities = [&](const FunctionDecl *FD) { + SmallVector<CapabilityExpr> Args; + for (const auto *A : FD->specific_attrs<RequiresCapabilityAttr>()) { + for (const Expr *E : A->args()) + Args.push_back(SxBuilder.translateAttrExpr(E, nullptr)); + } + return Args; + }; + auto FDArgs = collectCapabilities(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); - } - } + auto DArgs = collectCapabilities(D); + if (DArgs.size() != FDArgs.size()) + Handler.handleAttributeMismatch(FD, D); } } @@ -2314,7 +2307,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { CurrentFunction = dyn_cast<FunctionDecl>(D); if (CurrentFunction) - diagnoseMismatchedFunctionAttrs(CurrentFunction, Handler); + checkMismatchedFunctionAttrs(CurrentFunction); if (D->hasAttr<NoThreadSafetyAnalysisAttr>()) return; >From a22c24394cd038347cceab0811c7e59e5275f3ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Fri, 4 Oct 2024 10:45:24 +0200 Subject: [PATCH 7/7] partially address review feedback --- .../clang/Analysis/Analyses/ThreadSafety.h | 5 ++-- .../clang/Basic/DiagnosticSemaKinds.td | 4 +-- clang/lib/Analysis/ThreadSafety.cpp | 28 ++++++++++--------- clang/lib/Sema/AnalysisBasedWarnings.cpp | 11 +++++--- .../SemaCXX/warn-thread-safety-analysis.cpp | 6 ++-- 5 files changed, 29 insertions(+), 25 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 169eef811f5793..8211f55b088906 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,8 +230,8 @@ 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) {} + 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. diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index f218a82122c881..829912735ff009 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4032,8 +4032,8 @@ 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; + "attribute mismatch between function declarations of %0">, + InGroup<ThreadSafetyAttributes>, 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 01ef18f7eba373..10af95184b1110 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1074,7 +1074,7 @@ class ThreadSafetyAnalyzer { void checkPtAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, ProtectedOperationKind POK); - void checkMismatchedFunctionAttrs(const FunctionDecl *FD); + void checkMismatchedFunctionAttrs(const NamedDecl *ND); }; } // namespace @@ -2265,25 +2265,27 @@ static bool neverReturns(const CFGBlock *B) { return false; } -void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs( - const FunctionDecl *FD) { - FD = FD->getMostRecentDecl(); +void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs( const NamedDecl *ND) { - auto collectCapabilities = [&](const FunctionDecl *FD) { - SmallVector<CapabilityExpr> Args; - for (const auto *A : FD->specific_attrs<RequiresCapabilityAttr>()) { + auto collectCapabilities = [&](const Decl *D) { + llvm::SmallVector<CapabilityExpr> Args; + for (const auto *A : D->specific_attrs<RequiresCapabilityAttr>()) { for (const Expr *E : A->args()) Args.push_back(SxBuilder.translateAttrExpr(E, nullptr)); } return Args; }; - auto FDArgs = collectCapabilities(FD); - for (const FunctionDecl *D = FD->getPreviousDecl(); D; + auto NDArgs = collectCapabilities(ND); + for (const Decl *D = ND->getPreviousDecl(); D; D = D->getPreviousDecl()) { auto DArgs = collectCapabilities(D); - if (DArgs.size() != FDArgs.size()) - Handler.handleAttributeMismatch(FD, D); + + for (const auto &[A, B] : zip_longest(NDArgs, DArgs)) { + if (!A || !B || !(*A).equals(*B)) { + Handler.handleAttributeMismatch(cast<NamedDecl>(ND), cast<NamedDecl>(D)); + } + } } } @@ -2306,8 +2308,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const NamedDecl *D = walker.getDecl(); CurrentFunction = dyn_cast<FunctionDecl>(D); - if (CurrentFunction) - checkMismatchedFunctionAttrs(CurrentFunction); + if (isa<FunctionDecl, ObjCMethodDecl>(D)) + checkMismatchedFunctionAttrs(D); if (D->hasAttr<NoThreadSafetyAnalysisAttr>()) return; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index fb4f97096fb962..b76e0f043ed362 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2073,11 +2073,14 @@ 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); + void handleAttributeMismatch(const NamedDecl *D1, + const NamedDecl *D2) override { + PartialDiagnosticAt Warning(D2->getLocation(), + S.PDiag(diag::warn_attribute_mismatch) << D1); Warnings.emplace_back(std::move(Warning), getNotes()); + + PartialDiagnosticAt Note(D1->getLocation(), S.PDiag(diag::note_previous_decl) << D2); + Warnings.emplace_back(std::move(Note), getNotes()); } void enterFunction(const FunctionDecl* FD) override { diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 8477200456d985..1b2caec2c26455 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,7 +2249,7 @@ void fooF1(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { f->a = 1; } -void fooF2(Foo *f); +void fooF2(Foo *f); // expected-warning {{attribute mismatch between function declarations of 'fooF2'}} void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { f->a = 2; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
