[clang] [clang] Warn on mismatched RequiresCapability attributes (PR #67520)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 25 02:17:10 PST 2024
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 e89c66ced10f0d785d41d3b11c8447388c41e16e 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/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?= <tbaeder at 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
More information about the cfe-commits
mailing list