[PATCH] D14328: [tsan] Handle libdispatch worker threads on OS X

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:95
@@ +94,3 @@
+      ThreadState *parent_thread_state = nullptr;  // No parent.
+      int tid = ThreadCreate(parent_thread_state, 0, (uptr)thread, false);
+      CHECK_NE(tid, 0);
----------------
Don't we need to pass detached=true?
If a thread is not detached, runtime will wait for pthread_join call before getting rid of all associated state.

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:97
@@ +96,3 @@
+      CHECK_NE(tid, 0);
+      ThreadState *new_thread_state = cur_thread();
+      ThreadStart(new_thread_state, tid, GetTid());
----------------
call this var 'thr'
each and every reference to the current thread is called 'thr' throughout the runtime code

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:101
@@ +100,3 @@
+  } else if (event == PTHREAD_INTROSPECTION_THREAD_DESTROY) {
+    ThreadState *thread_state = cur_thread();
+    if (thread_state->tctx->parent_tid == kInvalidTid) {
----------------
same here

================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:107
@@ +106,3 @@
+  
+  if (prev_pthread_introspection_hook != nullptr)
+    prev_pthread_introspection_hook(event, thread, addr, size);
----------------
I guess the order will actually be reversed, because user will install the hook after tsan runtime. So if user hook calls any intercepted functions or is instrumented, this still will crash.
We can solve it later.

================
Comment at: lib/tsan/rtl/tsan_rtl.h:668
@@ -667,1 +667,3 @@
 
+const unsigned kInvalidTid = (unsigned)-1;
+
----------------
I think it belongs to tsan_defs.h
tsan_rtl.h includes a bunch of tsan headers, so we won't be able to use the const in any of these headers. For this reason the very base, non-dependent on anything things are declared in tsan_defs.h

================
Comment at: lib/tsan/rtl/tsan_rtl_thread.cc:58
@@ -57,2 +57,3 @@
   OnCreatedArgs *args = static_cast<OnCreatedArgs *>(arg);
+  if (! args->thr) return;  // GCD workers don't have a parent thread.
   args->thr->fast_state.IncrementEpoch();
----------------
please-please-please one statement per line


http://reviews.llvm.org/D14328