LSan: leaks should report PID and TID of allocation

[DianeMeirowitz]

It would be much easier to find the source of a leak if LSan reported the PID and TID of the process and thread that allocated the leaked memory. Originally submitted here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94448

[kcc]

PID is already printed:

==151441==ERROR: LeakSanitizer: detected memory leaks
...

By TID, do you mean something like pthread_self, or internal ASan's/LSan's thread-id?

It's possible to print ASan's thread ID with a patch below.

Direct leak of 4 byte(s) in 1 object(s) allocated in T0 from:

But why would you need the TIDs to be printed?
What information does it actually convey for a leak, on top of the stack trace?

Adding it to LSan (standalone, -fsanitize=leak) is a bit more involved since the standalone lsan allocator doesn't track thread ids. Not sure if it's worth the effort though - I don't know if standalone lsan is frequently used.

--- a/compiler-rt/lib/asan/asan_allocator.cpp
+++ b/compiler-rt/lib/asan/asan_allocator.cpp
@@ -1072,6 +1072,11 @@ u32 LsanMetadata::stack_trace_id() const {
   return m->alloc_context_id;
 }
 
+u32 LsanMetadata::alloc_tid() const {
+  __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
+  return m->alloc_tid;
+}
+
 void ForEachChunk(ForEachChunkCallback callback, void *arg) {
   __asan::get_allocator().ForEachChunk(callback, arg);
 }
--- a/compiler-rt/lib/lsan/lsan_allocator.cpp
+++ b/compiler-rt/lib/lsan/lsan_allocator.cpp
@@ -290,6 +290,9 @@ uptr LsanMetadata::requested_size() const {
 u32 LsanMetadata::stack_trace_id() const {
   return reinterpret_cast<ChunkMetadata *>(metadata_)->stack_trace_id;
 }
+u32 LsanMetadata::alloc_tid() const {
+  return 0;  // unimplemented
+}
 
 void ForEachChunk(ForEachChunkCallback callback, void *arg) {
   allocator.ForEachChunk(callback, arg);
diff --git a/compiler-rt/lib/lsan/lsan_common.cpp b/compiler-rt/lib/lsan/lsan_common.cpp
index 32ea4e88003..8472464ede8 100644
--- a/compiler-rt/lib/lsan/lsan_common.cpp
+++ b/compiler-rt/lib/lsan/lsan_common.cpp
@@ -509,7 +509,7 @@ static void CollectLeaksCb(uptr chunk, void *arg) {
       stack_trace_id = m.stack_trace_id();
     }
     leak_report->AddLeakedChunk(chunk, stack_trace_id, m.requested_size(),
-                                m.tag());
+                                m.tag(), m.alloc_tid());
   }
 }
 
@@ -675,12 +675,13 @@ static Suppression *GetSuppressionForStack(u32 stack_trace_id) {
 const uptr kMaxLeaksConsidered = 5000;
 
 void LeakReport::AddLeakedChunk(uptr chunk, u32 stack_trace_id,
-                                uptr leaked_size, ChunkTag tag) {
+                                uptr leaked_size, ChunkTag tag, u32 alloc_tid) {
   CHECK(tag == kDirectlyLeaked || tag == kIndirectlyLeaked);
   bool is_directly_leaked = (tag == kDirectlyLeaked);
   uptr i;
   for (i = 0; i < leaks_.size(); i++) {
     if (leaks_[i].stack_trace_id == stack_trace_id &&
+        leaks_[i].alloc_tid == alloc_tid &&
         leaks_[i].is_directly_leaked == is_directly_leaked) {
       leaks_[i].hit_count++;
       leaks_[i].total_size += leaked_size;
@@ -690,7 +691,7 @@ void LeakReport::AddLeakedChunk(uptr chunk, u32 stack_trace_id,
   if (i == leaks_.size()) {
     if (leaks_.size() == kMaxLeaksConsidered) return;
     Leak leak = { next_id_++, /* hit_count */ 1, leaked_size, stack_trace_id,
-                  is_directly_leaked, /* is_suppressed */ false };
+                  is_directly_leaked, /* is_suppressed */ false, alloc_tid};
     leaks_.push_back(leak);
   }
   if (flags()->report_objects) {
@@ -734,9 +735,10 @@ void LeakReport::ReportTopLeaks(uptr num_leaks_to_report) {
 void LeakReport::PrintReportForLeak(uptr index) {
   Decorator d;
   Printf("%s", d.Leak());
-  Printf("%s leak of %zu byte(s) in %zu object(s) allocated from:\n",
+  Printf("%s leak of %zu byte(s) in %zu object(s) allocated in T%d from:\n",
          leaks_[index].is_directly_leaked ? "Direct" : "Indirect",
-         leaks_[index].total_size, leaks_[index].hit_count);
+         leaks_[index].total_size, leaks_[index].hit_count,
+         leaks_[index].alloc_tid);
   Printf("%s", d.Default());
 
   PrintStackTraceById(leaks_[index].stack_trace_id);
diff --git a/compiler-rt/lib/lsan/lsan_common.h b/compiler-rt/lib/lsan/lsan_common.h
index 6252d52c197..00420483591 100644
--- a/compiler-rt/lib/lsan/lsan_common.h
+++ b/compiler-rt/lib/lsan/lsan_common.h
@@ -86,6 +86,7 @@ struct Leak {
   u32 stack_trace_id;
   bool is_directly_leaked;
   bool is_suppressed;
+  u32 alloc_tid;
 };
 
 struct LeakedObject {
@@ -99,7 +100,7 @@ class LeakReport {
  public:
   LeakReport() {}
   void AddLeakedChunk(uptr chunk, u32 stack_trace_id, uptr leaked_size,
-                      ChunkTag tag);
+                      ChunkTag tag, u32 alloc_tid);
   void ReportTopLeaks(uptr max_leaks);
   void PrintSummary();
   void ApplySuppressions();
@@ -260,6 +261,7 @@ class LsanMetadata {
   void set_tag(ChunkTag value);
   uptr requested_size() const;
   u32 stack_trace_id() const;
+  u32 alloc_tid() const;
  private:
   void *metadata_;
 };

[DianeMeirowitz]

I was thinking something like pthread_self(), yes. But If this increases the size of allocation information, it's probably not worth it, though it looks like it won't increase size.

I am not sure how helpful the information would be for debugging leaks, actually. So feel free to close this issue if you don't see a benefit to doing it.

[kcc]

LSan is ~7 years old and you are the first one to ask for TIDs in leak reports.
I'd prefer to keep things as is primarily because we don't have cycles to implement rarely required functionality. Please reopen if you find a compelling use case.

[DianeMeirowitz]

Please leave it as it is then. And thank you.