Page MenuHomePhabricator

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

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

Details

Summary

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.

rdar://problem/63537240

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
compiler-rt/lib/lsan/lsan_common.cpp
549

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

559

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

compiler-rt/lib/lsan/lsan_common.h
52–53

forward declaration instead of the new include?

146

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

delcypher added inline comments.Jan 22 2021, 12:47 PM
compiler-rt/lib/lsan/lsan_common.cpp
549

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

559

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?

compiler-rt/lib/lsan/lsan_common.h
52–53

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.

146

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.