[clang] dfec26b - Thread safety analysis: Don't warn about managed locks on join points

Aaron Puchert via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 6 13:30:22 PDT 2021


Author: Aaron Puchert
Date: 2021-04-06T22:29:48+02:00
New Revision: dfec26b186d2f0c80f2b70901b7cc5747f5b377c

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

LOG: Thread safety analysis: Don't warn about managed locks on join points

We already did so for scoped locks acquired in the constructor, this
change extends the treatment to deferred locks and scoped unlocking, so
locks acquired outside of the constructor. Obviously this makes things
more consistent.

Originally I thought this was a bad idea, because obviously it
introduces false negatives when it comes to double locking, but these
are typically easily found in tests, and the primary goal of the Thread
safety analysis is not to find double locks but race conditions.
Since the scoped lock will release the mutex anyway when the scope ends,
the inconsistent state is just temporary and probably fine.

Reviewed By: delesley

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

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 84e0e91f597fe..00678c7129a31 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -983,7 +983,7 @@ class ScopedLockableFactEntry : public FactEntry {
     } else {
       FSet.removeLock(FactMan, !Cp);
       FSet.addLock(FactMan,
-                   std::make_unique<LockableFactEntry>(Cp, kind, loc));
+                   std::make_unique<LockableFactEntry>(Cp, kind, loc, true));
     }
   }
 

diff  --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index d1520b1decbd3..b837206138a67 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2595,6 +2595,7 @@ void Foo::test2() {
   if (c) {            // test join point -- held/not held during release
     rlock.Release();
   }
+  // No warning on join point because the lock will be released by the scope object anyway.
 }
 
 void Foo::test3() {
@@ -2615,7 +2616,7 @@ void Foo::test5() {
   if (c) {
     rlock.Release();
   }
-  // no warning on join point for managed lock.
+  // No warning on join point because the lock will be released by the scope object anyway.
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
@@ -2659,6 +2660,7 @@ class SCOPED_LOCKABLE RelockableMutexLock {
 
 Mutex mu;
 int x GUARDED_BY(mu);
+bool b;
 
 void print(int);
 
@@ -2740,6 +2742,23 @@ void doubleLock2() {
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
+void lockJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  if (b)
+    scope.Lock();
+  // No warning on join point because the lock will be released by the scope object anyway.
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void unlockJoin() {
+  RelockableMutexLock scope(&mu, DeferTraits{});
+  scope.Lock();
+  if (b)
+    scope.Unlock();
+  // No warning on join point because the lock will be released by the scope object anyway.
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
 void directUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   mu.Unlock();
@@ -2871,10 +2890,9 @@ void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
 
 void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
   MutexUnlock scope(&mu);
-  if (c) {
-    scope.Lock(); // expected-note{{mutex acquired here}}
-  }
-  // expected-warning at +1{{mutex 'mu' is not held on every path through here}}
+  if (c)
+    scope.Lock();
+  // No warning on join point because the lock will be released by the scope object anyway.
   scope.Lock();
 }
 


        


More information about the cfe-commits mailing list