[PATCH] [LSan] Common leak checking module.

[Sergey Matveev]

================
Comment at: lib/lsan/lsan_common.h:25
@@ +24,3 @@
+// Chunk tags.
+enum {
+  TAG_DIRECTLY_LEAKED = 0,  // default
----------------
Alexander Potapenko wrote:
> Please name this enum and use the enum type instead of uptr when operating tags.
done

================
Comment at: lib/lsan/lsan_common.h:44
@@ +43,3 @@
+// Aligned pointers everywhere.
+const uptr kSourceAll =
+    kSourceGlobals | kSourceStacks | kSourceTLS | kSourceRegisters;
----------------
Evgeniy Stepanov wrote:
> That's a misleading name. Call it kSourceAllAligned?
done

================
Comment at: lib/lsan/lsan_common.h:88
@@ +87,3 @@
+class LeakReport {
+  InternalVector<Leak> leaks_;
+
----------------
Alexander Potapenko wrote:
> Methods before data members, public before private.
fixed

================
Comment at: lib/lsan/lsan_common.h:112
@@ +111,3 @@
+
+class PrintLeakedCb {
+ public:
----------------
Evgeniy Stepanov wrote:
> This needs at least a comment.
done

================
Comment at: lib/lsan/lsan_common.h:159
@@ +158,3 @@
+class LsanMetadata {
+  void *metadata_;
+ public:
----------------
Alexander Potapenko wrote:
> Methods before data members, please.
fixed

================
Comment at: lib/lsan/lsan_common.cc:34
@@ +33,3 @@
+  f->max_leaks = 0;
+
+  f->log_pointers = false;
----------------
Alexander Potapenko wrote:
> Spare newline?
removed

================
Comment at: lib/lsan/lsan_common.cc:64
@@ +63,3 @@
+#ifdef __x86_64__
+  return ((p >> 47) == 0);
+#else
----------------
Alexander Potapenko wrote:
> Please clarify in a comment why you're checking for the upper 17 bits to be zeroes.
done

================
Comment at: lib/lsan/lsan_common.cc:200
@@ +199,3 @@
+  FloodFillReachable(&frontier);
+  ProcessSpecialAllocations(&frontier);
+  FloodFillReachable(&frontier);
----------------
Evgeniy Stepanov wrote:
> call it ProcessPlatformAllocations or ProcessPlatformSpecificAllocations?
done

================
Comment at: lib/lsan/lsan_common.cc:225
@@ +224,3 @@
+  StopTheWorld(callback, arg);
+  // Allocator must be unlocked by the callback.
+  UnlockThreadRegistry();
----------------
Alexander Potapenko wrote:
> Am I understanding right that the critical sections protected by the allocator lock and the thread registry lock intersect, but are not nested? This is usually bad. Can you lock the thread registry before the allocator instead?
fixed

================
Comment at: lib/lsan/lsan_common.cc:334
@@ +333,3 @@
+
+void LeakReport::Add(u32 stack_trace_id, uptr leaked_size, uptr tag) {
+  CHECK(tag == TAG_DIRECTLY_LEAKED || tag == TAG_INDIRECTLY_LEAKED);
----------------
Alexander Potapenko wrote:
> s/uptr tag/enum tag_type tag/
> or something like that.
done

================
Comment at: lib/lsan/lsan_common.cc:344
@@ +343,3 @@
+    }
+  Leak leak = { 1, leaked_size, stack_trace_id, is_directly_leaked };
+  leaks_.push_back(leak);
----------------
Alexander Potapenko wrote:
> /*hit_count*/1
fixed

================
Comment at: lib/lsan/lsan_common.cc:348
@@ +347,3 @@
+
+static inline bool IsLarger(const Leak &leak1, const Leak &leak2) {
+  return leak1.total_size > leak2.total_size;
----------------
Alexander Potapenko wrote:
> You're passing a pointer to this function into InternalSort. Why make it inline?
fixed


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