Thread safety analysis: Fix false negative on break

[aaronpuchert]

Summary

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.

[aaronpuchert]

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

341

This is just a side effect, not the reason I did the change.

It's consistent though with our usual behavior on joining forward edges, see e.g. sls_fun_bad_11.

2634

This warning was missing, and is the reason for this change.

[aaron.ballman]

LGTM, thank you for explaining the test cases, that helped (and I agree that the drive-by test changes look sensible to me).

clang/lib/Analysis/ThreadSafety.cpp

for (const auto *PrevBlock : SpecialBlocks) {
         const Stmt *Terminator = PrevBlock->getTerminatorStmt();
         bool IsLoop = Terminator && isa<ContinueStmt>(Terminator);
 
         FactSet PrevLockset;
         getEdgeLockset(PrevLockset, PrevBlockInfo->ExitSet,
                        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);
       }
     }
 
     BuildLockset LocksetBuilder(this, *CurrBlockInfo);
 
     // Visit all the statements in the basic block.
     for (const auto &BI : *CurrBlock) {
       switch (BI.getKind()) {

clang/test/PCH/thread-safety-attrs.cpp

while (getBool()) {
     sls_mu.Unlock();
     if (getBool()) {
       if (getBool()) {
         break; // expected-warning{{mutex 'sls_mu' is not held on every path through here}}
       }
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 #endif

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

while (getBool()) {
     sls_mu.Unlock();
     if (getBool()) {
       if (getBool()) {
         break; // expected-warning{{mutex 'sls_mu' is not held on every path through here}}
       }
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 //-----------------------------------------//
 // Handling lock expressions in attribute args
 // -------------------------------------------//
 
 Mutex aa_mu;
 
class Foo {
   bool c;
   int a GUARDED_BY(mu_);
 
   void test1();
   void test2();
   void test3();
   void test4();
   void test5();
+  void test6();
 };
 
 
 void Foo::test1() {
   ReleasableMutexLock rlock(&mu_);
   rlock.Release();
 }
 
void Foo::test5() {
   ReleasableMutexLock rlock(&mu_);
   if (c) {
     rlock.Release();
   }
   // No warning on join point because the lock will be released by the scope object anyway.
   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
 
 
 namespace RelockableScopedLock {
 
 class DeferTraits {};