Reland "[D8] Clean up ArrayBuffer Allocators in shell."

This is a reland of 0c2faa06
Original change's description:
> [D8] Clean up ArrayBuffer Allocators in shell.
>
> - Reworks the class hierarchy in d8.cc to conform to the allocator API.
>   In particular, allocators should malloc/free or equivalent unless
>   v8::ArrayBuffer::Allocator::Reserve is called.
> - Modifies ExternalizedContents to remember the allocation mode.
> - ArrayAllocatorBase now tracks its allocations to make sure it doesn't
>   call Free on externalized array buffers it didn't allocate.
>
> Bug: chromium:756050
> Change-Id: Ic2d07d36358f1b4fa542bea27f93d1d51a1757e1
> Reviewed-on: https://chromium-review.googlesource.com/807355
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Ben Titzer <titzer@chromium.org>
> Reviewed-by: Eric Holk <eholk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49893}

Bug: chromium:756050,v8:7146
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I8fe3a9d9af43196e16d02342a47347a76c0a1341
Reviewed-on: https://chromium-review.googlesource.com/811724Reviewed-by: Eric Holk <eholk@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49945}

src/api.cc

@@ -8221,14 +8221,14 @@ v8::SharedArrayBuffer::Contents v8::SharedArrayBuffer::GetContents() {
   i::Handle<i::JSArrayBuffer> self = Utils::OpenHandle(this);
   size_t byte_length = static_cast<size_t>(self->byte_length()->Number());
   Contents contents;
+  contents.allocation_base_ = self->allocation_base();
+  contents.allocation_length_ = self->allocation_length();
+  contents.allocation_mode_ =
+      self->has_guard_region()
+          ? ArrayBufferAllocator::Allocator::AllocationMode::kReservation
+          : ArrayBufferAllocator::Allocator::AllocationMode::kNormal;
   contents.data_ = self->backing_store();
   contents.byte_length_ = byte_length;
-  // SharedArrayBuffers never have guard regions, so their allocation and data
-  // are equivalent.
-  contents.allocation_base_ = self->backing_store();
-  contents.allocation_length_ = byte_length;
-  contents.allocation_mode_ =
-      ArrayBufferAllocator::Allocator::AllocationMode::kNormal;
   return contents;
 }
 

src/d8.cc

@@ -66,136 +66,100 @@ namespace v8 {
 
 namespace {
 
-const int MB = 1024 * 1024;
-const int kMaxWorkers = 50;
-const int kMaxSerializerMemoryUsage = 1 * MB;  // Arbitrary maximum for testing.
+const int kMB = 1024 * 1024;
+const size_t kTwoGB = 2u * 1024u * 1024u * 1024u;
 
-#define USE_VM 1
-#define VM_THRESHOLD 65536
-// TODO(titzer): allocations should fail if >= 2gb because of
-// array buffers storing the lengths as a SMI internally.
-#define TWO_GB (2u * 1024u * 1024u * 1024u)
+const int kMaxWorkers = 50;
+const int kMaxSerializerMemoryUsage =
+    1 * kMB;  // Arbitrary maximum for testing.
 
-// Forwards memory reservation and protection functions to the V8 default
-// allocator. Used by ShellArrayBufferAllocator and MockArrayBufferAllocator.
+// The ArrayBuffer allocator for the shell. It implements kNormal allocations
+// and delegates kReservation allocations to the default V8 allocator.
 class ArrayBufferAllocatorBase : public v8::ArrayBuffer::Allocator {
-  std::unique_ptr<Allocator> allocator_ =
-      std::unique_ptr<Allocator>(NewDefaultAllocator());
-
  public:
-  void* Reserve(size_t length) override { return allocator_->Reserve(length); }
-
-  void Free(void*, size_t) override = 0;
-
-  void Free(void* data, size_t length, AllocationMode mode) override {
-    switch (mode) {
-      case AllocationMode::kNormal: {
-        return Free(data, length);
-      }
-      case AllocationMode::kReservation: {
-        return allocator_->Free(data, length, mode);
-      }
-    }
-  }
-
-  void SetProtection(void* data, size_t length,
-                     Protection protection) override {
-    allocator_->SetProtection(data, length, protection);
-  }
-};
+  virtual ~ArrayBufferAllocatorBase() = default;
 
-class ShellArrayBufferAllocator : public ArrayBufferAllocatorBase {
- public:
   void* Allocate(size_t length) override {
-#if USE_VM
-    if (RoundToPageSize(&length)) {
-      void* data = VirtualMemoryAllocate(length);
-#if DEBUG
-      if (data) {
-        // In debug mode, check the memory is zero-initialized.
-        size_t limit = length / sizeof(uint64_t);
-        uint64_t* ptr = reinterpret_cast<uint64_t*>(data);
-        for (size_t i = 0; i < limit; i++) {
-          DCHECK_EQ(0u, ptr[i]);
-        }
-      }
+    size_t alloc_length = GetAllocLength(length);
+    // TODO(titzer): allocations should fail if >= 2gb because array buffers
+    // store their lengths as a SMI internally.
+    if (alloc_length > kTwoGB) return nullptr;
+#if V8_OS_AIX && _LINUX_SOURCE_COMPAT
+    // Work around for GCC bug on AIX
+    // See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79839
+    void* data = __linux_calloc(alloc_length, 1);
+#else
+    void* data = calloc(alloc_length, 1);
 #endif
+    MSAN_MEMORY_IS_INITIALIZED(data, alloc_length);
     return data;
   }
-#endif
-    void* data = AllocateUninitialized(length);
-    return data == nullptr ? data : memset(data, 0, length);
-  }
+
   void* AllocateUninitialized(size_t length) override {
-#if USE_VM
-    if (RoundToPageSize(&length)) return VirtualMemoryAllocate(length);
-#endif
-// Work around for GCC bug on AIX
-// See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79839
+    size_t alloc_length = GetAllocLength(length);
+    // TODO(titzer): allocations should fail if >= 2gb because array buffers
+    // store their lengths as a SMI internally.
+    if (alloc_length > kTwoGB) return nullptr;
 #if V8_OS_AIX && _LINUX_SOURCE_COMPAT
-    return __linux_malloc(length);
+    // Work around for GCC bug on AIX
+    // See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79839
+    void* data = __linux_malloc(alloc_length);
 #else
-    return malloc(length);
+    void* data = malloc(alloc_length);
 #endif
+    return data;
   }
-  using ArrayBufferAllocatorBase::Free;
+
   void Free(void* data, size_t length) override {
-#if USE_VM
-    if (RoundToPageSize(&length)) {
-      CHECK(base::OS::Free(data, length));
-      return;
-    }
-#endif
     free(data);
   }
-  // If {length} is at least {VM_THRESHOLD}, round up to next page size and
-  // return {true}. Otherwise return {false}.
-  bool RoundToPageSize(size_t* length) {
-    size_t page_size = base::OS::AllocatePageSize();
-    if (*length >= VM_THRESHOLD && *length < TWO_GB) {
-      *length = RoundUp(*length, page_size);
-      return true;
+
+  void Free(void* data, size_t length, AllocationMode mode) override {
+    size_t alloc_length = GetAllocLength(length);
+    if (mode == AllocationMode::kNormal) {
+      Free(data, alloc_length);
+    } else {
+      allocator_->Free(data, alloc_length, mode);
     }
-    return false;
   }
-#if USE_VM
-  void* VirtualMemoryAllocate(size_t length) {
-    size_t page_size = base::OS::AllocatePageSize();
-    size_t alloc_size = RoundUp(length, page_size);
-    void* address = base::OS::Allocate(nullptr, alloc_size, page_size,
-                                       base::OS::MemoryPermission::kReadWrite);
-    if (address != nullptr) {
-#if defined(LEAK_SANITIZER)
-      __lsan_register_root_region(address, alloc_size);
-#endif
-      MSAN_MEMORY_IS_INITIALIZED(address, alloc_size);
+
+  void* Reserve(size_t length) override {
+    void* data = allocator_->Reserve(GetAllocLength(length));
+    return data;
   }
-    return address;
+
+  void SetProtection(void* data, size_t length,
+                     Protection protection) override {
+    allocator_->SetProtection(data, length, protection);
   }
-#endif
+
+ private:
+  virtual size_t GetAllocLength(size_t length) const = 0;
+
+  std::unique_ptr<Allocator> allocator_ =
+      std::unique_ptr<Allocator>(NewDefaultAllocator());
 };
 
-class MockArrayBufferAllocator : public ArrayBufferAllocatorBase {
-  const size_t kAllocationLimit = 10 * MB;
-  size_t get_actual_length(size_t length) const {
-    return length > kAllocationLimit ? base::OS::AllocatePageSize() : length;
+class ShellArrayBufferAllocator : public ArrayBufferAllocatorBase {
+ public:
+  virtual ~ShellArrayBufferAllocator() = default;
+
+ private:
+  virtual size_t GetAllocLength(size_t length) const {
+    size_t page_size = base::OS::AllocatePageSize();
+    size_t alloc_size = RoundUp(length, page_size);
+    return alloc_size;
   }
+};
 
+class MockArrayBufferAllocator : public ArrayBufferAllocatorBase {
  public:
-  void* Allocate(size_t length) override {
-    const size_t actual_length = get_actual_length(length);
-    void* data = AllocateUninitialized(actual_length);
-    return data == nullptr ? data : memset(data, 0, actual_length);
-  }
-  void* AllocateUninitialized(size_t length) override {
-    return malloc(get_actual_length(length));
-  }
-  void Free(void* p, size_t) override { free(p); }
-  void Free(void* data, size_t length, AllocationMode mode) override {
-    ArrayBufferAllocatorBase::Free(data, get_actual_length(length), mode);
-  }
-  void* Reserve(size_t length) override {
-    return ArrayBufferAllocatorBase::Reserve(get_actual_length(length));
+  virtual ~MockArrayBufferAllocator() = default;
+
+ private:
+  size_t GetAllocLength(size_t length) const override {
+    const size_t kAllocationLimit = 10 * kMB;
+    return length > kAllocationLimit ? base::OS::AllocatePageSize() : length;
   }
 };
 
@@ -300,7 +264,7 @@ base::Thread::Options GetThreadOptions(const char* name) {
   // which is not enough to parse the big literal expressions used in tests.
   // The stack size should be at least StackGuard::kLimitSize + some
   // OS-specific padding for thread startup code.  2Mbytes seems to be enough.
-  return base::Thread::Options(name, 2 * MB);
+  return base::Thread::Options(name, 2 * kMB);
 }
 
 }  // namespace
@@ -2570,7 +2534,9 @@ void SourceGroup::JoinThread() {
 }
 
 ExternalizedContents::~ExternalizedContents() {
-  Shell::array_buffer_allocator->Free(data_, size_);
+  if (base_ != nullptr) {
+    Shell::array_buffer_allocator->Free(base_, length_, mode_);
+  }
 }
 
 void SerializationDataQueue::Enqueue(std::unique_ptr<SerializationData> data) {
@@ -3479,3 +3445,6 @@ int main(int argc, char* argv[]) {
   return v8::Shell::Main(argc, argv);
 }
 #endif
+
+#undef CHECK
+#undef DCHECK

src/d8.h

@@ -149,28 +149,36 @@ class SourceGroup {
 class ExternalizedContents {
  public:
   explicit ExternalizedContents(const ArrayBuffer::Contents& contents)
-      : data_(contents.Data()), size_(contents.ByteLength()) {}
+      : base_(contents.AllocationBase()),
+        length_(contents.AllocationLength()),
+        mode_(contents.AllocationMode()) {}
   explicit ExternalizedContents(const SharedArrayBuffer::Contents& contents)
-      : data_(contents.Data()), size_(contents.ByteLength()) {}
+      : base_(contents.AllocationBase()),
+        length_(contents.AllocationLength()),
+        mode_(contents.AllocationMode()) {}
   ExternalizedContents(ExternalizedContents&& other)
-      : data_(other.data_), size_(other.size_) {
-    other.data_ = nullptr;
-    other.size_ = 0;
+      : base_(other.base_), length_(other.length_), mode_(other.mode_) {
+    other.base_ = nullptr;
+    other.length_ = 0;
+    other.mode_ = ArrayBuffer::Allocator::AllocationMode::kNormal;
   }
   ExternalizedContents& operator=(ExternalizedContents&& other) {
     if (this != &other) {
-      data_ = other.data_;
-      size_ = other.size_;
-      other.data_ = nullptr;
-      other.size_ = 0;
+      base_ = other.base_;
+      length_ = other.length_;
+      mode_ = other.mode_;
+      other.base_ = nullptr;
+      other.length_ = 0;
+      other.mode_ = ArrayBuffer::Allocator::AllocationMode::kNormal;
     }
     return *this;
   }
   ~ExternalizedContents();
 
  private:
-  void* data_;
-  size_t size_;
+  void* base_;
+  size_t length_;
+  ArrayBuffer::Allocator::AllocationMode mode_;
 
   DISALLOW_COPY_AND_ASSIGN(ExternalizedContents);
 };