[compiler-rt] r202401 - [sanitizer] do not acquire a global mutex in deadlock detector when dealing with Unlock (it is essentially a thread-local operation)

[Kostya Serebryany]

Author: kcc
Date: Thu Feb 27 08:38:42 2014
New Revision: 202401

URL: http://llvm.org/viewvc/llvm-project?rev=202401&view=rev
Log:
[sanitizer] do not acquire a global mutex in deadlock detector when dealing with Unlock (it is essentially a thread-local operation)

Modified:
    compiler-rt/trunk/lib/sanitizer_common/sanitizer_deadlock_detector.h
    compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_deadlock_detector_test.cc
    compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_mutex.cc

Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_deadlock_detector.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_deadlock_detector.h?rev=202401&r1=202400&r2=202401&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_deadlock_detector.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_deadlock_detector.h Thu Feb 27 08:38:42 2014
@@ -48,16 +48,17 @@ class DeadlockDetectorTLS {
     epoch_ = current_epoch;
   }
 
+  uptr getEpoch() const { return epoch_; }
+
   void addLock(uptr lock_id, uptr current_epoch) {
     // Printf("addLock: %zx %zx\n", lock_id, current_epoch);
     CHECK_EQ(epoch_, current_epoch);
     CHECK(bv_.setBit(lock_id));
   }
 
-  void removeLock(uptr lock_id, uptr current_epoch) {
+  void removeLock(uptr lock_id) {
     // Printf("remLock: %zx %zx\n", lock_id, current_epoch);
-    CHECK_EQ(epoch_, current_epoch);
-    bv_.clearBit(lock_id);  // May already be cleared due to epoch update.
+    CHECK(bv_.clearBit(lock_id));
   }
 
   const BV &getLocks(uptr current_epoch) const {
@@ -75,7 +76,8 @@ class DeadlockDetectorTLS {
 // and one DeadlockDetectorTLS object per evey thread.
 // This class is not thread safe, all concurrent accesses should be guarded
 // by an external lock.
-// Not thread-safe, all accesses should be protected by an external lock.
+// Most of the methods of this class are not thread-safe (i.e. should
+// be protected by an external lock) unless explicitly told otherwise.
 template <class BV>
 class DeadlockDetector {
  public:
@@ -133,7 +135,7 @@ class DeadlockDetector {
   }
 
   // Handles the lock event, returns true if there is a cycle.
-  // FIXME: handle RW locks, recusive locks, etc.
+  // FIXME: handle RW locks, recursive locks, etc.
   bool onLock(DeadlockDetectorTLS<BV> *dtls, uptr cur_node) {
     ensureCurrentEpoch(dtls);
     uptr cur_idx = nodeToIndex(cur_node);
@@ -171,10 +173,11 @@ class DeadlockDetector {
     return res;
   }
 
-  // Handle the unlock event.
+  // Handle the unlock event. This operation is thread-safe
+  // as it only touches the dtls.
   void onUnlock(DeadlockDetectorTLS<BV> *dtls, uptr node) {
-    ensureCurrentEpoch(dtls);
-    dtls->removeLock(nodeToIndex(node), current_epoch_);
+    if (dtls->getEpoch() == nodeToEpoch(node))
+      dtls->removeLock(nodeToIndexUnchecked(node));
   }
 
   bool isHeld(DeadlockDetectorTLS<BV> *dtls, uptr node) const {
@@ -202,7 +205,7 @@ class DeadlockDetector {
 
   void check_node(uptr node) const {
     CHECK_GE(node, size());
-    CHECK_EQ(current_epoch_, node / size() * size());
+    CHECK_EQ(current_epoch_, nodeToEpoch(node));
   }
 
   uptr indexToNode(uptr idx) const {
@@ -210,11 +213,15 @@ class DeadlockDetector {
     return idx + current_epoch_;
   }
 
+  uptr nodeToIndexUnchecked(uptr node) const { return node % size(); }
+
   uptr nodeToIndex(uptr node) const {
     check_node(node);
-    return node % size();
+    return nodeToIndexUnchecked(node);
   }
 
+  uptr nodeToEpoch(uptr node) const { return node / size() * size(); }
+
   uptr getAvailableNode(uptr data) {
     uptr idx = available_nodes_.getAndClearFirstOne();
     data_[idx] = data;

Modified: compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_deadlock_detector_test.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_deadlock_detector_test.cc?rev=202401&r1=202400&r2=202401&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_deadlock_detector_test.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_deadlock_detector_test.cc Thu Feb 27 08:38:42 2014
@@ -269,9 +269,8 @@ void RunMultipleEpochsTest() {
   EXPECT_EQ(d.testOnlyGetEpoch(), 4 * d.size());
 
   // Can not handle the locks from the previous epoch.
-  // The user should update the lock id.
+  // The caller should update the lock id.
   EXPECT_DEATH(d.onLock(&dtls, l0), "CHECK failed.*current_epoch_");
-  EXPECT_DEATH(d.onUnlock(&dtls, l1), "CHECK failed.*current_epoch_");
 }
 
 TEST(DeadlockDetector, MultipleEpochsTest) {

Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_mutex.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_mutex.cc?rev=202401&r1=202400&r2=202401&view=diff
==============================================================================
--- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_mutex.cc (original)
+++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_mutex.cc Thu Feb 27 08:38:42 2014
@@ -32,8 +32,6 @@ static void EnsureDeadlockDetectorID(Con
 static void DDUnlock(Context *ctx, ThreadState *thr, SyncVar *s) {
   if (!common_flags()->detect_deadlocks) return;
   if (s->recursion != 0) return;
-  Lock lk(&ctx->dd_mtx);
-  EnsureDeadlockDetectorID(ctx, thr, s);
   // Printf("T%d MutexUnlock: %zx; recursion %d\n", thr->tid,
   //        s->deadlock_detector_id, s->recursion);
   ctx->dd.onUnlock(&thr->deadlock_detector_tls, s->deadlock_detector_id);