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

[delcypher]

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

[kubamracek]

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

[delcypher]

In D56207#1343971, @kubamracek wrote:

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.

[vitalybuka]

In D56207#1343995, @delcypher wrote:

In D56207#1343971, @kubamracek wrote:

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.

[delcypher]

In D56207#1344528, @vitalybuka wrote:

In D56207#1343995, @delcypher wrote:

In D56207#1343971, @kubamracek wrote:

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)?

[delcypher]

@kcc @vitalybuka Ping.

[vitalybuka]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

How are we going init AllocatorRemoteView in production code?

[delcypher]

Use new RemoteAddressSpaceView::ScopedContext.

[delcypher]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

@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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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

702

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

[delcypher]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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.

702

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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]

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

696

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

without -> "without significant issues"

[delcypher]

• Update tests to use new CopyFromTarget(...) methods.
• Simplify tests to use ThisTASVT<AddressSpaceView> templates.

[vitalybuka]

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

lib/sanitizer_common/tests/sanitizer_allocator_test.cc

 //===-- sanitizer_allocator_test.cc ---------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer/AddressSanitizer runtime.
 // Tests for sanitizer_allocator.h.
 //
 //===----------------------------------------------------------------------===//
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "sanitizer_common/sanitizer_common.h"
+#include "sanitizer_common/sanitizer_remote_address_space_view.h"
 
 #include "sanitizer_test_utils.h"
 #include "sanitizer_pthread_wrappers.h"
 
 #include "gtest/gtest.h"
 
 #include <stdio.h>
 #include <stdlib.h>
TEST(SanitizerCommon, SizeClassAllocator32MapUnmapCallback) {
   a->TestOnlyUnmap();
   EXPECT_EQ(TestMapUnmapCallback::unmap_count, 1);
   delete a;
   // fprintf(stderr, "Map: %d Unmap: %d\n",
   //         TestMapUnmapCallback::map_count,
   //         TestMapUnmapCallback::unmap_count);
 }
 
+template <typename AddressSpaceView>
+using LargeMmapAllocatorASVT =
+    LargeMmapAllocator<NoOpMapUnmapCallback, DefaultLargeMmapAllocatorPtrArray,
+                       AddressSpaceView>;
+
 TEST(SanitizerCommon, LargeMmapAllocatorMapUnmapCallback) {
   TestMapUnmapCallback::map_count = 0;
   TestMapUnmapCallback::unmap_count = 0;
   LargeMmapAllocator<TestMapUnmapCallback> a;
   a.Init();
   AllocatorStats stats;
   stats.Init();
   void *x = a.Allocate(&stats, 1 << 20, 1);
TEST(SanitizerCommon, LargeMmapAllocator) {
   uptr page_size = GetPageSizeCached();
   char *p = (char *)a.Allocate(&stats, page_size, 1);
   CHECK_EQ(p, a.GetBlockBegin(p));
   CHECK_EQ(p, (char *)a.GetBlockBegin(p + page_size - 1));
   CHECK_NE(p, (char *)a.GetBlockBegin(p + page_size));
   a.Deallocate(&stats, p);
 }
 
-template
-<class PrimaryAllocator, class SecondaryAllocator, class AllocatorCache>
+template <template <class> class PrimaryAllocatorASVT,
+          template <class> class SecondaryAllocatorASVT,
+          template <class> class AllocatorCacheT>
 void TestCombinedAllocator() {
-  typedef
-      CombinedAllocator<PrimaryAllocator, AllocatorCache, SecondaryAllocator>
-      Allocator;
+  using AllocatorCache =
+      AllocatorCacheT<PrimaryAllocatorASVT<LocalAddressSpaceView>>;
+  using Allocator = CombinedAllocator<
+      PrimaryAllocatorASVT<LocalAddressSpaceView>, AllocatorCache,
+      SecondaryAllocatorASVT<LocalAddressSpaceView>, LocalAddressSpaceView>;
+  using AllocatorRemoteView = CombinedAllocator<
+      PrimaryAllocatorASVT<RemoteAddressSpaceView>,
+      AllocatorCacheT<PrimaryAllocatorASVT<RemoteAddressSpaceView>>,
+      SecondaryAllocatorASVT<RemoteAddressSpaceView>, RemoteAddressSpaceView>;
   Allocator *a = new Allocator;
   a->Init(kReleaseToOSIntervalNever);
   std::mt19937 r;
 
   AllocatorCache cache;
   memset(&cache, 0, sizeof(cache));
   a->InitCache(&cache);
 
for (uptr i = 0; i < kNumAllocs; i++) {
       allocated.push_back(x);
     }
 
     std::shuffle(allocated.begin(), allocated.end(), r);
 
     // Test ForEachChunk(...)
     {
       std::set<void *> reported_chunks;
+      std::set<void *> reported_chunks_remote_view;
       auto cb = [](uptr chunk, void *arg) {
         auto reported_chunks_ptr = reinterpret_cast<std::set<void *> *>(arg);
         auto pair =
             reported_chunks_ptr->insert(reinterpret_cast<void *>(chunk));
         // Check chunk is never reported more than once.
         ASSERT_TRUE(pair.second);
       };
       a->ForEachChunk(cb, reinterpret_cast<void *>(&reported_chunks));
+
+      // Test the RemoteAddressSpaceView functionality of the allocator.
+      {
+        // We need the data layout when instantiated with LocalAddressSpaceView
+        // or RemoteAddressSpaceView to be identical. Checking all of this is
+        // cumbersome and not maintainable so just check for size mismatch for
+        // now.
+        static_assert(sizeof(Allocator) == sizeof(AllocatorRemoteView),
+                      "size mismatch");
+        AllocatorRemoteView *a_remote_view =
+            reinterpret_cast<AllocatorRemoteView *>(a);
+        // Create in-process VMReadContext object.
+        ScopedRemoteAddressSpaceViewContext remote_view_context;
+
+        // Now try to enumerate the chunks again but this time using the
+        // `RemoteAddressSpaceView` implementation.
+        a_remote_view->ForEachChunk(
+            cb, reinterpret_cast<void *>(&reported_chunks_remote_view));
+      }
+
       for (const auto &allocated_ptr : allocated) {
         ASSERT_NE(reported_chunks.find(allocated_ptr), reported_chunks.end());
+        ASSERT_NE(reported_chunks_remote_view.find(allocated_ptr),
+                  reported_chunks_remote_view.end());
       }
     }
 
     for (uptr i = 0; i < kNumAllocs; i++) {
       void *x = allocated[i];
       uptr *meta = reinterpret_cast<uptr*>(a->GetMetaData(x));
       CHECK_NE(*meta, 0);
       CHECK(a->PointerIsMine(x));
       *meta = 0;
       a->Deallocate(&cache, x);
     }
     allocated.clear();
     a->SwallowCache(&cache);
   }
   a->DestroyCache(&cache);
   a->TestOnlyUnmap();
 }
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
 TEST(SanitizerCommon, CombinedAllocator64) {
-  TestCombinedAllocator<Allocator64,
-      LargeMmapAllocator<>,
-      SizeClassAllocatorLocalCache<Allocator64> > ();
+  TestCombinedAllocator<Allocator64ASVT, LargeMmapAllocatorASVT,
+                        SizeClassAllocatorLocalCache>();
 }
 
 TEST(SanitizerCommon, CombinedAllocator64Dynamic) {
-  TestCombinedAllocator<Allocator64Dynamic,
-      LargeMmapAllocator<>,
-      SizeClassAllocatorLocalCache<Allocator64Dynamic> > ();
+  TestCombinedAllocator<Allocator64DynamicASVT, LargeMmapAllocatorASVT,
+                        SizeClassAllocatorLocalCache>();
 }
 
 #if !SANITIZER_ANDROID
 TEST(SanitizerCommon, CombinedAllocator64Compact) {
-  TestCombinedAllocator<Allocator64Compact,
-      LargeMmapAllocator<>,
-      SizeClassAllocatorLocalCache<Allocator64Compact> > ();
+  TestCombinedAllocator<Allocator64CompactASVT, LargeMmapAllocatorASVT,
+                        SizeClassAllocatorLocalCache>();
 }
 #endif
 
 TEST(SanitizerCommon, CombinedAllocator64VeryCompact) {
-  TestCombinedAllocator<Allocator64VeryCompact,
-      LargeMmapAllocator<>,
-      SizeClassAllocatorLocalCache<Allocator64VeryCompact> > ();
+  TestCombinedAllocator<Allocator64VeryCompactASVT, LargeMmapAllocatorASVT,
+                        SizeClassAllocatorLocalCache>();
 }
 #endif
 
 TEST(SanitizerCommon, CombinedAllocator32Compact) {
-  TestCombinedAllocator<Allocator32Compact,
-      LargeMmapAllocator<>,
-      SizeClassAllocatorLocalCache<Allocator32Compact> > ();
+  TestCombinedAllocator<Allocator32CompactASVT, LargeMmapAllocatorASVT,
+                        SizeClassAllocatorLocalCache>();
 }
 
 template <class AllocatorCache>
 void TestSizeClassAllocatorLocalCache() {
   AllocatorCache cache;
   typedef typename AllocatorCache::Allocator Allocator;
   Allocator *a = new Allocator();
 
for (int i = 0; i < kSize; i++) {
     EXPECT_EQ('c', char_buf[i]);
   }
 }
 
 void IterationTestCallback(uptr chunk, void *arg) {
   reinterpret_cast<std::set<uptr> *>(arg)->insert(chunk);
 }
 
-template <class Allocator>
+template <template <class> class AllocatorASVT>
 void TestSizeClassAllocatorIteration() {
+  using Allocator = AllocatorASVT<LocalAddressSpaceView>;
+  using AllocatorRemoteView = AllocatorASVT<RemoteAddressSpaceView>;
   Allocator *a = new Allocator;
   a->Init(kReleaseToOSIntervalNever);
   SizeClassAllocatorLocalCache<Allocator> cache;
   memset(&cache, 0, sizeof(cache));
   cache.Init(0);
 
   static const uptr sizes[] = {1, 16, 30, 40, 100, 1000, 10000,
     50000, 60000, 100000, 120000, 300000, 500000, 1000000, 2000000};
for (uptr s = 0; s < ARRAY_SIZE(sizes); s++) {
     for (uptr j = 0; j < n_iter; j++) {
       uptr class_id0 = Allocator::SizeClassMapT::ClassID(size);
       void *x = cache.Allocate(a, class_id0);
       allocated.push_back(x);
     }
   }
 
   std::set<uptr> reported_chunks;
+  std::set<uptr> reported_chunks_remote_view;
   a->ForceLock();
   a->ForEachChunk(IterationTestCallback, &reported_chunks);
   a->ForceUnlock();
 
+  // Test the RemoteAddressSpaceView functionality of the
+  // allocator. This is done outside the lock because in the
+  // RemoteAddressSpaceView case there is no guarantee that the remote allocator
+  // is locked.
+  {
+    // We need the data layout when instantiated with LocalAddressSpaceView
+    // or RemoteAddressSpaceView to be identical. Checking all of this is
+    // cumbersome and not maintainable so just check for size mismatch for
+    // now.
+    static_assert(sizeof(Allocator) == sizeof(AllocatorRemoteView),
+                  "size mismatch");
+    AllocatorRemoteView *a_remote_view =
+        reinterpret_cast<AllocatorRemoteView *>(a);
+
+    // Create in-process VMReadContext object.
+    ScopedRemoteAddressSpaceViewContext remote_view_context;
+
+    // Now try to enumerate the chunks again but this time using the
+    // `RemoteAddressSpaceView` implementation.
+    a_remote_view->ForEachChunk(IterationTestCallback,
+                                &reported_chunks_remote_view);
+  }
+
   for (uptr i = 0; i < allocated.size(); i++) {
     // Don't use EXPECT_NE. Reporting the first mismatch is enough.
     ASSERT_NE(reported_chunks.find(reinterpret_cast<uptr>(allocated[i])),
               reported_chunks.end());
+    ASSERT_NE(
+        reported_chunks_remote_view.find(reinterpret_cast<uptr>(allocated[i])),
+        reported_chunks_remote_view.end());
   }
 
   a->TestOnlyUnmap();
   delete a;
 }
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
 // These tests can fail on Windows if memory is somewhat full and lit happens
 // to run them all at the same time. FIXME: Make them not flaky and reenable.
 #if !SANITIZER_WINDOWS
 TEST(SanitizerCommon, SizeClassAllocator64Iteration) {
-  TestSizeClassAllocatorIteration<Allocator64>();
+  TestSizeClassAllocatorIteration<Allocator64ASVT>();
 }
 TEST(SanitizerCommon, SizeClassAllocator64DynamicIteration) {
-  TestSizeClassAllocatorIteration<Allocator64Dynamic>();
+  TestSizeClassAllocatorIteration<Allocator64DynamicASVT>();
 }
 #endif
 #endif
 
 TEST(SanitizerCommon, SizeClassAllocator32Iteration) {
-  TestSizeClassAllocatorIteration<Allocator32Compact>();
+  TestSizeClassAllocatorIteration<Allocator32CompactASVT>();
 }
 
 TEST(SanitizerCommon, LargeMmapAllocatorIteration) {
-  LargeMmapAllocator<NoOpMapUnmapCallback> a;
+  LargeMmapAllocatorASVT<LocalAddressSpaceView> a;
+  using LargeMmapAllocatorRemoteView =
+      LargeMmapAllocatorASVT<RemoteAddressSpaceView>;
   a.Init();
   AllocatorStats stats;
   stats.Init();
 
   static const uptr kNumAllocs = 1000;
   char *allocated[kNumAllocs];
   static const uptr size = 40;
   // Allocate some.
   for (uptr i = 0; i < kNumAllocs; i++)
     allocated[i] = (char *)a.Allocate(&stats, size, 1);
 
   std::set<uptr> reported_chunks;
+  std::set<uptr> reported_chunks_remote_view;
   a.ForceLock();
   a.ForEachChunk(IterationTestCallback, &reported_chunks);
   a.ForceUnlock();
 
+  // Test the RemoteAddressSpaceView functionality of the allocator.
+  {
+    // We need the data layout when instantiated with LocalAddressSpaceView
+    // or RemoteAddressSpaceView to be identical. Checking all of this is
+    // cumbersome and not maintainable so just check for size mismatch for
+    // now.
+    static_assert(sizeof(a) == sizeof(LargeMmapAllocatorRemoteView),
+                  "size mismatch");
+    LargeMmapAllocatorRemoteView *a_remote_view =
+        reinterpret_cast<LargeMmapAllocatorRemoteView *>(&a);
+
+    // Create in-process VMReadContext object.
+    ScopedRemoteAddressSpaceViewContext remote_view_context;
+
+    // Now try to enumerate the chunks again but this time using the
+    // `RemoteAddressSpaceView` implementation.
+    a_remote_view->ForEachChunk(
+        IterationTestCallback,
+        reinterpret_cast<void *>(&reported_chunks_remote_view));
+  }
+
   for (uptr i = 0; i < kNumAllocs; i++) {
     // Don't use EXPECT_NE. Reporting the first mismatch is enough.
     ASSERT_NE(reported_chunks.find(reinterpret_cast<uptr>(allocated[i])),
               reported_chunks.end());
+    ASSERT_NE(
+        reported_chunks_remote_view.find(reinterpret_cast<uptr>(allocated[i])),
+        reported_chunks_remote_view.end());
   }
   for (uptr i = 0; i < kNumAllocs; i++)
     a.Deallocate(&stats, allocated[i]);
 }
 
 TEST(SanitizerCommon, LargeMmapAllocatorBlockBegin) {
   LargeMmapAllocator<NoOpMapUnmapCallback> a;
   a.Init();