[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 30 05:49:54 PDT 2024
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 5f411735a5366499481c09a317aa170af44796f3 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/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?= <tbaeder at 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?= <tbaeder at 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) {
More information about the cfe-commits
mailing list