Add tid validation checks to blocking mutex and rw mutex

[fjricci]

Summary

Blocking mutex currently only performs tid validation checks
in the mac and windows implementations. Add these checks to the
linux implementation as well.

Also add analogous validation checks to rw mutex. In order to
access the current tid, this requires moving the implementation
of rw mutex out of the sanitizer mutex header file.

[dvyukov]

This adds 5 calls and 3 syscalls per rw mutex lock/unlock. What's the performance effect of this?

[fjricci]

I'm working on evaluating that currently. I assume that since mac and windows already use these checks, it won't be a significant decrease in performance (and if it is, we should consider removing the checks from those platforms).

[dvyukov]

lib/sanitizer_common/sanitizer_common.cc

77

"state == kReadLock" must be "state >= kReadLock" because kReadLock is a counter.
Checking state != kUnlocked when owner_ == GetTid() is pointless because state is always kWriteLock when owner_ == GetTid().
This should be:
CHECK(state >= kReadLock || owner_ == GetTid());
Also note the reversed order of conditions -- syscall is slower than a check of local variable.

[fjricci]

Fixed via D29728

lib/sanitizer_common/sanitizer_common.cc

 const char *SanitizerToolName = "SanitizerTool";
 
 atomic_uint32_t current_verbosity;
 uptr PageSizeCached;
 
 StaticSpinMutex report_file_mu;
 ReportFile report_file = {&report_file_mu, kStderrFd, "", "", 0};
 
+RWMutex::RWMutex() {
+  owner_ = 0;
+  atomic_store(&state_, kUnlocked, memory_order_relaxed);
+}
+
+RWMutex::~RWMutex() {
+  CHECK_EQ(atomic_load(&state_, memory_order_relaxed), kUnlocked);
+}
+
+void RWMutex::Lock() {
+  CHECK_NE(owner_, GetTid());
+  u32 cmp = kUnlocked;
+  if (!atomic_compare_exchange_strong(&state_, &cmp, kWriteLock,
+                                      memory_order_acquire)) {
+    LockSlow();
+  }
+  CHECK_EQ(owner_, 0);
+  owner_ = GetTid();
+}
+
+void RWMutex::Unlock() {
+  CHECK_EQ(owner_, GetTid());
+  owner_ = 0;
+  u32 prev = atomic_fetch_sub(&state_, kWriteLock, memory_order_release);
+  DCHECK_NE(prev & kWriteLock, 0);
+  (void)prev;
+}
+
+void RWMutex::ReadLock() {
+  u32 prev = atomic_fetch_add(&state_, kReadLock, memory_order_acquire);
+  if ((prev & kWriteLock) != 0) {
+    ReadLockSlow();
+  }
+}
+
+void RWMutex::ReadUnlock() {
+  u32 prev = atomic_fetch_sub(&state_, kReadLock, memory_order_release);
+  DCHECK_EQ(prev & kWriteLock, 0);
+  DCHECK_GT(prev & ~kWriteLock, 0);
+  (void)prev;
+}
+
+void RWMutex::CheckLocked() {
+  u32 state = atomic_load(&state_, memory_order_relaxed);
+  CHECK((owner_ == GetTid()) ? state != kUnlocked : state == kReadLock);
+}
+
+void NOINLINE RWMutex::LockSlow() {
+  for (int i = 0;; i++) {
+    if (i < 10)
+      proc_yield(10);
+    else
+      internal_sched_yield();
+    u32 cmp = atomic_load(&state_, memory_order_relaxed);
+    if (cmp == kUnlocked &&
+        atomic_compare_exchange_weak(&state_, &cmp, kWriteLock,
+                                     memory_order_acquire))
+        return;
+  }
+}
+
+void NOINLINE RWMutex::ReadLockSlow() {
+  for (int i = 0;; i++) {
+    if (i < 10)
+      proc_yield(10);
+    else
+      internal_sched_yield();
+    u32 prev = atomic_load(&state_, memory_order_acquire);
+    if ((prev & kWriteLock) == 0)
+      return;
+  }
+}
+
 void RawWrite(const char *buffer) {
   report_file.Write(buffer, internal_strlen(buffer));
 }
 
 void ReportFile::ReopenIfNecessary() {
   mu->CheckLocked();
   if (fd == kStdoutFd || fd == kStderrFd) return;
 

lib/sanitizer_common/sanitizer_linux.cc

enum MutexState {
   MtxSleeping = 2
 };
 
 BlockingMutex::BlockingMutex() {
   internal_memset(this, 0, sizeof(*this));
 }
 
 void BlockingMutex::Lock() {
-  CHECK_EQ(owner_, 0);
+  CHECK_NE(owner_, GetTid());
   atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
-  if (atomic_exchange(m, MtxLocked, memory_order_acquire) == MtxUnlocked)
-    return;
-  while (atomic_exchange(m, MtxSleeping, memory_order_acquire) != MtxUnlocked) {
+  if (atomic_exchange(m, MtxLocked, memory_order_acquire) != MtxUnlocked) {
+    while (atomic_exchange(m, MtxSleeping, memory_order_acquire)
+           != MtxUnlocked) {
 #if SANITIZER_FREEBSD
       _umtx_op(m, UMTX_OP_WAIT_UINT, MtxSleeping, 0, 0);
 #else
-    internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAIT, MtxSleeping, 0, 0, 0);
+      internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAIT, MtxSleeping,
+                       0, 0, 0);
 #endif
     }
   }
+  CHECK_EQ(owner_, 0);
+  owner_ = GetTid();
+}
 
 void BlockingMutex::Unlock() {
+  CHECK_EQ(owner_, GetTid());
+  owner_ = 0;
   atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
   u32 v = atomic_exchange(m, MtxUnlocked, memory_order_relaxed);
   CHECK_NE(v, MtxUnlocked);
   if (v == MtxSleeping) {
 #if SANITIZER_FREEBSD
     _umtx_op(m, UMTX_OP_WAKE, 1, 0, 0);
 #else
     internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAKE, 1, 0, 0, 0);
 #endif
   }
 }
 
 void BlockingMutex::CheckLocked() {
+  CHECK_EQ(owner_, GetTid());
   atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
   CHECK_NE(MtxUnlocked, atomic_load(m, memory_order_relaxed));
 }
 
 // ----------------- sanitizer_linux.h
 // The actual size of this structure is specified by d_reclen.
 // Note that getdents64 uses a different structure format. We only provide the
 // 32-bit syscall here.

lib/sanitizer_common/sanitizer_mutex.h

  private:
   uptr opaque_storage_[10];
   uptr owner_;  // for debugging
 };
 
 // Reader-writer spin mutex.
 class RWMutex {
  public:
-  RWMutex() {
-    atomic_store(&state_, kUnlocked, memory_order_relaxed);
-  }
-
-  ~RWMutex() {
-    CHECK_EQ(atomic_load(&state_, memory_order_relaxed), kUnlocked);
-  }
-
-  void Lock() {
-    u32 cmp = kUnlocked;
-    if (atomic_compare_exchange_strong(&state_, &cmp, kWriteLock,
-                                       memory_order_acquire))
-      return;
-    LockSlow();
-  }
-
-  void Unlock() {
-    u32 prev = atomic_fetch_sub(&state_, kWriteLock, memory_order_release);
-    DCHECK_NE(prev & kWriteLock, 0);
-    (void)prev;
-  }
-
-  void ReadLock() {
-    u32 prev = atomic_fetch_add(&state_, kReadLock, memory_order_acquire);
-    if ((prev & kWriteLock) == 0)
-      return;
-    ReadLockSlow();
-  }
-
-  void ReadUnlock() {
-    u32 prev = atomic_fetch_sub(&state_, kReadLock, memory_order_release);
-    DCHECK_EQ(prev & kWriteLock, 0);
-    DCHECK_GT(prev & ~kWriteLock, 0);
-    (void)prev;
-  }
-
-  void CheckLocked() {
-    CHECK_NE(atomic_load(&state_, memory_order_relaxed), kUnlocked);
-  }
-
+  RWMutex();
+  ~RWMutex();
+  void Lock();
+  void Unlock();
+  void ReadLock();
+  void ReadUnlock();
+  void CheckLocked();
  private:
   atomic_uint32_t state_;
+  uptr owner_;  // for debugging, unset for read-only locks
 
   enum {
     kUnlocked = 0,
     kWriteLock = 1,
     kReadLock = 2
   };
 
-  void NOINLINE LockSlow() {
-    for (int i = 0;; i++) {
-      if (i < 10)
-        proc_yield(10);
-      else
-        internal_sched_yield();
-      u32 cmp = atomic_load(&state_, memory_order_relaxed);
-      if (cmp == kUnlocked &&
-          atomic_compare_exchange_weak(&state_, &cmp, kWriteLock,
-                                       memory_order_acquire))
-          return;
-    }
-  }
-
-  void NOINLINE ReadLockSlow() {
-    for (int i = 0;; i++) {
-      if (i < 10)
-        proc_yield(10);
-      else
-        internal_sched_yield();
-      u32 prev = atomic_load(&state_, memory_order_acquire);
-      if ((prev & kWriteLock) == 0)
-        return;
-    }
-  }
+  void NOINLINE LockSlow();
+  void NOINLINE ReadLockSlow();
 
   RWMutex(const RWMutex&);
   void operator = (const RWMutex&);
 };
 
 template<typename MutexType>
 class GenericScopedLock {
  public: