Thread safety analysis: add -Wthread-safety-verbose flag, which adds additional notes that are helpful when compiling statistics on thread safety warnings.

llvm-svn: 215677

clang/include/clang/Analysis/Analyses/ThreadSafety.h

@@ -181,6 +181,16 @@ public:
   virtual void handleFunExcludesLock(StringRef Kind, Name FunName,
                                      Name LockName, SourceLocation Loc) {}
 
+  /// Called by the analysis when starting analysis of a function.
+  /// Used to issue suggestions for changes to annotations.
+  virtual void enterFunction(const FunctionDecl *FD) {}
+
+  /// Called by the analysis when finishing analysis of a function.
+  virtual void leaveFunction(const FunctionDecl *FD) {}
+
+  /// Return the number of errors found within this function so far.
+  virtual int numErrors() { return 0; }
+
   bool issueBetaWarnings() { return IssueBetaWarnings; }
   void setIssueBetaWarnings(bool b) { IssueBetaWarnings = b; }
 

clang/include/clang/Basic/DiagnosticGroups.td

@@ -596,6 +596,7 @@ def ThreadSafety : DiagGroup<"thread-safety",
                               ThreadSafetyAnalysis,
                               ThreadSafetyPrecise,
                               ThreadSafetyNegative]>;
+def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
 def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 
 // Uniqueness Analysis warnings

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2349,9 +2349,15 @@ def warn_fun_requires_lock_precise :
   InGroup<ThreadSafetyPrecise>, DefaultIgnore;
 def note_found_mutex_near_match : Note<"found near match '%0'">;
 
+// Verbose thread safety warnings
+def warn_thread_safety_verbose : Warning<"Thread safety verbose warning.">, 
+  InGroup<ThreadSafetyVerbose>, DefaultIgnore;
+def note_thread_warning_in_fun : Note<"Thread warning in function '%0'">;
+def note_guarded_by_declared_here : Note<"Guarded_by declared here.">;
+
 // Dummy warning that will trigger "beta" warnings from the analysis if enabled. 
-def warn_thread_safety_beta : Warning<
+def warn_thread_safety_beta : Warning<"Thread safety beta warning.">, 
-  "Thread safety beta warning.">, InGroup<ThreadSafetyBeta>, DefaultIgnore;
+  InGroup<ThreadSafetyBeta>, DefaultIgnore;
 
 // Consumed warnings
 def warn_use_in_invalid_state : Warning<

clang/lib/Analysis/ThreadSafety.cpp

@@ -1810,6 +1810,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
 
   CFG *CFGraph = walker.getGraph();
   const NamedDecl *D = walker.getDecl();
+  const FunctionDecl *CurrentFunction = dyn_cast<FunctionDecl>(D);
   CurrentMethod = dyn_cast<CXXMethodDecl>(D);
 
   if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
@@ -1824,6 +1825,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   if (isa<CXXDestructorDecl>(D))
     return;  // Don't check inside destructors.
 
+  Handler.enterFunction(CurrentFunction);
+
   BlockInfo.resize(CFGraph->getNumBlockIDs(),
     CFGBlockInfo::getEmptyBlockInfo(LocalVarMap));
 
@@ -2079,6 +2082,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
                    LEK_LockedAtEndOfFunction,
                    LEK_NotLockedAtEndOfFunction,
                    false);
+
+  Handler.leaveFunction(CurrentFunction);
 }
 
 

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -1451,6 +1451,30 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
   DiagList Warnings;
   SourceLocation FunLocation, FunEndLocation;
 
+  const FunctionDecl *CurrentFunction;
+  bool Verbose;
+
+  OptionalNotes getNotes() {
+    if (Verbose && CurrentFunction) {
+      PartialDiagnosticAt FNote(CurrentFunction->getBody()->getLocStart(),
+        S.PDiag(diag::note_thread_warning_in_fun)
+        << CurrentFunction->getNameAsString());
+      return OptionalNotes(1, FNote);
+    }
+    else return OptionalNotes();
+  }
+
+  OptionalNotes getNotes(const PartialDiagnosticAt &Note) {
+    OptionalNotes ONS(1, Note);
+    if (Verbose && CurrentFunction) {
+      PartialDiagnosticAt FNote(CurrentFunction->getBody()->getLocStart(),
+        S.PDiag(diag::note_thread_warning_in_fun)
+        << CurrentFunction->getNameAsString());
+      ONS.push_back(FNote);
+    }
+    return ONS;
+  }
+
   // Helper functions
   void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName,
                         SourceLocation Loc) {
@@ -1459,12 +1483,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
     if (!Loc.isValid())
       Loc = FunLocation;
     PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << LockName);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
 
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
-    : S(S), FunLocation(FL), FunEndLocation(FEL) {}
+    : S(S), FunLocation(FL), FunEndLocation(FEL),
+      CurrentFunction(nullptr), Verbose(false) {}
+
+  void setVerbose(bool b) { Verbose = b; }
 
   /// \brief Emit all buffered diagnostics in order of sourcelocation.
   /// We need to output diagnostics produced while iterating through
@@ -1482,12 +1509,14 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
   void handleInvalidLockExp(StringRef Kind, SourceLocation Loc) override {
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_cannot_resolve_lock)
                                          << Loc);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
+
   void handleUnmatchedUnlock(StringRef Kind, Name LockName,
                              SourceLocation Loc) override {
     warnLockMismatch(diag::warn_unlock_but_no_lock, Kind, LockName, Loc);
   }
+
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
                                  LockKind Expected, LockKind Received,
                                  SourceLocation Loc) override {
@@ -1496,8 +1525,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch)
                                          << Kind << LockName << Received
                                          << Expected);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
+
   void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation Loc) override {
     warnLockMismatch(diag::warn_double_lock, Kind, LockName, Loc);
   }
@@ -1529,10 +1559,10 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
     if (LocLocked.isValid()) {
       PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here)
                                               << Kind);
-      Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+      Warnings.push_back(DelayedDiag(Warning, getNotes(Note)));
       return;
     }
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
 
   void handleExclusiveAndShared(StringRef Kind, Name LockName,
@@ -1543,7 +1573,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
                                     << Kind << LockName);
     PartialDiagnosticAt Note(Loc2, S.PDiag(diag::note_lock_exclusive_and_shared)
                                        << Kind << LockName);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+    Warnings.push_back(DelayedDiag(Warning, getNotes(Note)));
   }
 
   void handleNoMutexHeld(StringRef Kind, const NamedDecl *D,
@@ -1556,7 +1586,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
                         diag::warn_var_deref_requires_any_lock;
     PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
       << D->getNameAsString() << getLockKindFromAccessKind(AK));
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
 
   void handleMutexNotHeld(StringRef Kind, const NamedDecl *D,
@@ -1581,7 +1611,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
                                                        << LockName << LK);
       PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match)
                                         << *PossibleMatch);
-      Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+      Warnings.push_back(DelayedDiag(Warning, getNotes(Note)));
     } else {
       switch (POK) {
         case POK_VarAccess:
@@ -1597,7 +1627,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
       PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
                                                        << D->getNameAsString()
                                                        << LockName << LK);
-      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+      if (Verbose && POK == POK_VarAccess) {
+        PartialDiagnosticAt Note(D->getLocation(),
+          S.PDiag(diag::note_guarded_by_declared_here) <<
+          D->getNameAsString());
+        Warnings.push_back(DelayedDiag(Warning, getNotes(Note)));
+      }
+      else {
+        Warnings.push_back(DelayedDiag(Warning, getNotes()));
+      }
     }
   }
 
@@ -1606,7 +1644,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
     PartialDiagnosticAt Warning(Loc,
         S.PDiag(diag::warn_acquire_requires_negative_cap)
         << Kind << LockName << Neg);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
   }
 
 
@@ -1614,7 +1652,15 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
                              SourceLocation Loc) override {
     PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex)
                                          << Kind << FunName << LockName);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
+    Warnings.push_back(DelayedDiag(Warning, getNotes()));
+  }
+
+  void enterFunction(const FunctionDecl* FD) override {
+    CurrentFunction = FD;
+  }
+
+  void leaveFunction(const FunctionDecl* FD) override {
+    CurrentFunction = 0;
   }
 };
 
@@ -1908,6 +1954,8 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
     threadSafety::ThreadSafetyReporter Reporter(S, FL, FEL);
     if (!Diags.isIgnored(diag::warn_thread_safety_beta, D->getLocStart()))
       Reporter.setIssueBetaWarnings(true);
+    if (!Diags.isIgnored(diag::warn_thread_safety_verbose, D->getLocStart()))
+      Reporter.setVerbose(true);
 
     threadSafety::runThreadSafetyAnalysis(AC, Reporter);
     Reporter.emitDiagnostics();

clang/test/SemaCXX/warn-thread-safety-verbose.cpp

@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions %s
+
+#define LOCKABLE            __attribute__ ((lockable))
+#define SCOPED_LOCKABLE     __attribute__ ((scoped_lockable))
+#define GUARDED_BY(x)       __attribute__ ((guarded_by(x)))
+#define GUARDED_VAR         __attribute__ ((guarded_var))
+#define PT_GUARDED_BY(x)    __attribute__ ((pt_guarded_by(x)))
+#define PT_GUARDED_VAR      __attribute__ ((pt_guarded_var))
+#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__)))
+#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__)))
+#define EXCLUSIVE_LOCK_FUNCTION(...)    __attribute__ ((exclusive_lock_function(__VA_ARGS__)))
+#define SHARED_LOCK_FUNCTION(...)       __attribute__ ((shared_lock_function(__VA_ARGS__)))
+#define ASSERT_EXCLUSIVE_LOCK(...)      __attribute__ ((assert_exclusive_lock(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...)         __attribute__ ((assert_shared_lock(__VA_ARGS__)))
+#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__)))
+#define SHARED_TRYLOCK_FUNCTION(...)    __attribute__ ((shared_trylock_function(__VA_ARGS__)))
+#define UNLOCK_FUNCTION(...)            __attribute__ ((unlock_function(__VA_ARGS__)))
+#define LOCK_RETURNED(x)    __attribute__ ((lock_returned(x)))
+#define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__)))
+#define EXCLUSIVE_LOCKS_REQUIRED(...) \
+  __attribute__ ((exclusive_locks_required(__VA_ARGS__)))
+#define SHARED_LOCKS_REQUIRED(...) \
+  __attribute__ ((shared_locks_required(__VA_ARGS__)))
+#define NO_THREAD_SAFETY_ANALYSIS  __attribute__ ((no_thread_safety_analysis))
+
+
+class  __attribute__((lockable)) Mutex {
+ public:
+  void Lock() __attribute__((exclusive_lock_function));
+  void ReaderLock() __attribute__((shared_lock_function));
+  void Unlock() __attribute__((unlock_function));
+  bool TryLock() __attribute__((exclusive_trylock_function(true)));
+  bool ReaderTryLock() __attribute__((shared_trylock_function(true)));
+  void LockWhen(const int &cond) __attribute__((exclusive_lock_function));
+
+  // for negative capabilities
+  const Mutex& operator!() const { return *this; }
+
+  void AssertHeld()       ASSERT_EXCLUSIVE_LOCK();
+  void AssertReaderHeld() ASSERT_SHARED_LOCK();
+};
+
+
+class Test {
+  Mutex mu;
+  int a GUARDED_BY(mu);  // expected-note3 {{Guarded_by declared here.}}
+
+  void foo1() EXCLUSIVE_LOCKS_REQUIRED(mu);
+  void foo2() SHARED_LOCKS_REQUIRED(mu);
+  void foo3() LOCKS_EXCLUDED(mu);
+
+  void test1() {  // expected-note {{Thread warning in function 'test1'}}
+    a = 0;        // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+  }
+
+  void test2() {  // expected-note {{Thread warning in function 'test2'}}
+    int b = a;    // expected-warning {{reading variable 'a' requires holding mutex 'mu'}}
+  }
+
+  void test3() {  // expected-note {{Thread warning in function 'test3'}}
+    foo1();       // expected-warning {{calling function 'foo1' requires holding mutex 'mu' exclusively}}
+  }
+
+  void test4() {  // expected-note {{Thread warning in function 'test4'}}
+    foo2();       // expected-warning {{calling function 'foo2' requires holding mutex 'mu'}}
+  }
+
+  void test5() {  // expected-note {{Thread warning in function 'test5'}}
+    mu.ReaderLock();
+    foo1();       // expected-warning {{calling function 'foo1' requires holding mutex 'mu' exclusively}}
+    mu.Unlock();
+  }
+
+  void test6() {  // expected-note {{Thread warning in function 'test6'}}
+    mu.ReaderLock();
+    a = 0;        // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+    mu.Unlock();
+  }
+
+  void test7() {  // expected-note {{Thread warning in function 'test7'}}
+    mu.Lock();
+    foo3();       // expected-warning {{cannot call function 'foo3' while mutex 'mu' is held}}
+    mu.Unlock();
+  }
+};
+