[clang] Warn on mismatched RequiresCapability attributes

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) {}

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'">,

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");

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,37 @@ static bool neverReturns(const CFGBlock *B) {
   return false;
 }
 
+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) {
+
+  // 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 = collectAttrArgs<AttrT>(SxBuilder, 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
@@ -2282,6 +2315,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   const NamedDecl *D = walker.getDecl();
   CurrentFunction = dyn_cast<FunctionDecl>(D);
 
+  checkMismatchedFunctionAttrs(D);
+
   if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
     return;
 

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

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