[clang] 9b889f8 - Thread safety analysis: Warn when demoting locks on back edges

[Aaron Puchert via cfe-commits]

Author: Aaron Puchert
Date: 2021-09-18T13:46:55+02:00
New Revision: 9b889f826ff587e9758c80823419512d502e457d

URL: https://github.com/llvm/llvm-project/commit/9b889f826ff587e9758c80823419512d502e457d
DIFF: https://github.com/llvm/llvm-project/commit/9b889f826ff587e9758c80823419512d502e457d.diff

LOG: Thread safety analysis: Warn when demoting locks on back edges

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.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D106713

Added: 
    

Modified: 
    clang/lib/Analysis/ThreadSafety.cpp
    clang/test/SemaCXX/warn-thread-safety-analysis.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 5b2c882c4235..41a55f9579bd 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1050,7 +1050,7 @@ class ThreadSafetyAnalyzer {
                       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,
@@ -2188,25 +2188,28 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
   }
 }
 
-/// 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;
     }
+    Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());
+    // Take the exclusive capability to reduce further warnings.
+    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();
   }
 }
 
@@ -2237,8 +2240,8 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
 
     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,

diff  --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index e9d41da80517..125a1958c8ba 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2789,6 +2789,25 @@ void loopRelease() {
   }
 }
 
+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) {
@@ -2812,6 +2831,29 @@ void loopReleaseContinue() {
   }
 }
 
+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)