[PATCH] D95183: [LSan] Introduce a callback mechanism to allow adding data reachable from ThreadContexts to the frontier.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:525
+  GetThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      ForEachRegisteredThreadContextCb, frontier);
+}
----------------
to avoid frontier access from asan
could you please just:
```
InternalMmapVector<uptr> ptrs;
GetThreadRegistryLocked()->RunCallbackForEachThreadLocked(
      ForEachRegisteredThreadContextCb, &ptrs);

for (: ptr) {
  // process ptr here.
}
```

another option is nested callback but it seems unnecessary


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:540
   ProcessThreads(suspended_threads, frontier);
+  ProcessThreadRegistry(frontier);
   ProcessRootRegions(frontier);
----------------
I would prefer if we handle this in ProcessThreads(suspended_threads, frontier) for suspended_threads
But I guess we don't know if just created threads are in suspended_threads.

Still this is very Thread related
so maybe call GetThreadRegistryLocked()->RunCallbackForEachThreadLocked(
from the ProcessThreads() with commend that GetThreadRegistryLocked may contains threads 


================
Comment at: compiler-rt/lib/lsan/lsan_common.h:53
 class FlagParser;
 class ThreadRegistry;
 struct DTLS;
----------------
 forward declaration instead of the new include?


================
Comment at: compiler-rt/lib/lsan/lsan_common.h:146
 void ForEachExtraStackRangeCb(uptr begin, uptr end, void* arg);
+void ForEachRegisteredThreadContextCb(ThreadContextBase *tctx, void *arg);
 // Run stoptheworld while holding any platform-specific locks, as well as the
----------------
Maybe ForEachRegisteredThreadContextCb -> GetAdditionalThreadContextPtrs to show what is expected from this CB


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95183/new/

https://reviews.llvm.org/D95183