[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