Thread safety analysis: Warn when demoting locks on back edges

[aaronpuchert]

Summary

Previously in D104261 we warned about dropping locks from back edges,
this is the corresponding change for exclusive/shared joins. If we're
entering the loop with an exclusive change, which is then relaxed to a
shared lock before we loop back, we have already analyzed the loop body
with the stronger exclusive lock and thus might have false positives.

There is a minor non-observable change: we modify the exit lock set of a
function, but since that isn't used further it doesn't change anything.

[aaron.ballman]

This seems reasonable to me, but I leave it to @delesley for the final sign-off.

[aaronpuchert]

Ping @delesley.

[aaronpuchert]

Ping.

[aaron.ballman]

Accepting the review -- if you don't hear back from @delesley in the next few days, I think it's fine to land.

[aaronpuchert]

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

[aaron.ballman]

In D106713#3007878, @aaronpuchert wrote:

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

I think that's reasonable; the alternative is that we warn in Clang 12, don't warn in Clang 13, then start warning again in Clang 14 which does not seem very user friendly.

[aaronpuchert]

In D106713#3009130, @aaron.ballman wrote:

In D106713#3007878, @aaronpuchert wrote:

@aaron.ballman, since this is reintroducing some warnings after the relaxation in D102026, should we bring this to Clang 13?

I think that's reasonable; the alternative is that we warn in Clang 12, don't warn in Clang 13, then start warning again in Clang 14 which does not seem very user friendly.

@tstellar, could we still bring this into release/13.x? Doesn't fix a crash, but it prevents inconsistencies between our releases and I also tested it on a pretty large code base. So I'm pretty confident it doesn't regress anything.

[tstellar]

@aaronpuchert Can you file a bug?

[aaronpuchert]

In D106713#3009542, @tstellar wrote:

@aaronpuchert Can you file a bug?

Done: https://bugs.llvm.org/show_bug.cgi?id=51913.

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);
+  bool join(const FactEntry &a, const FactEntry &b, bool CanModify);
 
   void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,
                         SourceLocation JoinLoc, LockErrorKind EntryLEK,
                         LockErrorKind ExitLEK);
 
   void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,
                         SourceLocation JoinLoc, LockErrorKind LEK) {
     intersectAndWarn(EntrySet, ExitSet, JoinLoc, LEK, LEK);
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.
+/// Given two facts merging on a join point, possibly warn and decide whether to
+/// keep or replace.
 ///
-/// \return  false if we should keep \p A, true if we should keep \p B.
-bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B) {
+/// \param CanModify Whether we can replace \p A by \p B.
+/// \return  false if we should keep \p A, true if we should take \p B.
+bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B,
+                                bool CanModify) {
   if (A.kind() != B.kind()) {
     // For managed capabilities, the destructor should unlock in the right mode
     // anyway. For asserted capabilities no unlocking is needed.
     if ((A.managed() || A.asserted()) && (B.managed() || B.asserted())) {
-      // The shared capability subsumes the exclusive capability.
-      return B.kind() == LK_Shared;
-    } else {
-      Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
-      // Take the exclusive capability to reduce further warnings.
-      return B.kind() == LK_Exclusive;
-    }
+      // The shared capability subsumes the exclusive capability, if possible.
+      bool ShouldTakeB = B.kind() == LK_Shared;
+      if (CanModify || !ShouldTakeB)
+        return ShouldTakeB;
+    }
+    return CanModify && B.kind() == LK_Exclusive;
   } else {
     // The non-asserted capability is the one we want to track.
-    return A.asserted() && !B.asserted();
+    return CanModify && 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:
void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
   FactSet EntrySetOrig = EntrySet;
 
   // Find locks in ExitSet that conflict or are not in EntrySet, and warn.
   for (const auto &Fact : ExitSet) {
     const FactEntry &ExitFact = FactMan[Fact];
 
     FactSet::iterator EntryIt = EntrySet.findLockIter(FactMan, ExitFact);
     if (EntryIt != EntrySet.end()) {
-      if (join(FactMan[*EntryIt], ExitFact) &&
-          EntryLEK == LEK_LockedSomePredecessors)
+      if (join(FactMan[*EntryIt], ExitFact,
+               EntryLEK != LEK_LockedSomeLoopIterations))
         *EntryIt = Fact;
     } else if (!ExitFact.managed()) {
       ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
                                              EntryLEK, Handler);
     }
   }
 
   // Find locks in EntrySet that are not in ExitSet, and remove them.

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

void loopRelease() {
   // We have to warn on this join point despite the lock being managed ...
   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();
   }
 }
 
+void loopPromote() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  for (unsigned i = 1; i < 10; ++i) {
+    x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+    if (i == 5)
+      scope.PromoteShared();
+  }
+}
+
+void loopDemote() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is here}}
+  // We have to warn on this join point despite the lock being managed ...
+  for (unsigned i = 1; i < 10; ++i) {
+    x = 1; // ... because we might miss that this doesn't always happen under exclusive lock.
+    if (i == 5)
+      scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}}
+  }
+}
+
 void loopAcquireContinue() {
   RelockableMutexLock scope(&mu, DeferTraits{});
   for (unsigned i = 1; i < 10; ++i) {
     x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
     if (i == 5) {
       scope.Lock();
       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) {
     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}}
     }
   }
 }
 
+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}}
+    if (i == 5) {
+      scope.PromoteShared();
+      continue;
+    }
+  }
+}
+
+void loopDemoteContinue() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is here}}
+  // We have to warn on this join point despite the lock being managed ...
+  for (unsigned i = 1; i < 10; ++i) {
+    x = 1; // ... because we might miss that this doesn't always happen under exclusive lock.
+    if (i == 5) {
+      scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}}
+      continue;
+    }
+  }
+}
+
 void exclusiveSharedJoin() {
   RelockableMutexLock scope(&mu, DeferTraits{});
   if (b)
     scope.Lock();
   else
     scope.ReaderLock();
   // No warning on join point because the lock will be released by the scope object anyway.
   print(x);