[PATCH] [LSan] Common leak checking module.

Kostya Serebryany kcc at google.com
Fri May 17 00:05:21 PDT 2013



================
Comment at: lib/lsan/lsan_common.h:44
@@ +43,3 @@
+// Aligned pointers everywhere.
+const uptr kSourceAllALigned =
+    kSourceGlobals | kSourceStacks | kSourceTLS | kSourceRegisters;
----------------
did you mean Aligned (you have ALligned with capital L)

================
Comment at: lib/lsan/lsan_common.cc:89
@@ +88,3 @@
+    // LargeMmapAllocator involves a lock and a linear search. Instead, we
+    // should acquire the list of chunks from secondary allocator and do our own
+    // checking.
----------------
I disagree. We should make GetBlockBegin faster instead of exposing more allocator guts here.
Please remove the comment starting from "Instead"

================
Comment at: lib/lsan/lsan_common.cc:334
@@ +333,3 @@
+  bool is_directly_leaked = (tag == kDirectlyLeaked);
+  for (uptr i = 0; i < leaks_.size(); i++)
+    if (leaks_[i].stack_trace_id == stack_trace_id &&
----------------
You did not address my and Eugeniy's comments here. 
We don't want an unbounded N^2 algorithm, even if the chances of exploding is small.
I still think that we should set max_leaks to e.g. 1000 by default. 
This will simplify the logic around max_leaks and bound this loop at the same time. 

Also, this loop may be hot, please rewrite it as 
for (uptr i = 0, n = leaks_.size(); i < n; i++)

LLVM style requires to do this for all loops.
I don't insist (sanitizer code is not truly LLVM-style anyway), 
but this is still a requirement for all potentially hot loops. 

Using a hash map here will be an overkill -- there is no user value in reporting > 1000 unique leaks.


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



More information about the llvm-commits mailing list