[PATCH] [LSan] Common leak checking module.

[Kostya Serebryany]

  In general: can you split this CL even further and commit it in parts, each part with tests?
  This may end up simpler not just for the reviewers, but for you as well.


================
Comment at: lib/lsan/lsan_common.h:74
@@ +73,3 @@
+// Normal leak check. Find leaks and print a report according to flags.
+void DoLeakCheck();
+
----------------
Sergey Matveev wrote:
> 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).
So? the boilerplate code is exactly the thing which I want *not* to duplicate.

================
Comment at: lib/lsan/lsan_common_linux.cc:11
@@ +10,3 @@
+// This file is a part of LeakSanitizer.
+// Implementation of common leak checking functionality.
+//
----------------
update comment (about being linux-specific)

================
Comment at: lib/lsan/lsan_common_linux.cc:27
@@ +26,3 @@
+
+const char kLinkerName[] = "ld";
+char linker_placeholder[sizeof(LoadedModule)] ALIGNED(64);
----------------
any reason to not make these static?

================
Comment at: lib/lsan/lsan_common_linux.cc:60
@@ +59,3 @@
+    uptr end = begin + phdr->p_memsz;
+    uptr allocator_begin, allocator_end;
+    GetAllocatorGlobalRange(&allocator_begin, &allocator_end);
----------------
initialize with 0, just in case.

================
Comment at: lib/lsan/lsan_common_linux.cc:108
@@ +107,3 @@
+// ld-linux.so as reachable.
+void ProcessSpecialAllocations(InternalVector<uptr> *frontier, Flags *flags) {
+  if (!flags->use_tls()) return;
----------------
passing Flags here looks weird. 
Either don't use them here at all, or move the flags() function into the .h file (as it is done in e.g. asan)

================
Comment at: lib/lsan/lsan_common.h:97
@@ +96,3 @@
+void ProcessGlobalRegions(InternalVector<uptr> *frontier);
+void ProcessSpecialAllocations(InternalVector<uptr> *frontier, Flags *flags);
+
----------------
is this called anywhere at all? Do you need to include dead code into this CL?

================
Comment at: lib/lsan/lsan_common.cc:122
@@ +121,3 @@
+    if (!thread_found) {
+      // If a thread can't be found in the thread registry, it's probably in the
+      // process of destruction. Log this event and move on.
----------------
isn't it better to continue here instead of using else?

if (!thread_found) {
...
  continue;
}
// the rest w/o else

maybe also outline the loop body into ProcessOnThread() 

================
Comment at: lib/lsan/lsan_common_linux.cc:99
@@ +98,3 @@
+  if (m.allocated() && m.tag() != TAG_REACHABLE) {
+    if (linker->containsAddress(GetCallerPC(m.stack_trace_id()))) {
+      m.set_tag(TAG_REACHABLE);
----------------
is this functionality testable? 


================
Comment at: lib/lsan/lsan_common.cc:331
@@ +330,3 @@
+  bool is_directly_leaked = (tag == TAG_DIRECTLY_LEAKED);
+  for (uptr i = 0; i < leaks_.size(); i++)
+    if (leaks_[i].stack_trace_id == stack_trace_id &&
----------------
stop this after e.g. 10000 leaks (configurable by a flag. can we reuse max_leaks?)
otherwise you may die in quadratic algorithm w/o being able to report anything useful. 

================
Comment at: lib/lsan/lsan_common.cc:350
@@ +349,3 @@
+  InternalSort(&leaks_, leaks_.size(), IsLarger);
+  max_leaks = max_leaks > 0 ? Min(max_leaks, leaks_.size()) : leaks_.size();
+  for (uptr i = 0; i < max_leaks; i++) {
----------------
you can simplify the logic if you set max_leaks to e.g. 10000 by default. 


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