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

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:525
+  GetThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      ForEachRegisteredThreadContextCb, frontier);
+}
----------------
vitalybuka wrote:
> 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
Good idea. This means that ASan and LSan can share some logic here and it avoids the problems I ran into trying to use `flags()->log_pointers` from `asan_allocator.cpp`


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:540
   ProcessThreads(suspended_threads, frontier);
+  ProcessThreadRegistry(frontier);
   ProcessRootRegions(frontier);
----------------
vitalybuka wrote:
> 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 
> 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.

I don't think we can use `suspended_threads`. I glanced at the origin of the `suspend_threads` list and on Linux that comes from reading `/proc/*/task` (see `ThreadLister::ThreadLister`). That won't necessarily contain be the threads (not started) we are interested in. The types are also wrong. The list of suspended threads look like `tid_t` but we actually need the `ThreadContext` objects which would require a O(N) look up into the thread registry anyway. It's simpler to just walk the thread registry.

I could move the code walking the thread registry into `ProcessThreads`. However, that function is already pretty big. **Can I just change `ProcessThreads()` to call `ProcessThreadRegistry()` at the end to make the code easier to read?**


================
Comment at: compiler-rt/lib/lsan/lsan_common.h:53
 class FlagParser;
 class ThreadRegistry;
 struct DTLS;
----------------
vitalybuka wrote:
>  forward declaration instead of the new include?
I'll try it out. I can't actually remember why this include was necessary. It might be that's actually needed for the subsequent patch.


================
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
----------------
vitalybuka wrote:
> Maybe ForEachRegisteredThreadContextCb -> GetAdditionalThreadContextPtrs to show what is expected from this CB
Good idea. I'll do rename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95183