Thread safety analysis: Eliminate parameter from intersectAndWarn (NFC)

[aaronpuchert]

Summary

We were modifying precisely when intersecting the lock sets of multiple
predecessors without back edge. That's no coincidence: we can't modify
on back edges, it doesn't make sense to modify at the end of a function,
and otherwise we always want to intersect on forward edges, because we
can build a new lock set for those.

[aaron.ballman]

LGTM!

clang/lib/Analysis/ThreadSafety.cpp

public:
   const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C,
                                      bool &Negate);
 
   void getEdgeLockset(FactSet &Result, const FactSet &ExitSet,
                       const CFGBlock* PredBlock,
                       const CFGBlock *CurrBlock);
 
   void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
-                        SourceLocation JoinLoc,
-                        LockErrorKind LEK1, LockErrorKind LEK2,
-                        bool Modify=true);
+                        SourceLocation JoinLoc, LockErrorKind LEK1,
+                        LockErrorKind LEK2);
 
   void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
-                        SourceLocation JoinLoc, LockErrorKind LEK1,
-                        bool Modify=true) {
-    intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1, Modify);
+                        SourceLocation JoinLoc, LockErrorKind LEK1) {
+    intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1);
   }
 
   void runAnalysis(AnalysisDeclContext &AC);
 };
 
 } // namespace
 
 /// Process acquired_before and acquired_after attributes on Vd.

 /// \param FSet2 The second lockset.
 /// \param JoinLoc The location of the join point for error reporting
 /// \param LEK1 The error message to report if a mutex is missing from LSet1
 /// \param LEK2 The error message to report if a mutex is missing from Lset2
 void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1,
                                             const FactSet &FSet2,
                                             SourceLocation JoinLoc,
                                             LockErrorKind LEK1,
-                                            LockErrorKind LEK2,
-                                            bool Modify) {
+                                            LockErrorKind LEK2) {
   FactSet FSet1Orig = FSet1;
 
   // Find locks in FSet2 that conflict or are not in FSet1, and warn.
   for (const auto &Fact : FSet2) {
     const FactEntry &LDat2 = FactMan[Fact];
 
     FactSet::iterator Iter1 = FSet1.findLockIter(FactMan, LDat2);
     if (Iter1 != FSet1.end()) {
       const FactEntry &LDat1 = FactMan[*Iter1];
       if (LDat1.kind() != LDat2.kind()) {
         Handler.handleExclusiveAndShared("mutex", LDat2.toString(), LDat2.loc(),
                                          LDat1.loc());
-        if (Modify && LDat1.kind() != LK_Exclusive) {
+        if (LEK1 == LEK_LockedSomePredecessors &&
+            LDat1.kind() != LK_Exclusive) {
           // Take the exclusive lock, which is the one in FSet2.
           *Iter1 = Fact;
         }
-      } else if (Modify && LDat1.asserted() && !LDat2.asserted()) {
+      } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
+                 !LDat2.asserted()) {
         // The non-asserted lock in FSet2 is the one we want to track.
         *Iter1 = Fact;
       }
     } else {
       LDat2.handleRemovalFromIntersection(FSet2, FactMan, JoinLoc, LEK1,
                                           Handler);
     }
   }
 
   // Find locks in FSet1 that are not in FSet2, and remove them.
   for (const auto &Fact : FSet1Orig) {
     const FactEntry *LDat1 = &FactMan[Fact];
     const FactEntry *LDat2 = FSet2.findLock(FactMan, *LDat1);
 
     if (!LDat2) {
       LDat1->handleRemovalFromIntersection(FSet1Orig, FactMan, JoinLoc, LEK2,
                                            Handler);
-      if (Modify)
+      if (LEK2 == LEK_LockedSomePredecessors)
         FSet1.removeLock(FactMan, *LDat1);
     }
   }
 }
 
 // Return true if block B never continues to its successors.
 static bool neverReturns(const CFGBlock *B) {
   if (B->hasNoReturnElement())
for (const auto *PrevBlock : SpecialBlocks) {
 
         FactSet PrevLockset;
         getEdgeLockset(PrevLockset, PrevBlockInfo->ExitSet,
                        PrevBlock, CurrBlock);
 
         // Do not update EntrySet.
         intersectAndWarn(
             CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
-            IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
-            !IsLoop);
+            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()) {
for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(),
          SE  = CurrBlock->succ_end(); SI != SE; ++SI) {
       // if CurrBlock -> *SI is *not* a back edge
       if (*SI == nullptr || !VisitedBlocks.alreadySet(*SI))
         continue;
 
       CFGBlock *FirstLoopBlock = *SI;
       CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()];
       CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID];
-      intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet,
-                       PreLoop->EntryLoc,
-                       LEK_LockedSomeLoopIterations,
-                       false);
+      intersectAndWarn(LoopEnd->ExitSet, PreLoop->EntrySet, PreLoop->EntryLoc,
+                       LEK_LockedSomeLoopIterations);
     }
   }
 
   CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()];
   CFGBlockInfo *Final   = &BlockInfo[CFGraph->getExit().getBlockID()];
 
   // Skip the final check if the exit block is unreachable.
   if (!Final->Reachable)
ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
                                          Lock, LK_Exclusive, D->getLocation()));
   for (const auto &Lock : SharedLocksAcquired)
     ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
                                          Lock, LK_Shared, D->getLocation()));
   for (const auto &Lock : LocksReleased)
     ExpectedExitSet.removeLock(FactMan, Lock);
 
   // FIXME: Should we call this function for all blocks which exit the function?
-  intersectAndWarn(ExpectedExitSet, Final->ExitSet,
-                   Final->ExitLoc,
-                   LEK_LockedAtEndOfFunction,
-                   LEK_NotLockedAtEndOfFunction,
-                   false);
+  intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
+                   LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
 
   Handler.leaveFunction(CurrentFunction);
 }
 
 /// Check a function's CFG for thread-safety violations.
 ///
 /// We traverse the blocks in the CFG, compute the set of mutexes that are held
 /// at the end of each block, and issue warnings for thread safety violations.