From eada51757926c382bff6900a2b1f705be3e2a0b1 Mon Sep 17 00:00:00 2001 From: Bill Budge Date: Sat, 6 Jan 2018 00:16:14 +0000 Subject: [PATCH] Revert "[wasm] use allocation tracker to track reserved address space" This reverts commit 9c79b37aa7e94e7d40a92fd4b2b3fa49ff38b1a8. Reason for revert: breaks TSAN https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.v8%2FV8_Linux64_TSAN%2F18959%2F%2B%2Frecipes%2Fsteps%2FCheck%2F0%2Flogs%2Finstance-gc%2F0 Original change's description: > [wasm] use allocation tracker to track reserved address space > > This is a step towards falling back on bounds checks when there are too many > guarded Wasm memories. > > Bug: v8:7143 > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng > Change-Id: I01916cbdd5ddb08fe1d946ab83b801f37a8fe1c6 > Reviewed-on: https://chromium-review.googlesource.com/832944 > Commit-Queue: Eric Holk > Reviewed-by: Bill Budge > Cr-Commit-Position: refs/heads/master@{#50390} TBR=bbudge@chromium.org,gdeepti@chromium.org,eholk@chromium.org,eholk@google.com Change-Id: I207b9466377ba50be17794e71407b0ebc8eb88e2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:7143 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng Reviewed-on: https://chromium-review.googlesource.com/853140 Reviewed-by: Bill Budge Commit-Queue: Bill Budge Cr-Commit-Position: refs/heads/master@{#50392} --- src/api.cc | 16 ------- src/objects.cc | 8 ---- src/wasm/wasm-engine.h | 8 +--- src/wasm/wasm-memory.cc | 75 +++++++----------------------- src/wasm/wasm-memory.h | 22 +-------- test/mjsunit/wasm/module-memory.js | 23 --------- 6 files changed, 20 insertions(+), 132 deletions(-) diff --git a/src/api.cc b/src/api.cc index 21504df6ae..218e0e1736 100644 --- a/src/api.cc +++ b/src/api.cc @@ -7645,14 +7645,6 @@ v8::ArrayBuffer::Contents v8::ArrayBuffer::Externalize() { Utils::ApiCheck(!self->is_external(), "v8_ArrayBuffer_Externalize", "ArrayBuffer already externalized"); self->set_is_external(true); - if (self->has_guard_region()) { - // Since this is being externalized, the Wasm Allocation Tracker can no - // longer track it. - // - // TODO(eholk): Find a way to track this across externalization - isolate->wasm_engine()->allocation_tracker()->ReleaseAddressSpace( - self->allocation_length()); - } isolate->heap()->UnregisterArrayBuffer(*self); return GetContents(); @@ -7868,14 +7860,6 @@ v8::SharedArrayBuffer::Contents v8::SharedArrayBuffer::Externalize() { Utils::ApiCheck(!self->is_external(), "v8_SharedArrayBuffer_Externalize", "SharedArrayBuffer already externalized"); self->set_is_external(true); - if (self->has_guard_region()) { - // Since this is being externalized, the Wasm Allocation Tracker can no - // longer track it. - // - // TODO(eholk): Find a way to track this across externalization - isolate->wasm_engine()->allocation_tracker()->ReleaseAddressSpace( - self->allocation_length()); - } isolate->heap()->UnregisterArrayBuffer(*self); return GetContents(); } diff --git a/src/objects.cc b/src/objects.cc index 7f9c33baa1..bcfea15ac2 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -73,7 +73,6 @@ #include "src/trap-handler/trap-handler.h" #include "src/unicode-cache-inl.h" #include "src/utils-inl.h" -#include "src/wasm/wasm-engine.h" #include "src/wasm/wasm-objects.h" #include "src/zone/zone.h" @@ -18959,13 +18958,6 @@ void JSArrayBuffer::FreeBackingStore() { // static void JSArrayBuffer::FreeBackingStore(Isolate* isolate, Allocation allocation) { - if (allocation.mode == ArrayBuffer::Allocator::AllocationMode::kReservation) { - // TODO(eholk): check with WasmAllocationTracker to make sure this is - // actually a buffer we are tracking. - isolate->wasm_engine()->allocation_tracker()->ReleaseAddressSpace( - allocation.length); - } - isolate->array_buffer_allocator()->Free(allocation.allocation_base, allocation.length, allocation.mode); } diff --git a/src/wasm/wasm-engine.h b/src/wasm/wasm-engine.h index 665663173f..d267a2ec40 100644 --- a/src/wasm/wasm-engine.h +++ b/src/wasm/wasm-engine.h @@ -9,13 +9,14 @@ #include "src/wasm/compilation-manager.h" #include "src/wasm/wasm-code-manager.h" -#include "src/wasm/wasm-memory.h" namespace v8 { namespace internal { namespace wasm { +class CompilationManager; + // The central data structure that represents an engine instance capable of // loading, instantiating, and executing WASM code. class WasmEngine { @@ -27,14 +28,9 @@ class WasmEngine { WasmCodeManager* code_manager() const { return code_manager_.get(); } - WasmAllocationTracker* allocation_tracker() { return &allocation_tracker_; } - private: CompilationManager compilation_manager_; std::unique_ptr code_manager_; - WasmAllocationTracker allocation_tracker_; - - DISALLOW_COPY_AND_ASSIGN(WasmEngine); }; } // namespace wasm diff --git a/src/wasm/wasm-memory.cc b/src/wasm/wasm-memory.cc index 0c3bbfc4ed..1968b7441f 100644 --- a/src/wasm/wasm-memory.cc +++ b/src/wasm/wasm-memory.cc @@ -4,7 +4,6 @@ #include "src/wasm/wasm-memory.h" #include "src/objects-inl.h" -#include "src/wasm/wasm-engine.h" #include "src/wasm/wasm-limits.h" #include "src/wasm/wasm-module.h" @@ -12,70 +11,30 @@ namespace v8 { namespace internal { namespace wasm { -WasmAllocationTracker::~WasmAllocationTracker() { - // All reserved address space should be released before the allocation tracker - // is destroyed. - DCHECK_EQ(allocated_address_space_, 0); -} - -bool WasmAllocationTracker::ReserveAddressSpace(size_t num_bytes) { -// Address space reservations are currently only meaningful using guard -// regions, which is currently only supported on 64-bit systems. On other -// platforms, we always fall back on bounds checks. -#if V8_TARGET_ARCH_64_BIT - static constexpr size_t kAddressSpaceLimit = 0x10000000000L; // 1 TiB - - size_t const new_count = allocated_address_space_ + num_bytes; - DCHECK_GE(new_count, allocated_address_space_); - if (new_count <= kAddressSpaceLimit) { - allocated_address_space_ = new_count; - return true; - } -#endif - return false; -} - -void WasmAllocationTracker::ReleaseAddressSpace(size_t num_bytes) { - DCHECK_LE(num_bytes, allocated_address_space_); - allocated_address_space_ -= num_bytes; -} - void* TryAllocateBackingStore(Isolate* isolate, size_t size, - bool require_guard_regions, - void** allocation_base, - size_t* allocation_length) { - // TODO(eholk): Right now require_guard_regions has no effect on 32-bit + bool enable_guard_regions, void*& allocation_base, + size_t& allocation_length) { + // TODO(eholk): Right now enable_guard_regions has no effect on 32-bit // systems. It may be safer to fail instead, given that other code might do // things that would be unsafe if they expected guard pages where there // weren't any. - if (require_guard_regions) { + if (enable_guard_regions) { // TODO(eholk): On Windows we want to make sure we don't commit the guard // pages yet. // We always allocate the largest possible offset into the heap, so the // addressable memory after the guard page can be made inaccessible. - *allocation_length = RoundUp(kWasmMaxHeapOffset, CommitPageSize()); + allocation_length = RoundUp(kWasmMaxHeapOffset, CommitPageSize()); DCHECK_EQ(0, size % CommitPageSize()); - WasmAllocationTracker* const allocation_tracker = - isolate->wasm_engine()->allocation_tracker(); - - // Let the WasmAllocationTracker know we are going to reserve a bunch of - // address space. - if (!allocation_tracker->ReserveAddressSpace(*allocation_length)) { - // If we are over the address space limit, fail. - return nullptr; - } - // The Reserve makes the whole region inaccessible by default. - *allocation_base = - isolate->array_buffer_allocator()->Reserve(*allocation_length); - if (*allocation_base == nullptr) { - allocation_tracker->ReleaseAddressSpace(*allocation_length); + allocation_base = + isolate->array_buffer_allocator()->Reserve(allocation_length); + if (allocation_base == nullptr) { return nullptr; } - void* memory = *allocation_base; + void* memory = allocation_base; // Make the part we care about accessible. isolate->array_buffer_allocator()->SetProtection( @@ -88,8 +47,8 @@ void* TryAllocateBackingStore(Isolate* isolate, size_t size, } else { void* memory = size == 0 ? nullptr : isolate->array_buffer_allocator()->Allocate(size); - *allocation_base = memory; - *allocation_length = size; + allocation_base = memory; + allocation_length = size; return memory; } } @@ -114,7 +73,7 @@ Handle SetupArrayBuffer(Isolate* isolate, void* allocation_base, } Handle NewArrayBuffer(Isolate* isolate, size_t size, - bool require_guard_regions, + bool enable_guard_regions, SharedFlag shared) { // Check against kMaxInt, since the byte length is stored as int in the // JSArrayBuffer. Note that wasm_max_mem_pages can be raised from the command @@ -128,10 +87,10 @@ Handle NewArrayBuffer(Isolate* isolate, size_t size, void* allocation_base = nullptr; // Set by TryAllocateBackingStore size_t allocation_length = 0; // Set by TryAllocateBackingStore // Do not reserve memory till non zero memory is encountered. - void* memory = (size == 0) ? nullptr - : TryAllocateBackingStore( - isolate, size, require_guard_regions, - &allocation_base, &allocation_length); + void* memory = + (size == 0) ? nullptr + : TryAllocateBackingStore(isolate, size, enable_guard_regions, + allocation_base, allocation_length); if (size > 0 && memory == nullptr) { return Handle::null(); @@ -147,7 +106,7 @@ Handle NewArrayBuffer(Isolate* isolate, size_t size, constexpr bool is_external = false; return SetupArrayBuffer(isolate, allocation_base, allocation_length, memory, - size, is_external, require_guard_regions, shared); + size, is_external, enable_guard_regions, shared); } void ExternalizeMemoryBuffer(Isolate* isolate, Handle buffer, diff --git a/src/wasm/wasm-memory.h b/src/wasm/wasm-memory.h index 449ba955e3..46a83de339 100644 --- a/src/wasm/wasm-memory.h +++ b/src/wasm/wasm-memory.h @@ -13,28 +13,8 @@ namespace v8 { namespace internal { namespace wasm { -class WasmAllocationTracker { - public: - WasmAllocationTracker() {} - ~WasmAllocationTracker(); - - // ReserveAddressSpace attempts to increase the reserved address space counter - // to determine whether there is enough headroom to allocate another guarded - // Wasm memory. Returns true if successful (meaning it is okay to go ahead and - // allocate the buffer), false otherwise. - bool ReserveAddressSpace(size_t num_bytes); - - // Reduces the address space counter so that the space can be reused. - void ReleaseAddressSpace(size_t num_bytes); - - private: - size_t allocated_address_space_ = 0; - - DISALLOW_COPY_AND_ASSIGN(WasmAllocationTracker); -}; - Handle NewArrayBuffer( - Isolate*, size_t size, bool require_guard_regions, + Isolate*, size_t size, bool enable_guard_regions, SharedFlag shared = SharedFlag::kNotShared); Handle SetupArrayBuffer( diff --git a/test/mjsunit/wasm/module-memory.js b/test/mjsunit/wasm/module-memory.js index e9d2bb954d..f5b5981436 100644 --- a/test/mjsunit/wasm/module-memory.js +++ b/test/mjsunit/wasm/module-memory.js @@ -172,26 +172,3 @@ function testOOBThrows() { } testOOBThrows(); - -function testAddressSpaceLimit() { - // 1TiB, see wasm-memory.h - const kMaxAddressSpace = 1 * 1024 * 1024 * 1024 * 1024; - const kAddressSpacePerMemory = 8 * 1024 * 1024 * 1024; - - try { - let memories = []; - let address_space = 0; - while (address_space <= kMaxAddressSpace + 1) { - memories.push(new WebAssembly.Memory({initial: 1})); - address_space += kAddressSpacePerMemory; - } - } catch (e) { - assertTrue(e instanceof RangeError); - return; - } - failWithMessage("allocated too much memory"); -} - -if(%IsWasmTrapHandlerEnabled()) { - testAddressSpaceLimit(); -}