sanitizer_common: replace RWMutex/BlockingMutex with Mutex

[dvyukov]

Summary

Mutex supports reader access, OS blocking, spinning,
portable and smaller than BlockingMutex.
Overall it's supposed to be better than RWMutex/BlockingMutex.
Replace RWMutex/BlockingMutex with Mutex.

[dvyukov]

also remove BlockingMutex test.

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

345

I think there are a few locations which rely on this constructor. It compiles, but Mutex's constructor takes MutexType and LINKER_INITIALIZED==MutexInvalid.

% git grep 'BlockingMutex.*LINKER_INITIALIZED'
compiler-rt/lib/asan/asan_globals.cpp:static BlockingMutex mu_for_globals(LINKER_INITIALIZED);
compiler-rt/lib/asan/asan_report.cpp:static BlockingMutex error_message_buf_mutex(LINKER_INITIALIZED);
compiler-rt/lib/asan/asan_stats.cpp:static BlockingMutex print_lock(LINKER_INITIALIZED);
compiler-rt/lib/asan/asan_stats.cpp:static BlockingMutex dead_threads_stats_lock(LINKER_INITIALIZED);
compiler-rt/lib/asan/asan_thread.cpp:static BlockingMutex mu_for_thread_context(LINKER_INITIALIZED);
compiler-rt/lib/cfi/cfi.cpp:BlockingMutex shadow_update_lock(LINKER_INITIALIZED);
compiler-rt/lib/cfi/cfi.cpp:static BlockingMutex interceptor_init_lock(LINKER_INITIALIZED);
compiler-rt/lib/lsan/lsan_common.cpp:BlockingMutex global_mutex(LINKER_INITIALIZED);
compiler-rt/lib/memprof/memprof_stats.cpp:static BlockingMutex print_lock(LINKER_INITIALIZED);
compiler-rt/lib/memprof/memprof_stats.cpp:static BlockingMutex dead_threads_stats_lock(LINKER_INITIALIZED);
compiler-rt/lib/memprof/memprof_thread.cpp:static BlockingMutex mu_for_thread_context(LINKER_INITIALIZED);
compiler-rt/lib/sanitizer_common/sanitizer_coverage_fuchsia.cpp:  BlockingMutex setup_lock_ = BlockingMutex(LINKER_INITIALIZED);
compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp:  //   BlockingMutex::BlockingMutex() : BlockingMutex(LINKER_INITIALIZED) {}
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp:static BlockingMutex syslog_lock(LINKER_INITIALIZED);
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp:static BlockingMutex crashreporter_info_mutex(LINKER_INITIALIZED);
compiler-rt/lib/sanitizer_common/tests/sanitizer_mutex_test.cpp:  BlockingMutex *mtx = new(mtxmem) BlockingMutex(LINKER_INITIALIZED);
compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp:static BlockingMutex tctx_allocator_lock(LINKER_INITIALIZED)

The cleanest would probably to just change all them to use the default constructor which is now constexpr.

[dvyukov]

rebase to main HEAD

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

345

Very good point.
I've sent 2 other changes, see "Stack" section.

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

375

Perhaps turn this into a TODO / FIXME so it shows up in 'git grep TODO' and less likely to be forgotten.

I assume this will disappear soon?

( s/Temporal/Temporary/, s/typedef's/typedefs/ )

[dvyukov]

improved TODO comment

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

375

I assume this will disappear soon?

Yes, I will resolve it once this change puts down root.
It's not a no-op change, so has some chances of being reverted.

compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp

if (status != ZX_ERR_BAD_STATE)  // Normal race.
     CHECK_EQ(status, ZX_OK);
 }
 
 void FutexWake(atomic_uint32_t *p, u32 count) {
   zx_status_t status = _zx_futex_wake(reinterpret_cast<zx_futex_t *>(p), count);
   CHECK_EQ(status, ZX_OK);
 }
 
-enum MutexState : int { MtxUnlocked = 0, MtxLocked = 1, MtxSleeping = 2 };
-
-BlockingMutex::BlockingMutex() {
-  // NOTE!  It's important that this use internal_memset, because plain
-  // memset might be intercepted (e.g., actually be __asan_memset).
-  // Defining this so the compiler initializes each field, e.g.:
-  //   BlockingMutex::BlockingMutex() : BlockingMutex(LINKER_INITIALIZED) {}
-  // might result in the compiler generating a call to memset, which would
-  // have the same problem.
-  internal_memset(this, 0, sizeof(*this));
-}
-
-void BlockingMutex::Lock() {
-  CHECK_EQ(owner_, 0);
-  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) {
-    zx_status_t status =
-        _zx_futex_wait(reinterpret_cast<zx_futex_t *>(m), MtxSleeping,
-                       ZX_HANDLE_INVALID, ZX_TIME_INFINITE);
-    if (status != ZX_ERR_BAD_STATE)  // Normal race.
-      CHECK_EQ(status, ZX_OK);
-  }
-}
-
-void BlockingMutex::Unlock() {
-  atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
-  u32 v = atomic_exchange(m, MtxUnlocked, memory_order_release);
-  CHECK_NE(v, MtxUnlocked);
-  if (v == MtxSleeping) {
-    zx_status_t status = _zx_futex_wake(reinterpret_cast<zx_futex_t *>(m), 1);
-    CHECK_EQ(status, ZX_OK);
-  }
-}
-
-void BlockingMutex::CheckLocked() const {
-  auto m = reinterpret_cast<atomic_uint32_t const *>(&opaque_storage_);
-  CHECK_NE(MtxUnlocked, atomic_load(m, memory_order_relaxed));
-}
-
 uptr GetPageSize() { return _zx_system_get_page_size(); }
 
 uptr GetMmapGranularity() { return _zx_system_get_page_size(); }
 
 sanitizer_shadow_bounds_t ShadowBounds;
 
 void InitShadowBounds() { ShadowBounds = __sanitizer_shadow_bounds(); }
 

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

#    if SANITIZER_FREEBSD
   _umtx_op(p, UMTX_OP_WAKE, count, 0, 0);
 #    elif SANITIZER_NETBSD
                    /* No userspace futex-like synchronization */
 #    else
   internal_syscall(SYSCALL(futex), (uptr)p, FUTEX_WAKE_PRIVATE, count, 0, 0, 0);
 #    endif
 }
 
-enum { MtxUnlocked = 0, MtxLocked = 1, MtxSleeping = 2 };
-
-BlockingMutex::BlockingMutex() {
-  internal_memset(this, 0, sizeof(*this));
-}
-
-void BlockingMutex::Lock() {
-  CHECK_EQ(owner_, 0);
-  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 SANITIZER_FREEBSD
-    _umtx_op(m, UMTX_OP_WAIT_UINT, MtxSleeping, 0, 0);
-#elif SANITIZER_NETBSD
-    sched_yield(); /* No userspace futex-like synchronization */
-#else
-    internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAIT_PRIVATE, MtxSleeping,
-                     0, 0, 0);
-#endif
-  }
-}
-
-void BlockingMutex::Unlock() {
-  atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
-  u32 v = atomic_exchange(m, MtxUnlocked, memory_order_release);
-  CHECK_NE(v, MtxUnlocked);
-  if (v == MtxSleeping) {
-#if SANITIZER_FREEBSD
-    _umtx_op(m, UMTX_OP_WAKE, 1, 0, 0);
-#elif SANITIZER_NETBSD
-                   /* No userspace futex-like synchronization */
-#else
-    internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAKE_PRIVATE, 1, 0, 0, 0);
-#endif
-  }
-}
-
-void BlockingMutex::CheckLocked() const {
-  auto m = reinterpret_cast<atomic_uint32_t const *>(&opaque_storage_);
-  CHECK_NE(MtxUnlocked, atomic_load(m, memory_order_relaxed));
-}
 #  endif  // !SANITIZER_SOLARIS
 
 // ----------------- 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.
 #if SANITIZER_NETBSD
 // Not used

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 
 void FutexWait(atomic_uint32_t *p, u32 cmp) {
   // FIXME: implement actual blocking.
   sched_yield();
 }
 
 void FutexWake(atomic_uint32_t *p, u32 count) {}
 
-BlockingMutex::BlockingMutex() {
-  internal_memset(this, 0, sizeof(*this));
-}
-
-void BlockingMutex::Lock() {
-  CHECK(sizeof(OSSpinLock) <= sizeof(opaque_storage_));
-  CHECK_EQ(OS_SPINLOCK_INIT, 0);
-  CHECK_EQ(owner_, 0);
-  OSSpinLockLock((OSSpinLock*)&opaque_storage_);
-}
-
-void BlockingMutex::Unlock() {
-  OSSpinLockUnlock((OSSpinLock*)&opaque_storage_);
-}
-
-void BlockingMutex::CheckLocked() const {
-  CHECK_NE(*(const OSSpinLock*)&opaque_storage_, 0);
-}
-
 u64 NanoTime() {
   timeval tv;
   internal_memset(&tv, 0, sizeof(tv));
   gettimeofday(&tv, 0);
   return (u64)tv.tv_sec * 1000*1000*1000 + tv.tv_usec * 1000;
 }
 
 // This needs to be called during initialization to avoid being racy.

compiler-rt/lib/sanitizer_common/sanitizer_mutex.h

private:
   Mutex(LinkerInitialized) = delete;
   Mutex(const Mutex &) = delete;
   void operator=(const Mutex &) = delete;
 };
 
 void FutexWait(atomic_uint32_t *p, u32 cmp);
 void FutexWake(atomic_uint32_t *p, u32 count);
 
-class MUTEX BlockingMutex {
- public:
-  explicit constexpr BlockingMutex(LinkerInitialized)
-      : opaque_storage_ {0, }, owner_ {0} {}
-  BlockingMutex();
-  void Lock() ACQUIRE();
-  void Unlock() RELEASE();
-
-  // This function does not guarantee an explicit check that the calling thread
-  // is the thread which owns the mutex. This behavior, while more strictly
-  // correct, causes problems in cases like StopTheWorld, where a parent thread
-  // owns the mutex but a child checks that it is locked. Rather than
-  // maintaining complex state to work around those situations, the check only
-  // checks that the mutex is owned, and assumes callers to be generally
-  // well-behaved.
-  void CheckLocked() const CHECK_LOCKED();
-
- private:
-  // Solaris mutex_t has a member that requires 64-bit alignment.
-  ALIGNED(8) uptr opaque_storage_[10];
-  uptr owner_;  // for debugging
-};
-
-// Reader-writer spin mutex.
-class MUTEX RWMutex {
- public:
-  RWMutex() {
-    atomic_store(&state_, kUnlocked, memory_order_relaxed);
-  }
-
-  ~RWMutex() {
-    CHECK_EQ(atomic_load(&state_, memory_order_relaxed), kUnlocked);
-  }
-
-  void Lock() ACQUIRE() {
-    u32 cmp = kUnlocked;
-    if (atomic_compare_exchange_strong(&state_, &cmp, kWriteLock,
-                                       memory_order_acquire))
-      return;
-    LockSlow();
-  }
-
-  void Unlock() RELEASE() {
-    u32 prev = atomic_fetch_sub(&state_, kWriteLock, memory_order_release);
-    DCHECK_NE(prev & kWriteLock, 0);
-    (void)prev;
-  }
-
-  void ReadLock() ACQUIRE_SHARED() {
-    u32 prev = atomic_fetch_add(&state_, kReadLock, memory_order_acquire);
-    if ((prev & kWriteLock) == 0)
-      return;
-    ReadLockSlow();
-  }
-
-  void ReadUnlock() RELEASE_SHARED() {
-    u32 prev = atomic_fetch_sub(&state_, kReadLock, memory_order_release);
-    DCHECK_EQ(prev & kWriteLock, 0);
-    DCHECK_GT(prev & ~kWriteLock, 0);
-    (void)prev;
-  }
-
-  void CheckLocked() const CHECK_LOCKED() {
-    CHECK_NE(atomic_load(&state_, memory_order_relaxed), kUnlocked);
-  }
-
- private:
-  atomic_uint32_t state_;
-
-  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;
-    }
-  }
-
-  RWMutex(const RWMutex &) = delete;
-  void operator=(const RWMutex &) = delete;
-};
-
 template <typename MutexType>
 class SCOPED_LOCK GenericScopedLock {
  public:
   explicit GenericScopedLock(MutexType *mu) ACQUIRE(mu) : mu_(mu) {
     mu_->Lock();
   }
 
   ~GenericScopedLock() RELEASE() { mu_->Unlock(); }

 
  private:
   MutexType *mu_;
 
   GenericScopedReadLock(const GenericScopedReadLock &) = delete;
   void operator=(const GenericScopedReadLock &) = delete;
 };
 
+// TODO: Temporary measure for incremental migration.
+// These typedefs should be removed and all uses renamed to Mutex.
+typedef Mutex BlockingMutex;
+typedef Mutex RWMutex;
+
 typedef GenericScopedLock<StaticSpinMutex> SpinMutexLock;
 typedef GenericScopedLock<BlockingMutex> BlockingMutexLock;
 typedef GenericScopedLock<RWMutex> RWMutexLock;
 typedef GenericScopedReadLock<RWMutex> RWMutexReadLock;
 typedef GenericScopedLock<Mutex> Lock;
 typedef GenericScopedReadLock<Mutex> ReadLock;
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MUTEX_H

compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp

 // ----------------- sanitizer_common.h
 void FutexWait(atomic_uint32_t *p, u32 cmp) {
   // FIXME: implement actual blocking.
   sched_yield();
 }
 
 void FutexWake(atomic_uint32_t *p, u32 count) {}
 
-BlockingMutex::BlockingMutex() {
-  CHECK(sizeof(mutex_t) <= sizeof(opaque_storage_));
-  internal_memset(this, 0, sizeof(*this));
-  CHECK_EQ(mutex_init((mutex_t *)&opaque_storage_, USYNC_THREAD, NULL), 0);
-}
-
-void BlockingMutex::Lock() {
-  CHECK(sizeof(mutex_t) <= sizeof(opaque_storage_));
-  CHECK_NE(owner_, (uptr)thr_self());
-  CHECK_EQ(mutex_lock((mutex_t *)&opaque_storage_), 0);
-  CHECK(!owner_);
-  owner_ = (uptr)thr_self();
-}
-
-void BlockingMutex::Unlock() {
-  CHECK(owner_ == (uptr)thr_self());
-  owner_ = 0;
-  CHECK_EQ(mutex_unlock((mutex_t *)&opaque_storage_), 0);
-}
-
-void BlockingMutex::CheckLocked() const { CHECK_EQ((uptr)thr_self(), owner_); }
-
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_SOLARIS

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

 
 void FutexWake(atomic_uint32_t *p, u32 count) {
   if (count == 1)
     WakeByAddressSingle(p);
   else
     WakeByAddressAll(p);
 }
 
-// ---------------------- BlockingMutex ---------------- {{{1
-
-BlockingMutex::BlockingMutex() {
-  CHECK(sizeof(SRWLOCK) <= sizeof(opaque_storage_));
-  internal_memset(this, 0, sizeof(*this));
-}
-
-void BlockingMutex::Lock() {
-  AcquireSRWLockExclusive((PSRWLOCK)opaque_storage_);
-  CHECK_EQ(owner_, 0);
-  owner_ = GetThreadSelf();
-}
-
-void BlockingMutex::Unlock() {
-  CheckLocked();
-  owner_ = 0;
-  ReleaseSRWLockExclusive((PSRWLOCK)opaque_storage_);
-}
-
-void BlockingMutex::CheckLocked() const { CHECK_EQ(owner_, GetThreadSelf()); }
-
 uptr GetTlsSize() {
   return 0;
 }
 
 void InitTlsSize() {
 }
 
 void GetThreadStackAndTls(bool main, uptr *stk_addr, uptr *stk_size,

compiler-rt/lib/sanitizer_common/tests/sanitizer_mutex_test.cpp

TEST(SanitizerCommon, SpinMutexTry) {
   TestData<SpinMutex> data(&mtx);
   pthread_t threads[kThreads];
   for (int i = 0; i < kThreads; i++)
     PTHREAD_CREATE(&threads[i], 0, try_thread<SpinMutex>, &data);
   for (int i = 0; i < kThreads; i++)
     PTHREAD_JOIN(threads[i], 0);
 }
 
-TEST(SanitizerCommon, BlockingMutex) {
-  u64 mtxmem[1024] = {};
-  BlockingMutex *mtx = new(mtxmem) BlockingMutex(LINKER_INITIALIZED);
-  TestData<BlockingMutex> data(mtx);
-  pthread_t threads[kThreads];
-  for (int i = 0; i < kThreads; i++)
-    PTHREAD_CREATE(&threads[i], 0, lock_thread<BlockingMutex>, &data);
-  for (int i = 0; i < kThreads; i++)
-    PTHREAD_JOIN(threads[i], 0);
-  check_locked(mtx);
-}
-
 TEST(SanitizerCommon, Mutex) {
   Mutex mtx;
   TestData<Mutex> data(&mtx);
   pthread_t threads[kThreads];
   for (int i = 0; i < kThreads; i++)
     PTHREAD_CREATE(&threads[i], 0, read_write_thread<Mutex>, &data);
   for (int i = 0; i < kThreads; i++) PTHREAD_JOIN(threads[i], 0);
 }