From f8fd6ec3dd89cf1278c0ac00d5ae9e0f480e4d4a Mon Sep 17 00:00:00 2001 From: mtrofin Date: Fri, 13 Jan 2017 12:47:44 -0800 Subject: [PATCH] [wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError We weren't throwing LinkError where appropriate progress BUG=v8:5835 Review-Url: https://codereview.chromium.org/2629523007 Cr-Commit-Position: refs/heads/master@{#42342} --- src/asmjs/asm-js.cc | 4 +- src/wasm/wasm-js.cc | 13 ++- src/wasm/wasm-module.cc | 43 +++++---- src/wasm/wasm-module.h | 5 +- test/cctest/wasm/test-run-wasm-module.cc | 4 +- test/mjsunit/wasm/errors.js | 40 ++++++-- test/mjsunit/wasm/js-api.js | 118 +++++++++++++---------- 7 files changed, 141 insertions(+), 86 deletions(-) diff --git a/src/asmjs/asm-js.cc b/src/asmjs/asm-js.cc index 3b3b870ce3..b4026b0b19 100644 --- a/src/asmjs/asm-js.cc +++ b/src/asmjs/asm-js.cc @@ -254,8 +254,8 @@ MaybeHandle AsmJs::InstantiateAsmWasm(i::Isolate* isolate, Handle foreign) { base::ElapsedTimer instantiate_timer; instantiate_timer.Start(); - i::Handle module( - i::JSObject::cast(wasm_data->get(kWasmDataCompiledModule))); + i::Handle module( + i::WasmModuleObject::cast(wasm_data->get(kWasmDataCompiledModule))); i::Handle foreign_globals( i::FixedArray::cast(wasm_data->get(kWasmDataForeignGlobals))); diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc index 76abca0d44..f2c630dd4a 100644 --- a/src/wasm/wasm-js.cc +++ b/src/wasm/wasm-js.cc @@ -188,7 +188,7 @@ void WebAssemblyModule(const v8::FunctionCallbackInfo& args) { } MaybeLocal InstantiateModuleImpl( - i::Isolate* i_isolate, i::Handle i_module_obj, + i::Isolate* i_isolate, i::Handle i_module_obj, const v8::FunctionCallbackInfo& args, ErrorThrower* thrower) { // It so happens that in both the WebAssembly.instantiate, as well as // WebAssembly.Instance ctor, the positions of the ffi object and memory @@ -199,6 +199,10 @@ MaybeLocal InstantiateModuleImpl( MaybeLocal nothing; i::Handle ffi = i::Handle::null(); + // This is a first - level validation of the argument. If present, we only + // check its type. {Instantiate} will further check that if the module + // has imports, the argument must be present, as well as piecemeal + // import satisfaction. if (args.Length() > kFfiOffset && !args[kFfiOffset]->IsUndefined()) { if (!args[kFfiOffset]->IsObject()) { thrower->TypeError("Argument %d must be an object", kFfiOffset); @@ -208,6 +212,7 @@ MaybeLocal InstantiateModuleImpl( ffi = i::Handle::cast(v8::Utils::OpenHandle(*obj)); } + // The memory argument is a legacy, not spec - compliant artifact. i::Handle memory = i::Handle::null(); if (args.Length() > kMemOffset && !args[kMemOffset]->IsUndefined()) { if (!args[kMemOffset]->IsObject()) { @@ -317,8 +322,8 @@ void WebAssemblyInstance(const v8::FunctionCallbackInfo& args) { } Local module_obj = Local::Cast(args[0]); - i::Handle i_module_obj = - i::Handle::cast(v8::Utils::OpenHandle(*module_obj)); + i::Handle i_module_obj = + i::Handle::cast(v8::Utils::OpenHandle(*module_obj)); MaybeLocal instance = InstantiateModuleImpl(i_isolate, i_module_obj, args, &thrower); @@ -339,7 +344,7 @@ void WebAssemblyInstantiate(const v8::FunctionCallbackInfo& args) { thrower.TypeError("Argument 0 must be a buffer source"); return; } - i::MaybeHandle module_obj = + i::MaybeHandle module_obj = CreateModuleObject(isolate, args[0], &thrower); Local context = isolate->GetCurrentContext(); diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc index 17f8280974..b3c2d433e1 100644 --- a/src/wasm/wasm-module.cc +++ b/src/wasm/wasm-module.cc @@ -1121,9 +1121,10 @@ void wasm::UpdateDispatchTables(Isolate* isolate, class WasmInstanceBuilder { public: WasmInstanceBuilder(Isolate* isolate, ErrorThrower* thrower, - Handle module_object, Handle ffi, - Handle memory) + Handle module_object, + Handle ffi, Handle memory) : isolate_(isolate), + module_(module_object->compiled_module()->module()), thrower_(thrower), module_object_(module_object), ffi_(ffi), @@ -1132,6 +1133,15 @@ class WasmInstanceBuilder { // Build an instance, in all of its glory. MaybeHandle Build() { MaybeHandle nothing; + + // Check that an imports argument was provided, if the module requires it. + // No point in continuing otherwise. + if (!module_->import_table.empty() && ffi_.is_null()) { + thrower_->TypeError( + "Imports argument must be present and must be an object"); + return nothing; + } + HistogramTimerScope wasm_instantiate_module_time_scope( isolate_->counters()->wasm_instantiate_module_time()); Factory* factory = isolate_->factory(); @@ -1154,8 +1164,7 @@ class WasmInstanceBuilder { Handle original; { DisallowHeapAllocation no_gc; - original = handle( - WasmCompiledModule::cast(module_object_->GetInternalField(0))); + original = handle(module_object_->compiled_module()); if (original->has_weak_owning_instance()) { owner = handle(WasmInstanceObject::cast( original->weak_owning_instance()->value())); @@ -1206,7 +1215,6 @@ class WasmInstanceBuilder { compiled_module_->set_code_table(code_table); compiled_module_->set_native_context(isolate_->native_context()); } - module_ = compiled_module_->module(); //-------------------------------------------------------------------------- // Allocate the instance object. @@ -1442,7 +1450,7 @@ class WasmInstanceBuilder { DCHECK(!isolate_->has_pending_exception()); TRACE("Finishing instance %d\n", compiled_module_->instance_id()); - TRACE_CHAIN(WasmCompiledModule::cast(module_object_->GetInternalField(0))); + TRACE_CHAIN(module_object_->compiled_module()); return instance; } @@ -1456,9 +1464,9 @@ class WasmInstanceBuilder { }; Isolate* isolate_; - WasmModule* module_; + WasmModule* const module_; ErrorThrower* thrower_; - Handle module_object_; + Handle module_object_; Handle ffi_; Handle memory_; Handle globals_; @@ -1475,9 +1483,9 @@ class WasmInstanceBuilder { import_name->length(), import_name->ToCString().get(), error); } - MaybeHandle ReportTypeError(const char* error, uint32_t index, + MaybeHandle ReportLinkError(const char* error, uint32_t index, Handle module_name) { - thrower_->TypeError("Import #%d module=\"%.*s\" error: %s", index, + thrower_->LinkError("Import #%d module=\"%.*s\" error: %s", index, module_name->length(), module_name->ToCString().get(), error); return MaybeHandle(); @@ -1486,22 +1494,22 @@ class WasmInstanceBuilder { // Look up an import value in the {ffi_} object. MaybeHandle LookupImport(uint32_t index, Handle module_name, Handle import_name) { - if (ffi_.is_null()) { - return ReportTypeError("FFI is not an object", index, module_name); - } + // We pre-validated in the js-api layer that the ffi object is present, and + // a JSObject, if the module has imports. + DCHECK(!ffi_.is_null()); // Look up the module first. MaybeHandle result = Object::GetPropertyOrElement(ffi_, module_name); if (result.is_null()) { - return ReportTypeError("module not found", index, module_name); + return ReportLinkError("module not found", index, module_name); } Handle module = result.ToHandleChecked(); // Look up the value in the module. if (!module->IsJSReceiver()) { - return ReportTypeError("module is not an object or function", index, + return ReportLinkError("module is not an object or function", index, module_name); } @@ -2101,8 +2109,9 @@ class WasmInstanceBuilder { // Instantiates a WASM module, creating a WebAssembly.Instance from a // WebAssembly.Module. MaybeHandle WasmModule::Instantiate( - Isolate* isolate, ErrorThrower* thrower, Handle wasm_module, - Handle ffi, Handle memory) { + Isolate* isolate, ErrorThrower* thrower, + Handle wasm_module, Handle ffi, + Handle memory) { WasmInstanceBuilder builder(isolate, thrower, wasm_module, ffi, memory); return builder.Build(); } diff --git a/src/wasm/wasm-module.h b/src/wasm/wasm-module.h index ae53f73675..ff680711df 100644 --- a/src/wasm/wasm-module.h +++ b/src/wasm/wasm-module.h @@ -216,8 +216,9 @@ struct V8_EXPORT_PRIVATE WasmModule { // Creates a new instantiation of the module in the given isolate. static MaybeHandle Instantiate( - Isolate* isolate, ErrorThrower* thrower, Handle wasm_module, - Handle ffi, Handle memory); + Isolate* isolate, ErrorThrower* thrower, + Handle wasm_module, Handle ffi, + Handle memory); MaybeHandle CompileFunctions( Isolate* isolate, Handle> module_wrapper, diff --git a/test/cctest/wasm/test-run-wasm-module.cc b/test/cctest/wasm/test-run-wasm-module.cc index 54f851bab5..4938408a1d 100644 --- a/test/cctest/wasm/test-run-wasm-module.cc +++ b/test/cctest/wasm/test-run-wasm-module.cc @@ -240,8 +240,8 @@ class WasmSerializationTest { ErrorThrower thrower(current_isolate(), ""); v8::Local deserialized_module; CHECK(Deserialize().ToLocal(&deserialized_module)); - Handle module_object = - Handle::cast(v8::Utils::OpenHandle(*deserialized_module)); + Handle module_object = Handle::cast( + v8::Utils::OpenHandle(*deserialized_module)); { DisallowHeapAllocation assume_no_gc; Handle compiled_part( diff --git a/test/mjsunit/wasm/errors.js b/test/mjsunit/wasm/errors.js index 7dbd6e5cb6..b87702fa83 100644 --- a/test/mjsunit/wasm/errors.js +++ b/test/mjsunit/wasm/errors.js @@ -25,6 +25,23 @@ function instance(bytes, imports = {}) { return new WebAssembly.Instance(module(bytes), imports); } +// instantiate should succeed but run should fail. +function instantiateAndFailAtRuntime(bytes, imports = {}) { + var instance = undefined; + try { + instance = new WebAssembly.Instance(module(bytes), imports); + } catch(e) { + // If we fail at startup. + if (e instanceof WebAssembly.RuntimeError) { + throw e; + } + // Swallow other instantiation errors because we expect instantiation + // to succeed but runtime to fail. + return; + } + instance.exports.run(); +} + function builder() { return new WasmModuleBuilder; } @@ -33,21 +50,23 @@ function assertCompileError(bytes) { assertThrows(() => module(bytes), WebAssembly.CompileError); } +// default imports to {} so we get LinkError by default, thus allowing us to +// distinguish the TypeError we want to catch function assertTypeError(bytes, imports = {}) { assertThrows(() => instance(bytes, imports), TypeError); } -function assertLinkError(bytes, imports = {}) { +function assertLinkError(bytes, imports) { assertThrows(() => instance(bytes, imports), WebAssembly.LinkError); } -function assertRuntimeError(bytes, imports = {}) { - assertThrows(() => instance(bytes, imports).exports.run(), +function assertRuntimeError(bytes, imports) { + assertThrows(() => instantiateAndFailAtRuntime(bytes, imports), WebAssembly.RuntimeError); } -function assertConversionError(bytes, imports = {}) { - assertThrows(() => instance(bytes, imports).exports.run(), TypeError); +function assertConversionError(bytes, imports) { + assertThrows(() => instantiateAndFailAtRuntime(bytes, imports), TypeError); } (function TestDecodingError() { @@ -72,7 +91,7 @@ function assertConversionError(bytes, imports = {}) { b = builder(); b.addImport("foo", "bar", kSig_v_v); - assertTypeError(b.toBuffer(), {}); + assertLinkError(b.toBuffer(), {}); b = builder(); b.addImport("foo", "bar", kSig_v_v); assertLinkError(b.toBuffer(), {foo: {}}); @@ -82,7 +101,7 @@ function assertConversionError(bytes, imports = {}) { b = builder(); b.addImportedGlobal("foo", "bar", kWasmI32); - assertTypeError(b.toBuffer(), {}); + assertLinkError(b.toBuffer(), {}); b = builder(); b.addImportedGlobal("foo", "bar", kWasmI32); assertLinkError(b.toBuffer(), {foo: {}}); @@ -95,7 +114,7 @@ function assertConversionError(bytes, imports = {}) { b = builder(); b.addImportedMemory("foo", "bar"); - assertTypeError(b.toBuffer(), {}); + assertLinkError(b.toBuffer(), {}); b = builder(); b.addImportedMemory("foo", "bar"); assertLinkError(b.toBuffer(), {foo: {}}); @@ -105,7 +124,7 @@ function assertConversionError(bytes, imports = {}) { {foo: {bar: () => new WebAssembly.Memory({initial: 0})}}); b = builder(); - b.addFunction("f", kSig_v_v).addBody([ + b.addFunction("startup", kSig_v_v).addBody([ kExprUnreachable, ]).end().addStart(0); assertRuntimeError(b.toBuffer()); @@ -135,8 +154,9 @@ function assertConversionError(bytes, imports = {}) { b.addImport("foo", "bar", kSig_v_l); assertConversionError(b.addFunction("run", kSig_v_v).addBody([ kExprI64Const, 0, kExprCallFunction, 0 - ]).exportFunc().end().toBuffer()); + ]).exportFunc().end().toBuffer(), {foo:{bar: (l)=>{}}}); + b = builder() assertConversionError(builder().addFunction("run", kSig_l_v).addBody([ kExprI64Const, 0 ]).exportFunc().end().toBuffer()); diff --git a/test/mjsunit/wasm/js-api.js b/test/mjsunit/wasm/js-api.js index 1106853b43..812fb06b8d 100644 --- a/test/mjsunit/wasm/js-api.js +++ b/test/mjsunit/wasm/js-api.js @@ -41,6 +41,12 @@ let importingModuleBinary = (() => { return new Int8Array(builder.toBuffer()); })(); +let memoryImportingModuleBinary = (() => { + var builder = new WasmModuleBuilder(); + builder.addImportedMemory("", "my_memory"); + return new Int8Array(builder.toBuffer()); +})(); + let moduleBinaryImporting2Memories = (() => { var builder = new WasmModuleBuilder(); builder.addImportedMemory("", "memory1"); @@ -256,10 +262,16 @@ let Instance = WebAssembly.Instance; assertEq(Instance, instanceDesc.value); assertEq(Instance.length, 1); assertEq(Instance.name, "Instance"); + assertErrorMessage(() => Instance(), TypeError, /constructor without new is forbidden/); assertErrorMessage(() => new Instance(1), TypeError, "first argument must be a WebAssembly.Module"); assertErrorMessage(() => new Instance({}), TypeError, "first argument must be a WebAssembly.Module"); assertErrorMessage(() => new Instance(emptyModule, null), TypeError, "second argument must be an object"); +assertErrorMessage(() => new Instance(importingModule, null), TypeError, ""); +assertErrorMessage(() => new Instance(importingModule, undefined), TypeError, ""); +assertErrorMessage(() => new Instance(importingModule, {"":{g:()=>{}}}), LinkError, ""); +assertErrorMessage(() => new Instance(importingModule, {t:{f:()=>{}}}), LinkError, ""); + assertEq(new Instance(emptyModule) instanceof Instance, true); assertEq(new Instance(emptyModule, {}) instanceof Instance, true); @@ -563,55 +575,63 @@ function assertCompileSuccess(bytes) { assertCompileSuccess(emptyModuleBinary); assertCompileSuccess(emptyModuleBinary.buffer); -if (false) { // TODO: implement WebAssembly.instantiate - // 'WebAssembly.instantiate' data property - let instantiateDesc = Object.getOwnPropertyDescriptor(WebAssembly, 'instantiate'); - assertEq(typeof instantiateDesc.value, "function"); - assertEq(instantiateDesc.writable, true); - assertEq(instantiateDesc.enumerable, false); - assertEq(instantiateDesc.configurable, true); +// 'WebAssembly.instantiate' data property +let instantiateDesc = Object.getOwnPropertyDescriptor(WebAssembly, 'instantiate'); +assertEq(typeof instantiateDesc.value, "function"); +assertEq(instantiateDesc.writable, true); +assertEq(instantiateDesc.enumerable, false); +assertEq(instantiateDesc.configurable, true); - // 'WebAssembly.instantiate' function - let instantiate = WebAssembly.instantiate; - assertEq(instantiate, instantiateDesc.value); - assertEq(instantiate.length, 2); - assertEq(instantiate.name, "instantiate"); - function assertInstantiateError(args, err, msg) { - var error = null; - try { - instantiate(...args).catch(e => error = e); - } catch(e) { - error = e; - } - drainJobQueue(); - assertEq(error instanceof err, true); - assertEq(Boolean(error.stack.match("jsapi.js")), true); - assertEq(Boolean(error.message.match(msg)), true); +// 'WebAssembly.instantiate' function +let instantiate = WebAssembly.instantiate; +assertEq(instantiate, instantiateDesc.value); +assertEq(instantiate.length, 1); +assertEq(instantiate.name, "instantiate"); +function assertInstantiateError(args, err, msg) { + var error = null; + try { + instantiate(...args).catch(e => error = e); + } catch(e) { + error = e; } - assertInstantiateError([], TypeError, /requires more than 0 arguments/); - assertInstantiateError([undefined], TypeError, /first argument must be a WebAssembly.Module, ArrayBuffer or typed array object/); - assertInstantiateError([1], TypeError, /first argument must be a WebAssembly.Module, ArrayBuffer or typed array object/); - assertInstantiateError([{}], TypeError, /first argument must be a WebAssembly.Module, ArrayBuffer or typed array object/); - assertInstantiateError([new Uint8Array()], CompileError, /failed to match magic number/); - assertInstantiateError([new ArrayBuffer()], CompileError, /failed to match magic number/); - assertInstantiateError([importingModule], TypeError, /second argument must be an object/); - assertInstantiateError([importingModule, null], TypeError, /second argument must be an object/); - assertInstantiateError([importingModuleBinary, null], TypeError, /second argument must be an object/); - function assertInstantiateSuccess(module, imports) { - var result = null; - instantiate(module, imports).then(r => result = r); - drainJobQueue(); - if (module instanceof Module) { - assertEq(result instanceof Instance, true); - } else { - assertEq(result.module instanceof Module, true); - assertEq(result.instance instanceof Instance, true); - } - } - assertInstantiateSuccess(emptyModule); - assertInstantiateSuccess(emptyModuleBinary); - assertInstantiateSuccess(emptyModuleBinary.buffer); - assertInstantiateSuccess(importingModule, {"":{f:()=>{}}}); - assertInstantiateSuccess(importingModuleBinary, {"":{f:()=>{}}}); - assertInstantiateSuccess(importingModuleBinary.buffer, {"":{f:()=>{}}}); + drainJobQueue(); + assertEq(error instanceof err, true); + assertEq(Boolean(error.stack.match("js-api.js")), true); + //TOassertEq(Boolean(error.message.match(msg)), true); } +var scratch_memory = new WebAssembly.Memory(new ArrayBuffer(10)); +assertInstantiateError([], TypeError, /requires more than 0 arguments/); +assertInstantiateError([undefined], TypeError, /first argument must be a BufferSource/); +assertInstantiateError([1], TypeError, /first argument must be a BufferSource/); +assertInstantiateError([{}], TypeError, /first argument must be a BufferSource/); +assertInstantiateError([new Uint8Array()], CompileError, /failed to match magic number/); +assertInstantiateError([new ArrayBuffer()], CompileError, /failed to match magic number/); +assertInstantiateError([new Uint8Array("hi!")], CompileError, /failed to match magic number/); +assertInstantiateError([new ArrayBuffer("hi!")], CompileError, /failed to match magic number/); +assertInstantiateError([importingModule], TypeError, /second argument must be an object/); +assertInstantiateError([importingModule, null], TypeError, /second argument must be an object/); +assertInstantiateError([importingModuleBinary, null], TypeError, /second argument must be an object/); +assertInstantiateError([emptyModule, null], TypeError, /first argument must be a BufferSource/); +assertInstantiateError([importingModule, {"":{f:()=>{}}}], TypeError, /first argument must be a BufferSource/); +assertInstantiateError([importingModuleBinary, null], TypeError, /TODO: error messages?/); +assertInstantiateError([importingModuleBinary, undefined], TypeError, /TODO: error messages?/); +assertInstantiateError([importingModuleBinary, {}], LinkError, /TODO: error messages?/); +assertInstantiateError([importingModuleBinary, {"":{g:()=>{}}}], LinkError, /TODO: error messages?/); +assertInstantiateError([importingModuleBinary, {t:{f:()=>{}}}], LinkError, /TODO: error messages?/); +assertInstantiateError([memoryImportingModuleBinary, null], TypeError, /TODO: error messages?/); +assertInstantiateError([memoryImportingModuleBinary, undefined], TypeError, /TODO: error messages?/); +assertInstantiateError([memoryImportingModuleBinary, {}], LinkError, /TODO: error messages?/); +assertInstantiateError([memoryImportingModuleBinary, {"mod": {"my_memory": scratch_memory}}], LinkError, /TODO: error messages?/); +assertInstantiateError([memoryImportingModuleBinary, {"": {"memory": scratch_memory}}], LinkError, /TODO: error messages?/); + +function assertInstantiateSuccess(module_bytes, imports) { + var result = null; + instantiate(module_bytes, imports).then(r => result = r).catch(e => print(e)); + drainJobQueue(); + assertEq(result instanceof Instance, true); +} +assertInstantiateSuccess(emptyModuleBinary); +assertInstantiateSuccess(emptyModuleBinary.buffer); +assertInstantiateSuccess(importingModuleBinary, {"":{f:()=>{}}}); +assertInstantiateSuccess(importingModuleBinary.buffer, {"":{f:()=>{}}}); +assertInstantiateSuccess(memoryImportingModuleBinary, {"": {"my_memory": scratch_memory}});