Page MenuHomePhabricator

[asan] Provide interface to iterate over all Fake stack regions
Needs ReviewPublic

Authored by johanengelen on Oct 28 2019, 3:15 PM.

Details

Reviewers
kcc
pcc
Summary

This adds an interface for a program to iterate over all Fake stack regions.
In particular, this enables a garbage collector to correctly observe object references on the Fake stack (the normal stack would be scanned, but the Fake stack cannot be scanned without this added interface).

Two functions are added. One to scan only the provided thread ID's Fake stack, and one to scan all Fake stacks for all threads known to ASan.

Diff Detail

Event Timeline

johanengelen created this revision.Oct 28 2019, 3:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 3:15 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
johanengelen edited the summary of this revision. (Show Details)

Included a testcase now.

kcc added a comment.Dec 30 2019, 4:06 PM

How is this going to work when one thread calls __sanitizer_for_each_extra_stack_range with another thread's ID,
while that other thread creates and discards frames, or while that other thread is being destroyed?

Also, we already have an API used by some implementations of GC.
__asan_get_current_fake_stack & co
Is it not sufficient? Why?

Finally, please explain why you need this. I am reluctant to add functionality that I don't understand the need for.

thanks!

In D69537#1799478, @kcc wrote:

How is this going to work when one thread calls __sanitizer_for_each_extra_stack_range with another thread's ID,
while that other thread creates and discards frames, or while that other thread is being destroyed?

What is assumed is that the other threads have been stopped, indeed that should be added to its documentation. It'd be great if I can acquire a lock such that it is guaranteed that other threads do not modify the fakestack data structures (but that may be hard because IIRC the fake frame code for very small frames is 'inlined' by codegen and won't wait on a lock?). I can remove the os_tid parameter from that function, so it only works on the current thread's fakestack. The other function can then still be used to iterate over _all_ fake stacks of the process (we have a stop-the-world GC).

Also, we already have an API used by some implementations of GC.
__asan_get_current_fake_stack & co
Is it not sufficient? Why?

Can you point to code/text on how other GC's use that interface to scan for pointers over the whole fakestack?
I don't see how the current API gives me access to all the memory locations that are inside fakestacks. __asan_get_current_fake_stack returns a pointer to a FakeStack object, but the definition of that object is not public so I cannot call FakeStack::ForEachFakeFrame. I can use __asan_addr_is_in_fake_stack to get the begin and end of one frame inside the current fakestack, so I could call it many times with an address inside each fakestack frame, but where do I get these addresses from?

Finally, please explain why you need this. I am reluctant to add functionality that I don't understand the need for.

What I need is to be able to scan over all fakestack frames to scan for pointers. It's the same need that LSan has. What I did in this PR is make a public API with similar functionality that LSan implements internally (LSan can do that because it is in the same library as ASan). I need to be able to scan for such pointers from a different thread, i.e. the GC thread should be able to access the fake frame of other threads.

What's ugly in this PR that __sanitizer_for_each_extra_stack_range is a copy (with public interface types) of ForEachExtraStackRange, so that should be fixed.
__sanitizer_for_each_extra_stack_range_all_threads is meant to provide an interface to ThreadRegistry::RunCallbackForEachThreadLocked (and FakeStack::ForEachFakeFrame).

Thanks for your help.
I've been studying the sanitizer code for many hours now trying to find this functionality because I couldn't believe it is not publically available. There must be someone else with the exact same need as me. But judging from how LSan does it, this is the way to do it and it is indeed not publically available yet.

kcc added a comment.Jan 14 2020, 10:36 AM
In D69537#1799478, @kcc wrote:

How is this going to work when one thread calls __sanitizer_for_each_extra_stack_range with another thread's ID,
while that other thread creates and discards frames, or while that other thread is being destroyed?

What is assumed is that the other threads have been stopped,

That's a very important assumption :)

indeed that should be added to its documentation. It'd be great if I can acquire a lock such that it is guaranteed that other threads do not modify the fakestack data structures

I don't think you can do this easily.

(but that may be hard because IIRC the fake frame code for very small frames is 'inlined' by codegen and won't wait on a lock?). I can remove the os_tid parameter from that function, so it only works on the current thread's fakestack.

Yes, you can. But then, what will be the difference between the proposed interface and the existing one?

The other function can then still be used to iterate over _all_ fake stacks of the process (we have a stop-the-world GC).

Also, we already have an API used by some implementations of GC.
__asan_get_current_fake_stack & co
Is it not sufficient? Why?

Can you point to code/text on how other GC's use that interface to scan for pointers over the whole fakestack?

https://cs.chromium.org/search/?q=__asan_get_current_fake_stack&sq=package:chromium&type=cs

I don't see how the current API gives me access to all the memory locations that are inside fakestacks. __asan_get_current_fake_stack returns a pointer to a FakeStack object, but the definition of that object is not public so I cannot call FakeStack::ForEachFakeFrame. I can use __asan_addr_is_in_fake_stack to get the begin and end of one frame inside the current fakestack, so I could call it many times with an address inside each fakestack frame, but where do I get these addresses from?

Finally, please explain why you need this. I am reluctant to add functionality that I don't understand the need for.

What I need is to be able to scan over all fakestack frames to scan for pointers. It's the same need that LSan has. What I did in this PR is make a public API with similar functionality that LSan implements internally (LSan can do that because it is in the same library as ASan). I need to be able to scan for such pointers from a different thread, i.e. the GC thread should be able to access the fake frame of other threads.

What's ugly in this PR that __sanitizer_for_each_extra_stack_range is a copy (with public interface types) of ForEachExtraStackRange, so that should be fixed.
__sanitizer_for_each_extra_stack_range_all_threads is meant to provide an interface to ThreadRegistry::RunCallbackForEachThreadLocked (and FakeStack::ForEachFakeFrame).

Thanks for your help.
I've been studying the sanitizer code for many hours now trying to find this functionality because I couldn't believe it is not publically available. There must be someone else with the exact same need as me. But judging from how LSan does it, this is the way to do it and it is indeed not publically available yet.