[PATCH] Interceptors etc for standalone LSan.

[Dmitry Vyukov]

================
Comment at: lib/lsan/lsan_allocator.h:34
@@ +33,3 @@
+void *Allocate(StackTrace const &stack, uptr size, uptr alignment,
+               bool cleared = false);
+void Deallocate(void *p);
----------------
Sergey Matveev wrote:
> Dmitry Vyukov wrote:
> > Alexander Potapenko wrote:
> > > I think it's better to avoid the default parameter here.
> > Default values are discouraged by the code style. I think you will call this in very few places anyway, so passing it explicitly is fine.
> Why is it ok to have it in Allocate() in sanitizer_allocator.h but not here?
It is not OK there as well.

================
Comment at: lib/lsan/lsan_allocator.cc:149
@@ +148,3 @@
+
+LsanMetadata::LsanMetadata(void *chunk) {
+  metadata_ = Metadata(chunk);
----------------
Sergey Matveev wrote:
> Dmitry Vyukov wrote:
> > Where is the definition of this class?
> > Do you actually need it? Looks somewhat useless.
> > Please do not prefix things with Lsan in Lsan. This is a deadend.
> > 
> This class is a wrapper over tool-specific metadata that implements operations used by LSan. It's defined in lsan.h and implemented differently in each tool. So it makes sense to prefix it with Lsan.
OK

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

================
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);
+}
----------------
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.


================
Comment at: lib/lsan/lsan_thread.cc:210
@@ +209,3 @@
+  thread_registry->Lock();
+  thread_registry->RunCallbackForEachThreadLocked(LockAllocatorsCallback,
+                                                       NULL);
----------------
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?


http://llvm-reviews.chandlerc.com/D702