[clang] [clang] WIP: Warn on mismatched RequiresCapability attributes (PR #67520)

[Timm Baeder via cfe-commits]

https://github.com/tbaederr updated https://github.com/llvm/llvm-project/pull/67520

>From 55efd18bb177150a1fd170cb1535e225854967a6 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] Warn on RequiresCapability attribute mismatch

---
 .../clang/Analysis/Analyses/ThreadSafety.h    |  3 +++
 .../clang/Basic/DiagnosticSemaKinds.td        |  4 ++-
 clang/lib/Analysis/ThreadSafety.cpp           | 25 +++++++++++++++++++
 clang/lib/Sema/AnalysisBasedWarnings.cpp      | 12 +++++++++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 10 ++++----
 5 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 0866b09bab2995..8211f55b088906 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 583475327c5227..b57a2d6c899f8e 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 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 5577f45aa5217f..2438d2af9df630 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;
+  };
+
+  auto NDArgs = collectCapabilities(ND);
+  for (const Decl *D = ND->getPreviousDecl(); D; D = D->getPreviousDecl()) {
+    auto 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 6496a33b8f5a50..cdec30cf135fd7 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2073,6 +2073,18 @@ 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);
+    Warnings.emplace_back(std::move(Warning), getNotes());
+
+    PartialDiagnosticAt Note(PrevDecl->getLocation(),
+                             S.PDiag(diag::note_previous_decl) << PrevDecl);
+    Warnings.emplace_back(std::move(Note), getNotes());
+  }
+
   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 8477200456d985..aa8f46ce57d8a9 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-warning {{attribute mismatch between function declarations of 'fooF2'}}
+void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { // expected-note {{declared here}}
   f->a = 2;
 }
 
@@ -2269,7 +2269,7 @@ 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}}
   myFoo.fooT1(dummy);  // \