[lsan] Move out suppression of invalid PCs from StopTheWorld

[vitalybuka]

Summary

This removes the last use of StackDepot from StopTheWorld.

Depends on D115284.

[vitalybuka]

rebase

[morehouse]

compiler-rt/lib/lsan/lsan_common.cpp

184

We can use stack instead of StackDepotGet(stack_trace_id) to avoid another lookup. We can also then remove stack_trace_id from the parameter list.

552

I don't see these checks anywhere in SuppressInvalid. Why is it safe to omit them? Will we introduce new false negatives because we're suppressing more things?

[morehouse]

compiler-rt/lib/lsan/lsan_common.cpp

552

Never mind. I see now that CollectLeaksCb filters out things that don't meet these conditions:

if (!m.allocated()) return;
if (m.tag() == kDirectlyLeaked || m.tag() == kIndirectlyLeaked)
  leaks->push_back({chunk, m.stack_trace_id(), m.requested_size(), m.tag()});

[vitalybuka]

rebase

[glandium]

For some reason this change makes Firefox Linux ASAN builds break our CI in a way that doesn't even leave logs (it looks like the workers completely die)

[vitalybuka]

In D115319#3393379, @glandium wrote:

For some reason this change makes Firefox Linux ASAN builds break our CI in a way that doesn't even leave logs (it looks like the workers completely die)

Sorry, no sure what to do with available info.
Have you been able to solve the problem?

[sbc100]

compiler-rt/lib/lsan/lsan_common.cpp

159

I ran into an issue with this change when attempting to upgrade compiler-rt in emscripten.

It seems that this function will always return 0 if stack traces are disabled by doing malloc_context_size=0/1 ?

This means that SuppressInvalid always returns true and therefore everything get suppressed and not report is generated at add.

We have been recommending malloc_context_size=0/1 as a way to speed up msan/lsan under emscripten but this change seems to mean that completely disabled lsan (no report generated at all).

Does lsan now require malloc_context_size of at least 2?

[vitalybuka]

compiler-rt/lib/lsan/lsan_common.cpp

159

Sounds like reasonable use-case. If so this is unintentional and we need to fix and add a regression test.
How sure you are that this is just from this patch? This function was already present. On a quick look

we had

uptr caller_pc = 0;
    if (stack_id > 0)
      caller_pc = GetCallerPC(StackDepotGet(stack_id));
    // If caller_pc is unknown, this chunk may be allocated in a coroutine. Mark
    // it as reachable, as we can't properly report its allocation stack anyway.
    if (caller_pc == 0 || (param->skip_linker_allocations &&
                           GetLinker()->containsAddress(caller_pc))) {
      m.set_tag(kIgnored);
      param->frontier->push_back(chunk);
    }

which I believe suppress report as well

[sbc100]

compiler-rt/lib/lsan/lsan_common.cpp

159

Ah, I suppose it could have pre-dated this patch.

I know it used to work in LLVM 13 and stopped working when I updated to 14.

compiler-rt/lib/lsan/lsan_common.cpp

do {                        \
         Report(__VA_ARGS__);    \
     } while (0)
 
 class LeakSuppressionContext {
   bool parsed = false;
   SuppressionContext context;
   bool suppressed_stacks_sorted = true;
   InternalMmapVector<u32> suppressed_stacks;
+  const LoadedModule *suppress_module = nullptr;
 
-  Suppression *GetSuppressionForAddr(uptr addr);
   void LazyInit();
+  Suppression *GetSuppressionForAddr(uptr addr);
+  bool SuppressInvalid(const StackTrace &stack);
   bool SuppressByRule(const StackTrace &stack, uptr hit_count, uptr total_size);
 
  public:
   LeakSuppressionContext(const char *supprression_types[],
                          int suppression_types_num)
       : context(supprression_types, suppression_types_num) {}
 
   bool Suppress(u32 stack_trace_id, uptr hit_count, uptr total_size);

 
 void LeakSuppressionContext::LazyInit() {
   if (!parsed) {
     parsed = true;
     context.ParseFromFile(flags()->suppressions);
     if (&__lsan_default_suppressions)
       context.Parse(__lsan_default_suppressions());
     context.Parse(kStdSuppressions);
+    if (flags()->use_tls && flags()->use_ld_allocations)
+      suppress_module = GetLinker();
   }
 }
 
 Suppression *LeakSuppressionContext::GetSuppressionForAddr(uptr addr) {
   Suppression *s = nullptr;
 
   // Suppress by module name.
   if (const char *module_name =
           Symbolizer::GetOrInit()->GetModuleNameForPc(addr))
     if (context.Match(module_name, kSuppressionLeak, &s))
       return s;
 
   // Suppress by file or function name.
   SymbolizedStack *frames = Symbolizer::GetOrInit()->SymbolizePC(addr);
   for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
     if (context.Match(cur->info.function, kSuppressionLeak, &s) ||
         context.Match(cur->info.file, kSuppressionLeak, &s)) {
       break;
     }
   }
   frames->ClearAll();
   return s;
 }
 
+static uptr GetCallerPC(const StackTrace &stack) {
+  // The top frame is our malloc/calloc/etc. The next frame is the caller.
+  if (stack.size >= 2)
+    return stack.trace[1];
+  return 0;
+}
+
+// On Linux, treats all chunks allocated from ld-linux.so as reachable, which
+// covers dynamically allocated TLS blocks, internal dynamic loader's loaded
+// modules accounting etc.
+// Dynamic TLS blocks contain the TLS variables of dynamically loaded modules.
+// They are allocated with a __libc_memalign() call in allocate_and_init()
+// (elf/dl-tls.c). Glibc won't tell us the address ranges occupied by those
+// blocks, but we can make sure they come from our own allocator by intercepting
+// __libc_memalign(). On top of that, there is no easy way to reach them. Their
+// addresses are stored in a dynamically allocated array (the DTV) which is
+// referenced from the static TLS. Unfortunately, we can't just rely on the DTV
+// being reachable from the static TLS, and the dynamic TLS being reachable from
+// the DTV. This is because the initial DTV is allocated before our interception
+// mechanism kicks in, and thus we don't recognize it as allocated memory. We
+// can't special-case it either, since we don't know its size.
+// Our solution is to include in the root set all allocations made from
+// ld-linux.so (which is where allocate_and_init() is implemented). This is
+// guaranteed to include all dynamic TLS blocks (and possibly other allocations
+// which we don't care about).
+// On all other platforms, this simply checks to ensure that the caller pc is
+// valid before reporting chunks as leaked.
+bool LeakSuppressionContext::SuppressInvalid(const StackTrace &stack) {
+  uptr caller_pc = GetCallerPC(stack);
+  // If caller_pc is unknown, this chunk may be allocated in a coroutine. Mark
+  // it as reachable, as we can't properly report its allocation stack anyway.
+  return !caller_pc ||
+         (suppress_module && suppress_module->containsAddress(caller_pc));
+}
+
 bool LeakSuppressionContext::SuppressByRule(const StackTrace &stack,
                                             uptr hit_count, uptr total_size) {
   for (uptr i = 0; i < stack.size; i++) {
     Suppression *s = GetSuppressionForAddr(
         StackTrace::GetPreviousInstructionPc(stack.trace[i]));
     if (s) {
       s->weight += total_size;
       atomic_fetch_add(&s->hit_count, hit_count, memory_order_relaxed);
       return true;
     }
   }
   return false;
 }
 
 bool LeakSuppressionContext::Suppress(u32 stack_trace_id, uptr hit_count,
                                       uptr total_size) {
   LazyInit();
   StackTrace stack = StackDepotGet(stack_trace_id);
-  if (!SuppressByRule(stack, hit_count, total_size))
+  if (!SuppressInvalid(stack) && !SuppressByRule(stack, hit_count, total_size))
     return false;
   suppressed_stacks_sorted = false;
   suppressed_stacks.push_back(stack_trace_id);
   return true;
 }
 
 static LeakSuppressionContext *GetSuppressionContext() {
   CHECK(suppression_ctx);
static void CollectIgnoredCb(uptr chunk, void *arg) {
   LsanMetadata m(chunk);
   if (m.allocated() && m.tag() == kIgnored) {
     LOG_POINTERS("Ignored: chunk %p-%p of size %zu.\n", (void *)chunk,
                  (void *)(chunk + m.requested_size()), m.requested_size());
     reinterpret_cast<Frontier *>(arg)->push_back(chunk);
   }
 }
 
-static uptr GetCallerPC(const StackTrace &stack) {
-  // The top frame is our malloc/calloc/etc. The next frame is the caller.
-  if (stack.size >= 2)
-    return stack.trace[1];
-  return 0;
-}
-
-struct InvalidPCParam {
-  Frontier *frontier;
-  bool skip_linker_allocations;
-};
-
-// ForEachChunk callback. If the caller pc is invalid or is within the linker,
-// mark as reachable. Called by ProcessPlatformSpecificAllocations.
-static void MarkInvalidPCCb(uptr chunk, void *arg) {
-  CHECK(arg);
-  InvalidPCParam *param = reinterpret_cast<InvalidPCParam *>(arg);
-  chunk = GetUserBegin(chunk);
-  LsanMetadata m(chunk);
-  if (m.allocated() && m.tag() != kReachable && m.tag() != kIgnored) {
-    u32 stack_id = m.stack_trace_id();
-    uptr caller_pc = 0;
-    if (stack_id > 0)
-      caller_pc = GetCallerPC(StackDepotGet(stack_id));
-    // If caller_pc is unknown, this chunk may be allocated in a coroutine. Mark
-    // it as reachable, as we can't properly report its allocation stack anyway.
-    if (caller_pc == 0 || (param->skip_linker_allocations &&
-                           GetLinker()->containsAddress(caller_pc))) {
-      m.set_tag(kIgnored);
-      param->frontier->push_back(chunk);
-    }
-  }
-}
-
-// On Linux, treats all chunks allocated from ld-linux.so as reachable, which
-// covers dynamically allocated TLS blocks, internal dynamic loader's loaded
-// modules accounting etc.
-// Dynamic TLS blocks contain the TLS variables of dynamically loaded modules.
-// They are allocated with a __libc_memalign() call in allocate_and_init()
-// (elf/dl-tls.c). Glibc won't tell us the address ranges occupied by those
-// blocks, but we can make sure they come from our own allocator by intercepting
-// __libc_memalign(). On top of that, there is no easy way to reach them. Their
-// addresses are stored in a dynamically allocated array (the DTV) which is
-// referenced from the static TLS. Unfortunately, we can't just rely on the DTV
-// being reachable from the static TLS, and the dynamic TLS being reachable from
-// the DTV. This is because the initial DTV is allocated before our interception
-// mechanism kicks in, and thus we don't recognize it as allocated memory. We
-// can't special-case it either, since we don't know its size.
-// Our solution is to include in the root set all allocations made from
-// ld-linux.so (which is where allocate_and_init() is implemented). This is
-// guaranteed to include all dynamic TLS blocks (and possibly other allocations
-// which we don't care about).
-// On all other platforms, this simply checks to ensure that the caller pc is
-// valid before reporting chunks as leaked.
-static void ProcessPC(Frontier *frontier) {
-  InvalidPCParam arg;
-  arg.frontier = frontier;
-  arg.skip_linker_allocations =
-      flags()->use_tls && flags()->use_ld_allocations && GetLinker() != nullptr;
-  ForEachChunk(MarkInvalidPCCb, &arg);
-}
-
 // Sets the appropriate tag on each chunk.
 static void ClassifyAllChunks(SuspendedThreadsList const &suspended_threads,
                               Frontier *frontier) {
   const InternalMmapVector<u32> &suppressed_stacks =
       GetSuppressionContext()->GetSortedSuppressedStacks();
   if (!suppressed_stacks.empty()) {
     ForEachChunk(IgnoredSuppressedCb,
                  const_cast<InternalMmapVector<u32> *>(&suppressed_stacks));
   }
   ForEachChunk(CollectIgnoredCb, frontier);
   ProcessGlobalRegions(frontier);
   ProcessThreads(suspended_threads, frontier);
   ProcessRootRegions(frontier);
   FloodFillTag(frontier, kReachable);
 
-  CHECK_EQ(0, frontier->size());
-  ProcessPC(frontier);
-
   // The check here is relatively expensive, so we do this in a separate flood
   // fill. That way we can skip the check for chunks that are reachable
   // otherwise.
   LOG_POINTERS("Processing platform-specific allocations.\n");
   ProcessPlatformSpecificAllocations(frontier);
   FloodFillTag(frontier, kReachable);
 
   // Iterate over leaked chunks and mark those that are reachable from other