Thread-safety analysis: differentiate between two forms of analysis; a precise

analysis that may give false positives because it is confused by aliasing, and
a less precise analysis that has fewer false positives, but may have false
negatives.  The more precise warnings are enabled by -Wthread-safety-precise.
An additional note clarify the warnings in the precise case.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163537 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Analysis/Analyses/ThreadSafety.h

@@ -132,7 +132,8 @@ public:
   /// \param Loc -- The location of the protected operation.
   virtual void handleMutexNotHeld(const NamedDecl *D,
                                   ProtectedOperationKind POK, Name LockName,
-                                  LockKind LK, SourceLocation Loc) {}
+                                  LockKind LK, SourceLocation Loc,
+                                  Name *PossibleMatch=0) {}
 
   /// Warn when a function is called while an excluded mutex is locked. For
   /// example, the mutex may be locked inside the function.

include/clang/Basic/DiagnosticGroups.td

@@ -386,9 +386,12 @@ def Most : DiagGroup<"most", [
 
 // Thread Safety warnings 
 def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
-def ThreadSafety : DiagGroup<"thread-safety", 
-                             [ThreadSafetyAttributes, ThreadSafetyAnalysis]>;
+def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
+def ThreadSafety : DiagGroup<"thread-safety",
+                             [ThreadSafetyAttributes, 
+                              ThreadSafetyAnalysis,
+                              ThreadSafetyPrecise]>;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as

include/clang/Basic/DiagnosticSemaKinds.td

@@ -1866,14 +1866,6 @@ def warn_lock_exclusive_and_shared : Warning<
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def note_lock_exclusive_and_shared : Note<
   "the other lock of mutex '%0' is here">;
-def warn_variable_requires_lock : Warning<
-  "%select{reading|writing}2 variable '%0' requires locking "
-  "%select{'%1'|'%1' exclusively}2">,
-  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
-def warn_var_deref_requires_lock : Warning<
-  "%select{reading|writing}2 the value pointed to by '%0' requires locking "
-  "%select{'%1'|'%1' exclusively}2">,
-  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def warn_variable_requires_any_lock : Warning<
   "%select{reading|writing}1 variable '%0' requires locking "
   "%select{any mutex|any mutex exclusively}1">,
@@ -1882,9 +1874,6 @@ def warn_var_deref_requires_any_lock : Warning<
   "%select{reading|writing}1 the value pointed to by '%0' requires locking "
   "%select{any mutex|any mutex exclusively}1">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
-def warn_fun_requires_lock : Warning<
-  "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">,
-  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def warn_fun_excludes_mutex : Warning<
   "cannot call function '%0' while mutex '%1' is locked">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
@@ -1892,6 +1881,32 @@ def warn_cannot_resolve_lock : Warning<
   "cannot resolve lock expression">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 
+// Imprecise thread safety warnings
+def warn_variable_requires_lock : Warning<
+  "%select{reading|writing}2 variable '%0' requires locking "
+  "%select{'%1'|'%1' exclusively}2">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
+def warn_var_deref_requires_lock : Warning<
+  "%select{reading|writing}2 the value pointed to by '%0' requires locking "
+  "%select{'%1'|'%1' exclusively}2">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
+def warn_fun_requires_lock : Warning<
+  "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
+
+// Precise thread safety warnings
+def warn_variable_requires_lock_precise : Warning<
+  "%select{reading|writing}2 variable '%0' requires locking "
+  "%select{'%1'|'%1' exclusively}2">,
+  InGroup<ThreadSafetyPrecise>, DefaultIgnore;
+def warn_var_deref_requires_lock_precise : Warning<
+  "%select{reading|writing}2 the value pointed to by '%0' requires locking "
+  "%select{'%1'|'%1' exclusively}2">,
+  InGroup<ThreadSafetyPrecise>, DefaultIgnore;
+def warn_fun_requires_lock_precise : Warning<
+  "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">,
+  InGroup<ThreadSafetyPrecise>, DefaultIgnore;
+def note_found_mutex_near_match : Note<"found near match '%0'">;
 
 def warn_impcast_vector_scalar : Warning<
   "implicit conversion turns vector to scalar: %0 to %1">,

lib/Analysis/ThreadSafety.cpp

@@ -575,6 +575,15 @@ public:
     return false;
   }
 
+  // A partial match between a.mu and b.mu returns true a and b have the same
+  // type (and thus mu refers to the same mutex declaration), regardless of
+  // whether a and b are different objects or not.
+  bool partiallyMatches(const SExpr &Other) const {
+    if (NodeVec[0].kind() == EOP_Dot)
+      return NodeVec[0].matches(Other.NodeVec[0]);
+    return false;
+  }
+
   /// \brief Pretty print a lock expression for use in error messages.
   std::string toString(unsigned i = 0) const {
     assert(isValid());
@@ -820,7 +829,7 @@ public:
     return false;
   }
 
-  LockData* findLock(FactManager& FM, const SExpr& M) const {
+  LockData* findLock(FactManager &FM, const SExpr &M) const {
     for (const_iterator I = begin(), E = end(); I != E; ++I) {
       const SExpr &Exp = FM[*I].MutID;
       if (Exp.matches(M))
@@ -829,7 +838,7 @@ public:
     return 0;
   }
 
-  LockData* findLockUniv(FactManager& FM, const SExpr& M) const {
+  LockData* findLockUniv(FactManager &FM, const SExpr &M) const {
     for (const_iterator I = begin(), E = end(); I != E; ++I) {
       const SExpr &Exp = FM[*I].MutID;
       if (Exp.matches(M) || Exp.isUniversal())
@@ -837,6 +846,14 @@ public:
     }
     return 0;
   }
+
+  FactEntry* findPartialMatch(FactManager &FM, const SExpr &M) const {
+    for (const_iterator I=begin(), E=end(); I != E; ++I) {
+      const SExpr& Exp = FM[*I].MutID;
+      if (Exp.partiallyMatches(M)) return &FM[*I];
+    }
+    return 0;
+  }
 };
 
 
@@ -1742,7 +1759,26 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,
   }
 
   LockData* LDat = FSet.findLockUniv(Analyzer->FactMan, Mutex);
-  if (!LDat || !LDat->isAtLeast(LK))
+  bool NoError = true;
+  if (!LDat) {
+    // No exact match found.  Look for a partial match.
+    FactEntry* FEntry = FSet.findPartialMatch(Analyzer->FactMan, Mutex);
+    if (FEntry) {
+      // Warn that there's no precise match.
+      LDat = &FEntry->LDat;
+      std::string PartMatchStr = FEntry->MutID.toString();
+      StringRef   PartMatchName(PartMatchStr);
+      Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK,
+                                           Exp->getExprLoc(), &PartMatchName);
+    } else {
+      // Warn that there's no match at all.
+      Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK,
+                                           Exp->getExprLoc());
+    }
+    NoError = false;
+  }
+  // Make sure the mutex we found is the right kind.
+  if (NoError && LDat && !LDat->isAtLeast(LK))
     Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK,
                                          Exp->getExprLoc());
 }

lib/Sema/AnalysisBasedWarnings.cpp

@@ -1089,22 +1089,42 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {
   }
 
   void handleMutexNotHeld(const NamedDecl *D, ProtectedOperationKind POK,
-                          Name LockName, LockKind LK, SourceLocation Loc) {
+                          Name LockName, LockKind LK, SourceLocation Loc,
+                          Name *PossibleMatch) {
     unsigned DiagID = 0;
-    switch (POK) {
-      case POK_VarAccess:
-        DiagID = diag::warn_variable_requires_lock;
-        break;
-      case POK_VarDereference:
-        DiagID = diag::warn_var_deref_requires_lock;
-        break;
-      case POK_FunctionCall:
-        DiagID = diag::warn_fun_requires_lock;
-        break;
+    if (PossibleMatch) {
+      switch (POK) {
+        case POK_VarAccess:
+          DiagID = diag::warn_variable_requires_lock_precise;
+          break;
+        case POK_VarDereference:
+          DiagID = diag::warn_var_deref_requires_lock_precise;
+          break;
+        case POK_FunctionCall:
+          DiagID = diag::warn_fun_requires_lock_precise;
+          break;
+      }
+      PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
+        << D->getName() << LockName << LK);
+      PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match)
+                               << *PossibleMatch);
+      Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+    } else {
+      switch (POK) {
+        case POK_VarAccess:
+          DiagID = diag::warn_variable_requires_lock;
+          break;
+        case POK_VarDereference:
+          DiagID = diag::warn_var_deref_requires_lock;
+          break;
+        case POK_FunctionCall:
+          DiagID = diag::warn_fun_requires_lock;
+          break;
+      }
+      PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
+        << D->getName() << LockName << LK);
+      Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
     }
-    PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
-      << D->getName() << LockName << LK);
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
   }
 
   void handleFunExcludesLock(Name FunName, Name LockName, SourceLocation Loc) {

test/SemaCXX/warn-thread-safety-analysis.cpp

@@ -533,7 +533,8 @@ void late_bad_0() {
   LateFoo fooB;
   fooA.mu.Lock();
   fooB.a = 5; // \
-    // expected-warning{{writing variable 'a' requires locking 'fooB.mu' exclusively}}
+    // expected-warning{{writing variable 'a' requires locking 'fooB.mu' exclusively}} \
+    // expected-note{{found near match 'fooA.mu'}}
   fooA.mu.Unlock();
 }
 
@@ -553,7 +554,8 @@ void late_bad_2() {
   LateBar BarA;
   BarA.FooPointer->mu.Lock();
   BarA.Foo.a = 2; // \
-    // expected-warning{{writing variable 'a' requires locking 'BarA.Foo.mu' exclusively}}
+    // expected-warning{{writing variable 'a' requires locking 'BarA.Foo.mu' exclusively}} \
+    // expected-note{{found near match 'BarA.FooPointer->mu'}}
   BarA.FooPointer->mu.Unlock();
 }
 
@@ -561,7 +563,8 @@ void late_bad_3() {
   LateBar BarA;
   BarA.Foo.mu.Lock();
   BarA.FooPointer->a = 2; // \
-    // expected-warning{{writing variable 'a' requires locking 'BarA.FooPointer->mu' exclusively}}
+    // expected-warning{{writing variable 'a' requires locking 'BarA.FooPointer->mu' exclusively}} \
+    // expected-note{{found near match 'BarA.Foo.mu'}}
   BarA.Foo.mu.Unlock();
 }
 
@@ -569,7 +572,8 @@ void late_bad_4() {
   LateBar BarA;
   BarA.Foo.mu.Lock();
   BarA.Foo2.a = 2; // \
-    // expected-warning{{writing variable 'a' requires locking 'BarA.Foo2.mu' exclusively}}
+    // expected-warning{{writing variable 'a' requires locking 'BarA.Foo2.mu' exclusively}} \
+    // expected-note{{found near match 'BarA.Foo.mu'}}
   BarA.Foo.mu.Unlock();
 }
 
@@ -1237,7 +1241,9 @@ void func()
 {
   b1->MyLock();
   b1->a_ = 5;
-  b2->a_ = 3; // expected-warning {{writing variable 'a_' requires locking 'b2->mu1_' exclusively}}
+  b2->a_ = 3; // \
+    // expected-warning {{writing variable 'a_' requires locking 'b2->mu1_' exclusively}} \
+    // expected-note {{found near match 'b1->mu1_'}}
   b2->MyLock();
   b2->MyUnlock();
   b1->MyUnlock();
@@ -1267,11 +1273,13 @@ int func(int i)
   int x;
   b3->mu1_.Lock();
   res = b1.a_ + b3->b_; // expected-warning {{reading variable 'a_' requires locking 'b1.mu1_'}} \
-    // expected-warning {{writing variable 'res' requires locking 'mu' exclusively}}
+    // expected-warning {{writing variable 'res' requires locking 'mu' exclusively}} \
+    // expected-note {{found near match 'b3->mu1_'}}
   *p = i; // expected-warning {{reading variable 'p' requires locking 'mu'}} \
     // expected-warning {{writing the value pointed to by 'p' requires locking 'mu' exclusively}}
   b1.a_ = res + b3->b_; // expected-warning {{reading variable 'res' requires locking 'mu'}} \
-    // expected-warning {{writing variable 'a_' requires locking 'b1.mu1_' exclusively}}
+    // expected-warning {{writing variable 'a_' requires locking 'b1.mu1_' exclusively}} \
+    // expected-note {{found near match 'b3->mu1_'}}
   b3->b_ = *b1.q; // expected-warning {{reading the value pointed to by 'q' requires locking 'mu'}}
   b3->mu1_.Unlock();
   b1.b_ = res; // expected-warning {{reading variable 'res' requires locking 'mu'}}
@@ -1296,8 +1304,12 @@ class Foo {
 
      child->Func(new_foo); // There shouldn't be any warning here as the
                            // acquired lock is not in child.
-     child->bar(7); // expected-warning {{calling function 'bar' requires exclusive lock on 'child->lock_'}}
-     child->a_ = 5; // expected-warning {{writing variable 'a_' requires locking 'child->lock_' exclusively}}
+     child->bar(7); // \
+       // expected-warning {{calling function 'bar' requires exclusive lock on 'child->lock_'}} \
+       // expected-note {{found near match 'lock_'}}
+     child->a_ = 5; // \
+       // expected-warning {{writing variable 'a_' requires locking 'child->lock_' exclusively}} \
+       // expected-note {{found near match 'lock_'}}
      lock_.Unlock();
   }
 
@@ -1495,7 +1507,8 @@ namespace substitution_test {
       DataLocker dlr;
       dlr.lockData(d1);
       foo(d2); // \
-        // expected-warning {{calling function 'foo' requires exclusive lock on 'd2->mu'}}
+        // expected-warning {{calling function 'foo' requires exclusive lock on 'd2->mu'}} \
+        // expected-note {{found near match 'd1->mu'}}
       dlr.unlockData(d1);
     }
   };
@@ -1833,7 +1846,8 @@ void test() {
 
   f1.mu_.Unlock();
   bt.barTD(&f1);  // \
-    // expected-warning {{calling function 'barTD' requires exclusive lock on 'f1.mu_'}}
+    // expected-warning {{calling function 'barTD' requires exclusive lock on 'f1.mu_'}} \
+    // expected-note {{found near match 'bt.fooBase.mu_'}}
 
   bt.fooBase.mu_.Unlock();
   bt.fooBaseT.mu_.Unlock();
@@ -2239,27 +2253,32 @@ void test() {
 
   bar.getFoo().mu_.Lock();
   bar.getFooey().a = 0; // \
-    // expected-warning {{writing variable 'a' requires locking 'bar.getFooey().mu_' exclusively}}
+    // expected-warning {{writing variable 'a' requires locking 'bar.getFooey().mu_' exclusively}} \
+    // expected-note {{found near match 'bar.getFoo().mu_'}}
   bar.getFoo().mu_.Unlock();
 
   bar.getFoo2(a).mu_.Lock();
   bar.getFoo2(b).a = 0; // \
-    // expected-warning {{writing variable 'a' requires locking 'bar.getFoo2(b).mu_' exclusively}}
+    // expected-warning {{writing variable 'a' requires locking 'bar.getFoo2(b).mu_' exclusively}} \
+    // expected-note {{found near match 'bar.getFoo2(a).mu_'}}
   bar.getFoo2(a).mu_.Unlock();
 
   bar.getFoo3(a, b).mu_.Lock();
   bar.getFoo3(a, c).a = 0;  // \
-    // expected-warning {{writing variable 'a' requires locking 'bar.getFoo3(a,c).mu_' exclusively}}
+    // expected-warning {{writing variable 'a' requires locking 'bar.getFoo3(a,c).mu_' exclusively}} \
+    // expected-note {{'bar.getFoo3(a,b).mu_'}}
   bar.getFoo3(a, b).mu_.Unlock();
 
   getBarFoo(bar, a).mu_.Lock();
   getBarFoo(bar, b).a = 0;  // \
-    // expected-warning {{writing variable 'a' requires locking 'getBarFoo(bar,b).mu_' exclusively}}
+    // expected-warning {{writing variable 'a' requires locking 'getBarFoo(bar,b).mu_' exclusively}} \
+    // expected-note {{'getBarFoo(bar,a).mu_'}}
   getBarFoo(bar, a).mu_.Unlock();
 
   (a > 0 ? fooArray[1] : fooArray[b]).mu_.Lock();
   (a > 0 ? fooArray[b] : fooArray[c]).a = 0; // \
-    // expected-warning {{writing variable 'a' requires locking '((a#_)#_#fooArray[b]).mu_' exclusively}}
+    // expected-warning {{writing variable 'a' requires locking '((a#_)#_#fooArray[b]).mu_' exclusively}} \
+    // expected-note {{'((a#_)#_#fooArray[_]).mu_'}}
   (a > 0 ? fooArray[1] : fooArray[b]).mu_.Unlock();
 }
 
@@ -2318,7 +2337,9 @@ void test1(Foo* f1, Foo* f2) {
 
   f1->a = 0;
   f1->foo();
-  f1->foo2(f2);    // expected-warning {{calling function 'foo2' requires exclusive lock on 'f2->mu_'}}
+  f1->foo2(f2); // \
+    // expected-warning {{calling function 'foo2' requires exclusive lock on 'f2->mu_'}} \
+    // expected-note {{found near match 'f1->mu_'}}
 
   Foo::getMu(f2)->Lock();
   f1->foo2(f2);
@@ -2358,7 +2379,9 @@ void test2(Bar* b1, Bar* b2) {
 
   b1->b = 0;
   b1->bar();
-  b1->bar2(b2);    // expected-warning {{calling function 'bar2' requires exclusive lock on 'b2->mu_'}}
+  b1->bar2(b2);  // \
+    // expected-warning {{calling function 'bar2' requires exclusive lock on 'b2->mu_'}} \
+    // // expected-note {{found near match 'b1->mu_'}}
 
   b2->getMu()->Lock();
   b1->bar2(b2);