[PATCH] D56207: Update allocator unit tests to test the `RemoteAddressSpaceView` template instantiation.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done and an inline comment as not done.
delcypher added inline comments.


================
Comment at: lib/sanitizer_common/tests/sanitizer_allocator_test.cc:696
+        AllocatorRemoteView *a_remote_view =
+            reinterpret_cast<AllocatorRemoteView *>(a);
+        // Create in-process VMReadContext object.
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > delcypher wrote:
> > > > vitalybuka wrote:
> > > > > How are we going init AllocatorRemoteView in production code?
> > > > 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.
> > > I don't like this reinterpret of two complex unrelated classes.
> > > We should probably add code to load needed state from Allocator into AllocatorRemoteView
> > @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. 
> > 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.
> 
> 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.



Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56207