[PATCH] [LSan] Common leak checking module.

[Kostya Serebryany]

  First comments. More will follow after you address these.
  General comments:
    - we are exposing too much knowledge of the internal allocator structure (caches, etc). I don't like it, but I am not sure what to do here. :(
    - I still think that we could split this file into one linux-specific and one OS-independent
    - some of the performance critical stuff will have to be inlined from other modules (not necessary in this CL). E.g. LsanMetadata


================
Comment at: lib/lsan/lsan_common.h:80
@@ +79,3 @@
+  u32 stack_trace_id;
+  uptr tag : 2;
+};
----------------
why do you need a bitfield here? Why not just u32? 
Also,  do you actually need the tag here, or just a bool (is_directly_leaked)?

================
Comment at: lib/lsan/lsan_common_linux.cc:31
@@ +30,3 @@
+const char kLinkerName[] = "ld";
+const uptr kMaxModules = 200;
+LoadedModule *linker = 0;
----------------
We've seen many thousands

================
Comment at: lib/lsan/lsan_common_linux.cc:65
@@ +64,3 @@
+// Match paths of the form /path/to/name-something.1.2.3.so.
+static bool LibraryNameIs(LoadedModule *module, const char name[]) {
+  const char *base_name = module->full_name();
----------------
This should go into the LoadedModule class itself, with a set of positive/negative unit tests.
Please send a separate CL.

================
Comment at: lib/lsan/lsan_common_linux.cc:86
@@ +85,3 @@
+    if (LibraryNameIs(&modules.data()[i], kLinkerName)) {
+      internal_memcpy(linker_placeholder, &modules.data()[i],
+                      sizeof(LoadedModule));
----------------
memcpy-ing a non-POD? That's gross. 

================
Comment at: lib/lsan/lsan_common_linux.cc:108
@@ +107,3 @@
+  p >>= 47;
+  return (p == 0) || (p == (2 << 16) - 1);
+#else
----------------
Why do you care about pointers starting with ffff? These are kernel pointers, not user space.
Also, anything that is smaller than e.g. 10000 is not a pointer.

================
Comment at: lib/lsan/lsan_common_linux.cc:122
@@ +121,3 @@
+                                 InternalVector<uptr> *frontier,
+                                 const char region_type[], uptr tag) {
+  const uptr alignment = flags()->pointer_alignment();
----------------
const char *region_type

================
Comment at: lib/lsan/lsan_common_linux.cc:130
@@ +129,3 @@
+  while (pp + sizeof(uptr) <= end) {
+    void *p = *((void**)pp);
+    void *chunk;
----------------
try to use reinterpret_cast<void**>(pp) here and elsewhere

================
Comment at: lib/lsan/lsan_common_linux.cc:136
@@ +135,3 @@
+    // checking.
+    if (IsPointer((uptr)p) && (chunk = PointsIntoChunk(p))) {
+      LsanMetadata m(chunk);
----------------
if (IsPointer(reinterpret_cast<uptr>(p))) continue;
void *chunk = PointsIntoChunk();
if (!chunk) continue;  ... 

================
Comment at: lib/lsan/lsan_common_linux.cc:132
@@ +131,3 @@
+    void *chunk;
+    // TODO(smatveev): PointsIntoChunk is SLOW because GetBlockBegin() in
+    // LargeMmapAllocator involves a lock and a linear search. Instead, we
----------------
use FIXME w.o. name (here and elsewhere)
This is the style used in this code base

================
Comment at: lib/lsan/lsan_common_linux.cc:135
@@ +134,3 @@
+    // should acquire the list of chunks from secondary allocator and do our own
+    // checking.
+    if (IsPointer((uptr)p) && (chunk = PointsIntoChunk(p))) {
----------------
I think this should be a functionality of LargeMmapAllocator itself

================
Comment at: lib/lsan/lsan_common_linux.cc:241
@@ +240,3 @@
+        // that allocator cache will be part of static TLS image.
+        CHECK(tls_begin <= cache_begin);
+        CHECK(tls_end >= cache_end);
----------------
use CHECK_LE, here and everywhere else

================
Comment at: lib/lsan/lsan_common_linux.cc:273
@@ +272,3 @@
+
+void ProcessLinkerAllocationsCb::operator()(void *p) const {
+  LsanMetadata m(p);
----------------
comment?

================
Comment at: lib/lsan/lsan_common_linux.cc:457
@@ +456,3 @@
+  if (max_leaks > 0 && max_leaks < leaks_.size())
+    Printf("\nThe %llu largest leaks:\n");
+  InternalSort(&leaks_, leaks_.size(), IsLarger);
----------------
forgot the argument?

================
Comment at: lib/lsan/lsan_common.h:74
@@ +73,3 @@
+// Normal leak check. Find leaks and print a report according to flags.
+void DoLeakCheck();
+
----------------
Can DoLeakCheck call ReportLeaked? 
This way we will be testing the actual stuff, not some test code.


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