Implement LinkError; import tweaks

- Implement new WebAssembly.LinkError exception
- Implement stricter checks for glboal imports
- Add tests
- Refactor handling of import names
- Add TODOs for empty import names

R=titzer@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2584843002
Cr-Commit-Position: refs/heads/master@{#41764}

src/bootstrapper.cc

@@ -2220,6 +2220,10 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
     InstallError(isolate, dummy, factory->CompileError_string(),
                  Context::WASM_COMPILE_ERROR_FUNCTION_INDEX);
 
+    // -- L i n k E r r o r
+    InstallError(isolate, dummy, factory->LinkError_string(),
+                 Context::WASM_LINK_ERROR_FUNCTION_INDEX);
+
     // -- R u n t i m e E r r o r
     InstallError(isolate, dummy, factory->RuntimeError_string(),
                  Context::WASM_RUNTIME_ERROR_FUNCTION_INDEX);

src/contexts.h

@@ -126,6 +126,7 @@ enum ContextLookupFlags {
   V(URI_ERROR_FUNCTION_INDEX, JSFunction, uri_error_function)                 \
   V(WASM_COMPILE_ERROR_FUNCTION_INDEX, JSFunction,                            \
     wasm_compile_error_function)                                              \
+  V(WASM_LINK_ERROR_FUNCTION_INDEX, JSFunction, wasm_link_error_function)     \
   V(WASM_RUNTIME_ERROR_FUNCTION_INDEX, JSFunction, wasm_runtime_error_function)
 
 #define NATIVE_CONTEXT_JS_ARRAY_ITERATOR_MAPS(V)                               \

src/factory.cc

@@ -1370,6 +1370,7 @@ DEFINE_ERROR(ReferenceError, reference_error)
 DEFINE_ERROR(SyntaxError, syntax_error)
 DEFINE_ERROR(TypeError, type_error)
 DEFINE_ERROR(WasmCompileError, wasm_compile_error)
+DEFINE_ERROR(WasmLinkError, wasm_link_error)
 DEFINE_ERROR(WasmRuntimeError, wasm_runtime_error)
 #undef DEFINE_ERROR
 

src/factory.h

@@ -649,6 +649,7 @@ class V8_EXPORT_PRIVATE Factory final {
   DECLARE_ERROR(SyntaxError)
   DECLARE_ERROR(TypeError)
   DECLARE_ERROR(WasmCompileError)
+  DECLARE_ERROR(WasmLinkError)
   DECLARE_ERROR(WasmRuntimeError)
 #undef DECLARE_ERROR
 

src/heap-symbols.h

@@ -164,6 +164,7 @@
   V(TypeError_string, "TypeError")                                 \
   V(type_string, "type")                                           \
   V(CompileError_string, "CompileError")                           \
+  V(LinkError_string, "LinkError")                                 \
   V(RuntimeError_string, "RuntimeError")                           \
   V(uint16x8_string, "uint16x8")                                   \
   V(Uint16x8_string, "Uint16x8")                                   \

src/wasm/wasm-js.cc

@@ -699,6 +699,10 @@ void WasmJs::InstallWasmConstructors(Isolate* isolate,
       isolate->native_context()->wasm_compile_error_function());
   JSObject::AddProperty(webassembly, isolate->factory()->CompileError_string(),
                         compile_error, attributes);
+  Handle<JSFunction> link_error(
+      isolate->native_context()->wasm_link_error_function());
+  JSObject::AddProperty(webassembly, isolate->factory()->LinkError_string(),
+                        link_error, attributes);
   Handle<JSFunction> runtime_error(
       isolate->native_context()->wasm_runtime_error_function());
   JSObject::AddProperty(webassembly, isolate->factory()->RuntimeError_string(),

src/wasm/wasm-module.cc

@@ -1390,60 +1390,55 @@ class WasmInstanceBuilder {
   std::vector<TableInstance> table_instances_;
   std::vector<Handle<JSFunction>> js_wrappers_;
 
-  // Helper routine to print out errors with imports (FFI).
-  MaybeHandle<JSFunction> ReportFFIError(const char* error, uint32_t index,
+  // Helper routines to print out errors with imports.
+  void ReportLinkError(const char* error, uint32_t index,
                        Handle<String> module_name,
-                                         MaybeHandle<String> function_name) {
-    Handle<String> function_name_handle;
-    if (function_name.ToHandle(&function_name_handle)) {
-      thrower_->TypeError(
+                       Handle<String> import_name) {
+    thrower_->LinkError(
         "Import #%d module=\"%.*s\" function=\"%.*s\" error: %s", index,
         module_name->length(), module_name->ToCString().get(),
-          function_name_handle->length(),
-          function_name_handle->ToCString().get(), error);
-    } else {
+        import_name->length(), import_name->ToCString().get(), error);
+  }
+
+  MaybeHandle<Object> ReportTypeError(const char* error, uint32_t index,
+                                      Handle<String> module_name) {
     thrower_->TypeError("Import #%d module=\"%.*s\" error: %s", index,
                         module_name->length(), module_name->ToCString().get(),
                         error);
-    }
-    thrower_->TypeError("Import ");
-    return MaybeHandle<JSFunction>();
+    return MaybeHandle<Object>();
   }
 
   // Look up an import value in the {ffi_} object.
   MaybeHandle<Object> LookupImport(uint32_t index, Handle<String> module_name,
-                                   MaybeHandle<String> import_name) {
+                                   Handle<String> import_name) {
     if (ffi_.is_null()) {
-      return ReportFFIError("FFI is not an object", index, module_name,
-                            import_name);
+      return ReportTypeError("FFI is not an object", index, module_name);
     }
 
     // Look up the module first.
     MaybeHandle<Object> result =
         Object::GetPropertyOrElement(ffi_, module_name);
     if (result.is_null()) {
-      return ReportFFIError("module not found", index, module_name,
-                            import_name);
+      return ReportTypeError("module not found", index, module_name);
     }
 
     Handle<Object> module = result.ToHandleChecked();
 
-    if (!import_name.is_null()) {
+    // TODO(bradnelson): Making this conditional on non-empty names violates the
+    // Wasm spec, but seems to be a hack intended for the asm-to-wasm pipeline.
+    // We need to get rid of it.
+    if (import_name->length() != 0) {
       // Look up the value in the module.
       if (!module->IsJSReceiver()) {
-        return ReportFFIError("module is not an object or function", index,
-                              module_name, import_name);
+        return ReportTypeError("module is not an object or function", index,
+                             module_name);
       }
 
-      result =
-          Object::GetPropertyOrElement(module, import_name.ToHandleChecked());
+      result = Object::GetPropertyOrElement(module, import_name);
       if (result.is_null()) {
-        return ReportFFIError("import not found", index, module_name,
-                              import_name);
+        ReportLinkError("import not found", index, module_name, import_name);
+        return MaybeHandle<JSFunction>();
       }
-    } else {
-      // No function specified. Use the "default export".
-      result = module;
     }
 
     return result;
@@ -1473,7 +1468,7 @@ class WasmInstanceBuilder {
       uint32_t dest_offset = EvalUint32InitExpr(segment.dest_addr);
       if (dest_offset >= mem_size || source_size >= mem_size ||
           dest_offset > (mem_size - source_size)) {
-        thrower_->TypeError("data segment (start = %" PRIu32 ", size = %" PRIu32
+        thrower_->LinkError("data segment (start = %" PRIu32 ", size = %" PRIu32
                             ") does not fit into memory (size = %" PRIuS ")",
                             dest_offset, source_size, mem_size);
         return;
@@ -1525,40 +1520,43 @@ class WasmInstanceBuilder {
     for (int index = 0; index < static_cast<int>(module_->import_table.size());
          ++index) {
       WasmImport& import = module_->import_table[index];
-      Handle<String> module_name =
+
+      Handle<String> module_name;
+      MaybeHandle<String> maybe_module_name =
           ExtractStringFromModuleBytes(isolate_, compiled_module_,
                                        import.module_name_offset,
-                                       import.module_name_length)
-              .ToHandleChecked();
-      Handle<String> function_name = Handle<String>::null();
-      if (import.field_name_length > 0) {
-        function_name = ExtractStringFromModuleBytes(isolate_, compiled_module_,
+                                       import.module_name_length);
+      if (!maybe_module_name.ToHandle(&module_name)) return -1;
+
+      Handle<String> import_name;
+      MaybeHandle<String> maybe_import_name =
+          ExtractStringFromModuleBytes(isolate_, compiled_module_,
                                        import.field_name_offset,
-                                                     import.field_name_length)
-                            .ToHandleChecked();
-      }
+                                       import.field_name_length);
+      if (!maybe_import_name.ToHandle(&import_name)) return -1;
 
       MaybeHandle<Object> result =
-          LookupImport(index, module_name, function_name);
+          LookupImport(index, module_name, import_name);
       if (thrower_->error()) return -1;
+      Handle<Object> value = result.ToHandleChecked();
 
       switch (import.kind) {
         case kExternalFunction: {
           // Function imports must be callable.
-          Handle<Object> function = result.ToHandleChecked();
-          if (!function->IsCallable()) {
-            ReportFFIError("function import requires a callable", index,
-                           module_name, function_name);
+          if (!value->IsCallable()) {
+            ReportLinkError("function import requires a callable", index,
+                            module_name, import_name);
             return -1;
           }
 
           Handle<Code> import_wrapper = CompileImportWrapper(
               isolate_, index, module_->functions[import.index].sig,
-              Handle<JSReceiver>::cast(function), module_name, function_name,
+              Handle<JSReceiver>::cast(value), module_name, import_name,
               module_->origin);
           if (import_wrapper.is_null()) {
-            ReportFFIError("imported function does not match the expected type",
-                           index, module_name, function_name);
+            ReportLinkError(
+                "imported function does not match the expected type",
+                index, module_name, import_name);
             return -1;
           }
           code_table->set(num_imported_functions, *import_wrapper);
@@ -1567,10 +1565,9 @@ class WasmInstanceBuilder {
           break;
         }
         case kExternalTable: {
-          Handle<Object> value = result.ToHandleChecked();
           if (!WasmJs::IsWasmTableObject(isolate_, value)) {
-            ReportFFIError("table import requires a WebAssembly.Table", index,
-                           module_name, function_name);
+            ReportLinkError("table import requires a WebAssembly.Table", index,
+                            module_name, import_name);
             return -1;
           }
           WasmIndirectFunctionTable& table =
@@ -1583,7 +1580,7 @@ class WasmInstanceBuilder {
           // TODO(titzer): import table size must match exactly for now.
           int table_size = table_instance.js_wrappers->length();
           if (table_size != static_cast<int>(table.min_size)) {
-            thrower_->TypeError(
+            thrower_->LinkError(
                 "table import %d is wrong size (%d), expected %u", index,
                 table_size, table.min_size);
             return -1;
@@ -1604,7 +1601,7 @@ class WasmInstanceBuilder {
             WasmFunction* function =
                 GetWasmFunctionForImportWrapper(isolate_, val);
             if (function == nullptr) {
-              thrower_->TypeError("table import %d[%d] is not a WASM function",
+              thrower_->LinkError("table import %d[%d] is not a WASM function",
                                   index, i);
               return -1;
             }
@@ -1618,13 +1615,12 @@ class WasmInstanceBuilder {
           break;
         }
         case kExternalMemory: {
-          Handle<Object> object = result.ToHandleChecked();
-          if (!WasmJs::IsWasmMemoryObject(isolate_, object)) {
-            ReportFFIError("memory import must be a WebAssembly.Memory object",
-                           index, module_name, function_name);
+          if (!WasmJs::IsWasmMemoryObject(isolate_, value)) {
+            ReportLinkError("memory import must be a WebAssembly.Memory object",
+                            index, module_name, import_name);
             return -1;
           }
-          auto memory = Handle<WasmMemoryObject>::cast(object);
+          auto memory = Handle<WasmMemoryObject>::cast(value);
           DCHECK(WasmJs::IsWasmMemoryObject(isolate_, memory));
           instance->set_memory_object(*memory);
           memory_ = Handle<JSArrayBuffer>(memory->get_buffer(), isolate_);
@@ -1633,15 +1629,12 @@ class WasmInstanceBuilder {
         case kExternalGlobal: {
           // Global imports are converted to numbers and written into the
           // {globals_} array buffer.
-          Handle<Object> object = result.ToHandleChecked();
-          MaybeHandle<Object> number = Object::ToNumber(object);
-          if (number.is_null()) {
-            ReportFFIError("global import could not be converted to number",
-                           index, module_name, function_name);
+          if (!value->IsNumber()) {
+            ReportLinkError("global import must be a number",
+                            index, module_name, import_name);
             return -1;
           }
-          Handle<Object> val = number.ToHandleChecked();
-          WriteGlobalValue(module_->globals[import.index], val);
+          WriteGlobalValue(module_->globals[import.index], value);
           break;
         }
         default:
@@ -1863,7 +1856,7 @@ class WasmInstanceBuilder {
       v8::Maybe<bool> status = JSReceiver::DefineOwnProperty(
           isolate_, exports_object, name, &desc, Object::THROW_ON_ERROR);
       if (!status.IsJust()) {
-        thrower_->TypeError("export of %.*s failed.", name->length(),
+        thrower_->LinkError("export of %.*s failed.", name->length(),
                             name->ToCString().get());
         return;
       }
@@ -1913,7 +1906,7 @@ class WasmInstanceBuilder {
         if (base > static_cast<uint32_t>(table_size) ||
             (base + table_init.entries.size() >
              static_cast<uint32_t>(table_size))) {
-          thrower_->CompileError("table initializer is out of bounds");
+          thrower_->LinkError("table initializer is out of bounds");
           continue;
         }
         for (int i = 0; i < static_cast<int>(table_init.entries.size()); ++i) {

src/wasm/wasm-result.cc

@@ -70,6 +70,14 @@ void ErrorThrower::CompileError(const char* format, ...) {
   va_end(arguments);
 }
 
+void ErrorThrower::LinkError(const char* format, ...) {
+  if (error()) return;
+  va_list arguments;
+  va_start(arguments, format);
+  Format(isolate_->wasm_link_error_function(), format, arguments);
+  va_end(arguments);
+}
+
 void ErrorThrower::RuntimeError(const char* format, ...) {
   if (error()) return;
   va_list arguments;

src/wasm/wasm-result.h

@@ -95,6 +95,7 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   PRINTF_FORMAT(2, 3) void TypeError(const char* fmt, ...);
   PRINTF_FORMAT(2, 3) void RangeError(const char* fmt, ...);
   PRINTF_FORMAT(2, 3) void CompileError(const char* fmt, ...);
+  PRINTF_FORMAT(2, 3) void LinkError(const char* fmt, ...);
   PRINTF_FORMAT(2, 3) void RuntimeError(const char* fmt, ...);
 
   template <typename T>

test/mjsunit/wasm/errors.js

@@ -33,10 +33,14 @@ function assertCompileError(bytes) {
   assertThrows(() => module(bytes), WebAssembly.CompileError);
 }
 
-function assertLinkError(bytes, imports = {}) {
+function assertTypeError(bytes, imports = {}) {
   assertThrows(() => instance(bytes, imports), TypeError);
 }
 
+function assertLinkError(bytes, imports = {}) {
+  assertThrows(() => instance(bytes, imports), WebAssembly.LinkError);
+}
+
 function assertRuntimeError(bytes, imports = {}) {
   assertThrows(() => instance(bytes, imports).exports.run(),
     WebAssembly.RuntimeError);
@@ -68,7 +72,7 @@ function assertConversionError(bytes, imports = {}) {
 
   b = builder();
   b.addImportWithModule("foo", "bar", kSig_v_v);
-  assertLinkError(b.toBuffer(), {});
+  assertTypeError(b.toBuffer(), {});
   b = builder();
   b.addImportWithModule("foo", "bar", kSig_v_v);
   assertLinkError(b.toBuffer(), {foo: {}});
@@ -78,29 +82,33 @@ function assertConversionError(bytes, imports = {}) {
 
   b = builder();
   b.addImportedGlobal("foo", "bar", kAstI32);
-  assertLinkError(b.toBuffer(), {});
-  // TODO(titzer): implement stricter import checks for globals.
-  // b = builder();
-  // b.addImportedGlobal("foo", "bar", kAstI32);
-  // assertLinkError(b.toBuffer(), {foo: {}});
-  // b = builder();
-  // b.addImportedGlobal("foo", "bar", kAstI32);
-  // assertLinkError(b.toBuffer(), {foo: {bar: ""}});
-  // b = builder();
-  // b.addImportedGlobal("foo", "bar", kAstI32);
-  // assertLinkError(b.toBuffer(), {foo: {bar: () => 9}});
+  assertTypeError(b.toBuffer(), {});
+  b = builder();
+  b.addImportedGlobal("foo", "bar", kAstI32);
+  assertLinkError(b.toBuffer(), {foo: {}});
+  b = builder();
+  b.addImportedGlobal("foo", "bar", kAstI32);
+  assertLinkError(b.toBuffer(), {foo: {bar: ""}});
+  b = builder();
+  b.addImportedGlobal("foo", "bar", kAstI32);
+  assertLinkError(b.toBuffer(), {foo: {bar: () => 9}});
 
   b = builder();
   b.addImportedMemory("foo", "bar");
-  assertLinkError(b.toBuffer(), {});
+  assertTypeError(b.toBuffer(), {});
   b = builder();
   b.addImportedMemory("foo", "bar");
   assertLinkError(b.toBuffer(), {foo: {}});
-  // TODO(titzer): implement stricter import checks for globals.
-  // b = builder();
-  // b.addImportedMemory("foo", "bar", 1);
-  // assertLinkError(b.toBuffer(),
-  //     {foo: {bar: new WebAssembly.Memory({initial: 0})}});
+  b = builder();
+  b.addImportedMemory("foo", "bar", 1);
+  assertLinkError(b.toBuffer(),
+      {foo: {bar: () => new WebAssembly.Memory({initial: 0})}});
+
+  b = builder();
+  b.addFunction("f", kSig_v_v).addBody([
+    kExprUnreachable,
+  ]).end().addStart(0);
+  assertRuntimeError(b.toBuffer());
 })();
 
 (function TestTrapError() {
@@ -128,6 +136,7 @@ function assertConversionError(bytes, imports = {}) {
   assertConversionError(b.addFunction("run", kSig_v_v).addBody([
     kExprI64Const, 0, kExprCallFunction, 0
   ]).exportFunc().end().toBuffer());
+
   assertConversionError(builder().addFunction("run", kSig_l_v).addBody([
     kExprI64Const, 0
   ]).exportFunc().end().toBuffer());

test/mjsunit/wasm/export-table.js

@@ -54,6 +54,27 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
   assertSame(module.exports.blah, module.exports.foo);
 })();
 
+(function testEmptyName() {
+  print("TestEmptyName...");
+  var kReturnValue = 93;
+
+  var builder = new WasmModuleBuilder();
+
+  builder.addFunction("main", kSig_i_v)
+    .addBody([
+      kExprI8Const,
+      kReturnValue,
+      kExprReturn
+    ])
+    .exportAs("");
+
+  var module = builder.instantiate();
+
+  assertEquals("object", typeof module.exports);
+  assertEquals("function", typeof module.exports[""]);
+
+  assertEquals(kReturnValue, module.exports[""]());
+})();
 
 (function testNumericName() {
   print("TestNumericName...");

test/mjsunit/wasm/globals.js

@@ -24,7 +24,6 @@ function TestImported(type, val, expected) {
 TestImported(kAstI32, 300.1, 300);
 TestImported(kAstF32, 87234.87238, Math.fround(87234.87238));
 TestImported(kAstF64, 77777.88888, 77777.88888);
-TestImported(kAstF64, "89", 89);
 
 
 function TestExported(type, val, expected) {

test/mjsunit/wasm/import-table.js

@@ -281,3 +281,23 @@ function testCallImport2(foo, bar, expected) {
 }
 
 testCallImport2(function() { return 33; }, function () { return 44; }, 77);
+
+
+function testImportName(name) {
+  var builder = new WasmModuleBuilder();
+  builder.addImportWithModule("M", name, kSig_i_v);
+  builder.addFunction("main", kSig_i_v)
+    .addBody([
+      kExprCallFunction, 0
+    ])
+    .exportFunc();
+
+  let main = builder.instantiate({M: {[name]: () => 42}}).exports.main;
+  assertEquals(42, main());
+}
+
+testImportName("bla");
+testImportName("0");
+testImportName("  a @#$2 324 ");
+// TODO(bradnelson): This should succeed.
+// testImportName("");