[PATCH] D101202: Thread safety analysis: Fix false negative on break

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We weren't modifying the lock set when intersecting with one coming
from a break-terminated block. This is inconsistent, since break isn't a
back edge, and it leads to false negatives with scoped locks. We usually
don't warn for those when joining locksets aren't the same, we just
silently remove locks that are not in the intersection. But not warning
and not removing them isn't right.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101202

Files:
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/PCH/thread-safety-attrs.cpp
  clang/test/SemaCXX/warn-thread-safety-analysis.cpp


Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -337,7 +337,8 @@
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 //-----------------------------------------//
@@ -2582,6 +2583,7 @@
   void test3();
   void test4();
   void test5();
+  void test6();
 };
 
 
@@ -2620,6 +2622,18 @@
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not 
held}}
 }
 
+void Foo::test6() {
+  ReleasableMutexLock rlock(&mu_);
+  do {
+    if (c) {
+      rlock.Release();
+      break;
+    }
+  } while (c);
+  // No warning on join point because the lock will be released by the scope 
object anyway
+  a = 1; // expected-warning {{writing variable 'a' requires holding mutex 
'mu_' exclusively}}
+}
+
 
 } // end namespace ReleasableScopedLock
 
Index: clang/test/PCH/thread-safety-attrs.cpp
===================================================================
--- clang/test/PCH/thread-safety-attrs.cpp
+++ clang/test/PCH/thread-safety-attrs.cpp
@@ -311,7 +311,8 @@
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 #endif
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2464,11 +2464,10 @@
                        PrevBlock, CurrBlock);
 
         // Do not update EntrySet.
-        intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset,
-                         PrevBlockInfo->ExitLoc,
-                         IsLoop ? LEK_LockedSomeLoopIterations
-                                : LEK_LockedSomePredecessors,
-                         false);
+        intersectAndWarn(
+            CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
+            IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
+            !IsLoop);
       }
     }
 



Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -337,7 +337,8 @@
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 //-----------------------------------------//
@@ -2582,6 +2583,7 @@
   void test3();
   void test4();
   void test5();
+  void test6();
 };
 
 
@@ -2620,6 +2622,18 @@
   rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
 }
 
+void Foo::test6() {
+  ReleasableMutexLock rlock(&mu_);
+  do {
+    if (c) {
+      rlock.Release();
+      break;
+    }
+  } while (c);
+  // No warning on join point because the lock will be released by the scope object anyway
+  a = 1; // expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}}
+}
+
 
 } // end namespace ReleasableScopedLock
 
Index: clang/test/PCH/thread-safety-attrs.cpp
===================================================================
--- clang/test/PCH/thread-safety-attrs.cpp
+++ clang/test/PCH/thread-safety-attrs.cpp
@@ -311,7 +311,8 @@
     }
     sls_mu.Lock();
   }
-  sls_mu.Unlock();
+  sls_mu.Unlock(); // \
+    // expected-warning{{releasing mutex 'sls_mu' that was not held}}
 }
 
 #endif
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2464,11 +2464,10 @@
                        PrevBlock, CurrBlock);
 
         // Do not update EntrySet.
-        intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset,
-                         PrevBlockInfo->ExitLoc,
-                         IsLoop ? LEK_LockedSomeLoopIterations
-                                : LEK_LockedSomePredecessors,
-                         false);
+        intersectAndWarn(
+            CurrBlockInfo->EntrySet, PrevLockset, PrevBlockInfo->ExitLoc,
+            IsLoop ? LEK_LockedSomeLoopIterations : LEK_LockedSomePredecessors,
+            !IsLoop);
       }
     }
 

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits