[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 8 06:13:44 PDT 2024
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>,
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/67520 at github.com>
https://github.com/tbaederr updated https://github.com/llvm/llvm-project/pull/67520
>From 055f76253589623daaa8771330f5d53dc2969699 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Thu, 20 Jun 2024 07:39:20 +0200
Subject: [PATCH 1/8] 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 583475327c5227..a8a0c947ed9b94 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 ab1709e19298767efa1de4cb2d019f0f5a43f247 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Mon, 30 Sep 2024 13:13:06 +0200
Subject: [PATCH 2/8] 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 a8a0c947ed9b94..583475327c5227 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 e58aa081657897dc6c7a4c9b4ad9d56862ef7b95 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Mon, 30 Sep 2024 14:48:58 +0200
Subject: [PATCH 3/8] 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 583475327c5227..e85aed98527789 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 af983349a89b58..26fb39479d6549 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5842,6 +5842,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 36062185f62357bc808f7265964c337e343f0192 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Tue, 1 Oct 2024 15:44:19 +0200
Subject: [PATCH 4/8] 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 e85aed98527789..583475327c5227 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 26fb39479d6549..af983349a89b58 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5842,30 +5842,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 e941bd9426cdf25167f6c53e1b4962ff15baceb3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Tue, 1 Oct 2024 15:44:29 +0200
Subject: [PATCH 5/8] 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 583475327c5227..a8a0c947ed9b94 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 b9943118782c24c1e78091665c52a84926ed3050 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Tue, 1 Oct 2024 17:18:42 +0200
Subject: [PATCH 6/8] -
---
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 59b415bad2d1413fbe5c7dcc9f8724177224182d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 4 Oct 2024 10:45:24 +0200
Subject: [PATCH 7/8] 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 a8a0c947ed9b94..b57a2d6c899f8e 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;
}
>From f45cffe2300f0f986ec0a5ff2b3b6fea0aab168f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Tue, 8 Oct 2024 15:13:17 +0200
Subject: [PATCH 8/8] Use CapExprSet
---
clang/lib/Analysis/ThreadSafety.cpp | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 10af95184b1110..9941d9d6b1d7a9 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2268,12 +2268,12 @@ static bool neverReturns(const CFGBlock *B) {
void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs( const NamedDecl *ND) {
auto collectCapabilities = [&](const Decl *D) {
- llvm::SmallVector<CapabilityExpr> Args;
+ CapExprSet Caps;
for (const auto *A : D->specific_attrs<RequiresCapabilityAttr>()) {
for (const Expr *E : A->args())
- Args.push_back(SxBuilder.translateAttrExpr(E, nullptr));
+ Caps.push_back_nodup(SxBuilder.translateAttrExpr(E, nullptr));
}
- return Args;
+ return Caps;
};
auto NDArgs = collectCapabilities(ND);
@@ -2282,9 +2282,8 @@ void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs( const NamedDecl *ND) {
auto DArgs = collectCapabilities(D);
for (const auto &[A, B] : zip_longest(NDArgs, DArgs)) {
- if (!A || !B || !(*A).equals(*B)) {
+ if (!A || !B || !(*A).equals(*B))
Handler.handleAttributeMismatch(cast<NamedDecl>(ND), cast<NamedDecl>(D));
- }
}
}
}
@@ -2497,7 +2496,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
case CFGElement::AutomaticObjectDtor: {
CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
const auto *DD = AD.getDestructorDecl(AC.getASTContext());
- if (!DD->hasAttrs())
+ if (!DD || !DD->hasAttrs())
break;
LocksetBuilder.handleCall(nullptr, DD,
More information about the cfe-commits
mailing list