[clang] daca6ed - Thread safety analysis: Fix false negative on break

[Aaron Puchert via cfe-commits]

Author: Aaron Puchert
Date: 2021-05-03T14:03:17+02:00
New Revision: daca6edb31efae048a420595fae9750aaa91c733

URL: https://github.com/llvm/llvm-project/commit/daca6edb31efae048a420595fae9750aaa91c733
DIFF: https://github.com/llvm/llvm-project/commit/daca6edb31efae048a420595fae9750aaa91c733.diff

LOG: Thread safety analysis: Fix false negative on break

We weren't modifying the lock set when intersecting with one coming
from a break-terminated block. This is inconsistent, since break isn't a
back edge, and it leads to false negatives with scoped locks. We usually
don't warn for those when joining locksets aren't the same, we just
silently remove locks that are not in the intersection. But not warning
and not removing them isn't right.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D101202

Added: 
    

Modified: 
    clang/lib/Analysis/ThreadSafety.cpp
    clang/test/PCH/thread-safety-attrs.cpp
    clang/test/SemaCXX/warn-thread-safety-analysis.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index bb3a4f1616fca..4377fc58a1d55 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2467,11 +2467,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
                        PrevBlock, CurrBlock);
 
         // Do not update EntrySet.
-        intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset,
-                         PrevBlockInfo->ExitLoc,
-                         IsLoop ? LEK_LockedSomeLoopIterations
-                                : LEK_LockedSomePredecessors,
-                         false);
+        intersectAndWarn(
+            CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
+            IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
+            !IsLoop);
       }
     }
 

diff  --git a/clang/test/PCH/thread-safety-attrs.cpp b/clang/test/PCH/thread-safety-attrs.cpp
index ae2a413af31c8..3e0c081f134f9 100644
--- a/clang/test/PCH/thread-safety-attrs.cpp
+++ b/clang/test/PCH/thread-safety-attrs.cpp
@@ -311,7 +311,8 @@ void sls_fun_bad_12() {
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 #endif

diff  --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index b837206138a67..369952eb397a2 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -337,7 +337,8 @@ void sls_fun_bad_12() {
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 //-----------------------------------------//
@@ -2582,6 +2583,7 @@ class Foo {
   void test3();
   void test4();
   void test5();
+  void test6();
 };
 
 
@@ -2620,6 +2622,18 @@ void Foo::test5() {
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
+void Foo::test6() {
+  ReleasableMutexLock rlock(&mu_);
+  do {
+    if (c) {
+      rlock.Release();
+      break;
+    }
+  } while (c);
+  // No warning on join point because the lock will be released by the scope object anyway
+  a = 1; // expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}}
+}
+
 
 } // end namespace ReleasableScopedLock