[tsan] Handle dispatch_once on OS X

[kubamracek]

Summary

Reimplement dispatch_once in an interceptor to solve these issues that may produce false positives with TSan on OS X:

1. there is a racy load inside an inlined part of dispatch_once,
2. the fast path in dispatch_once doesn't perform an acquire load, so we don't properly synchronize the initialization and subsequent uses of whatever is initialized,
3. dispatch_once is already used in a lot of already-compiled code, so TSan doesn't see the inlined fast-path.

This patch uses a trick to avoid ever taking the fast path (by never storing ~0 into the predicate), which means the interceptor will always be called even from already-compiled code. Within the interceptor, our own atomic reads and writes are not written into shadow cells, so the race in the inlined part is not reported (because the accesses are only loads).

[dvyukov]

LGTM with nits

lib/tsan/rtl/tsan_libdispatch_mac.cc

30 ↗

(On Diff #40613)

I can't imagine how undefined behavior can be intentional. Also, whether it is intentional or not is irrelevant here. So just drop "intentionally".

45 ↗

(On Diff #40613)

C++ and C cast chained look weird. Do either just C cast, or just reinterpret_cast.

65 ↗

(On Diff #40613)

For my education: how do these blocks work? Are they allocated with malloc, and then atomically reference-counted?

[kubamracek]

lib/tsan/rtl/tsan_libdispatch_mac.cc

65 ↗

(On Diff #40613)

Initially, they are created on the stack (a descriptor containing a function pointer and data). In function calls, we just pass a pointer to this stack variable. Only when we need to store a block reference to somewhere else (global variable, heap location), we transform it into a heap object (malloc'd). In this case (dispatch_once_f), we never do that, because we only pass it around as arguments and invoke it.

Would there be any issues if it was always malloc'd?

[dvyukov]

Would there be any issues if it was always malloc'd?

[dvyukov]

Would there be any issues if it was always malloc'd?

I don't know.
We would intercept malloc/free from within interceptor scope. And also may or may not intercept some atomic operations depending on implementation.

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_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)
 endif()

compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.h

 #define SCOPED_INTERCEPTOR_RAW(func, ...) \
     ThreadState *thr = cur_thread(); \
     const uptr caller_pc = GET_CALLER_PC(); \
     ScopedInterceptor si(thr, #func, caller_pc); \
     const uptr pc = StackTrace::GetCurrentPc(); \
     (void)pc; \
 /**/
 
+#define SCOPED_TSAN_INTERCEPTOR(func, ...) \
+    SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
+    if (REAL(func) == 0) { \
+      Report("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
+      Die(); \
+    }                                                    \
+    if (thr->ignore_interceptors || thr->in_ignored_lib) \
+      return REAL(func)(__VA_ARGS__); \
+/**/
+
+#define TSAN_INTERCEPTOR(ret, func, ...) INTERCEPTOR(ret, func, __VA_ARGS__)
+
 #if SANITIZER_FREEBSD
 #define __libc_free __free
 #define __libc_malloc __malloc
 #endif
 
 extern "C" void __libc_free(void *ptr);
 extern "C" void *__libc_malloc(uptr size);
 
 #endif  // TSAN_INTERCEPTORS_H

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

ScopedInterceptor::~ScopedInterceptor() {
   }
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);
     FuncExit(thr_);
     CheckNoLocks(thr_);
   }
 }
 
-#define SCOPED_TSAN_INTERCEPTOR(func, ...) \
-    SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
-    if (REAL(func) == 0) { \
-      Report("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
-      Die(); \
-    }                                                    \
-    if (thr->ignore_interceptors || thr->in_ignored_lib) \
-      return REAL(func)(__VA_ARGS__); \
-/**/
-
-#define TSAN_INTERCEPTOR(ret, func, ...) INTERCEPTOR(ret, func, __VA_ARGS__)
 #define TSAN_INTERCEPT(func) INTERCEPT_FUNCTION(func)
 #if SANITIZER_FREEBSD
 # define TSAN_INTERCEPT_VER(func, ver) INTERCEPT_FUNCTION(func)
 #else
 # define TSAN_INTERCEPT_VER(func, ver) INTERCEPT_FUNCTION_VER(func, ver)
 #endif
 
 #define READ_STRING_OF_LEN(thr, pc, s, len, n)                 \

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

+//===-- tsan_libdispatch_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 libdispatch (GCD) support.
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_common/sanitizer_platform.h"
+#if SANITIZER_MAC
+
+#include "sanitizer_common/sanitizer_common.h"
+#include "interception/interception.h"
+#include "tsan_interceptors.h"
+#include "tsan_platform.h"
+#include "tsan_rtl.h"
+
+#include <dispatch/dispatch.h>
+#include <pthread.h>
+
+namespace __tsan {
+
+// GCD's dispatch_once implementation has a fast path that contains a racy read
+// and it's inlined into user's code. Furthermore, this fast path doesn't
+// establish a proper happens-before relations between the initialization and
+// code following the call to dispatch_once. We could deal with this in
+// instrumented code, but there's not much we can do about it in system
+// libraries. Let's disable the fast path (by never storing the value ~0 to
+// predicate), so the interceptor is always called, and let's add proper release
+// and acquire semantics. Since TSan does not see its own atomic stores, the
+// race on predicate won't be reported - the only accesses to it that TSan sees
+// are the loads on the fast path. Loads don't race. Secondly, dispatch_once is
+// both a macro and a real function, we want to intercept the function, so we
+// need to undefine the macro.
+#undef dispatch_once
+TSAN_INTERCEPTOR(void, dispatch_once, dispatch_once_t *predicate,
+                 dispatch_block_t block) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_once, predicate, block);
+  atomic_uint32_t *a = reinterpret_cast<atomic_uint32_t *>(predicate);
+  u32 v = atomic_load(a, memory_order_acquire);
+  if (v == 0 &&
+      atomic_compare_exchange_strong(a, &v, 1, memory_order_relaxed)) {
+    block();
+    Release(thr, pc, (uptr)a);
+    atomic_store(a, 2, memory_order_release);
+  } else {
+    while (v != 2) {
+      internal_sched_yield();
+      v = atomic_load(a, memory_order_acquire);
+    }
+    Acquire(thr, pc, (uptr)a);
+  }
+}
+
+#undef dispatch_once_f
+TSAN_INTERCEPTOR(void, dispatch_once_f, dispatch_once_t *predicate,
+                 void *context, dispatch_function_t function) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_once_f, predicate, context, function);
+  WRAP(dispatch_once)(predicate, ^(void) {
+    function(context);
+  });
+}
+
+}  // namespace __tsan
+
+#endif  // SANITIZER_MAC

compiler-rt/trunk/test/tsan/Darwin/gcd-once.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %run %t 2>&1
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+static const long kNumThreads = 4;
+
+long global;
+long global2;
+
+static dispatch_once_t once_token;
+static dispatch_once_t once_token2;
+
+void f(void *) {
+  global2 = 42;
+  usleep(100000);
+}
+
+void *Thread(void *a) {
+  barrier_wait(&barrier);
+
+  dispatch_once(&once_token, ^{
+    global = 42;
+    usleep(100000);
+  });
+  long x = global;
+
+  dispatch_once_f(&once_token2, NULL, f);
+  long x2 = global2;
+
+  fprintf(stderr, "global = %ld\n", x);
+  fprintf(stderr, "global2 = %ld\n", x2);
+  return 0;
+}
+
+int main() {
+  fprintf(stderr, "Hello world.\n");
+  barrier_init(&barrier, kNumThreads);
+
+  pthread_t t[kNumThreads];
+  for (int i = 0; i < kNumThreads; i++) {
+    pthread_create(&t[i], 0, Thread, 0);
+  }
+  for (int i = 0; i < kNumThreads; i++) {
+    pthread_join(t[i], 0);
+  }
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer