[PATCH] Interceptors etc for standalone LSan.

Sergey Matveev earthdok at google.com
Tue May 7 08:53:51 PDT 2013


  I am abandoning this CL and will instead submit it as several smaller CLs.


================
Comment at: lib/lsan/lsan_stack.cc:46
@@ +45,3 @@
+void PrintStackTrace(const uptr *trace, uptr size, bool symbolize) {
+  static char current_path[256];
+  static bool getcwd_attempted;
----------------
Kostya Serebryany wrote:
> Sergey Matveev wrote:
> > Kostya Serebryany wrote:
> > > static??? Please don't
> > Why not? It doesn't change between calls to this function. Is there really a need to call getcwd for every stack trace?
> will this be locked externally? 
> Also, I think we should not strip CWD, instead we should do the same thing as we do in asan (flag)
ok

================
Comment at: lib/lsan/lsan_thread.cc:18
@@ +17,3 @@
+#include <asm/prctl.h>
+#include <stddef.h>
+
----------------
Kostya Serebryany wrote:
> We don't include system headers in system-independent files. 
> So, please don't 
moved it to sanitizer_common

================
Comment at: lib/lsan/lsan_thread.cc:32
@@ +31,3 @@
+static ThreadRegistry *thread_registry;
+static THREADLOCAL u32 current_thread_tid = kInvalidTid;
+
----------------
Alexander Potapenko wrote:
> Is THREADLOCAL available on all the platforms you're compiling this file on?
yes

================
Comment at: lib/lsan/lsan_thread.cc:87
@@ +86,3 @@
+  return thread_registry->CreateThread(user_id, detached, parent_tid,
+                                       0 /*arg*/);
+}
----------------
Alexander Potapenko wrote:
> /* arg */ 0
ok

================
Comment at: lib/lsan/lsan_thread.cc:170
@@ +169,3 @@
+  CHECK_NE(tid, kInvalidTid);
+  thread_registry->JoinThread(tid, 0 /*arg*/);
+}
----------------
Alexander Potapenko wrote:
> /* arg */ 0
ok

================
Comment at: lib/lsan/lsan_thread.cc:178
@@ +177,3 @@
+                           uptr *cache_begin, uptr *cache_end) {
+    ThreadContext *context = FindThreadByOsIDLocked(os_id);
+    if (!context)
----------------
Alexander Potapenko wrote:
> 2-space indentation here.
fixed

================
Comment at: lib/lsan/lsan_interceptors.cc:231
@@ +230,3 @@
+
+// TODO(smatveev): find a home for this
+void Init() {
----------------
Sergey Matveev wrote:
> Kostya Serebryany wrote:
> > resolve TODO (lsan.cc ?)
> Currently lsan.cc is where the shared functionality resides. I think eventually it'll move into sanitizer_common, and this can go into lsan.cc. But I was planning to do that in another CL.
I put it in lsan.cc and renamed the common leak checking module to lsan_common.{h,cc}

================
Comment at: lib/lsan/lsan_interceptors.cc:130
@@ +129,3 @@
+      SANITIZER_INTERFACE_ATTRIBUTE;
+  void *__libc_memalign(size_t alignment, size_t size)
+      ALIAS("memalign") SANITIZER_INTERFACE_ATTRIBUTE;
----------------
Alexander Potapenko wrote:
> Please add a comment why are you intercepting a __libc_ function here, because you normally need not to.
> I remember having asked about that already - are you keeping track of the comments that were/weren't fixed?
added

================
Comment at: lib/lsan/lsan_interceptors.cc:127
@@ +126,3 @@
+extern "C" {
+  void cfree(void *p) ALIAS("free") SANITIZER_INTERFACE_ATTRIBUTE;
+  void *pvalloc(size_t size) ALIAS("valloc")
----------------
Alexander Potapenko wrote:
> no indentation here
fixed

================
Comment at: lib/lsan/lsan_interceptors.cc:160
@@ +159,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:
> Please put a comment somewhere describing the magic 4 number.
done

================
Comment at: lib/lsan/lsan_interceptors.cc:32
@@ +31,3 @@
+extern "C" {
+  int pthread_attr_init(void *attr);
+  int pthread_attr_destroy(void *attr);
----------------
Alexander Potapenko wrote:
> No indentation within the "extern C" block
fixed

================
Comment at: lib/lsan/lsan_allocator.h:25
@@ +24,3 @@
+struct ChunkMetadata {
+  bool allocated : 8; // Must be first.
+  uptr tag : 2;
----------------
Kostya Serebryany wrote:
> Sergey Matveev wrote:
> > Kostya Serebryany wrote:
> > > bool is always 8 bits. maybe make it u8? 
> > > (also, make sure to run lint)
> > ok.
> > 
> > I did run lint, is there an issue here?
> lint usually doesn't like one space before //
> This is what made me suspect that you did not run it
fixed


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



More information about the llvm-commits mailing list