From 60ee70bb40efea6f05476dc19f7a5b490193a107 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Thu, 6 Aug 2020 16:12:25 +0200 Subject: [PATCH] [wasm] Ensure that only TurboFan code is serialized We have the implicit assumption that Liftoff code will never be serialized, and we start relying on that when implementing new features (debugging, dynamic tiering). This CL makes the serializer fail if the module contains any Liftoff code. Existing tests are changed to ensure that we fully tiered up before serializing a module (similar to the logic in Chromium). The "wasm-clone-module" test needs to serialize the module before enabling the debugger. Note that chrome currently only serializes a module after it fully tiered up, so that should be fine. If other embedders need the ability to serialize a module in an arbitrary state, we will have to fix this later. With this CL we will be on the safe side though and (gracefully) fail serialization instead of accidentally serializing Liftoff code. R=ahaas@chromium.org Bug: v8:10777 Change-Id: I1245e5f7fda3447a544c1e3525e1239cde759174 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2336799 Commit-Queue: Clemens Backes Reviewed-by: Andreas Haas Cr-Commit-Position: refs/heads/master@{#69276} --- src/runtime/runtime-test.cc | 27 +++++++++---------- src/wasm/compilation-environment.h | 14 +++++----- src/wasm/module-compiler.cc | 11 ++++++++ src/wasm/wasm-serialization.cc | 24 ++++++++++++----- test/cctest/cctest.status | 11 ++++++++ .../cctest/wasm/test-streaming-compilation.cc | 3 ++- test/cctest/wasm/test-wasm-serialization.cc | 5 ++++ test/mjsunit/regress/wasm/regress-808848.js | 4 ++- test/mjsunit/regress/wasm/regress-808980.js | 4 ++- .../wasm/compiled-module-serialization.js | 4 ++- test/mjsunit/wasm/print-code.js | 4 ++- 11 files changed, 80 insertions(+), 31 deletions(-) diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index 995fe0d566..114b87ac70 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -1324,30 +1324,29 @@ RUNTIME_FUNCTION(Runtime_SerializeDeserializeNow) { return ReadOnlyRoots(isolate).undefined_value(); } -// Take a compiled wasm module and serialize it into an array buffer, which is -// then returned. +// Wait until the given module is fully tiered up, then serialize it into an +// array buffer. RUNTIME_FUNCTION(Runtime_SerializeWasmModule) { HandleScope scope(isolate); DCHECK_EQ(1, args.length()); CONVERT_ARG_HANDLE_CHECKED(WasmModuleObject, module_obj, 0); wasm::NativeModule* native_module = module_obj->native_module(); + native_module->compilation_state()->WaitForTopTierFinishedForTesting(); + DCHECK(!native_module->compilation_state()->failed()); + wasm::WasmSerializer wasm_serializer(native_module); size_t byte_length = wasm_serializer.GetSerializedNativeModuleSize(); - MaybeHandle result = - isolate->factory()->NewJSArrayBufferAndBackingStore( - byte_length, InitializedFlag::kUninitialized); + Handle array_buffer = + isolate->factory() + ->NewJSArrayBufferAndBackingStore(byte_length, + InitializedFlag::kUninitialized) + .ToHandleChecked(); - Handle array_buffer; - if (result.ToHandle(&array_buffer) && - wasm_serializer.SerializeNativeModule( - {reinterpret_cast(array_buffer->backing_store()), - byte_length})) { - return *array_buffer; - } - - UNREACHABLE(); + CHECK(wasm_serializer.SerializeNativeModule( + {static_cast(array_buffer->backing_store()), byte_length})); + return *array_buffer; } // Take an array buffer and attempt to reconstruct a compiled wasm module. diff --git a/src/wasm/compilation-environment.h b/src/wasm/compilation-environment.h index 29b359e50a..5b02e17f2e 100644 --- a/src/wasm/compilation-environment.h +++ b/src/wasm/compilation-environment.h @@ -101,7 +101,7 @@ enum class CompilationEvent : uint8_t { // The implementation of {CompilationState} lives in module-compiler.cc. // This is the PIMPL interface to that private class. -class CompilationState { +class V8_EXPORT_PRIVATE CompilationState { public: using callback_t = std::function; @@ -113,15 +113,17 @@ class CompilationState { void SetWireBytesStorage(std::shared_ptr); - V8_EXPORT_PRIVATE std::shared_ptr GetWireBytesStorage() - const; + std::shared_ptr GetWireBytesStorage() const; void AddCallback(callback_t); + // Wait until top tier finished, or compilation failed (to avoid deadlocks). + void WaitForTopTierFinishedForTesting(); + bool failed() const; - V8_EXPORT_PRIVATE bool baseline_compilation_finished() const; - V8_EXPORT_PRIVATE bool top_tier_compilation_finished() const; - V8_EXPORT_PRIVATE bool recompilation_finished() const; + bool baseline_compilation_finished() const; + bool top_tier_compilation_finished() const; + bool recompilation_finished() const; // Override {operator delete} to avoid implicit instantiation of {operator // delete} with {size_t} argument. The {size_t} argument would be incorrect. diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index a81f2567de..edc313edd1 100644 --- a/src/wasm/module-compiler.cc +++ b/src/wasm/module-compiler.cc @@ -688,6 +688,17 @@ void CompilationState::AddCallback(CompilationState::callback_t callback) { return Impl(this)->AddCallback(std::move(callback)); } +void CompilationState::WaitForTopTierFinishedForTesting() { + auto top_tier_finished_semaphore = std::make_shared(0); + AddCallback([top_tier_finished_semaphore](CompilationEvent event) { + if (event == CompilationEvent::kFailedCompilation || + event == CompilationEvent::kFinishedTopTierCompilation) { + top_tier_finished_semaphore->Signal(); + } + }); + top_tier_finished_semaphore->Wait(); +} + bool CompilationState::failed() const { return Impl(this)->failed(); } bool CompilationState::baseline_compilation_finished() const { diff --git a/src/wasm/wasm-serialization.cc b/src/wasm/wasm-serialization.cc index 8df5d4c88e..e262c463f7 100644 --- a/src/wasm/wasm-serialization.cc +++ b/src/wasm/wasm-serialization.cc @@ -280,8 +280,8 @@ class V8_EXPORT_PRIVATE NativeModuleSerializer { private: size_t MeasureCode(const WasmCode*) const; - void WriteHeader(Writer* writer); - void WriteCode(const WasmCode*, Writer* writer); + void WriteHeader(Writer*); + bool WriteCode(const WasmCode*, Writer*); const NativeModule* const native_module_; Vector code_table_; @@ -301,6 +301,9 @@ NativeModuleSerializer::NativeModuleSerializer( size_t NativeModuleSerializer::MeasureCode(const WasmCode* code) const { if (code == nullptr) return sizeof(bool); DCHECK_EQ(WasmCode::kFunction, code->kind()); + if (FLAG_wasm_lazy_compilation && code->tier() != ExecutionTier::kTurbofan) { + return sizeof(bool); + } return kCodeHeaderSize + code->instructions().size() + code->reloc_info().size() + code->source_positions().size() + code->protected_instructions_data().size(); @@ -322,13 +325,21 @@ void NativeModuleSerializer::WriteHeader(Writer* writer) { writer->Write(native_module_->num_imported_functions()); } -void NativeModuleSerializer::WriteCode(const WasmCode* code, Writer* writer) { +bool NativeModuleSerializer::WriteCode(const WasmCode* code, Writer* writer) { + DCHECK_IMPLIES(!FLAG_wasm_lazy_compilation, code != nullptr); if (code == nullptr) { writer->Write(false); - return; + return true; + } + DCHECK_EQ(WasmCode::kFunction, code->kind()); + if (code->tier() != ExecutionTier::kTurbofan) { + if (FLAG_wasm_lazy_compilation) { + writer->Write(false); + return true; + } + return false; } writer->Write(true); - DCHECK_EQ(WasmCode::kFunction, code->kind()); // Write the size of the entire code section, followed by the code header. writer->Write(code->constant_pool_offset()); writer->Write(code->safepoint_table_offset()); @@ -415,6 +426,7 @@ void NativeModuleSerializer::WriteCode(const WasmCode* code, Writer* writer) { if (code_start != serialized_code_start) { memcpy(serialized_code_start, code_start, code_size); } + return true; } bool NativeModuleSerializer::Write(Writer* writer) { @@ -424,7 +436,7 @@ bool NativeModuleSerializer::Write(Writer* writer) { WriteHeader(writer); for (WasmCode* code : code_table_) { - WriteCode(code, writer); + if (!WriteCode(code, writer)) return false; } return true; } diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status index f5b05b3975..40e45534b8 100644 --- a/test/cctest/cctest.status +++ b/test/cctest/cctest.status @@ -182,6 +182,17 @@ 'test-cpu-profiler/MultipleIsolates': [SKIP], }], # variant == nooptimization and (arch == arm or arch == arm64) and simulator_run +############################################################################## +['variant == nooptimization', { + # Wasm serialization relies on TurboFan to be available, hence does not work + # in the 'nooptimization' variant. + 'test-wasm-serialization/*': [SKIP], + 'test-streaming-compilation/SingleThreadedTestDeserializationBypassesCompilation': [SKIP], + 'test-streaming-compilation/SingleThreadedTestDeserializationFails': [SKIP], + 'test-streaming-compilation/AsyncTestDeserializationFails': [SKIP], + 'test-streaming-compilation/AsyncTestDeserializationBypassesCompilation': [SKIP], +}], # variant == nooptimization + ############################################################################## ['variant == no_lfa', { # https://crbug.com/v8/10219 diff --git a/test/cctest/wasm/test-streaming-compilation.cc b/test/cctest/wasm/test-streaming-compilation.cc index b86f7dd1d7..91919bae73 100644 --- a/test/cctest/wasm/test-streaming-compilation.cc +++ b/test/cctest/wasm/test-streaming-compilation.cc @@ -268,10 +268,11 @@ ZoneBuffer GetValidCompiledModuleBytes(Zone* zone, ZoneBuffer wire_bytes) { // Serialize the NativeModule. std::shared_ptr native_module = tester.native_module(); CHECK(native_module); + native_module->compilation_state()->WaitForTopTierFinishedForTesting(); i::wasm::WasmSerializer serializer(native_module.get()); size_t size = serializer.GetSerializedNativeModuleSize(); std::vector buffer(size); - CHECK(serializer.SerializeNativeModule({buffer.data(), size})); + CHECK(serializer.SerializeNativeModule(VectorOf(buffer))); ZoneBuffer result(zone, size); result.write(buffer.data(), size); return result; diff --git a/test/cctest/wasm/test-wasm-serialization.cc b/test/cctest/wasm/test-wasm-serialization.cc index 0b1668a6e8..3f560845e2 100644 --- a/test/cctest/wasm/test-wasm-serialization.cc +++ b/test/cctest/wasm/test-wasm-serialization.cc @@ -148,6 +148,10 @@ class WasmSerializationTest { // Check that the native module exists at this point. CHECK(weak_native_module.lock()); + auto* native_module = module_object->native_module(); + native_module->compilation_state()->WaitForTopTierFinishedForTesting(); + DCHECK(!native_module->compilation_state()->failed()); + v8::Local v8_module_obj = v8::Utils::ToLocal(Handle::cast(module_object)); CHECK(v8_module_obj->IsWasmModuleObject()); @@ -163,6 +167,7 @@ class WasmSerializationTest { wire_bytes_ = {bytes_copy, uncompiled_bytes.size()}; // keep alive data_ until the end data_ = compiled_module.Serialize(); + CHECK_LT(0, data_.size); } // Dispose of serialization isolate to destroy the reference to the // NativeModule, which removes it from the module cache in the wasm engine diff --git a/test/mjsunit/regress/wasm/regress-808848.js b/test/mjsunit/regress/wasm/regress-808848.js index 269489059f..21374e9cff 100644 --- a/test/mjsunit/regress/wasm/regress-808848.js +++ b/test/mjsunit/regress/wasm/regress-808848.js @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --allow-natives-syntax +// The test needs --wasm-tier-up because we can't serialize and deserialize +// Liftoff code. +// Flags: --allow-natives-syntax --wasm-tier-up load('test/mjsunit/wasm/wasm-module-builder.js'); diff --git a/test/mjsunit/regress/wasm/regress-808980.js b/test/mjsunit/regress/wasm/regress-808980.js index 6487a35cd3..5cb3dcb70e 100644 --- a/test/mjsunit/regress/wasm/regress-808980.js +++ b/test/mjsunit/regress/wasm/regress-808980.js @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --allow-natives-syntax --throws +// The test needs --wasm-tier-up because we can't serialize and deserialize +// Liftoff code. +// Flags: --allow-natives-syntax --throws --wasm-tier-up load('test/mjsunit/wasm/wasm-module-builder.js'); let kTableSize = 3; diff --git a/test/mjsunit/wasm/compiled-module-serialization.js b/test/mjsunit/wasm/compiled-module-serialization.js index 859a3095ae..63568fe657 100644 --- a/test/mjsunit/wasm/compiled-module-serialization.js +++ b/test/mjsunit/wasm/compiled-module-serialization.js @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --expose-wasm --allow-natives-syntax --expose-gc +// The test needs --wasm-tier-up because we can't serialize and deserialize +// Liftoff code. +// Flags: --expose-wasm --allow-natives-syntax --expose-gc --wasm-tier-up load("test/mjsunit/wasm/wasm-module-builder.js"); diff --git a/test/mjsunit/wasm/print-code.js b/test/mjsunit/wasm/print-code.js index c604ca75f0..876a545846 100644 --- a/test/mjsunit/wasm/print-code.js +++ b/test/mjsunit/wasm/print-code.js @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --allow-natives-syntax --print-wasm-code +// The test needs --wasm-tier-up because we can't serialize and deserialize +// Liftoff code. +// Flags: --allow-natives-syntax --print-wasm-code --wasm-tier-up // Just test that printing the code of the following wasm modules does not // crash.