Page MenuHomePhabricator

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

Authored by earthdok on Nov 27 2014, 9:28 AM.

Details

Reviewers
kcc
samsonov
Summary

In the current scheme of things, the call to ThreadStart() in the child
thread is not synchronized with the parent thread. So, if a pointer is passed to
pthread_create, there may be a window of time during which this pointer will not
be discoverable by LSan. I.e. the pthread_create interceptor has already
returneed and thus the pointer is no longer on the parent stack, but we don't
yet know the location of the child stack. This has caused bogus leak reports
(see http://llvm.org/bugs/show_bug.cgi?id=21621/).

This patch makes the pthread_create interceptor wait until the child thread is
properly registered before returning.

Diff Detail

Event Timeline

earthdok updated this revision to Diff 16700.Nov 27 2014, 9:28 AM
earthdok retitled this revision from to [ASan, LSan] Improve tracking of thread creation..
earthdok updated this object.
earthdok edited the test plan for this revision. (Show Details)
earthdok added a reviewer: kcc.
earthdok added a subscriber: Unknown Object (MLST).

The problem with the solution I proposed in http://llvm.org/bugs/show_bug.cgi?id=21621#c6 is that we don't know the thread's OS PID until ThreadStart() has been called. So, instead of checking whether the thread that interests us is in "created" state, LSan would have to process all "created" threads separately. That's not super complicated, but I like this solution better. At least it brings us one step closer to a unified pthread_create interceptor in ASan/MSan/TSan.

glider added a subscriber: glider.Nov 27 2014, 10:19 AM

I don't think it's a good idea to stop the parent until the child is
created. Do we do that in other tools?

TSan does this.

samsonov added inline comments.
lib/asan/asan_interceptors.cc
202

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.

earthdok added inline comments.Dec 2 2014, 5:37 AM
lib/asan/asan_interceptors.cc
202

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)?

samsonov added inline comments.Dec 2 2014, 12:23 PM
lib/asan/asan_interceptors.cc
202

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.

earthdok added inline comments.Dec 3 2014, 8:12 AM
lib/asan/asan_interceptors.cc
202

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.

samsonov added inline comments.Dec 3 2014, 4:31 PM
lib/asan/asan_interceptors.cc
202

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.

earthdok updated this revision to Diff 16931.Dec 4 2014, 9:05 AM
  • struct
samsonov accepted this revision.Dec 4 2014, 1:16 PM
samsonov added a reviewer: samsonov.

I'd appreciate a comment describing a motivation, in AsanThread::ThreadStart decl and/or in pthread_create() interceptor. Thanks!

lib/asan/asan_interceptors.cc
175

nullptr

lib/asan/asan_rtl.cc
684

nullptr

This revision is now accepted and ready to land.Dec 4 2014, 1:16 PM
earthdok updated this revision to Diff 16957.Dec 4 2014, 3:27 PM
earthdok edited edge metadata.
  • add comment and use nullptrs
lib/asan/asan_interceptors.cc
202

Done.

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

Any suggestions?

BTW, I think the ThreadRegistry::CreateThread() call should be moved into AsanThread::Create(). This code is duplicated across 4 uses of AsanThread::Create(), and anyway most other ThreadRegistry access is done via AsanThread (including ThreadRegistry::FinishThread()).

Committed r223419

In D6441#24, @earthdok wrote:

BTW, I think the ThreadRegistry::CreateThread() call should be moved into AsanThread::Create(). This code is duplicated across 4 uses of AsanThread::Create(), and anyway most other ThreadRegistry access is done via AsanThread (including ThreadRegistry::FinishThread()).

I agree, that would be nice.

earthdok closed this revision.Dec 5 2014, 9:37 AM