[PATCH] [ASan, LSan] Improve tracking of thread creation.

[Alexey Samsonov]

================
Comment at: lib/asan/asan_interceptors.cc:197
@@ +196,3 @@
+    asanThreadRegistry().CreateThread(*(uptr*)t, detached, current_tid, &args);
+    atomic_store(&atomic_t, reinterpret_cast<uptr>(t), memory_order_release);
+    while (atomic_load(&atomic_t, memory_order_acquire) != 0)
----------------
earthdok wrote:
> samsonov wrote:
> > earthdok wrote:
> > > samsonov wrote:
> > > > Reusing the same atomic_uintptr_t for different purpose looks kind of confusing. IIUC first we use it to publish the pointer to AsanThread*, and then use it to signal that the thread is registered (and you need one more argument to AsanThread::ThreadStart for this purpose).
> > > > 
> > > > Would it be cleaner to have
> > > >   atomic_uintptr_t registered
> > > > as a field of AsanThread object? You can atomically set it inside ThreadStart() and call t->WaitUntilRegistered() here.
> > > I agree that it's a bit confusing, but I'm averse to bloating AsanThread with a member that would only be used in this scope. How about two atomics in a struct passed as the void* argument to REAL(pthread_create)?
> > sizeof(AsanThread) == 117920. Bloating, huh? :)
> > IMO flag marking that thread is started qualifies for thread "state" and it would be easier to make it a class member.
> > 
> It is possible for the child thread to set |registered|, run the callback and terminate while the parent thread is waiting in internal_sched_yield(). Then the parent thread will segfault when it attempts to read from |registered|, as the AsanThread object is now unmapped. This actually happens in about 20 tests.
Oh, that's funny :) Fine then, let's create a struct with two atomics - one to pass the pointer to AsanThread object, and the other one to signal that thread has started and we can return from pthread_create().

This is somewhat intrusive change, and I think we should test it thoroughly.

http://reviews.llvm.org/D6441