[PATCH] [LSan] Common leak checking module.

[Sergey Matveev]

  > 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. :(

  On the one hand, I'm going to rework the way ignored ranges are exposed, like we discussed. On the other hand, we're probably going to need even more knowledge of the internal allocator structure when we start optimizing for performance. I.e., primary and secondary allocators would have to be treated separately.

  > I still think that we could split this file into one linux-specific and one OS-independent

  I factored out everything that deals with ld-linux and dl_iterate_phdr, but it's still far from perfect. There are a lot of assumptions that might not hold on other systems.

  > some of the performance critical stuff will have to be inlined from other modules (not necessary in this CL). E.g. LsanMetadata

  Yep, I have that in mind. That would require moving most of this code to the header. I deliberately avoid any performance optimizations until there's a good set of benchmarks.


================
Comment at: lib/lsan/lsan_common.h:74
@@ +73,3 @@
+// Normal leak check. Find leaks and print a report according to flags.
+void DoLeakCheck();
+
----------------
Kostya Serebryany wrote:
> Can DoLeakCheck call ReportLeaked? 
> This way we will be testing the actual stuff, not some test code.
No, because there's boilerplate code in both functions (locks, stoptheworld).

================
Comment at: lib/lsan/lsan_common.h:80
@@ +79,3 @@
+  u32 stack_trace_id;
+  uptr tag : 2;
+};
----------------
Kostya Serebryany wrote:
> 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)?
Changed to bool.

================
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);
----------------
Kostya Serebryany wrote:
> forgot the argument?
oops

================
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();
----------------
Kostya Serebryany wrote:
> const char *region_type
ok

================
Comment at: lib/lsan/lsan_common_linux.cc:108
@@ +107,3 @@
+  p >>= 47;
+  return (p == 0) || (p == (2 << 16) - 1);
+#else
----------------
Kostya Serebryany wrote:
> 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.
fixed

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

================
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
----------------
Kostya Serebryany wrote:
> use FIXME w.o. name (here and elsewhere)
> This is the style used in this code base
ok

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

================
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);
----------------
Kostya Serebryany wrote:
> use CHECK_LE, here and everywhere else
ok

================
Comment at: lib/lsan/lsan_common_linux.cc:273
@@ +272,3 @@
+
+void ProcessLinkerAllocationsCb::operator()(void *p) const {
+  LsanMetadata m(p);
----------------
Kostya Serebryany wrote:
> comment?
There's a comment for ProcessLinkerAllocation, I think it's enough? None of the callables have their own comments, but they're eponymous with the functions that use them.

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

================
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();
----------------
Kostya Serebryany wrote:
> This should go into the LoadedModule class itself, with a set of positive/negative unit tests.
> Please send a separate CL.
done

================
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));
----------------
Kostya Serebryany wrote:
> memcpy-ing a non-POD? That's gross. 
fixed


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