Thread safety analysis: Drop special block handling

[aaronpuchert]

Summary

Previous changes like D101202 and D104261 have eliminated the special
status that break and continue once had, since now we're making
decisions purely based on the structure of the CFG without regard for
the underlying source code constructs.

This means we don't gain anything from defering handling for these
blocks. Dropping it moves some diagnostics, though arguably into a
better place. We're working around a "quirk" in the CFG that perhaps
wasn't visible before: while loops have an empty "transition block"
where continue statements and the regular loop exit meet, before
continuing to the loop entry. To get a source location for that, we
slightly extend our handling for empty blocks. The source location for
the transition ends up to be the loop entry then, but formally this
isn't a back edge. We pretend it is anyway. (This is safe: we can always
treat edges as back edges, it just means we allow less and don't modify
the lock set. The other way around it wouldn't be safe.)

[aaron.ballman]

I think this looks reasonable, but @delesley should weigh in.

clang/lib/Analysis/ThreadSafety.cpp

2418

This can be removed now, correct?

2450

clang-tidy got this one right. :-)

[aaronpuchert]

Drop SpecialBlocks and fix clang-tidy issue.

[aaronpuchert]

Ping.

[aaron.ballman]

Marking this explicitly as accepted, I'd say you're fine to land and we can handle any concerns from @delesley post commit.

clang/lib/Analysis/ThreadSafety.cpp

if (CurrBlockInfo->ExitLoc.isValid()) {
         }
       }
     } else if (CurrBlock->pred_size() == 1 && *CurrBlock->pred_begin() &&
                CurrBlock != &CFGraph->getExit()) {
       // The block is empty, and has a single predecessor. Use its exit
       // location.
       CurrBlockInfo->EntryLoc = CurrBlockInfo->ExitLoc =
           BlockInfo[(*CurrBlock->pred_begin())->getBlockID()].ExitLoc;
+    } else if (CurrBlock->succ_size() == 1 && *CurrBlock->succ_begin()) {
+      // The block is empty, and has a single successor. Use its entry
+      // location.
+      CurrBlockInfo->EntryLoc = CurrBlockInfo->ExitLoc =
+          BlockInfo[(*CurrBlock->succ_begin())->getBlockID()].EntryLoc;
     }
   }
 }
 
 namespace {
 
 class LockableFactEntry : public FactEntry {
 public:
for (const auto *CurrBlock : *SortedGraph) {
     // the intersection contains Mutex L, and the union contains L and M.
     // Later we unlock M. At this point, we would output an error because we
     // never locked M; although the real error is probably that we forgot to
     // lock M on all code paths. Conversely, let's say that later we lock M.
     // In this case, we should compare against the intersection instead of the
     // union because the real error is probably that we forgot to unlock M on
     // all code paths.
     bool LocksetInitialized = false;
-    SmallVector<CFGBlock *, 8> SpecialBlocks;
     for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(),
          PE  = CurrBlock->pred_end(); PI != PE; ++PI) {
       // if *PI -> CurrBlock is a back edge
       if (*PI == nullptr || !VisitedBlocks.alreadySet(*PI))
         continue;
 
       unsigned PrevBlockID = (*PI)->getBlockID();
       CFGBlockInfo *PrevBlockInfo = &BlockInfo[PrevBlockID];
 
       // Ignore edges from blocks that can't return.
       if (neverReturns(*PI) || !PrevBlockInfo->Reachable)
         continue;
 
       // Okay, we can reach this block from the entry.
       CurrBlockInfo->Reachable = true;
 
-      // If the previous block ended in a 'continue' or 'break' statement, then
-      // a difference in locksets is probably due to a bug in that block, rather
-      // than in some other predecessor. In that case, keep the other
-      // predecessor's lockset.
-      if (const Stmt *Terminator = (*PI)->getTerminatorStmt()) {
-        if (isa<ContinueStmt>(Terminator) || isa<BreakStmt>(Terminator)) {
-          SpecialBlocks.push_back(*PI);
-          continue;
-        }
-      }
-
       FactSet PrevLockset;
       getEdgeLockset(PrevLockset, PrevBlockInfo->ExitSet, *PI, CurrBlock);
 
       if (!LocksetInitialized) {
         CurrBlockInfo->EntrySet = PrevLockset;
         LocksetInitialized = true;
       } else {
-        intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset,
-                         CurrBlockInfo->EntryLoc,
-                         LEK_LockedSomePredecessors);
+        // Surprisingly 'continue' doesn't always produce back edges, because
+        // the CFG has empty "transition" blocks where they meet with the end
+        // of the regular loop body. We still want to diagnose them as loop.
+        intersectAndWarn(
+            CurrBlockInfo->EntrySet, PrevLockset, CurrBlockInfo->EntryLoc,
+            isa_and_nonnull<ContinueStmt>((*PI)->getTerminatorStmt())
+                ? LEK_LockedSomeLoopIterations
+                : LEK_LockedSomePredecessors);
       }
     }
 
     // Skip rest of block if it's not reachable.
     if (!CurrBlockInfo->Reachable)
       continue;
 
-    // Process continue and break blocks. Assume that the lockset for the
-    // resulting block is unaffected by any discrepancies in them.
-    for (const auto *PrevBlock : SpecialBlocks) {
-      unsigned PrevBlockID = PrevBlock->getBlockID();
-      CFGBlockInfo *PrevBlockInfo = &BlockInfo[PrevBlockID];
-
-      if (!LocksetInitialized) {
-        CurrBlockInfo->EntrySet = PrevBlockInfo->ExitSet;
-        LocksetInitialized = true;
-      } else {
-        // Determine whether this edge is a loop terminator for diagnostic
-        // purposes. FIXME: A 'break' statement might be a loop terminator, but
-        // it might also be part of a switch. Also, a subsequent destructor
-        // might add to the lockset, in which case the real issue might be a
-        // double lock on the other path.
-        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);
-      }
-    }
-
     BuildLockset LocksetBuilder(this, *CurrBlockInfo);
 
     // Visit all the statements in the basic block.
     for (const auto &BI : *CurrBlock) {
       switch (BI.getKind()) {
         case CFGElement::Statement: {
           CFGStmt CS = BI.castAs<CFGStmt>();
           LocksetBuilder.Visit(CS.getStmt());

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

void sls_fun_bad_6() {
   }
   sls_mu.Unlock(); // \
     expected-warning{{mutex 'sls_mu' is not held on every path through here}}\
     expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 void sls_fun_bad_7() {
   sls_mu.Lock();
-  while (getBool()) {
+  while (getBool()) { // \
+        expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}}
     sls_mu.Unlock();
     if (getBool()) {
       if (getBool()) {
-        continue; // \
-        expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}}
+        continue;
       }
     }
     sls_mu.Lock(); // expected-note {{mutex acquired here}}
   }
   sls_mu.Unlock();
 }
 
 void sls_fun_bad_8() {

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

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

void sls_fun_bad_6() {
   }
   sls_mu.Unlock(); // \
     expected-warning{{mutex 'sls_mu' is not held on every path through here}}\
     expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 void sls_fun_bad_7() {
   sls_mu.Lock();
-  while (getBool()) {
+  while (getBool()) { // \
+        expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}}
     sls_mu.Unlock();
     if (getBool()) {
       if (getBool()) {
-        continue; // \
-        expected-warning{{expecting mutex 'sls_mu' to be held at start of each loop}}
+        continue;
       }
     }
     sls_mu.Lock(); // expected-note {{mutex acquired here}}
   }
   sls_mu.Unlock();
 }
 
 void sls_fun_bad_8() {

 }
 
 void sls_fun_bad_12() {
   sls_mu.Lock(); // expected-note {{mutex acquired here}}
   while (getBool()) {
     sls_mu.Unlock();
     if (getBool()) {
       if (getBool()) {
-        break; // expected-warning{{mutex 'sls_mu' is not held on every path through here}}
+        break;
       }
     }
     sls_mu.Lock();
   }
   sls_mu.Unlock(); // \
-    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
+    expected-warning{{mutex 'sls_mu' is not held on every path through here}} \
+    expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 //-----------------------------------------//
 // Handling lock expressions in attribute args
 // -------------------------------------------//
 
 Mutex aa_mu;
 
struct LOCKABLE mutex {
     void unlock() UNLOCK_FUNCTION();
     // ...
   };
   bool foo();
   bool bar();
   mutex m;
   void test() {
     m.lock();
-    while (foo()) {
+    while (foo()) { // expected-warning {{expecting mutex 'm' to be held at start of each loop}}
       m.unlock();
       // ...
       if (bar()) {
         // ...
         if (foo())
-          continue; // expected-warning {{expecting mutex 'm' to be held at start of each loop}}
+          continue;
         //...
       }
       // ...
       m.lock(); // expected-note {{mutex acquired here}}
     }
     m.unlock();
   }
 
if (i == 5) {
       continue;
     }
   }
 }
 
 void loopReleaseContinue() {
   RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{mutex acquired here}}
   // We have to warn on this join point despite the lock being managed ...
-  for (unsigned i = 1; i < 10; ++i) {
+  for (unsigned i = 1; i < 10; ++i) { // expected-warning {{expecting mutex 'mu' to be held at start of each loop}}
     x = 1; // ... because we might miss that this doesn't always happen under lock.
     if (i == 5) {
       scope.Unlock();
-      continue; // expected-warning {{expecting mutex 'mu' to be held at start of each loop}}
+      continue;
     }
   }
 }
 
 void loopPromoteContinue() {
   RelockableMutexLock scope(&mu, SharedTraits{});
   for (unsigned i = 1; i < 10; ++i) {
     x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}