[clang] cf0b337 - Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

[Aaron Puchert via cfe-commits]

Author: Aaron Puchert
Date: 2021-05-27T17:46:04+02:00
New Revision: cf0b337c1b1f064c81fe40124ddba178572778d6

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

LOG: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

Similar to how we allow managed and asserted locks to be held and not
held in joining branches, we also allow them to be held shared and
exclusive. The scoped lock should restore the original state at the end
of the scope in any event, and asserted locks need not be released.

We should probably only allow asserted locks to be subsumed by managed,
not by (directly) acquired locks, but that's for another change.

Reviewed By: delesley

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

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 c65e9f307f114..6727e1315b5f5 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2194,9 +2194,16 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
 /// \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;
+    // 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;
+    }
   } else {
     // The non-asserted capability is the one we want to track.
     return A.asserted() && !B.asserted();

diff  --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 369952eb397a2..8e8bb6f45dde4 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2773,6 +2773,67 @@ void unlockJoin() {
   x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
 }
 
+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);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void sharedExclusiveJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.ReaderLock();
+  else
+    scope.Lock();
+  // No warning on join point because the lock will be released by the scope object anyway.
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.Lock();
+  else
+    mu.AssertHeld();
+  x = 2;
+}
+
+void assertSharedJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.ReaderLock();
+  else
+    mu.AssertReaderHeld();
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertStrongerJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.ReaderLock();
+  else
+    mu.AssertHeld();
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void assertWeakerJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.Lock();
+  else
+    mu.AssertReaderHeld();
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
 void directUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   mu.Unlock();
@@ -4506,8 +4567,8 @@ class Foo {
 
   void test6() {
     mu_.AssertHeld();
-    mu_.Unlock();
-  }  // should this be a warning?
+    mu_.Unlock(); // should this be a warning?
+  }
 
   void test7() {
     if (c) {
@@ -4528,9 +4589,10 @@ class Foo {
     else {
       mu_.AssertHeld();
     }
+    // FIXME: should warn, because it's unclear whether we need to release or not.
     int b = a;
     a = 0;
-    mu_.Unlock();
+    mu_.Unlock(); // should this be a warning?
   }
 
   void test9() {
@@ -4558,6 +4620,28 @@ class Foo {
     int b = a;
     a = 0;
   }
+
+  void test12() {
+    if (c)
+      mu_.ReaderLock(); // expected-warning {{mutex 'mu_' is acquired exclusively and shared in the same scope}}
+    else
+      mu_.AssertHeld(); // expected-note {{the other acquisition of mutex 'mu_' is here}}
+    // FIXME: should instead warn because it's unclear whether we need to release or not.
+    int b = a;
+    a = 0;
+    mu_.Unlock();
+  }
+
+  void test13() {
+    if (c)
+      mu_.Lock(); // expected-warning {{mutex 'mu_' is acquired exclusively and shared in the same scope}}
+    else
+      mu_.AssertReaderHeld(); // expected-note {{the other acquisition of mutex 'mu_' is here}}
+    // FIXME: should instead warn because it's unclear whether we need to release or not.
+    int b = a;
+    a = 0;
+    mu_.Unlock();
+  }
 };
 
 }  // end namespace AssertHeldTest