Page MenuHomePhabricator

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

Authored by delcypher on Jan 21 2021, 4:18 PM.



This mechanism is intended to provide a way to treat the arg pointer
of a created (but not yet started) thread as reachable. In future
patches this will be implemented in ForEachRegisteredThreadContextCb.

A separate implementation of ForEachRegisteredThreadContextCb exists
for ASan and LSan runtimes because they need to be implemented
differently in future patches.


Diff Detail

Event Timeline

delcypher requested review of this revision.Jan 21 2021, 4:18 PM
delcypher created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 4:18 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Jan 21 2021, 8:53 PM

to avoid frontier access from asan
could you please just:

InternalMmapVector<uptr> ptrs;
      ForEachRegisteredThreadContextCb, &ptrs);

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

another option is nested callback but it seems unnecessary


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


forward declaration instead of the new include?


Maybe ForEachRegisteredThreadContextCb -> GetAdditionalThreadContextPtrs to show what is expected from this CB

delcypher added inline comments.Jan 22 2021, 12:47 PM

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


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?


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.


Good idea. I'll do rename.

delcypher marked 3 inline comments as not done.Jan 22 2021, 12:58 PM
delcypher updated this revision to Diff 318712.Jan 22 2021, 6:11 PM

Address first round of feedback.

delcypher marked 4 inline comments as done.Jan 22 2021, 6:14 PM
vitalybuka accepted this revision.Jan 22 2021, 7:20 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Jan 22 2021, 7:20 PM
This revision was landed with ongoing or failed builds.Jan 22 2021, 7:27 PM
This revision was automatically updated to reflect the committed changes.