[wasm-gc] Extend js-compatible signatures to include typed functions

Changes:
- Extend IsJSCompatibleSignature to include typed functions.
- Generalize WasmIsValidFuncRefValue to WasmIsValidRefValue, utilize
  DynamicTypeCheckRef. Use it in FromJS.
- Extend DynamicTypeCheckRef to eqRef type and WasmJSFunction
  references.
- Update call-ref.js test.

Change-Id: I71166ab8c1e716c21e79776c561e77b443add1da
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2412527Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69981}

src/compiler/wasm-compiler.cc

@@ -3054,15 +3054,6 @@ Node* WasmGraphBuilder::BuildCallRef(uint32_t sig_index, Vector<Node*> args,
 
     {
       // Function imported to module.
-      // TODO(9495): Make sure it works with functions imported from other
-      // modules. Currently, this will never happen: Since functions have to be
-      // tunneled through JS, and we currently do not have a JS API to pass
-      // specific function types, we habe to export/import function references
-      // as funcref. Then, we cannot cast down to the type of the function,
-      // because we do not have access to the defining module's types. This
-      // could be fixed either by building a richer JS API, or by implementing
-      // the type import proposal. That said, this code should work for those
-      // cases too.
       gasm_->Bind(&imported_label);
 
       Node* imported_instance = gasm_->Load(
@@ -6068,6 +6059,10 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
         if (representation == wasm::HeapType::kEq) {
           return BuildAllocateObjectWrapper(node);
         }
+        if (type.has_index() && module_->has_signature(type.ref_index())) {
+          // Typed function
+          return node;
+        }
         // TODO(7748): Figure out a JS interop story for arrays and structs.
         // If this is reached, then IsJSCompatibleSignature() is too permissive.
         UNREACHABLE();
@@ -6153,36 +6148,52 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
         graph()->NewNode(call, target, input, context, effect(), control()));
   }
 
-  Node* FromJS(Node* input, Node* js_context, wasm::ValueType type) {
-    switch (type.kind()) {
-      case wasm::ValueType::kRef:
-      case wasm::ValueType::kOptRef: {
-        switch (type.heap_representation()) {
-          case wasm::HeapType::kExtern:
-          case wasm::HeapType::kExn:
-            return input;
-          case wasm::HeapType::kFunc: {
-            Node* check =
-                BuildChangeSmiToInt32(SetEffect(BuildCallToRuntimeWithContext(
-                    Runtime::kWasmIsValidFuncRefValue, js_context, &input, 1)));
+  void BuildCheckValidRefValue(Node* input, Node* js_context,
+                               wasm::ValueType type) {
+    // Make sure ValueType fits in a Smi.
+    STATIC_ASSERT(wasm::ValueType::kLastUsedBit + 1 <= kSmiValueSize);
+    Node* inputs[] = {
+        instance_node_.get(), input,
+        IntPtrConstant(IntToSmi(static_cast<int>(type.raw_bit_field())))};
 
-            Diamond type_check(graph(), mcgraph()->common(), check,
-                               BranchHint::kTrue);
+    Node* check = BuildChangeSmiToInt32(SetEffect(BuildCallToRuntimeWithContext(
+        Runtime::kWasmIsValidRefValue, js_context, inputs, 3)));
+
+    Diamond type_check(graph(), mcgraph()->common(), check, BranchHint::kTrue);
     type_check.Chain(control());
     SetControl(type_check.if_false);
 
     Node* old_effect = effect();
-            BuildCallToRuntimeWithContext(Runtime::kWasmThrowTypeError,
-                                          js_context, nullptr, 0);
+    BuildCallToRuntimeWithContext(Runtime::kWasmThrowTypeError, js_context,
+                                  nullptr, 0);
 
     SetEffectControl(type_check.EffectPhi(old_effect, effect()),
                      type_check.merge);
+  }
 
+  Node* FromJS(Node* input, Node* js_context, wasm::ValueType type) {
+    switch (type.kind()) {
+      case wasm::ValueType::kRef:
+      case wasm::ValueType::kOptRef: {
+        switch (type.heap_representation()) {
+          case wasm::HeapType::kExtern:
+          case wasm::HeapType::kExn:
+            return input;
+          case wasm::HeapType::kFunc:
+            BuildCheckValidRefValue(input, js_context, type);
             return input;
-          }
           case wasm::HeapType::kEq:
+            BuildCheckValidRefValue(input, js_context, type);
             return BuildUnpackObjectWrapper(input);
+          case wasm::HeapType::kI31:
+            // If this is reached, then IsJSCompatibleSignature() is too
+            // permissive.
+            UNREACHABLE();
           default:
+            if (module_->has_signature(type.ref_index())) {
+              BuildCheckValidRefValue(input, js_context, type);
+              return input;
+            }
             // If this is reached, then IsJSCompatibleSignature() is too
             // permissive.
             UNREACHABLE();

src/runtime/runtime-wasm.cc

@@ -87,20 +87,23 @@ Object ThrowWasmError(Isolate* isolate, MessageTemplate message) {
 }
 }  // namespace
 
-RUNTIME_FUNCTION(Runtime_WasmIsValidFuncRefValue) {
+RUNTIME_FUNCTION(Runtime_WasmIsValidRefValue) {
   // This code is called from wrappers, so the "thread is wasm" flag is not set.
   DCHECK(!trap_handler::IsThreadInWasm());
   HandleScope scope(isolate);
-  DCHECK_EQ(1, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(Object, function, 0);
+  DCHECK_EQ(3, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0)
+  CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
+  // Make sure ValueType fits properly in a Smi.
+  STATIC_ASSERT(wasm::ValueType::kLastUsedBit + 1 <= kSmiValueSize);
+  CONVERT_SMI_ARG_CHECKED(raw_type, 2);
 
-  if (function->IsNull(isolate)) {
-    return Smi::FromInt(true);
-  }
-  if (WasmExternalFunction::IsWasmExternalFunction(*function)) {
-    return Smi::FromInt(true);
-  }
-  return Smi::FromInt(false);
+  wasm::ValueType type = wasm::ValueType::FromRawBitField(raw_type);
+  const char* error_message;
+
+  bool result = internal::wasm::DynamicTypeCheckRef(
+      isolate, instance->module(), value, type, &error_message);
+  return Smi::FromInt(result);
 }
 
 RUNTIME_FUNCTION(Runtime_WasmMemoryGrow) {

src/runtime/runtime.h

@@ -577,7 +577,7 @@ namespace internal {
   F(WasmTableCopy, 6, 1)              \
   F(WasmTableGrow, 3, 1)              \
   F(WasmTableFill, 4, 1)              \
-  F(WasmIsValidFuncRefValue, 1, 1)    \
+  F(WasmIsValidRefValue, 3, 1)        \
   F(WasmCompileLazy, 2, 1)            \
   F(WasmTriggerTierUp, 1, 1)          \
   F(WasmDebugBreak, 0, 1)             \

src/wasm/value-type.h

@@ -398,18 +398,27 @@ class ValueType {
     return buf.str();
   }
 
+  static constexpr int kLastUsedBit = 30;
+
  private:
+  // We only use 31 bits so ValueType fits in a Smi. This can be changed if
+  // needed.
   static constexpr int kKindBits = 5;
   static constexpr int kHeapTypeBits = 20;
-  static constexpr int kDepthBits = 7;
+  static constexpr int kDepthBits = 6;
   STATIC_ASSERT(kV8MaxWasmTypes < (1u << kHeapTypeBits));
   // Note: we currently conservatively allow only 5 bits, but have room to
-  // store 7, so we can raise the limit if needed.
+  // store 6, so we can raise the limit if needed.
   STATIC_ASSERT(kV8MaxRttSubtypingDepth < (1u << kDepthBits));
   using KindField = base::BitField<Kind, 0, kKindBits>;
   using HeapTypeField = KindField::Next<uint32_t, kHeapTypeBits>;
   using DepthField = HeapTypeField::Next<uint8_t, kDepthBits>;
 
+  // This is implemented defensively against field order changes.
+  STATIC_ASSERT(kLastUsedBit == std::max(KindField::kLastUsedBit,
+                                         std::max(HeapTypeField::kLastUsedBit,
+                                                  DepthField::kLastUsedBit)));
+
   constexpr explicit ValueType(uint32_t bit_field) : bit_field_(bit_field) {}
 
   constexpr const char* kind_name() const {

src/wasm/wasm-objects.cc

@@ -953,7 +953,6 @@ MaybeHandle<WasmGlobalObject> WasmGlobalObject::New(
     // Disallow GC until all fields have acceptable types.
     DisallowHeapAllocation no_gc;
     if (!instance.is_null()) global_obj->set_instance(*instance);
-    global_obj->set_raw_type(0);
     global_obj->set_type(type);
     global_obj->set_offset(offset);
     global_obj->set_is_mutable(is_mutable);
@@ -2024,7 +2023,7 @@ bool DynamicTypeCheckRef(Isolate* isolate, const WasmModule* module,
     case ValueType::kRef:
       switch (expected.heap_representation()) {
         case HeapType::kFunc: {
-          if (!WasmExportedFunction::IsWasmExportedFunction(*value)) {
+          if (!WasmExternalFunction::IsWasmExternalFunction(*value)) {
             *error_message =
                 "function-typed object must be null (if nullable) or an "
                 "exported function";
@@ -2035,18 +2034,27 @@ bool DynamicTypeCheckRef(Isolate* isolate, const WasmModule* module,
         case HeapType::kExtern:
         case HeapType::kExn:
           return true;
-        case HeapType::kEq:
-        case HeapType::kI31:
-          // TODO(7748): Implement when the JS API for structs/arrays/i31ref is
-          // decided on.
+        case HeapType::kEq: {
+          // TODO(7748): Change this when we have a decision on the JS API for
+          // structs/arrays.
+          Handle<Name> key = isolate->factory()->wasm_wrapped_object_symbol();
+          LookupIterator it(isolate, value, key,
+                            LookupIterator::OWN_SKIP_INTERCEPTOR);
+          if (it.state() == LookupIterator::DATA) return true;
           *error_message =
-              "Assigning to eqref/i31ref globals not supported yet.";
+              "eqref object must be null (if nullable) or wrapped with wasm "
+              "object wrapper";
+          return false;
+        }
+        case HeapType::kI31:
+          // TODO(7748): Implement when the JS API for i31ref is decided on.
+          *error_message = "Assigning JS objects to i31ref not supported yet.";
           return false;
         default:
           // Tables defined outside a module can't refer to user-defined types.
           if (module == nullptr) return false;
-          DCHECK(module->has_type(expected.heap_representation()));
-          if (module->has_signature(expected.heap_representation())) {
+          DCHECK(module->has_type(expected.ref_index()));
+          if (module->has_signature(expected.ref_index())) {
             if (WasmExportedFunction::IsWasmExportedFunction(*value)) {
               WasmExportedFunction function =
                   WasmExportedFunction::cast(*value);
@@ -2057,21 +2065,25 @@ bool DynamicTypeCheckRef(Isolate* isolate, const WasmModule* module,
                   kNonNullable);
               if (!IsSubtypeOf(real_type, expected, exporting_module, module)) {
                 *error_message =
-                    "exported function object must be a subtype of the "
-                    "global's formal type";
+                    "assigned exported function has to be a subtype of the "
+                    "expected type";
                 return false;
               }
               return true;
             }
 
             if (WasmJSFunction::IsWasmJSFunction(*value)) {
-              // TODO(9495): Implement when we are confident about the type
-              // reflection proposal.
+              // Since a WasmJSFunction cannot refer to indexed types (definable
+              // only in a module), we do not need to use EquivalentTypes().
+              if (!WasmJSFunction::cast(*value).MatchesSignature(
+                      module->signature(expected.ref_index()))) {
                 *error_message =
-                  "Assigning WasmJSFunction objects to globals not supported "
-                  "yet.";
+                    "assigned WasmJSFunction has to be a subtype of the "
+                    "expected type";
                 return false;
               }
+              return true;
+            }
 
             *error_message =
                 "function-typed object must be null (if nullable) or an "

src/wasm/wasm-opcodes.cc

@@ -8,6 +8,7 @@
 
 #include "src/codegen/signature.h"
 #include "src/wasm/wasm-features.h"
+#include "src/wasm/wasm-module.h"
 #include "src/wasm/wasm-opcodes-inl.h"
 
 namespace v8 {
@@ -38,23 +39,14 @@ bool IsJSCompatibleSignature(const FunctionSig* sig, const WasmModule* module,
     return false;
   }
   for (auto type : sig->all()) {
-    if (!enabled_features.has_bigint() && type == kWasmI64) {
+    // TODO(7748): Allow structs, arrays, rtts and i31s when their
+    //             JS-interaction is decided on.
+    if ((type == kWasmI64 && !enabled_features.has_bigint()) ||
+        type == kWasmS128 || type.is_reference_to(HeapType::kI31) ||
+        (type.has_index() && !module->has_signature(type.ref_index())) ||
+        type.is_rtt()) {
       return false;
     }
-
-    if (type == kWasmS128) return false;
-
-    if (type.is_object_reference_type()) {
-      uint32_t representation = type.heap_representation();
-      // TODO(7748): Once there's a story for JS interop for struct/array types,
-      // allow them here.
-      if (!(representation == HeapType::kExtern ||
-            representation == HeapType::kExn ||
-            representation == HeapType::kFunc ||
-            representation == HeapType::kEq)) {
-        return false;
-      }
-    }
   }
   return true;
 }

test/mjsunit/wasm/call-ref.js

@@ -7,72 +7,72 @@
 load("test/mjsunit/wasm/wasm-module-builder.js");
 
 (function Test1() {
+  var exporting_instance = (function () {
+    var builder = new WasmModuleBuilder();
+
+    builder.addFunction("addition", kSig_i_ii)
+      .addBody([kExprLocalGet, 0, kExprLocalGet, 1, kExprI32Add])
+      .exportFunc();
+
+    return builder.instantiate({});
+  })();
+
   var instance = (function () {
     var builder = new WasmModuleBuilder();
 
     var sig_index = builder.addType(kSig_i_ii);
 
-    var imported_webassembly_function_index =
+    var imported_type_reflection_function_index =
       builder.addImport("imports", "mul", sig_index);
 
     var imported_js_function_index =
-      builder.addImport("imports", "add", sig_index);
+      builder.addImport("imports", "js_add", sig_index);
 
-    builder.addExport("reexported_js_function",
-                      imported_js_function_index);
+    var imported_wasm_function_index =
+      builder.addImport("imports", "wasm_add", sig_index);
+
+    builder.addExport("unused", imported_wasm_function_index);
+    builder.addExport("reexported_js_function", imported_js_function_index);
     builder.addExport("reexported_webassembly_function",
-                      imported_webassembly_function_index);
+                      imported_type_reflection_function_index);
 
     var locally_defined_function =
       builder.addFunction("sub", sig_index)
-        .addBody([
-          kExprLocalGet, 0,
-          kExprLocalGet, 1,
-          kExprI32Sub
-        ])
+        .addBody([kExprLocalGet, 0, kExprLocalGet, 1, kExprI32Sub])
+        .exportFunc();
+
+    builder.addFunction("main", makeSig(
+      [wasmRefType(sig_index), kWasmI32, kWasmI32], [kWasmI32]))
+      .addBody([kExprLocalGet, 1, kExprLocalGet, 2, kExprLocalGet, 0,
+                kExprCallRef])
       .exportFunc();
 
-    builder.addFunction("main", makeSig([kWasmAnyFunc, kWasmI32, kWasmI32],
-                                        [kWasmI32]))
-      .addBody([
-        kExprLocalGet, 1,
-        kExprLocalGet, 2,
-        kExprLocalGet, 0,
-        kGCPrefix, kExprRttCanon, 0,
-        kGCPrefix, kExprRefCast, kWasmAnyFunc, 0,
-        kExprCallRef
-      ])
+    builder.addFunction("test_local", kSig_i_v)
+      .addBody([kExprI32Const, 55, kExprI32Const, 42,
+                kExprRefFunc, locally_defined_function.index, kExprCallRef])
       .exportFunc();
 
-    builder.addFunction("test_local", makeSig([], [kWasmI32]))
-    .addBody([
-      kExprI32Const, 55,
-      kExprI32Const, 42,
-      kExprRefFunc, locally_defined_function.index,
-      kExprCallRef
-    ])
+    builder.addFunction("test_js_import", kSig_i_v)
+      .addBody([kExprI32Const, 15, kExprI32Const, 42,
+                kExprRefFunc, imported_js_function_index, kExprCallRef])
       .exportFunc();
 
-    builder.addFunction("test_js_import", makeSig([], [kWasmI32]))
-      .addBody([
-        kExprI32Const, 15,
-        kExprI32Const, 42,
-        kExprRefFunc, imported_js_function_index,
-        kExprCallRef
-      ])
+      builder.addFunction("test_wasm_import", kSig_i_v)
+        .addBody([kExprI32Const, 15, kExprI32Const, 42,
+                  kExprRefFunc, imported_wasm_function_index, kExprCallRef])
         .exportFunc();
 
-    builder.addFunction("test_webassembly_import", makeSig([], [kWasmI32]))
-      .addBody([
-        kExprI32Const, 3,
-        kExprI32Const, 7,
-        kExprRefFunc, imported_webassembly_function_index,
-        kExprCallRef
-      ])
+    /* Future use
+    builder.addFunction("test_webassembly_import", kSig_i_v)
+      .addBody([kExprI32Const, 3, kExprI32Const, 7,
+                kExprRefFunc, imported_type_reflection_function_index,
+                kExprCallRef])
       .exportFunc();
+    */
 
     return builder.instantiate({imports: {
-      add: function(a, b) { return a + b; },
+      js_add: function(a, b) { return a + b; },
+      wasm_add: exporting_instance.exports.addition,
       mul: new WebAssembly.Function({parameters:['i32', 'i32'],
                                      results: ['i32']},
                                     function(a, b) { return a * b; })
@@ -97,9 +97,15 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
   assertEquals(19, instance.exports.main(
     instance.exports.reexported_js_function, 12, 7));
 
-  // TODO(7748): Make this work.
+  print("--imported function from another module--");
+  assertEquals(57, instance.exports.test_wasm_import());
+  print("--not imported function defined in another module--");
+  assertEquals(19, instance.exports.main(
+    exporting_instance.exports.addition, 12, 7));
+
+  // TODO(7748): Make these work once we know how we interact
+  //             with the 'type reflection' proposal.
   //print("--imported WebAssembly.Function--")
   //assertEquals(21, instance.exports.test_webassembly_import());
-
   //print(" --not imported WebAssembly.Function--")
 })();

test/mjsunit/wasm/reference-globals.js

@@ -74,7 +74,7 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
       builder.instantiate(
         {imports: { global: exporting_instance.exports.addition }})},
     WebAssembly.LinkError,
-    /exported function object must be a subtype of the global's formal type/
+    /assigned exported function has to be a subtype of the expected type/
   );
 
   var instance = (function () {