[tsan] Intercept libcxx __release_shared to avoid false positive with weak_ptrs and destructors in C++

kubamracek · kubamracek · commit 30ad0c941a87 · 2016-06-26T08:14:01.000Z
There is a "well-known" TSan false positive when using C++ weak_ptr/shared_ptr and code in destructors, e.g. described at <https://llvm.org/bugs/show_bug.cgi?id=22324>. The "standard" solution is to build and use a TSan-instrumented version of libcxx, which is not trivial for end-users. This patch tries a different approach (on OS X): It adds an interceptor for the specific function in libc++.dylib, which implements the atomic operation that needs to be visible to TSan. Differential Revision: http://reviews.llvm.org/D21609 llvm-svn: 273806
diff --git a/compiler-rt/lib/tsan/rtl/tsan_flags.inc b/compiler-rt/lib/tsan/rtl/tsan_flags.inc
@@ -78,3 +78,5 @@ TSAN_FLAG(bool, die_after_fork, true,
 TSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
 TSAN_FLAG(bool, ignore_interceptors_accesses, false,
           "Ignore reads and writes from all interceptors.")
+TSAN_FLAG(bool, shared_ptr_interceptor, true,
+          "Track atomic reference counting in libc++ shared_ptr and weak_ptr.")
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc b/compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc
@@ -273,6 +273,52 @@ TSAN_INTERCEPTOR(void, xpc_connection_send_message_with_reply,
   (connection, message, replyq, new_handler);
 }
 
+// On macOS, libc++ is always linked dynamically, so intercepting works the
+// usual way.
+#define STDCXX_INTERCEPTOR TSAN_INTERCEPTOR
+
+namespace {
+struct fake_shared_weak_count {
+  volatile a64 shared_owners;
+  volatile a64 shared_weak_owners;
+  virtual void _unused_0x0() = 0;
+  virtual void _unused_0x8() = 0;
+  virtual void on_zero_shared() = 0;
+  virtual void _unused_0x18() = 0;
+  virtual void on_zero_shared_weak() = 0;
+};
+}  // namespace
+
+// This adds a libc++ interceptor for:
+//     void __shared_weak_count::__release_shared() _NOEXCEPT;
+// Shared and weak pointers in C++ maintain reference counts via atomics in
+// libc++.dylib, which are TSan-invisible, and this leads to false positives in
+// destructor code.  This interceptor re-implements the whole function so that
+// the mo_acq_rel semantics of the atomic decrement are visible.
+//
+// Unfortunately, this interceptor cannot simply Acquire/Release some sync
+// object and call the original function, because it would have a race between
+// the sync and the destruction of the object.  Calling both under a lock will
+// not work because the destructor can invoke this interceptor again (and even
+// in a different thread, so recursive locks don't help).
+STDCXX_INTERCEPTOR(void, _ZNSt3__119__shared_weak_count16__release_sharedEv,
+                   fake_shared_weak_count *o) {
+  if (!flags()->shared_ptr_interceptor)
+    return REAL(_ZNSt3__119__shared_weak_count16__release_sharedEv)(o);
+
+  SCOPED_TSAN_INTERCEPTOR(_ZNSt3__119__shared_weak_count16__release_sharedEv,
+                          o);
+  if (__tsan_atomic64_fetch_add(&o->shared_owners, -1, mo_release) == 0) {
+    Acquire(thr, pc, (uptr)&o->shared_owners);
+    o->on_zero_shared();
+    if (__tsan_atomic64_fetch_add(&o->shared_weak_owners, -1, mo_release) ==
+        0) {
+      Acquire(thr, pc, (uptr)&o->shared_weak_owners);
+      o->on_zero_shared_weak();
+    }
+  }
+}
+
 }  // namespace __tsan
 
 #endif  // SANITIZER_MAC
diff --git a/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr-recursive.mm b/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr-recursive.mm
@@ -0,0 +1,36 @@
+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+#import <memory>
+
+struct InnerStruct {
+  ~InnerStruct() {
+    fprintf(stderr, "~InnerStruct\n");
+  }
+};
+
+struct MyStruct {
+  std::shared_ptr<InnerStruct> inner_object;
+  ~MyStruct() {
+    fprintf(stderr, "~MyStruct\n");
+  }
+};
+
+int main(int argc, const char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+
+  {
+    std::shared_ptr<MyStruct> shared(new MyStruct());
+    shared->inner_object = std::shared_ptr<InnerStruct>(new InnerStruct());
+  }
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: ~MyStruct
+// CHECK: ~InnerStruct
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer
diff --git a/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr-stress.mm b/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr-stress.mm
@@ -0,0 +1,75 @@
+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+#import <assert.h>
+#import <memory>
+#import <stdatomic.h>
+
+_Atomic(long) shared_call_counter = 0;
+_Atomic(long) weak_call_counter = 0;
+_Atomic(long) destructor_counter = 0;
+_Atomic(long) weak_destroyed_counter = 0;
+
+struct MyStruct {
+  _Atomic(long) self_counter = 0;
+  virtual void shared_call() {
+    atomic_fetch_add_explicit(&self_counter, 1, memory_order_relaxed);
+    atomic_fetch_add_explicit(&shared_call_counter, 1, memory_order_relaxed);
+  }
+  virtual void weak_call() {
+    atomic_fetch_add_explicit(&weak_call_counter, 1, memory_order_relaxed);
+  }
+  virtual ~MyStruct() {
+    long n = self_counter;
+    assert(n == 1000);
+    atomic_fetch_add_explicit(&destructor_counter, 1, memory_order_relaxed);
+  }
+};
+
+int main(int argc, const char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+
+  dispatch_queue_t q = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+
+  dispatch_group_t g = dispatch_group_create();
+
+  for (int i = 0; i < 1000; i++) {
+    std::shared_ptr<MyStruct> shared(new MyStruct());
+    std::weak_ptr<MyStruct> weak(shared);
+
+    dispatch_group_async(g, q, ^{
+      for (int j = 0; j < 1000; j++) {
+        std::shared_ptr<MyStruct> shared_copy(shared);
+        shared_copy->shared_call();
+      }
+    });
+    dispatch_group_async(g, q, ^{
+      for (int j = 0; j < 1000; j++) {
+        std::shared_ptr<MyStruct> weak_copy = weak.lock();
+        if (weak_copy) {
+          weak_copy->weak_call();
+        } else {
+          atomic_fetch_add_explicit(&weak_destroyed_counter, 1, memory_order_relaxed);
+          break;
+        }
+      }
+    });
+  }
+
+  dispatch_group_wait(g, DISPATCH_TIME_FOREVER);
+
+  fprintf(stderr, "shared_call_counter = %ld\n", shared_call_counter);
+  fprintf(stderr, "weak_call_counter = %ld\n", weak_call_counter);
+  fprintf(stderr, "destructor_counter = %ld\n", destructor_counter);
+  fprintf(stderr, "weak_destroyed_counter = %ld\n", weak_destroyed_counter);
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: shared_call_counter = 1000000
+// CHECK: destructor_counter = 1000
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer
diff --git a/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr.mm b/compiler-rt/test/tsan/Darwin/libcxx-shared-ptr.mm
@@ -0,0 +1,50 @@
+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+#import <memory>
+
+#import "../test.h"
+
+long my_global;
+
+struct MyStruct {
+  void setGlobal() {
+    my_global = 42;
+  }
+  ~MyStruct() {
+    my_global = 43;
+  }
+};
+
+int main(int argc, const char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+  print_address("addr=", 1, &my_global);
+  barrier_init(&barrier, 2);
+
+  std::shared_ptr<MyStruct> shared(new MyStruct());
+
+  dispatch_queue_t q = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+
+  std::weak_ptr<MyStruct> weak(shared);
+  
+  dispatch_async(q, ^{
+    {
+      std::shared_ptr<MyStruct> strong = weak.lock();
+      if (!strong) exit(1);
+
+      strong->setGlobal();
+    }
+    barrier_wait(&barrier);
+  });
+
+  barrier_wait(&barrier);
+  shared.reset();
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer
