From 7829af3275ff4644a2d0a1270abe1a1e4415e9fb Mon Sep 17 00:00:00 2001 From: mtrofin Date: Tue, 11 Apr 2017 17:01:04 -0700 Subject: [PATCH] [wasm] instantiate expressed in terms of compile Today, the semantics of: WebAssembly.instantiate and WebAssembly.compile().then(new WebAssemblyInstance) are subtly different, to the point where attempting the proposed change uncovered bugs. In the future, it's possible that .instantiate actually have different semantics - if we pre-specialized to the provided ffi, for example. Right now that's not the case. This CL: - gets our implementation closer to what developers may write using the compile -> new Instance alternative, in particular wrt promise creation. By reusing code paths, we uncover more bugs, and keep maintenance cost lower. - it gives us the response-based WebAssembly.instantiate implicitly. Otherwise, we'd need that same implementation on the blink side. The negative is maintenance: imagine if the bugs I mentioned could only be found when running in Blink. BUG=chromium:697028 Review-Url: https://codereview.chromium.org/2806073002 Cr-Commit-Position: refs/heads/master@{#44592} --- src/runtime/runtime-test.cc | 27 ++++ src/runtime/runtime.h | 2 + src/wasm/wasm-js.cc | 153 +++++++++++++++------ src/wasm/wasm-module.cc | 38 ----- src/wasm/wasm-module.h | 4 - test/mjsunit/wasm/instantiate-run-basic.js | 20 ++- test/mjsunit/wasm/js-api.js | 2 - test/mjsunit/wasm/wasm-api-overloading.js | 37 +++++ 8 files changed, 193 insertions(+), 90 deletions(-) create mode 100644 test/mjsunit/wasm/wasm-api-overloading.js diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index 3270055cab..95bc30d909 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -111,6 +111,15 @@ bool WasmInstantiateOverride(const v8::FunctionCallbackInfo& args) { return true; } +bool GetWasmFromResolvedPromise( + const v8::FunctionCallbackInfo& args) { + CHECK(args.Length() == 1); + v8::Local promise = v8::Local::Cast(args[0]); + CHECK(promise->State() == v8::Promise::PromiseState::kFulfilled); + args.GetReturnValue().Set(promise); + return true; +} + } // namespace namespace v8 { @@ -445,6 +454,24 @@ RUNTIME_FUNCTION(Runtime_ClearFunctionFeedback) { return isolate->heap()->undefined_value(); } +RUNTIME_FUNCTION(Runtime_SetWasmCompileFromPromiseOverload) { + v8::ExtensionCallback old = isolate->wasm_compile_callback(); + HandleScope scope(isolate); + Handle ret = + isolate->factory()->NewForeign(reinterpret_cast
(old)); + isolate->set_wasm_compile_callback(GetWasmFromResolvedPromise); + return *ret; +} + +RUNTIME_FUNCTION(Runtime_ResetWasmOverloads) { + HandleScope scope(isolate); + DCHECK_EQ(1, args.length()); + CONVERT_ARG_HANDLE_CHECKED(Foreign, old, 0); + isolate->set_wasm_compile_callback( + reinterpret_cast(old->foreign_address())); + return isolate->heap()->undefined_value(); +} + RUNTIME_FUNCTION(Runtime_CheckWasmWrapperElision) { // This only supports the case where the function being exported // calls an intermediate function, and the intermediate function diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 17a5704203..af3c6630e5 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -616,6 +616,8 @@ namespace internal { F(ValidateWasmOrphanedInstance, 1, 1) \ F(SetWasmCompileControls, 2, 1) \ F(SetWasmInstantiateControls, 0, 1) \ + F(SetWasmCompileFromPromiseOverload, 0, 1) \ + F(ResetWasmOverloads, 1, 1) \ F(HeapObjectVerify, 1, 1) \ F(WasmNumInterpretedCalls, 1, 1) \ F(RedirectToWasmInterpreter, 2, 1) diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc index 3b275d19ec..396f8987a6 100644 --- a/src/wasm/wasm-js.cc +++ b/src/wasm/wasm-js.cc @@ -32,6 +32,18 @@ namespace v8 { namespace { +#define ASSIGN(type, var, expr) \ + Local var; \ + do { \ + if (!expr.ToLocal(&var)) return; \ + } while (false) + +#define DO_BOOL(expr) \ + do { \ + bool ok; \ + if (!expr.To(&ok) || !ok) return; \ + } while (false) + // TODO(wasm): move brand check to the respective types, and don't throw // in it, rather, use a provided ErrorThrower, or let caller handle it. static bool HasBrand(i::Handle value, i::Handle sym) { @@ -117,16 +129,15 @@ i::wasm::ModuleWireBytes GetFirstArgumentAsBytes( return i::wasm::ModuleWireBytes(start, start + length); } -i::MaybeHandle GetSecondArgumentAsImports( - const v8::FunctionCallbackInfo& args, ErrorThrower* thrower) { - if (args.Length() < 2) return {}; - if (args[1]->IsUndefined()) return {}; +i::MaybeHandle GetValueAsImports(const Local& arg, + ErrorThrower* thrower) { + if (arg->IsUndefined()) return {}; - if (!args[1]->IsObject()) { + if (!arg->IsObject()) { thrower->TypeError("Argument 1 must be an object"); return {}; } - Local obj = Local::Cast(args[1]); + Local obj = Local::Cast(arg); return i::Handle::cast(v8::Utils::OpenHandle(*obj)); } @@ -140,8 +151,7 @@ void WebAssemblyCompile(const v8::FunctionCallbackInfo& args) { ErrorThrower thrower(i_isolate, "WebAssembly.compile()"); Local context = isolate->GetCurrentContext(); - v8::Local resolver; - if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) return; + ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context)); v8::ReturnValue return_value = args.GetReturnValue(); return_value.Set(resolver->GetPromise()); @@ -253,10 +263,70 @@ void WebAssemblyModuleCustomSections( args.GetReturnValue().Set(Utils::ToLocal(custom_sections)); } +// Entered as internal implementation detail of sync and async instantiate. +// args[0] *must* be a WebAssembly.Module. +void WebAssemblyInstantiateImpl( + const v8::FunctionCallbackInfo& args) { + DCHECK_GE(args.Length(), 1); + Local module = args[0]; + Local ffi = args.Data(); + + HandleScope scope(args.GetIsolate()); + v8::Isolate* isolate = args.GetIsolate(); + i::Isolate* i_isolate = reinterpret_cast(isolate); + ErrorThrower thrower(i_isolate, "WebAssembly Instantiation"); + i::MaybeHandle maybe_imports = + GetValueAsImports(ffi, &thrower); + if (thrower.error()) return; + + i::Handle module_obj = + i::Handle::cast( + Utils::OpenHandle(Object::Cast(*module))); + i::MaybeHandle instance_object = + i::wasm::SyncInstantiate(i_isolate, &thrower, module_obj, maybe_imports, + i::MaybeHandle()); + + if (instance_object.is_null()) { + // TODO(wasm): this *should* mean there's an error to throw, but + // we exit sometimes the instantiation pipeline without throwing. + // v8:6232. + return; + } + args.GetReturnValue().Set(Utils::ToLocal(instance_object.ToHandleChecked())); +} + +void WebAssemblyInstantiateToPair( + const v8::FunctionCallbackInfo& args) { + DCHECK_GE(args.Length(), 1); + Local module = args[0]; + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + const uint8_t* instance_str = reinterpret_cast("instance"); + const uint8_t* module_str = reinterpret_cast("module"); + ASSIGN(Function, vanilla_instantiate, + Function::New(context, WebAssemblyInstantiateImpl, args.Data())); + + ASSIGN(Value, instance, + vanilla_instantiate->Call(context, args.Holder(), 1, &module)); + Local ret = Object::New(isolate); + ASSIGN(String, instance_name, + String::NewFromOneByte(isolate, instance_str, + NewStringType::kInternalized)); + ASSIGN(String, module_name, + String::NewFromOneByte(isolate, module_str, + NewStringType::kInternalized)); + + DO_BOOL(ret->CreateDataProperty(context, instance_name, instance)); + DO_BOOL(ret->CreateDataProperty(context, module_name, module)); + args.GetReturnValue().Set(ret); +} + // new WebAssembly.Instance(module, imports) -> WebAssembly.Instance void WebAssemblyInstance(const v8::FunctionCallbackInfo& args) { HandleScope scope(args.GetIsolate()); - v8::Isolate* isolate = args.GetIsolate(); + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); i::Isolate* i_isolate = reinterpret_cast(isolate); if (i_isolate->wasm_instance_callback()(args)) return; @@ -265,14 +335,15 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo& args) { auto maybe_module = GetFirstArgumentAsModule(args, &thrower); if (thrower.error()) return; - auto maybe_imports = GetSecondArgumentAsImports(args, &thrower); - if (thrower.error()) return; + // If args.Length < 2, this will be undefined - see FunctionCallbackInfo. + // We'll check for that in WebAssemblyInstantiateImpl. + Local data = args[1]; - i::MaybeHandle instance_object = i::wasm::SyncInstantiate( - i_isolate, &thrower, maybe_module.ToHandleChecked(), maybe_imports, - i::MaybeHandle()); - if (instance_object.is_null()) return; - args.GetReturnValue().Set(Utils::ToLocal(instance_object.ToHandleChecked())); + ASSIGN(Function, impl, + Function::New(context, WebAssemblyInstantiateImpl, data)); + Local first_param = args[0]; + ASSIGN(Value, ret, impl->Call(context, args.Holder(), 1, &first_param)); + args.GetReturnValue().Set(ret); } // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance @@ -290,10 +361,9 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo& args) { Local context = isolate->GetCurrentContext(); i::Handle i_context = Utils::OpenHandle(*context); - v8::Local resolver; - if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) return; - v8::ReturnValue return_value = args.GetReturnValue(); - return_value.Set(resolver->GetPromise()); + ASSIGN(Promise::Resolver, resolver, Promise::Resolver::New(context)); + Local module_promise = resolver->GetPromise(); + args.GetReturnValue().Set(module_promise); if (args.Length() < 1) { thrower.TypeError( @@ -305,7 +375,8 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo& args) { return; } - i::Handle first_arg = Utils::OpenHandle(*args[0]); + Local first_arg_value = args[0]; + i::Handle first_arg = Utils::OpenHandle(*first_arg_value); if (!first_arg->IsJSObject()) { thrower.TypeError( "Argument 0 must be a buffer source or a WebAssembly.Module object"); @@ -315,31 +386,27 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo& args) { return; } - auto maybe_imports = GetSecondArgumentAsImports(args, &thrower); - if (thrower.error()) { - auto maybe = resolver->Reject(context, Utils::ToLocal(thrower.Reify())); - CHECK_IMPLIES(!maybe.FromMaybe(false), - i_isolate->has_scheduled_exception()); - return; - } - i::Handle promise = Utils::OpenHandle(*resolver->GetPromise()); + FunctionCallback instantiator = nullptr; if (HasBrand(first_arg, i::Handle(i_context->wasm_module_sym()))) { - // WebAssembly.instantiate(module, imports) -> WebAssembly.Instance - auto module_object = GetFirstArgumentAsModule(args, &thrower); - i::wasm::AsyncInstantiate(i_isolate, promise, - module_object.ToHandleChecked(), maybe_imports); + module_promise = resolver->GetPromise(); + DO_BOOL(resolver->Resolve(context, first_arg_value)); + instantiator = WebAssemblyInstantiateImpl; } else { - // WebAssembly.instantiate(bytes, imports) -> {module, instance} - auto bytes = GetFirstArgumentAsBytes(args, &thrower); - if (thrower.error()) { - auto maybe = resolver->Reject(context, Utils::ToLocal(thrower.Reify())); - CHECK_IMPLIES(!maybe.FromMaybe(false), - i_isolate->has_scheduled_exception()); - return; - } - i::wasm::AsyncCompileAndInstantiate(i_isolate, promise, bytes, - maybe_imports); + ASSIGN(Function, async_compile, Function::New(context, WebAssemblyCompile)); + ASSIGN(Value, async_compile_retval, + async_compile->Call(context, args.Holder(), 1, &first_arg_value)); + module_promise = Local::Cast(async_compile_retval); + instantiator = WebAssemblyInstantiateToPair; } + DCHECK(!module_promise.IsEmpty()); + DCHECK_NOT_NULL(instantiator); + // If args.Length < 2, this will be undefined - see FunctionCallbackInfo. + // We'll check for that in WebAssemblyInstantiateImpl. + Local data = args[1]; + ASSIGN(Function, instantiate_impl, + Function::New(context, instantiator, data)); + ASSIGN(Promise, result, module_promise->Then(context, instantiate_impl)); + args.GetReturnValue().Set(result); } bool GetIntegerProperty(v8::Isolate* isolate, ErrorThrower* thrower, diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc index 40015a70e1..df99e93e61 100644 --- a/src/wasm/wasm-module.cc +++ b/src/wasm/wasm-module.cc @@ -2604,44 +2604,6 @@ void wasm::AsyncInstantiate(Isolate* isolate, Handle promise, instance_object.ToHandleChecked()); } -void wasm::AsyncCompileAndInstantiate(Isolate* isolate, - Handle promise, - const ModuleWireBytes& bytes, - MaybeHandle imports) { - ErrorThrower thrower(isolate, nullptr); - - // Compile the module. - MaybeHandle module_object = - SyncCompile(isolate, &thrower, bytes); - if (thrower.error()) { - RejectPromise(isolate, handle(isolate->context()), &thrower, promise); - return; - } - Handle module = module_object.ToHandleChecked(); - - // Instantiate the module. - MaybeHandle instance_object = SyncInstantiate( - isolate, &thrower, module, imports, Handle::null()); - if (thrower.error()) { - RejectPromise(isolate, handle(isolate->context()), &thrower, promise); - return; - } - - Handle object_function = - Handle(isolate->native_context()->object_function(), isolate); - Handle ret = - isolate->factory()->NewJSObject(object_function, TENURED); - Handle module_property_name = - isolate->factory()->InternalizeUtf8String("module"); - Handle instance_property_name = - isolate->factory()->InternalizeUtf8String("instance"); - JSObject::AddProperty(ret, module_property_name, module, NONE); - JSObject::AddProperty(ret, instance_property_name, - instance_object.ToHandleChecked(), NONE); - - ResolvePromise(isolate, handle(isolate->context()), promise, ret); -} - // Encapsulates all the state and steps of an asynchronous compilation. // An asynchronous compile job consists of a number of tasks that are executed // as foreground and background tasks. Any phase that touches the V8 heap or diff --git a/src/wasm/wasm-module.h b/src/wasm/wasm-module.h index 32dea7befd..c296dc8cc4 100644 --- a/src/wasm/wasm-module.h +++ b/src/wasm/wasm-module.h @@ -472,10 +472,6 @@ V8_EXPORT_PRIVATE void AsyncInstantiate(Isolate* isolate, Handle module_object, MaybeHandle imports); -V8_EXPORT_PRIVATE void AsyncCompileAndInstantiate( - Isolate* isolate, Handle promise, const ModuleWireBytes& bytes, - MaybeHandle imports); - #if V8_TARGET_ARCH_64_BIT const bool kGuardRegionsSupported = true; #else diff --git a/test/mjsunit/wasm/instantiate-run-basic.js b/test/mjsunit/wasm/instantiate-run-basic.js index e9e9a9ac48..b0016ec9aa 100644 --- a/test/mjsunit/wasm/instantiate-run-basic.js +++ b/test/mjsunit/wasm/instantiate-run-basic.js @@ -2,19 +2,33 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --expose-wasm +// Flags: --allow-natives-syntax load("test/mjsunit/wasm/wasm-constants.js"); load("test/mjsunit/wasm/wasm-module-builder.js"); -(function BasicTest() { - var kReturnValue = 15; +const kReturnValue = 15; + +function getBuilder() { var builder = new WasmModuleBuilder(); builder.addFunction("main", kSig_i_i) .addBody([kExprI32Const, kReturnValue]) .exportFunc(); + return builder; +} +(function BasicTest() { + var builder = getBuilder(); var main = builder.instantiate().exports.main; assertEquals(kReturnValue, main()); })(); + +(function AsyncTest() { + var builder = getBuilder(); + var buffer = builder.toBuffer(); + assertPromiseResult( + WebAssembly.instantiate(buffer) + .then(pair => pair.instance.exports.main(), assertUnreachable) + .then(result => assertEquals(kReturnValue, result), assertUnreachable)); +})(); diff --git a/test/mjsunit/wasm/js-api.js b/test/mjsunit/wasm/js-api.js index 689a0adbc4..0f6b0816be 100644 --- a/test/mjsunit/wasm/js-api.js +++ b/test/mjsunit/wasm/js-api.js @@ -713,7 +713,6 @@ function assertCompileError(args, err, msg) { var error = null; assertPromiseResult(compile(...args), unexpectedSuccess, error => { assertTrue(error instanceof err); - assertTrue(Boolean(error.stack.match('js-api.js'))); // TODO assertTrue(Boolean(error.message.match(msg))); }); } @@ -760,7 +759,6 @@ function assertInstantiateError(args, err, msg) { var error = null; assertPromiseResult(instantiate(...args), unexpectedSuccess, error => { assertTrue(error instanceof err); - assertTrue(Boolean(error.stack.match('js-api.js'))); // TODO assertTrue(Boolean(error.message.match(msg))); }); } diff --git a/test/mjsunit/wasm/wasm-api-overloading.js b/test/mjsunit/wasm/wasm-api-overloading.js new file mode 100644 index 0000000000..196b13b078 --- /dev/null +++ b/test/mjsunit/wasm/wasm-api-overloading.js @@ -0,0 +1,37 @@ +// Copyright 2017 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +load("test/mjsunit/wasm/wasm-constants.js"); +load("test/mjsunit/wasm/wasm-module-builder.js"); + +let buffer = (() => { + let builder = new WasmModuleBuilder(); + builder.addFunction("f", kSig_i_v) + .addBody([kExprI32Const, 42]) + .exportAs("f"); + return builder.toBuffer(); +})(); + +var module = new WebAssembly.Module(buffer); +var wrapper = Promise.resolve(module); + +assertPromiseResult( + WebAssembly.instantiate(wrapper), + assertUnreachable, + e => assertTrue(e instanceof TypeError)); + +assertPromiseResult(( + () => { + var old = %SetWasmCompileFromPromiseOverload(); + var ret = WebAssembly.instantiate(wrapper); + %ResetWasmOverloads(old); + return ret; + })(), + pair => { + assertTrue(pair.instance instanceof WebAssembly.Instance); + assertTrue(pair.module instanceof WebAssembly.Module) + }, + assertUnreachable);