[PATCH] Interceptors etc for standalone LSan.

[Sergey Matveev]

================
Comment at: lib/lsan/lsan_allocator.cc:149
@@ +148,3 @@
+
+LsanMetadata::LsanMetadata(void *chunk) {
+  metadata_ = Metadata(chunk);
----------------
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.

================
Comment at: lib/lsan/lsan_interceptors.cc:60
@@ +59,3 @@
+  size *= nmemb;
+  void *p = Allocate(stack, size, 8, false);
+  memset(p, 0, size);
----------------
Dmitry Vyukov wrote:
> s/false/true/
oops

================
Comment at: lib/lsan/lsan_interceptors.cc:61
@@ +60,3 @@
+  void *p = Allocate(stack, size, 8, false);
+  memset(p, 0, size);
+  return p;
----------------
Dmitry Vyukov wrote:
> Don't we want to zeroize memory in *all* allocation functions to not treat old garbage as potential pointers?
I have this planned. I would prefer to have benchmarks first.

================
Comment at: lib/lsan/lsan_stack.cc:45
@@ +44,3 @@
+
+void PrintStackTrace(const uptr *trace, uptr size, bool symbolize) {
+  static char current_path[256];
----------------
Dmitry Vyukov wrote:
> Why does not it declared in lsan_stack.h?
> 
It's from lsan.h. It's only used by the leak checking code.

================
Comment at: lib/lsan/lsan_stack.cc:47
@@ +46,3 @@
+  static char current_path[256];
+  static bool getcwd_attempted;
+
----------------
Alexander Potapenko wrote:
> s/getcwd_attempted/current_path_initialized/
Those are two different things. What you're proposing is to try to initialize it every time this function is called. But I don't want to report an error more than once.

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

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

================
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:
> 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.


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