Page MenuHomePhabricator

Update allocator unit tests to test the `RemoteAddressSpaceView` template instantiation.
Needs RevisionPublic

Authored by delcypher on Jan 2 2019, 10:42 AM.

Details

Summary

These tests use a VMContext object that is set to read the local
address space. As a consequence these tests don't actually
really test the remote read functionality but they do:

  • Check that the allocators can be instantiated with

RemoteAddressSpaceView as the AddressSpaceView type on all
platforms.

  • Check that in-process enumeration still works correctly when the allocator is instatiated with RemoteAddressSpaceView.
  • Check that the size of the allocators doesn't change when instantiated with LocalAddressSpaceView or RemoteAddressSpaceView.

Ideally we'd check that the entire data layout of the classes hasn't
changed when changing the AddressSpaceView template parameter.
Unfortunately doing that manually would be a significant amount of work
that would hinder maintainability, so for now we just don't check the
data layout.

rdar://problem/45284065

Event Timeline

delcypher created this revision.Jan 2 2019, 10:42 AM
Herald added subscribers: Restricted Project, kubamracek. · View Herald TranscriptJan 2 2019, 10:42 AM

This needs https://reviews.llvm.org/D56206 to land first, right?

Oops. Yes. I forgot to add the dependency in Phabricator. I've done it now.

This needs https://reviews.llvm.org/D56206 to land first, right?

Oops. Yes. I forgot to add the dependency in Phabricator. I've done it now.

Tests suppose to pass after every patch.
Does D56206 work without D56207
If not, you should merge them.

This needs https://reviews.llvm.org/D56206 to land first, right?

Oops. Yes. I forgot to add the dependency in Phabricator. I've done it now.

Tests suppose to pass after every patch.
Does D56206 work without D56207
If not, you should merge them.

Yes, https://reviews.llvm.org/D56206 (the implementation, not this patch) works without https://reviews.llvm.org/D56207 (the tests for the implementation, this patch).

Or did you mean to ask that the other way round (i.e. Does https://reviews.llvm.org/D56207 work without https://reviews.llvm.org/D56206)?

vitalybuka added inline comments.Jan 10 2019, 1:27 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

How are we going init AllocatorRemoteView in production code?

delcypher updated this revision to Diff 181567.Jan 14 2019, 8:50 AM

Use new RemoteAddressSpaceView::ScopedContext.

delcypher marked an inline comment as done.Jan 14 2019, 9:00 AM
delcypher added inline comments.
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

It's not too far from what's in the test. In production we will use VMReadContext::read(...) to copy an in-use allocator from the target sanitizer process into the local process and then perform the reinterpret_cast just like this test does.

We can't write this test to actually use a remote allocator very easily so we just use a local one and check that RemoteAddressSpaceView works as expected with local memory.

A different test that tests the production workflow will be submitted in a different patch.

vitalybuka added inline comments.Jan 16 2019, 2:18 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

I don't like this reinterpret of two complex unrelated classes.
We should probably add code to load needed state from Allocator into AllocatorRemoteView

delcypher marked an inline comment as done.Jan 17 2019, 11:51 AM
delcypher added inline comments.
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

@vitalybuka I'm not a huge fan of it either but it's a consequence of the design @kcc, @kubmracek and myself settled on. Originally AddressSpaceView (moral equivalent, it wasn't called that back then) was not an allocator parameter and was instead a template parameter to every relevant method. This meant that we didn't have a different type for the in-process and out-of-process allocator copy.

The current design makes AddressSpaceView an allocator parameter which means there are two different types (one instantiated with LocalAddressSpaceView and RemoteAddressSpaceView).

We should probably add code to load needed state from Allocator into AllocatorRemoteView

If you mean a function that would encapsulate the load (but it's implementation is just loading the object with a single LoadWritable() call and then does reinterpret_cast<..>) that's fine. If you want to do it with multiple Load()/LoadWritable() calls then that isn't going to work very well because the load functions don't give any guarantees about where memory is copied to in the local process and we might not be able to make a contiguous object from the individual copies without doing lots more copying into a large buffer.

There isn't much we can do about ABI compatibility here because we don't know what the target processes's view of the Allocator<LocalAddressSpace> type looks like. It could be different and we have no real way of knowing.

In my implementation (later patches) we have a ABI version field that is stored elsewhere (it's meant for the enumeration process as a whole. Because ABI layout is also important for other types it is done elsewhere) that we can get from the target process. If the local ABI version and target ABI version don't match we abort trying to enumerate the allocator's allocations.

vitalybuka added inline comments.Jan 17 2019, 1:08 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

If you mean a function that would encapsulate the load (but it's implementation is just loading the object with a single LoadWritable() call and then does reinterpret_cast<..>) that's fine. If you want to do it with multiple Load()/LoadWritable() calls then that isn't going to work very well because the load functions don't give any guarantees about where memory is copied to in the local process and we might not be able to make a contiguous object from the individual copies without doing lots more copying into a large buffer.

There isn't much we can do about ABI compatibility here because we don't know what the target processes's view of the Allocator<LocalAddressSpace> type looks like. It could be different and we have no real way of knowing.

I mean

  1. Load() remote Allocator into local Allocator a;
  2. Create empty AllocatorRemoteView arv;
  3. Copy/Move necessary data from a to arv, field by field.
delcypher marked an inline comment as done and an inline comment as not done.Jan 18 2019, 8:54 AM
delcypher added inline comments.
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

I mean

  1. Load() remote Allocator into local Allocator a;
  2. Create empty AllocatorRemoteView arv;
  3. Copy/Move necessary data from a to arv, field by field.

I was worried that you meant this. Is this really necessary? This is going to add a bunch execution time overhead because I'm going to have to call Load() and then internal_memcpy() for every single field in the allocator (recursively for each data structure that takes AddressSpaceView as a template parameter). It's also going to add memory overhead because I there will be two copies of the allocator in memory (the Allocator<LocalAddressSpaceView> that we copy from and the Allocator<RemoteAddressSpaceView> we copy into).

This seems pretty unnecessary given that Allocator<LocalAddressSpaceView> and Allocator<RemoteAddressSpaceView> should have the same data layout (the AddressSpaceView parameter is never used in a way that would change the data layout).

This is also going to be a maintenance pain because these copy functions will have to be updated every time a new field is added to the allocator.

vitalybuka added inline comments.Jan 18 2019, 10:37 AM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

I should but it's not necessary would, even if size match.
And can cause very hard to understand bugs.

679

Also I missed how remote process will avoid data-races on chunks data?
On a first look you can get partially initialized chunk.

delcypher marked 2 inline comments as done.Jan 22 2019, 10:49 AM
delcypher added inline comments.
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

Okay. You've convinced me. I don't like very hard to understand bugs.

I'm going to write the change as a separate patch and insert before this one in the stack of patches to review.

679

Also I missed how remote process will avoid data-races on chunks data?

We won't unfortunately. This is an issue I've brought up with the implementers of the leaks tool on Darwin and apparently it has never been a problem in practice. I'm not entirely convinced by this but this is not something we can fix from the sanitizer side alone.

For now we will just have to accept it until leaks provides a mechanism that the sanitizers can use to be notified that they need to lock the allocators prior to allocator enumeration. Just to be clear though. During allocator enumeration the target process is frozen so the allocator data structures won't change. However there is no guarantee that the allocator wasn't in the middle of doing something when the process was frozen, so it's still racey.

For more context take a look at one of the future patches https://reviews.llvm.org/D56995, look for should_lock_allocator. That explains the current situation in more depth.

vitalybuka added inline comments.Jan 22 2019, 1:20 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

I suspect our problem is just SizeClassAllocator64::ForEachChunk
if you move it out of the SizeClassAllocator64, e.g. into standalone function or into combined allocator
then SizeClassAllocator64 will not depend on AddressSpaceView and
SizeClassAllocator64 local and remote types will be the same so it would be ok to just reinterpret or memcpy them.

delcypher marked an inline comment as done.Jan 24 2019, 4:03 AM
delcypher added inline comments.
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

That might be over-complicating things. I don't like the idea of moving code away from where it belongs.

I think we should go for a simple solution first. If it proves to have poor performance we can revisit the design.

vitalybuka added inline comments.Jan 25 2019, 9:59 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

Actually it's getting simpler: https://reviews.llvm.org/D57279
Here I've rebased most of your patches without https://github.com/vitalybuka/llvm-project/commits/master

vitalybuka added inline comments.Jan 25 2019, 10:01 PM
lib/sanitizer_common/tests/sanitizer_allocator_test.cc
673

Here I've rebased most of your patches without https://github.com/vitalybuka/llvm-project/commits/master

without -> "without significant issues"

  • Update tests to use new CopyFromTarget(...) methods.
  • Simplify tests to use ThisTASVT<AddressSpaceView> templates.
vitalybuka requested changes to this revision.Wed, Apr 24, 11:14 AM

Removing stale patches from the "Ready to Review" list.

This revision now requires changes to proceed.Wed, Apr 24, 11:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Apr 24, 11:14 AM