[PATCH] D14288: [tsan] Alternative ThreadState storage for OS X

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:43
@@ -42,1 +42,3 @@
 
+// On OS X, accessing TLVs via __thread or manually by using pthread_key_* is
+// problematic, because there are several places where interceptors are called
----------------
#ifndef SANITIZER_GO

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:51
@@ +50,3 @@
+// shadow memory is set up.
+// TODO(kuba.brecka): This currently leaks the ThreadState objects as we never
+// deallocate them.
----------------
How complex is that patch?
If it is not too complex, then I would prefer to resolve this TODO right in this patch.

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:55
@@ +54,3 @@
+  ThreadState **fake_tls;
+  if (pthread_main_np()) {
+    static ThreadState *main_thread_state = nullptr;
----------------
What does pthread_self return for main thread? Or is the issue with early bootstrap?
If it is bootstrap, then I think it is better to have a global var that contains pthread_self value for main thread. This variable is initialized in Iniitialize. If this variable is 0, then we also know that this is the main thread.
Basically it will allow us to replace pthread_main_np call with a check of a global.
Something along the lines of:

  uptr th = pthread_self();
  uptr mt = g_main_thread;
  if (mt == 0 || mt == th) {
    // main thread
  } else {
    // non-main thread
  }

Will this work?

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:64
@@ +63,3 @@
+  if (*fake_tls == nullptr) {
+    *fake_tls = (ThreadState *)InternalAlloc(sizeof(ThreadState), nullptr);
+    internal_memset(*fake_tls, 0, sizeof(ThreadState));
----------------
This is not async-signal-safe. A thread can receive a signal concurrent with the first call to cur_thread(), and signal handler will also call cur_thread().

================
Comment at: lib/tsan/rtl/tsan_rtl.cc:47
@@ -46,3 +46,3 @@
 
-#ifndef SANITIZER_GO
+#if defined(SANITIZER_GO) && !SANITIZER_MAC
 THREADLOCAL char cur_thread_placeholder[sizeof(ThreadState)] ALIGNED(64);
----------------
!defined(SANITIZER_GO)

================
Comment at: lib/tsan/rtl/tsan_rtl.h:412
@@ -411,3 +411,3 @@
 
-#ifndef SANITIZER_GO
+#if defined(SANITIZER_GO) && !SANITIZER_MAC
 __attribute__((tls_model("initial-exec")))
----------------
This must be !defined(SANITIZER_GO)


http://reviews.llvm.org/D14288