[PATCH] Interceptors etc for standalone LSan.

Kostya Serebryany kcc at google.com
Tue Apr 30 07:20:36 PDT 2013


  This CL has a few quite independent parts. Any reason not to split it?


================
Comment at: lib/lsan/lsan_allocator.h:25
@@ +24,3 @@
+struct ChunkMetadata {
+  bool allocated : 8; // Must be first.
+  uptr tag : 2;
----------------
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

================
Comment at: lib/lsan/lsan_allocator.cc:57
@@ +56,3 @@
+  m->requested_size = size;
+  atomic_store((atomic_uint8_t*)m, 1, memory_order_relaxed);
+}
----------------
Sergey Matveev wrote:
> Kostya Serebryany wrote:
> > Why not &m->allocated ? 
> > (Then m->allocated does not *have* to be first)
> Makes sense. I adopted this idiom from ASan, where chunk_state is a 8-bit u32 bit field member which is accessed in this fashion. Is there a reason why it's done that way there?
chunks state is u32 bitfield is because the next field has to be 24 bytes. 
If you need to pack things tightly here -- sure, use it. 
But your ChunkMetadata is rather sparse now. 
If you want to pack it (you do, probaaly) I'd make it like this: 

 uptr allocated: 8;  // Must be first. 
 uptr tag : 2;
 uptr requested_size : 54;
 u32 stack_trace_id;

Then of course use atomic_store((atomic_uint8_t*)m, 1, memory_order_relaxed)
  


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

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


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



More information about the llvm-commits mailing list