Thread safety analysis: Factor out function for merging locks (NFC)

[aaronpuchert]

Summary

It's going to become a bit more complicated, so let's have it separate.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

2199

The old code was looking at LDat1.kind() != LK_Exclusive -- any reason this isn't A.kind() != LK_Exclusive as well?

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2199

Given that both should be equivalent (knowing A.kind() != B.kind() and that there are just two kinds), I thought this condition fits better to the comment: we select the lock that's exclusive, instead of not selecting the lock that's not exclusive.

But I don't have a problem with A.kind() != LK_Exclusive if that sounds more natural to you.

[aaron.ballman]

LGTM!

clang/lib/Analysis/ThreadSafety.cpp

2199

Oohhhh, derp. I was forgetting that there were only two kinds. This looks fine to me, thanks!

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);
 
+  bool join(const FactEntry &a, const FactEntry &b);
+
   void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
                         SourceLocation JoinLoc, LockErrorKind LEK1,
                         LockErrorKind LEK2);
 
   void intersectAndWarn(FactSet &FSet1, const FactSet &FSet2,
                         SourceLocation JoinLoc, LockErrorKind LEK1) {
     intersectAndWarn(FSet1, FSet2, JoinLoc, LEK1, LEK1);
   }
if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
         if (!CtorD || !CtorD->hasAttrs())
           continue;
         handleCall(buildFakeCtorCall(CtorD, {E}, E->getBeginLoc()), CtorD, VD);
       }
     }
   }
 }
 
+/// Given two facts merging on a join point, decide whether to warn and which
+/// one to keep.
+///
+/// \return  false if we should keep \p A, true if we should keep \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B) {
+  if (A.kind() != B.kind()) {
+    Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+    // Take the exclusive capability to reduce further warnings.
+    return B.kind() == LK_Exclusive;
+  } else {
+    // The non-asserted capability is the one we want to track.
+    return A.asserted() && !B.asserted();
+  }
+}
+
 /// Compute the intersection of two locksets and issue warnings for any
 /// locks in the symmetric difference.
 ///
 /// This function is used at a merge point in the CFG when comparing the lockset
 /// of each branch being merged. For example, given the following sequence:
 /// A; if () then B; else C; D; we need to check that the lockset after B and C
 /// are the same. In the event of a difference, we use the intersection of these
 /// two locksets at the start of D.
void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1,
   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 (LEK1 == LEK_LockedSomePredecessors &&
-            LDat1.kind() != LK_Exclusive) {
-          // Take the exclusive lock, which is the one in FSet2.
-          *Iter1 = Fact;
-        }
-      } else if (LEK1 == LEK_LockedSomePredecessors && LDat1.asserted() &&
-                 !LDat2.asserted()) {
-        // The non-asserted lock in FSet2 is the one we want to track.
+      if (join(FactMan[*Iter1], LDat2) && LEK1 == LEK_LockedSomePredecessors)
         *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) {