Thread safety analysis: turn on checking within lock and unlock functions.

These checks are enabled with the -Wthread-safety-beta flag.

llvm-svn: 179046

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 4fe342d..479d9a3 100644 (file)
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2279,6 +2279,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   // Fill in source locations for all CFGBlocks.
   findBlockLocations(CFGraph, SortedGraph, BlockInfo);
 
+  MutexIDList ExclusiveLocksAcquired;
+  MutexIDList SharedLocksAcquired;
+  MutexIDList LocksReleased;
+
   // Add locks from exclusive_locks_required and shared_locks_required
   // to initial lockset. Also turn off checking for lock and unlock functions.
   // FIXME: is there a more intelligent way to check lock/unlock functions?
@@ -2300,15 +2304,30 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
       } else if (SharedLocksRequiredAttr *A
                    = dyn_cast<SharedLocksRequiredAttr>(Attr)) {
         getMutexIDs(SharedLocksToAdd, A, (Expr*) 0, D);
-      } else if (isa<UnlockFunctionAttr>(Attr)) {
-        // Don't try to check unlock functions for now
-        return;
-      } else if (isa<ExclusiveLockFunctionAttr>(Attr)) {
-        // Don't try to check lock functions for now
-        return;
-      } else if (isa<SharedLockFunctionAttr>(Attr)) {
-        // Don't try to check lock functions for now
-        return;
+      } else if (UnlockFunctionAttr *A = dyn_cast<UnlockFunctionAttr>(Attr)) {
+        if (!Handler.issueBetaWarnings())
+          return;
+        // UNLOCK_FUNCTION() is used to hide the underlying lock implementation.
+        // We must ignore such methods.
+        if (A->args_size() == 0)
+          return;
+        // FIXME -- deal with exclusive vs. shared unlock functions?
+        getMutexIDs(ExclusiveLocksToAdd, A, (Expr*) 0, D);
+        getMutexIDs(LocksReleased, A, (Expr*) 0, D);
+      } else if (ExclusiveLockFunctionAttr *A
+                   = dyn_cast<ExclusiveLockFunctionAttr>(Attr)) {
+        if (!Handler.issueBetaWarnings())
+          return;
+        if (A->args_size() == 0)
+          return;
+        getMutexIDs(ExclusiveLocksAcquired, A, (Expr*) 0, D);
+      } else if (SharedLockFunctionAttr *A
+                   = dyn_cast<SharedLockFunctionAttr>(Attr)) {
+        if (!Handler.issueBetaWarnings())
+          return;
+        if (A->args_size() == 0)
+          return;
+        getMutexIDs(SharedLocksAcquired, A, (Expr*) 0, D);
       } else if (isa<ExclusiveTrylockFunctionAttr>(Attr)) {
         // Don't try to check trylock functions for now
         return;
@@ -2491,8 +2510,27 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   if (!Final->Reachable)
     return;
 
+  // By default, we expect all locks held on entry to be held on exit.
+  FactSet ExpectedExitSet = Initial->EntrySet;
+
+  // Adjust the expected exit set by adding or removing locks, as declared
+  // by *-LOCK_FUNCTION and UNLOCK_FUNCTION.  The intersect below will then
+  // issue the appropriate warning.
+  // FIXME: the location here is not quite right.
+  for (unsigned i=0,n=ExclusiveLocksAcquired.size(); i<n; ++i) {
+    ExpectedExitSet.addLock(FactMan, ExclusiveLocksAcquired[i],
+                            LockData(D->getLocation(), LK_Exclusive));
+  }
+  for (unsigned i=0,n=SharedLocksAcquired.size(); i<n; ++i) {
+    ExpectedExitSet.addLock(FactMan, SharedLocksAcquired[i],
+                            LockData(D->getLocation(), LK_Shared));
+  }
+  for (unsigned i=0,n=LocksReleased.size(); i<n; ++i) {
+    ExpectedExitSet.removeLock(FactMan, LocksReleased[i]);
+  }
+
   // FIXME: Should we call this function for all blocks which exit the function?
-  intersectAndWarn(Initial->EntrySet, Final->ExitSet,
+  intersectAndWarn(ExpectedExitSet, Final->ExitSet,
                    Final->ExitLoc,
                    LEK_LockedAtEndOfFunction,
                    LEK_NotLockedAtEndOfFunction,

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 00d3c47..1295339 100644 (file)
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1333,8 +1333,12 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {
       LocEndOfScope = FunEndLocation;
 
     PartialDiagnosticAt Warning(LocEndOfScope, S.PDiag(DiagID) << LockName);
-    PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here));
-    Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+    if (LocLocked.isValid()) {
+      PartialDiagnosticAt Note(LocLocked, S.PDiag(diag::note_locked_here));
+      Warnings.push_back(DelayedDiag(Warning, OptionalNotes(1, Note)));
+      return;
+    }
+    Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));
   }
 
 

diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 3f41124..bc4b40e 100644 (file)
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1475,8 +1475,8 @@ namespace substitution_test {
   public:
     Mutex mu;
 
-    void lockData()    __attribute__((exclusive_lock_function(mu)))   { }
-    void unlockData()  __attribute__((unlock_function(mu)))           { }
+    void lockData()    __attribute__((exclusive_lock_function(mu)));
+    void unlockData()  __attribute__((unlock_function(mu)));
 
     void doSomething() __attribute__((exclusive_locks_required(mu)))  { }
   };
@@ -1484,8 +1484,8 @@ namespace substitution_test {
 
   class DataLocker {
   public:
-    void lockData  (MyData *d) __attribute__((exclusive_lock_function(d->mu))) { }
-    void unlockData(MyData *d) __attribute__((unlock_function(d->mu)))         { }
+    void lockData  (MyData *d) __attribute__((exclusive_lock_function(d->mu)));
+    void unlockData(MyData *d) __attribute__((unlock_function(d->mu)));
   };
 
 
@@ -2858,7 +2858,7 @@ void Foo::lock1()  EXCLUSIVE_LOCK_FUNCTION(mu1_) {
 }
 
 void Foo::slock1() SHARED_LOCK_FUNCTION(mu1_) {
-  mu1_.Lock();
+  mu1_.ReaderLock();
 }
 
 void Foo::lock3()  EXCLUSIVE_LOCK_FUNCTION(mu1_, mu2_, mu3_) {
@@ -3640,8 +3640,8 @@ class Foo {
                        LOCKS_EXCLUDED(mu2_);
   void lock()          EXCLUSIVE_LOCK_FUNCTION(mu1_)
                        EXCLUSIVE_LOCK_FUNCTION(mu2_);
-  void readerlock()    EXCLUSIVE_LOCK_FUNCTION(mu1_)
-                       EXCLUSIVE_LOCK_FUNCTION(mu2_);
+  void readerlock()    SHARED_LOCK_FUNCTION(mu1_)
+                       SHARED_LOCK_FUNCTION(mu2_);
   void unlock()        UNLOCK_FUNCTION(mu1_)
                        UNLOCK_FUNCTION(mu2_);
   bool trylock()       EXCLUSIVE_TRYLOCK_FUNCTION(true, mu1_)
@@ -3663,9 +3663,9 @@ void Foo::foo2() {
 }
 
 void Foo::foo3() { }
-void Foo::lock() { }
-void Foo::readerlock() { }
-void Foo::unlock() { }
+void Foo::lock() { mu1_.Lock();  mu2_.Lock(); }
+void Foo::readerlock() { mu1_.ReaderLock();  mu2_.ReaderLock(); }
+void Foo::unlock() { mu1_.Unlock();  mu2_.Unlock(); }
 bool Foo::trylock()       { return true; }
 bool Foo::readertrylock() { return true; }
 
@@ -3915,3 +3915,73 @@ void bar2() EXCLUSIVE_LOCKS_REQUIRED(getMutex1(), getMutex2());
 
 }  // end namespace UnevaluatedContextTest
 
+
+namespace LockUnlockFunctionTest {
+
+// Check built-in lock functions
+class LOCKABLE MyLockable  {
+public:
+  void lock()       EXCLUSIVE_LOCK_FUNCTION() { mu_.Lock(); }
+  void readerLock() SHARED_LOCK_FUNCTION()    { mu_.ReaderLock(); }
+  void unlock()     UNLOCK_FUNCTION()         { mu_.Unlock(); }
+
+private:
+  Mutex mu_;
+};
+
+
+class Foo {
+public:
+  // Correct lock/unlock functions
+  void lock() EXCLUSIVE_LOCK_FUNCTION(mu_) {
+    mu_.Lock();
+  }
+
+  void readerLock() SHARED_LOCK_FUNCTION(mu_) {
+    mu_.ReaderLock();
+  }
+
+  void unlock() UNLOCK_FUNCTION(mu_) {
+    mu_.Unlock();
+  }
+
+  // Check failure to lock.
+  void lockBad() EXCLUSIVE_LOCK_FUNCTION(mu_) {    // expected-note {{mutex acquired here}}
+    mu2_.Lock();
+    mu2_.Unlock();
+  }  // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}}
+
+  void readerLockBad() SHARED_LOCK_FUNCTION(mu_) {  // expected-note {{mutex acquired here}}
+    mu2_.Lock();
+    mu2_.Unlock();
+  }  // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}}
+
+  void unlockBad() UNLOCK_FUNCTION(mu_) {  // expected-note {{mutex acquired here}}
+    mu2_.Lock();
+    mu2_.Unlock();
+  }  // expected-warning {{mutex 'mu_' is still locked at the end of function}}
+
+  // Check locking the wrong thing.
+  void lockBad2() EXCLUSIVE_LOCK_FUNCTION(mu_) {   // expected-note {{mutex acquired here}}
+    mu2_.Lock();            // expected-note {{mutex acquired here}}
+  } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} \
+    // expected-warning {{mutex 'mu2_' is still locked at the end of function}}
+
+
+  void readerLockBad2() SHARED_LOCK_FUNCTION(mu_) {   // expected-note {{mutex acquired here}}
+    mu2_.ReaderLock();      // expected-note {{mutex acquired here}}
+  } // expected-warning {{expecting mutex 'mu_' to be locked at the end of function}} \
+    // expected-warning {{mutex 'mu2_' is still locked at the end of function}}
+
+
+  void unlockBad2() UNLOCK_FUNCTION(mu_) {  // expected-note {{mutex acquired here}}
+    mu2_.Unlock();  // expected-warning {{unlocking 'mu2_' that was not locked}}
+  }  // expected-warning {{mutex 'mu_' is still locked at the end of function}}
+
+private:
+  Mutex mu_;
+  Mutex mu2_;
+};
+
+}  // end namespace LockUnlockFunctionTest
+