[PATCH] Interceptors etc for standalone LSan.
Sergey Matveev
earthdok at google.com
Tue Apr 23 06:22:18 PDT 2013
================
Comment at: lib/lsan/lsan_allocator.cc:78
@@ +77,3 @@
+ return current_context->allocator_mutex();
+ // If the thread has not been registered yet, it's safe to access the cache
+ // without acquiring a mutex, because our global lock on pthread_create
----------------
Dmitry Vyukov wrote:
> Sergey Matveev wrote:
> > Dmitry Vyukov wrote:
> > > Who does call malloc() before the thread is registered? pthread_setspecific?
> > > Don't we have the same problem during thread destruction when we do not hold any global mutexes?
> > > pthread_setspecific?
> >
> > For instance, yes.
> > Also, good catch.
> >For instance, yes.
>
> We can call pthread_setspecific() when thread context is initialized. Are there other cases?
I haven't checked what's going on in libc internals. Maybe it's safe now but I don't want to set myself up for future bugs.
Anyway, I'm going to remove the lock on pthread_create and instead have a dedicated global mutex that baby threads can use to protect allocations before their private mutexes are available. The same mutex could be used during thread destruction.
================
Comment at: lib/lsan/lsan_thread.cc:149
@@ +148,3 @@
+bool FindThreadByOsIdCallback(ThreadContextBase *tctx, void *arg) {
+ return (tctx->os_id == (uptr)arg && tctx->status != ThreadStatusInvalid);
+}
----------------
Dmitry Vyukov wrote:
> Sergey Matveev wrote:
> > Dmitry Vyukov wrote:
> > > os_id is not valid for ThreadStatusDead threads as well
> > > I think you need to fix ThreadRegistry to zeroize os_id before when changing state to ThreadStatusDead.
> > Can't we just check the status here?
> We can, but it's better to not hold stale os_id in the thread in the place. That's asking for trouble.
> This leads to unpleasant bugs when a wrong thread is matched occasionally.
>
Ok.
================
Comment at: lib/lsan/lsan_thread.cc:210
@@ +209,3 @@
+ thread_registry->Lock();
+ thread_registry->RunCallbackForEachThreadLocked(LockAllocatorsCallback,
+ NULL);
----------------
Dmitry Vyukov wrote:
> Sergey Matveev wrote:
> > Dmitry Vyukov wrote:
> > > Why do you need to lock all these mutexes?
> > LSan will walk the entire heap and read/write to metadata of arbitrary chunks. So the metadata must be protected from other threads while we do the leak check.
> But all the threads are stopped during the walk, no?
Freezing threads at arbitrary points isn't quite enough to eliminate data races, is it?
http://llvm-reviews.chandlerc.com/D702
More information about the llvm-commits
mailing list