[wasm-gc] Fix and extend JS roundtrip for gc types

Changes:
- Wrap eqref and i31ref objects in the temporary wasm object wrapper
  (in addition to dataref and anyref). Accept those types in
  IsJSCompatibleSignature().
- Handle null correctly in all cases (i.e., do not wrap/unwrap it).
- Improve some error messages.
- Handle kRttWithDepth in one case where it was omitted.
- Some small structure improvements.
- Add an extensive test.

Bug: v8:7748, v8:11606
Change-Id: Ie519f2c87421664dd02cf29fe94f9a9d7510bae2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2794422
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73725}

src/compiler/wasm-compiler.cc

@@ -6221,53 +6221,66 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     switch (type.kind()) {
       case wasm::kI32:
         return BuildChangeInt32ToNumber(node);
-      case wasm::kS128:
-        UNREACHABLE();
-      case wasm::kI64: {
+      case wasm::kI64:
         return BuildChangeInt64ToBigInt(node);
-      }
       case wasm::kF32:
         return BuildChangeFloat32ToNumber(node);
       case wasm::kF64:
         return BuildChangeFloat64ToNumber(node);
       case wasm::kRef:
-      case wasm::kOptRef: {
-        uint32_t representation = type.heap_representation();
-        if (representation == wasm::HeapType::kExtern ||
-            representation == wasm::HeapType::kFunc) {
-          return node;
-        }
-        if (representation == wasm::HeapType::kData) {
-          // TODO(7748): Update this when JS interop is settled.
-          return BuildAllocateObjectWrapper(node);
-        }
-        if (representation == wasm::HeapType::kAny) {
-          // Only wrap {node} if it is an array or struct.
-          // TODO(7748): Update this when JS interop is settled.
-          auto done = gasm_->MakeLabel(MachineRepresentation::kTaggedPointer);
-          gasm_->GotoIfNot(gasm_->IsDataRefMap(gasm_->LoadMap(node)), &done,
-                           node);
-          Node* wrapped = BuildAllocateObjectWrapper(node);
-          gasm_->Goto(&done, wrapped);
-          gasm_->Bind(&done);
-          return done.PhiAt(0);
-        }
-        if (type.has_index() && module_->has_signature(type.ref_index())) {
-          // Typed function
-          return node;
+      case wasm::kOptRef:
+        switch (type.heap_representation()) {
+          case wasm::HeapType::kExtern:
+          case wasm::HeapType::kFunc:
+            return node;
+          case wasm::HeapType::kData:
+          case wasm::HeapType::kEq:
+          case wasm::HeapType::kI31:
+            // TODO(7748): Update this when JS interop is settled.
+            if (type.kind() == wasm::kOptRef) {
+              auto done =
+                  gasm_->MakeLabel(MachineRepresentation::kTaggedPointer);
+              // Do not wrap {null}.
+              gasm_->GotoIf(gasm_->WordEqual(node, RefNull()), &done, node);
+              gasm_->Goto(&done, BuildAllocateObjectWrapper(node));
+              gasm_->Bind(&done);
+              return done.PhiAt(0);
+            } else {
+              return BuildAllocateObjectWrapper(node);
+            }
+          case wasm::HeapType::kAny: {
+            // Only wrap {node} if it is an array/struct/i31, i.e., do not wrap
+            // functions and null.
+            // TODO(7748): Update this when JS interop is settled.
+            auto done = gasm_->MakeLabel(MachineRepresentation::kTaggedPointer);
+            gasm_->GotoIf(IsSmi(node), &done, BuildAllocateObjectWrapper(node));
+            // This includes the case where {node == null}.
+            gasm_->GotoIfNot(gasm_->IsDataRefMap(gasm_->LoadMap(node)), &done,
+                             node);
+            gasm_->Goto(&done, BuildAllocateObjectWrapper(node));
+            gasm_->Bind(&done);
+            return done.PhiAt(0);
+          }
+          default:
+            DCHECK(type.has_index());
+            if (module_->has_signature(type.ref_index())) {
+              // Typed function
+              return node;
+            }
+            // If this is reached, then IsJSCompatibleSignature() is too
+            // permissive.
+            // TODO(7748): Figure out a JS interop story for arrays and structs.
+            UNREACHABLE();
         }
-        // If this is reached, then IsJSCompatibleSignature() is too permissive.
-        // TODO(7748): Figure out a JS interop story for arrays and structs.
-        UNREACHABLE();
-      }
       case wasm::kRtt:
       case wasm::kRttWithDepth:
-        // TODO(7748): Figure out what to do for RTTs.
-        UNIMPLEMENTED();
       case wasm::kI8:
       case wasm::kI16:
+      case wasm::kS128:
       case wasm::kVoid:
       case wasm::kBottom:
+        // If this is reached, then IsJSCompatibleSignature() is too permissive.
+        // TODO(7748): Figure out what to do for RTTs.
         UNREACHABLE();
     }
   }
@@ -6281,9 +6294,11 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
         LOAD_INSTANCE_FIELD(NativeContext, MachineType::TaggedPointer()));
   }
 
-  enum UnpackFailureBehavior : bool { kReturnInput, kReturnNull };
-
-  Node* BuildUnpackObjectWrapper(Node* input, UnpackFailureBehavior failure) {
+  // Assumes {input} has been checked for validity against the target wasm type.
+  // Returns the value of the property associated with
+  // {wasm_wrapped_object_symbol} in {input}, or {input} itself if the property
+  // is not found.
+  Node* BuildUnpackObjectWrapper(Node* input) {
     Node* obj = gasm_->CallBuiltin(
         Builtins::kWasmGetOwnProperty, Operator::kEliminatable, input,
         LOAD_ROOT(wasm_wrapped_object_symbol, wasm_wrapped_object_symbol),
@@ -6296,8 +6311,7 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     Diamond check(graph(), mcgraph()->common(), is_undefined,
                   BranchHint::kFalse);
     check.Chain(control());
-    return check.Phi(MachineRepresentation::kTagged,
-                     failure == kReturnInput ? input : RefNull(), obj);
+    return check.Phi(MachineRepresentation::kTagged, input, obj);
   }
 
   Node* BuildChangeInt64ToBigInt(Node* input) {
@@ -6369,21 +6383,20 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
           case wasm::HeapType::kExtern:
             return input;
           case wasm::HeapType::kAny:
-            // If this is a wrapper for arrays/structs, unpack it.
+            // If this is a wrapper for arrays/structs/i31s, unpack it.
             // TODO(7748): Update this when JS interop has settled.
-            return BuildUnpackObjectWrapper(input, kReturnInput);
+            return BuildUnpackObjectWrapper(input);
           case wasm::HeapType::kFunc:
             BuildCheckValidRefValue(input, js_context, type);
             return input;
           case wasm::HeapType::kData:
-            // TODO(7748): Update this when JS interop has settled.
-            BuildCheckValidRefValue(input, js_context, type);
-            return BuildUnpackObjectWrapper(input, kReturnNull);
           case wasm::HeapType::kEq:
           case wasm::HeapType::kI31:
-            // If this is reached, then IsJSCompatibleSignature() is too
-            // permissive.
-            UNREACHABLE();
+            // TODO(7748): Update this when JS interop has settled.
+            BuildCheckValidRefValue(input, js_context, type);
+            // This will just return {input} if the object is not wrapped, i.e.
+            // if it is null (given the check just above).
+            return BuildUnpackObjectWrapper(input);
           default:
             if (module_->has_signature(type.ref_index())) {
               BuildCheckValidRefValue(input, js_context, type);
@@ -6408,15 +6421,16 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
         // i64 values can only come from BigInt.
         return BuildChangeBigIntToInt64(input, js_context, frame_state);
 
-      case wasm::kRtt:  // TODO(7748): Implement.
+      case wasm::kRtt:
       case wasm::kRttWithDepth:
       case wasm::kS128:
       case wasm::kI8:
       case wasm::kI16:
       case wasm::kBottom:
       case wasm::kVoid:
+        // If this is reached, then IsJSCompatibleSignature() is too permissive.
+        // TODO(7748): Figure out what to do for RTTs.
         UNREACHABLE();
-        break;
     }
   }
 

src/wasm/wasm-objects.cc

@@ -2195,24 +2195,39 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
         case HeapType::kExtern:
         case HeapType::kAny:
           return true;
-        case HeapType::kData: {
+        case HeapType::kData:
+        case HeapType::kEq:
+        case HeapType::kI31: {
           // 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 =
-              "dataref object must be null (if nullable) or wrapped with the "
-              "wasm object wrapper";
-          return false;
+          if (it.state() != LookupIterator::DATA) {
+            *error_message =
+                "eqref/dataref/i31ref object must be null (if nullable) or "
+                "wrapped with the wasm object wrapper";
+            return false;
+          }
+
+          if (expected.is_reference_to(HeapType::kEq)) return true;
+          Handle<Object> value = it.GetDataValue();
+
+          if (expected.is_reference_to(HeapType::kData)) {
+            if (value->IsSmi()) {
+              *error_message = "dataref-typed object must be a heap object";
+              return false;
+            }
+            return true;
+          } else {
+            DCHECK(expected.is_reference_to(HeapType::kI31));
+            if (!value->IsSmi()) {
+              *error_message = "i31ref-typed object cannot be a heap object";
+              return false;
+            }
+            return true;
+          }
         }
-        case HeapType::kEq:
-        case HeapType::kI31:
-          // TODO(7748): Implement when the JS API for i31ref is decided on.
-          *error_message =
-              "Assigning JS objects to eqref/i31ref not supported yet.";
-          return false;
         default:
           // Tables defined outside a module can't refer to user-defined types.
           if (module == nullptr) return false;
@@ -2271,14 +2286,26 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
           // TODO(7748): Implement when the JS API for structs/arrays is decided
           // on.
           *error_message =
-              "Assigning to struct/array globals not supported yet.";
+              "passing struct/array-typed objects between Webassembly and "
+              "Javascript is not supported yet.";
           return false;
       }
     case kRtt:
+    case kRttWithDepth:
       // TODO(7748): Implement when the JS API for rtts is decided on.
-      *error_message = "Assigning to rtt globals not supported yet.";
+      *error_message =
+          "passing rtts between Webassembly and Javascript is not supported "
+          "yet.";
       return false;
-    default:
+    case kI8:
+    case kI16:
+    case kI32:
+    case kI64:
+    case kF32:
+    case kF64:
+    case kS128:
+    case kVoid:
+    case kBottom:
       UNREACHABLE();
   }
 }

src/wasm/wasm-opcodes.cc

@@ -39,12 +39,10 @@ bool IsJSCompatibleSignature(const FunctionSig* sig, const WasmModule* module,
     return false;
   }
   for (auto type : sig->all()) {
-    // TODO(7748): Allow structs, arrays, rtts and i31s when their
-    //             JS-interaction is decided on.
-    if (type == kWasmS128 || type.is_reference_to(HeapType::kEq) ||
-        type.is_reference_to(HeapType::kI31) ||
-        (type.has_index() && !module->has_signature(type.ref_index())) ||
-        type.is_rtt()) {
+    // TODO(7748): Allow structs, arrays, and rtts when their JS-interaction is
+    // decided on.
+    if (type == kWasmS128 || type.is_rtt() ||
+        (type.has_index() && !module->has_signature(type.ref_index()))) {
       return false;
     }
   }

test/mjsunit/wasm/wasm-gc-js-roundtrip.js

+// Copyright 2021 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: --experimental-wasm-gc
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+let instance = (() => {
+  let builder = new WasmModuleBuilder();
+  let struct = builder.addStruct([makeField(kWasmI32, true)]);
+  let array = builder.addArray(kWasmF64, true);
+  let sig = builder.addType(makeSig([kWasmI32], [kWasmI32]));
+
+  let func = builder.addFunction('inc', sig)
+                 .addBody([kExprLocalGet, 0, kExprI32Const, 1, kExprI32Add])
+                 .exportAs('inc');
+
+  builder.addFunction('struct_producer', makeSig([], [kWasmDataRef]))
+      .addBody([
+        kGCPrefix, kExprRttCanon, struct, kGCPrefix, kExprStructNewDefault,
+        struct
+      ])
+      .exportFunc();
+
+  builder.addFunction('array_producer', makeSig([], [kWasmDataRef]))
+      .addBody([
+        kExprI32Const, 10, kGCPrefix, kExprRttCanon, array, kGCPrefix,
+        kExprArrayNewDefault, array
+      ])
+      .exportFunc();
+
+  builder.addFunction('i31_producer', makeSig([], [kWasmI31Ref]))
+      .addBody([kExprI32Const, 5, kGCPrefix, kExprI31New])
+      .exportFunc();
+
+  builder.addFunction('func_producer', makeSig([], [wasmRefType(sig)]))
+      .addBody([kExprRefFunc, func.index])
+      .exportFunc();
+
+  let test_types = {
+    i31: kWasmI31Ref,
+    struct: kWasmDataRef,
+    array: kWasmDataRef,
+    raw_struct: struct,
+    raw_array: array,
+    typed_func: sig,
+    data: kWasmDataRef,
+    eq: kWasmEqRef,
+    func: kWasmFuncRef,
+    any: kWasmAnyRef,
+  };
+
+  for (key in test_types) {
+    let type = wasmOptRefType(test_types[key]);
+    builder.addFunction(key + '_id', makeSig([type], [type]))
+        .addBody([kExprLocalGet, 0])
+        .exportFunc();
+    builder.addFunction(key + '_null', makeSig([], [type]))
+        .addBody([kExprRefNull, test_types[key]])
+        .exportFunc();
+  }
+
+  return builder.instantiate({});
+})();
+
+// Wasm-exposed null is the same as JS null.
+assertEquals(instance.exports.struct_null(), null);
+
+// We can roundtrip an i31.
+instance.exports.i31_id(instance.exports.i31_producer());
+// We can roundtrip any null as i31.
+instance.exports.i31_id(instance.exports.i31_null());
+instance.exports.i31_id(instance.exports.struct_null());
+// We cannot roundtrip a struct as i31.
+assertThrows(
+    () => instance.exports.i31_id(instance.exports.struct_producer()),
+    TypeError, 'type incompatibility when transforming from/to JS');
+
+// We can roundtrip a struct as dataref.
+instance.exports.data_id(instance.exports.struct_producer());
+// We can roundtrip an array as dataref.
+instance.exports.data_id(instance.exports.array_producer());
+// We can roundtrip any null as dataref.
+instance.exports.data_id(instance.exports.data_null());
+instance.exports.data_id(instance.exports.i31_null());
+// We cannot roundtrip an i31 as dataref.
+assertThrows(
+    () => instance.exports.data_id(instance.exports.i31_producer()), TypeError,
+    'type incompatibility when transforming from/to JS');
+
+// We can roundtrip a struct as eqref.
+instance.exports.eq_id(instance.exports.struct_producer());
+// We can roundtrip an array as eqref.
+instance.exports.eq_id(instance.exports.array_producer());
+// We can roundtrip an i31 as eqref.
+instance.exports.eq_id(instance.exports.i31_producer());
+// We can roundtrip any null as eqref.
+instance.exports.eq_id(instance.exports.data_null());
+instance.exports.eq_id(instance.exports.i31_null());
+instance.exports.eq_id(instance.exports.func_null());
+// We cannot roundtrip a func as eqref.
+assertThrows(
+    () => instance.exports.eq_id(instance.exports.func_producer()), TypeError,
+    'type incompatibility when transforming from/to JS');
+
+// We can roundtrip a struct as anyref.
+instance.exports.any_id(instance.exports.struct_producer());
+// We can roundtrip an array as anyref.
+instance.exports.any_id(instance.exports.array_producer());
+// We can roundtrip an i31 as anyref.
+instance.exports.any_id(instance.exports.i31_producer());
+// We can roundtrip a func as anyref.
+instance.exports.any_id(instance.exports.func_producer());
+// We can roundtrip any null as anyref.
+instance.exports.any_id(instance.exports.data_null());
+instance.exports.any_id(instance.exports.i31_null());
+instance.exports.any_id(instance.exports.func_null());
+// We can roundtrip a JS object as anyref.
+instance.exports.any_id(instance);
+
+// We can roundtrip a typed function.
+instance.exports.typed_func_id(instance.exports.func_producer());
+// We can roundtrip any null as typed funcion.
+instance.exports.typed_func_id(instance.exports.i31_null());
+instance.exports.typed_func_id(instance.exports.struct_null());
+// We cannot roundtrip a struct as typed funcion.
+assertThrows(
+    () => instance.exports.typed_func_id(instance.exports.struct_producer()),
+    TypeError, 'type incompatibility when transforming from/to JS');
+
+// We can roundtrip a func.
+instance.exports.func_id(instance.exports.func_producer());
+// We can roundtrip any null as func.
+instance.exports.func_id(instance.exports.i31_null());
+instance.exports.func_id(instance.exports.struct_null());
+// We cannot roundtrip an i31 as func.
+assertThrows(
+    () => instance.exports.func_id(instance.exports.i31_producer()), TypeError,
+    'type incompatibility when transforming from/to JS');
+
+// We cannot directly roundtrip structs or arrays.
+// TODO(7748): Switch these tests once we can.
+assertThrows(
+    () => instance.exports.raw_struct_id(instance.exports.struct_producer()),
+    TypeError, 'type incompatibility when transforming from/to JS');
+assertThrows(
+    () => instance.exports.raw_array_id(instance.exports.array_producer()),
+    TypeError, 'type incompatibility when transforming from/to JS');