[PATCH] Interceptors etc for standalone LSan.

[Sergey Matveev]

================
Comment at: lib/lsan/lsan_interceptors.cc:150
@@ +149,3 @@
+  void *param = p->param;
+  if (pthread_setspecific(g_thread_finalize_key, (void*)4)) {
+    Report("LeakSanitizer: failed to set thread key.\n");
----------------
Alexander Potapenko wrote:
> Why exactly 4?
This is actually copied from TSan/ASan, as is much of the interception code.

4 is PTHREAD_DESTRUCTOR_ITERATIONS from glibc, which is how many times glibc tries to run the thread destructors before it gives up. Basically we do our best to ensure our destructor is the last destructor to run, by waiting until the last round.

================
Comment at: lib/lsan/lsan_interceptors.cc:167
@@ +166,3 @@
+  pthread_attr_t myattr;
+  if (attr == 0) {
+    pthread_attr_init(&myattr);
----------------
Alexander Potapenko wrote:
> Why do you need myattr at all?
We need an attr object to adjust the stack size.

================
Comment at: lib/lsan/lsan_interceptors.cc:188
@@ +187,3 @@
+      atomic_store(&p.tid, tid, memory_order_release);
+      while (atomic_load(&p.tid, memory_order_acquire) != 0)
+        pthread_yield();
----------------
Alexander Potapenko wrote:
> Blocking in pthread_create until the pthread callback starts the execution sounds like an overkill. Do we need this?
Actually, we probably don't. The problem is that newly created threads (up until the call to SetCurrentThread) don't know which mutex to acquire when they access the allocator, because they can't access their thread context. But I think we can solve this by having a separate global mutex for such threads.

================
Comment at: lib/lsan/lsan_interceptors.cc:229
@@ +228,3 @@
+// TODO(smatveev): find a home for this
+void Init() {
+  static bool lsan_inited;
----------------
Alexander Potapenko wrote:
> How about making this a constructor? You won't need to call Init() from each function then.
I did it the same way ASan/MSan do. I think the idea here is to ensure that our interceptors work correctly even if another constructor runs first.

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

================
Comment at: lib/lsan/lsan_allocator.cc:42
@@ +41,3 @@
+
+void InitializeAllocator() {
+  static bool inited;
----------------
Alexander Potapenko wrote:
> Why not initialize the allocator from Init() instead?
MSan does it this way for some reason. But you're right - I'm actually doing redundant work here by calling Init() from interceptors and InitializeAllocator() from Allocate() etc. Fixed.


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