From b8eeb071c33e2e6bec1764dece7ec2a9d9229b00 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Wed, 11 Mar 2020 11:52:31 +0000 Subject: [PATCH] Revert "[wasm] Do memory.copy bounds check in C++ code" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c475e7046043d0531f8eb767a7627a53cb4495a8. Reason for revert: Fails on MSVC: https://ci.chromium.org/p/v8/builders/ci/V8%20Win64%20-%20msvc/12805 Original change's description: > [wasm] Do memory.copy bounds check in C++ code > > In the existing implementation we first did a bounds check in generated > code, and then called a simple C++ function to do the actual copying. > With this CL now we pass the WasmInstanceObject to the C++ function in > addition to the memory.copy parameters. Thereby we can do the bounds > check in C++, which is much easier, less error prone, and which also > speeds up code generation and reduces code size. Performance should not > be worse, because we were already doing the call to C++ anyways. > > R=​clemensb@chromium.org > > Bug: v8:10281 > Change-Id: I24488d92056f0b5df27a61783a274895bd37cc24 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2093434 > Commit-Queue: Andreas Haas > Reviewed-by: Clemens Backes > Cr-Commit-Position: refs/heads/master@{#66655} TBR=ahaas@chromium.org,clemensb@chromium.org Change-Id: Ic2491f635a292e004f6c95498a045ba102138dc5 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:10281 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2096623 Commit-Queue: Clemens Backes Reviewed-by: Clemens Backes Cr-Commit-Position: refs/heads/master@{#66658} --- src/codegen/external-reference.cc | 1 - src/codegen/external-reference.h | 1 - src/compiler/wasm-compiler.cc | 23 ++++++------ src/wasm/module-instantiate.cc | 7 ++-- src/wasm/wasm-external-refs.cc | 58 +++++++++++++++++-------------- src/wasm/wasm-external-refs.h | 4 +-- src/wasm/wasm-interpreter.cc | 6 ++-- 7 files changed, 53 insertions(+), 47 deletions(-) diff --git a/src/codegen/external-reference.cc b/src/codegen/external-reference.cc index 7a42e40461..b693117a1a 100644 --- a/src/codegen/external-reference.cc +++ b/src/codegen/external-reference.cc @@ -289,7 +289,6 @@ FUNCTION_REFERENCE(wasm_word32_rol, wasm::word32_rol_wrapper) FUNCTION_REFERENCE(wasm_word32_ror, wasm::word32_ror_wrapper) FUNCTION_REFERENCE(wasm_word64_rol, wasm::word64_rol_wrapper) FUNCTION_REFERENCE(wasm_word64_ror, wasm::word64_ror_wrapper) -FUNCTION_REFERENCE(wasm_memory_init, wasm::memory_init_wrapper) FUNCTION_REFERENCE(wasm_memory_copy, wasm::memory_copy_wrapper) FUNCTION_REFERENCE(wasm_memory_fill, wasm::memory_fill_wrapper) diff --git a/src/codegen/external-reference.h b/src/codegen/external-reference.h index 2c5c8348f4..9eb95e18d9 100644 --- a/src/codegen/external-reference.h +++ b/src/codegen/external-reference.h @@ -200,7 +200,6 @@ class StatsCounter; V(wasm_word64_ror, "wasm::word64_ror") \ V(wasm_word64_ctz, "wasm::word64_ctz") \ V(wasm_word64_popcnt, "wasm::word64_popcnt") \ - V(wasm_memory_init, "wasm::memory_init") \ V(wasm_memory_copy, "wasm::memory_copy") \ V(wasm_memory_fill, "wasm::memory_fill") \ V(call_enqueue_microtask_function, "MicrotaskQueue::CallEnqueueMicrotask") \ diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index 32b0db67ae..73a3cca306 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -4931,7 +4931,7 @@ Node* WasmGraphBuilder::MemoryInit(uint32_t data_segment_index, Node* dst, } Node* function = graph()->NewNode(mcgraph()->common()->ExternalConstant( - ExternalReference::wasm_memory_init())); + ExternalReference::wasm_memory_copy())); Node* stack_slot = StoreArgsInStackSlot({{MachineType::PointerRepresentation(), dst}, @@ -4981,19 +4981,22 @@ Node* WasmGraphBuilder::StoreArgsInStackSlot( Node* WasmGraphBuilder::MemoryCopy(Node* dst, Node* src, Node* size, wasm::WasmCodePosition position) { + Node* dst_fail = BoundsCheckMemRange(&dst, &size, position); + TrapIfTrue(wasm::kTrapMemOutOfBounds, dst_fail, position); + Node* src_fail = BoundsCheckMemRange(&src, &size, position); + TrapIfTrue(wasm::kTrapMemOutOfBounds, src_fail, position); + Node* function = graph()->NewNode(mcgraph()->common()->ExternalConstant( ExternalReference::wasm_memory_copy())); - Node* stack_slot = StoreArgsInStackSlot( - {{MachineType::PointerRepresentation(), instance_node_.get()}, - {MachineRepresentation::kWord32, dst}, - {MachineRepresentation::kWord32, src}, - {MachineRepresentation::kWord32, size}}); + Node* stack_slot = + StoreArgsInStackSlot({{MachineType::PointerRepresentation(), dst}, + {MachineType::PointerRepresentation(), src}, + {MachineRepresentation::kWord32, size}}); - MachineType sig_types[] = {MachineType::Bool(), MachineType::Pointer()}; - MachineSignature sig(1, 1, sig_types); - Node* call = SetEffect(BuildCCall(&sig, function, stack_slot)); - return TrapIfFalse(wasm::kTrapMemOutOfBounds, call, position); + MachineType sig_types[] = {MachineType::Pointer()}; + MachineSignature sig(0, 1, sig_types); + return SetEffect(BuildCCall(&sig, function, stack_slot)); } Node* WasmGraphBuilder::MemoryFill(Node* dst, Node* value, Node* size, diff --git a/src/wasm/module-instantiate.cc b/src/wasm/module-instantiate.cc index 696663b01b..8b89c2ff3c 100644 --- a/src/wasm/module-instantiate.cc +++ b/src/wasm/module-instantiate.cc @@ -669,8 +669,11 @@ void InstanceBuilder::LoadDataSegments(Handle instance) { } // No need to copy empty segments. if (size == 0) continue; - std::memcpy(instance->memory_start() + dest_offset, - wire_bytes.begin() + segment.source.offset(), size); + Address dest_addr = + reinterpret_cast
(instance->memory_start()) + dest_offset; + Address src_addr = reinterpret_cast
(wire_bytes.begin()) + + segment.source.offset(); + memory_copy(dest_addr, src_addr, size); } else { DCHECK(segment.active); // Segments of size == 0 are just nops. diff --git a/src/wasm/wasm-external-refs.cc b/src/wasm/wasm-external-refs.cc index 64f91001ef..b3568fc379 100644 --- a/src/wasm/wasm-external-refs.cc +++ b/src/wasm/wasm-external-refs.cc @@ -11,9 +11,7 @@ #include "src/base/bits.h" #include "src/base/ieee754.h" -#include "src/common/assert-scope.h" #include "src/utils/memcopy.h" -#include "src/wasm/wasm-objects-inl.h" #if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \ defined(THREAD_SANITIZER) || defined(LEAK_SANITIZER) || \ @@ -325,35 +323,41 @@ void float64_pow_wrapper(Address data) { WriteUnalignedValue(data, base::ieee754::pow(x, y)); } -void memory_init_wrapper(Address data) { +// Asan on Windows triggers exceptions in this function to allocate +// shadow memory lazily. When this function is called from WebAssembly, +// these exceptions would be handled by the trap handler before they get +// handled by Asan, and thereby confuse the thread-in-wasm flag. +// Therefore we disable ASAN for this function. Alternatively we could +// reset the thread-in-wasm flag before calling this function. However, +// as this is only a problem with Asan on Windows, we did not consider +// it worth the overhead. +DISABLE_ASAN void memory_copy(Address dst, Address src, uint32_t size) { + // Use explicit forward and backward copy to match the required semantics for + // the memory.copy instruction. It is assumed that the caller of this + // function has already performed bounds checks, so {src + size} and + // {dst + size} should not overflow. + DCHECK(src + size >= src && dst + size >= dst); + uint8_t* dst8 = reinterpret_cast(dst); + uint8_t* src8 = reinterpret_cast(src); + if (src < dst && src + size > dst && dst + size > src) { + dst8 += size - 1; + src8 += size - 1; + for (; size > 0; size--) { + *dst8-- = *src8--; + } + } else { + for (; size > 0; size--) { + *dst8++ = *src8++; + } + } +} + +void memory_copy_wrapper(Address data) { Address dst = ReadUnalignedValue
(data); Address src = ReadUnalignedValue
(data + sizeof(dst)); uint32_t size = ReadUnalignedValue(data + sizeof(dst) + sizeof(src)); - std::memmove(reinterpret_cast(dst), reinterpret_cast(src), - size); -} - -bool memory_copy_wrapper(Address data) { - DisallowHeapAllocation disallow_heap_allocation; - size_t offset = 0; - Object raw_instance = ReadUnalignedValue(data); - WasmInstanceObject instance = WasmInstanceObject::cast(raw_instance); - offset += sizeof(Object); - size_t dst = ReadUnalignedValue(data + offset); - offset += sizeof(uint32_t); - size_t src = ReadUnalignedValue(data + offset); - offset += sizeof(uint32_t); - size_t size = ReadUnalignedValue(data + offset); - - size_t mem_size = instance.memory_size(); - if (!base::IsInBounds(dst, size, mem_size)) return false; - if (!base::IsInBounds(src, size, mem_size)) return false; - - byte* memory_start = instance.memory_start(); - // Use std::memmove, because the ranges can overlap. - std::memmove(memory_start + dst, memory_start + src, size); - return true; + memory_copy(dst, src, size); } // Asan on Windows triggers exceptions in this function that confuse the diff --git a/src/wasm/wasm-external-refs.h b/src/wasm/wasm-external-refs.h index 8c85e40f57..6f02bc2d4d 100644 --- a/src/wasm/wasm-external-refs.h +++ b/src/wasm/wasm-external-refs.h @@ -71,9 +71,9 @@ V8_EXPORT_PRIVATE void word64_ror_wrapper(Address data); V8_EXPORT_PRIVATE void float64_pow_wrapper(Address data); -void memory_init_wrapper(Address data); +void memory_copy_wrapper(Address data); -bool memory_copy_wrapper(Address data); +void memory_copy(Address dst, Address src, uint32_t size); void memory_fill_wrapper(Address dst, uint32_t value, uint32_t size); diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 3127b65dc6..5a4b56e69b 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -1824,8 +1824,7 @@ class ThreadImpl { Address src_addr = instance_object_->data_segment_starts()[imm.data_segment_index] + src; - std::memmove(reinterpret_cast(dst_addr), - reinterpret_cast(src_addr), size); + memory_copy(dst_addr, src_addr, size); return true; } case kExprDataDrop: { @@ -1851,8 +1850,7 @@ class ThreadImpl { return false; } - std::memmove(reinterpret_cast(dst_addr), - reinterpret_cast(src_addr), size); + memory_copy(dst_addr, src_addr, size); return true; } case kExprMemoryFill: {