[tsan] Add interceptors for Darwin-specific locking APIs

[kubamracek]

Summary

On OS X, there are other-than-pthread locking APIs that are used quite extensively - OSSpinLock and os_lock_lock. Let's add interceptors for those.

[dvyukov]

lib/tsan/rtl/tsan_platform_mac.cc

190 ↗

(On Diff #41143)

This all should go into tsan_interceptors_mac.cc.
tsan_platform_* is portability layer, not meant to contain any race-detection logic.
Also, tsan_platform_mac.cc is built in Go mode, but Go does not need any of these interceptors.

191 ↗

(On Diff #41143)

I wonder whether this interceptors can come when thr->is_dead?
Add CHECK(!thr->is_dead) so that we are sure that it does not happen.

Not sure how widely os_lock_lock is used, maybe it needs the same.

test/tsan/Darwin/osspinlock-norace.cc

1 ↗

(On Diff #41143)

please also add a similar test for os_lock_lock

[kubamracek]

Updating patch to move interceptors into tsan_interceptors_mac.cc, added checks for thr->is_dead.

However, I also needed to change calls to MutexLock/MutexUnlock to just Acquire/Release, because they are used as a low-level primitive in the implementation of pthread mutexes and it caused false "double-locked mutex" warnings.

[kubamracek]

test/tsan/Darwin/osspinlock-norace.cc

2 ↗

(On Diff #41740)

os_lock_lock actually isn't declared in public headers, so we can't have a test for it without manually declaring the functions and structures. However, the Obj-C runtime uses os_lock_lock heavily, so any Obj-C code will exercise the wrappers for os_lock_lock.

[dvyukov]

LGTM

But MutexLock/MutexUnlock annotations are actually super useful for several reasons:

• they allow to detect deadlocks
• the allow to detect unsafe publication of objects with mutexes (which are usually undetectable otherwise, because a first mutex lock synchronizes the rest of the accesses)
• they allow to detect mutex unlock racing with free (which is a very frequent bug: last touch of object is an unlock)
• they allow to print sets of locked mutexes in reports
• they allow to report misuses of mutexes

and there can be other reasons and future improvements.

So please do MutexLock/MutexUnlock in a subsequent patch. The issue with double lock is not specific to only these mutexes. Go mutexes are also just zero chunks of memory without an explicit ctor. Java mutexes are the same. Tsan also allows to annotate user mutexes, which well can well be the same.

I see that in java interface (tsan_interface_java.cc) I do the following hack to set recursive flag on mutex:

MutexCreate(thr, pc, addr, true, true, true);
MutexLock(thr, pc, addr);

This is somewhat ugly and slow. I was thinking about a better solution to allow MutexLock to also update mutex flags. But the complication here is that for pthread mutexes/condvars we don't know whether the mutex is recursive or not. I think we need something along the following lines. MutexLocks accepts a tri-state recursive flag which can be Yes, No or DontKnow. If it DontKnow then we assume that MutexCreate has initialized it. If it is Yes or No then we update the mutex flag. Should not be too difficult to do, there are few existing callers but for them you can just pass DontKnow as it won't change existing behavior.

Does it sound good to you?

[kubamracek]

In D14987#301506, @dvyukov wrote:

So please do MutexLock/MutexUnlock in a subsequent patch. The issue with double lock is not specific to only these mutexes

OK, I'll try to extract a test case where I see the issue and then replace Acquire/Release with MutexLock/MutexUnlock.

[dvyukov]

OK, I'll try to extract a test case where I see the issue

Just lock it recursively?

compiler-rt/trunk/lib/tsan/CMakeLists.txt

set(TSAN_SOURCES
   rtl/tsan_symbolize.cc
   rtl/tsan_sync.cc)
 
 set(TSAN_CXX_SOURCES
   rtl/tsan_new_delete.cc)
 
 if(APPLE)
   list(APPEND TSAN_SOURCES
+    rtl/tsan_interceptors_mac.cc
     rtl/tsan_libdispatch_mac.cc
     rtl/tsan_platform_mac.cc
     rtl/tsan_platform_posix.cc)
 elseif(UNIX)
   # Assume Linux
   list(APPEND TSAN_SOURCES
     rtl/tsan_platform_linux.cc
     rtl/tsan_platform_posix.cc)

compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors_mac.cc

+//===-- tsan_interceptors_mac.cc ------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of ThreadSanitizer (TSan), a race detector.
+//
+// Mac-specific interceptors.
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_common/sanitizer_platform.h"
+#if SANITIZER_MAC
+
+#include "interception/interception.h"
+#include "tsan_interceptors.h"
+
+#include <libkern/OSAtomic.h>
+
+namespace __tsan {
+
+TSAN_INTERCEPTOR(void, OSSpinLockLock, volatile OSSpinLock *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(OSSpinLockLock)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(OSSpinLockLock, lock);
+  REAL(OSSpinLockLock)(lock);
+  Acquire(thr, pc, (uptr)lock);
+}
+
+TSAN_INTERCEPTOR(bool, OSSpinLockTry, volatile OSSpinLock *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(OSSpinLockTry)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(OSSpinLockTry, lock);
+  bool result = REAL(OSSpinLockTry)(lock);
+  if (result)
+    Acquire(thr, pc, (uptr)lock);
+  return result;
+}
+
+TSAN_INTERCEPTOR(void, OSSpinLockUnlock, volatile OSSpinLock *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(OSSpinLockUnlock)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(OSSpinLockUnlock, lock);
+  Release(thr, pc, (uptr)lock);
+  REAL(OSSpinLockUnlock)(lock);
+}
+
+TSAN_INTERCEPTOR(void, os_lock_lock, void *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(os_lock_lock)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(os_lock_lock, lock);
+  REAL(os_lock_lock)(lock);
+  Acquire(thr, pc, (uptr)lock);
+}
+
+TSAN_INTERCEPTOR(bool, os_lock_trylock, void *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(os_lock_trylock)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(os_lock_trylock, lock);
+  bool result = REAL(os_lock_trylock)(lock);
+  if (result)
+    Acquire(thr, pc, (uptr)lock);
+  return result;
+}
+
+TSAN_INTERCEPTOR(void, os_lock_unlock, void *lock) {
+  CHECK(!cur_thread()->is_dead);
+  if (!cur_thread()->is_inited) {
+    return REAL(os_lock_unlock)(lock);
+  }
+  SCOPED_TSAN_INTERCEPTOR(os_lock_unlock, lock);
+  Release(thr, pc, (uptr)lock);
+  REAL(os_lock_unlock)(lock);
+}
+
+}  // namespace __tsan
+
+#endif  // SANITIZER_MAC

compiler-rt/trunk/test/tsan/Darwin/osspinlock-norace.cc

+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+#include <libkern/OSAtomic.h>
+#include <pthread.h>
+#include <stdio.h>
+
+int Global;
+OSSpinLock lock;
+
+void *Thread(void *x) {
+  OSSpinLockLock(&lock);
+  Global++;
+  OSSpinLockUnlock(&lock);
+  return NULL;
+}
+
+int main() {
+  fprintf(stderr, "Hello world.\n");
+
+  pthread_t t[2];
+  pthread_create(&t[0], NULL, Thread, NULL);
+  pthread_create(&t[1], NULL, Thread, NULL);
+  pthread_join(t[0], NULL);
+  pthread_join(t[1], NULL);
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer