partially address review feedback

Author: tbaederr

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.

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<

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;

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 {

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;
 }