[wasm-gc] Always use JSToWasmObject at the JS-to-Wasm boundary

- Remove the {ValueRepr} parameter from Wasm table and global object
  internals. It is now the responsibility of the user to transform
  to/from a JS object. This removes duplicate work in some cases (type
  checking in the caller, transforming in the callee).
- For the reverse direction in the JS API, introduce
  {WasmObjectToJSReturnValue}.

Bug: v8:7748
Change-Id: Ie7625cc0f08d38fe74dbe57e69004de2d93b8a11
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3876184Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Matthias Liedtke <mliedtke@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83031}

src/debug/debug-wasm-objects.cc

@@ -1035,8 +1035,7 @@ Handle<ArrayList> AddWasmTableObjectInternalProperties(
   int length = table->current_length();
   Handle<FixedArray> entries = isolate->factory()->NewFixedArray(length);
   for (int i = 0; i < length; ++i) {
-    Handle<Object> entry =
-        WasmTableObject::Get(isolate, table, i, WasmTableObject::kWasm);
+    Handle<Object> entry = WasmTableObject::Get(isolate, table, i);
     wasm::WasmValue wasm_value(entry, table->type());
     Handle<WasmModuleObject> module(
         WasmInstanceObject::cast(table->instance()).module_object(), isolate);

src/runtime/runtime-wasm.cc

@@ -458,8 +458,7 @@ RUNTIME_FUNCTION(Runtime_WasmFunctionTableGet) {
     return ThrowWasmError(isolate, MessageTemplate::kWasmTrapTableOutOfBounds);
   }
 
-  return *WasmTableObject::Get(isolate, table, entry_index,
-                               WasmTableObject::kWasm);
+  return *WasmTableObject::Get(isolate, table, entry_index);
 }
 
 RUNTIME_FUNCTION(Runtime_WasmFunctionTableSet) {
@@ -483,8 +482,7 @@ RUNTIME_FUNCTION(Runtime_WasmFunctionTableSet) {
   if (!WasmTableObject::IsInBounds(isolate, table, entry_index)) {
     return ThrowWasmError(isolate, MessageTemplate::kWasmTrapTableOutOfBounds);
   }
-  WasmTableObject::Set(isolate, table, entry_index, element,
-                       WasmTableObject::kWasm);
+  WasmTableObject::Set(isolate, table, entry_index, element);
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
@@ -548,8 +546,7 @@ RUNTIME_FUNCTION(Runtime_WasmTableGrow) {
 
   Handle<WasmTableObject> table(
       WasmTableObject::cast(instance.tables().get(table_index)), isolate);
-  int result = WasmTableObject::Grow(isolate, table, delta, value,
-                                     WasmTableObject::kWasm);
+  int result = WasmTableObject::Grow(isolate, table, delta, value);
 
   return Smi::FromInt(result);
 }

src/wasm/c-api.cc

@@ -1858,10 +1858,16 @@ auto Global::get() const -> Val {
       return Val(v8_global->GetF64());
     case i::wasm::kRef:
     case i::wasm::kRefNull: {
-      // TODO(7748): Make sure this works for all heap types.
+      // TODO(7748): Handle types other than funcref and externref if needed.
       StoreImpl* store = impl(this)->store();
       i::HandleScope scope(store->i_isolate());
-      return Val(V8RefValueToWasm(store, v8_global->GetRef()));
+      i::Handle<i::Object> result = v8_global->GetRef();
+      if (result->IsWasmInternalFunction()) {
+        result =
+            handle(i::Handle<i::WasmInternalFunction>::cast(result)->external(),
+                   v8_global->GetIsolate());
+      }
+      return Val(V8RefValueToWasm(store, result));
     }
     case i::wasm::kRtt:
     case i::wasm::kS128:
@@ -1887,14 +1893,16 @@ void Global::set(const Val& val) {
     case F64:
       return v8_global->SetF64(val.f64());
     case ANYREF:
-      return v8_global->SetExternRef(
+      return v8_global->SetRef(
           WasmRefToV8(impl(this)->store()->i_isolate(), val.ref()));
     case FUNCREF: {
       i::Isolate* isolate = impl(this)->store()->i_isolate();
-      bool result =
-          v8_global->SetFuncRef(isolate, WasmRefToV8(isolate, val.ref()));
-      DCHECK(result);
-      USE(result);
+      auto external = WasmRefToV8(impl(this)->store()->i_isolate(), val.ref());
+      const char* error_message;
+      auto internal = i::wasm::JSToWasmObject(isolate, nullptr, external,
+                                              v8_global->type(), &error_message)
+                          .ToHandleChecked();
+      v8_global->SetRef(internal);
       return;
     }
     default:
@@ -1984,13 +1992,18 @@ auto Table::type() const -> own<TableType> {
   return TableType::make(ValType::make(kind), Limits(min, max));
 }
 
+// TODO(7748): Handle types other than funcref and externref if needed.
 auto Table::get(size_t index) const -> own<Ref> {
   i::Handle<i::WasmTableObject> table = impl(this)->v8_object();
   if (index >= static_cast<size_t>(table->current_length())) return own<Ref>();
   i::Isolate* isolate = table->GetIsolate();
   i::HandleScope handle_scope(isolate);
-  i::Handle<i::Object> result = i::WasmTableObject::Get(
-      isolate, table, static_cast<uint32_t>(index), i::WasmTableObject::kJS);
+  i::Handle<i::Object> result =
+      i::WasmTableObject::Get(isolate, table, static_cast<uint32_t>(index));
+  if (result->IsWasmInternalFunction()) {
+    result = handle(
+        i::Handle<i::WasmInternalFunction>::cast(result)->external(), isolate);
+  }
   DCHECK(result->IsNull(isolate) || result->IsJSReceiver());
   return V8RefValueToWasm(impl(this)->store(), result);
 }
@@ -2001,9 +2014,13 @@ auto Table::set(size_t index, const Ref* ref) -> bool {
   i::Isolate* isolate = table->GetIsolate();
   i::HandleScope handle_scope(isolate);
   i::Handle<i::Object> obj = WasmRefToV8(isolate, ref);
-  // TODO(12868): Enforce type restrictions for stringref tables.
-  i::WasmTableObject::Set(isolate, table, static_cast<uint32_t>(index), obj,
-                          i::WasmTableObject::kJS);
+  const char* error_message;
+  i::Handle<i::Object> obj_as_wasm =
+      i::wasm::JSToWasmObject(isolate, nullptr, obj, table->type(),
+                              &error_message)
+          .ToHandleChecked();
+  i::WasmTableObject::Set(isolate, table, static_cast<uint32_t>(index),
+                          obj_as_wasm);
   return true;
 }
 
@@ -2017,9 +2034,13 @@ auto Table::grow(size_t delta, const Ref* ref) -> bool {
   i::Isolate* isolate = table->GetIsolate();
   i::HandleScope scope(isolate);
   i::Handle<i::Object> obj = WasmRefToV8(isolate, ref);
-  int result =
-      i::WasmTableObject::Grow(isolate, table, static_cast<uint32_t>(delta),
-                               obj, i::WasmTableObject::kJS);
+  const char* error_message;
+  i::Handle<i::Object> obj_as_wasm =
+      i::wasm::JSToWasmObject(isolate, nullptr, obj, table->type(),
+                              &error_message)
+          .ToHandleChecked();
+  int result = i::WasmTableObject::Grow(
+      isolate, table, static_cast<uint32_t>(delta), obj_as_wasm);
   return result >= 0;
 }
 

src/wasm/module-instantiate.cc

@@ -1535,25 +1535,14 @@ bool InstanceBuilder::ProcessImportedGlobal(Handle<WasmInstanceObject> instance,
 
   if (global.type.is_reference()) {
     const char* error_message;
-    if (wasm::JSToWasmObject(isolate_, module_, value, global.type,
+    Handle<Object> wasm_value;
+    if (!wasm::JSToWasmObject(isolate_, module_, value, global.type,
                               &error_message)
-            .is_null()) {
+             .ToHandle(&wasm_value)) {
       ReportLinkError(error_message, global_index, module_name, import_name);
       return false;
     }
-    if (IsSubtypeOf(global.type, kWasmFuncRef, module_) && !value->IsNull()) {
-      value =
-          WasmInternalFunction::FromExternal(value, isolate_).ToHandleChecked();
-    } else if (!v8_flags.wasm_gc_js_interop &&
-               global.type.heap_representation() != HeapType::kExtern &&
-               !value->IsNull()) {
-      bool unpacked = TryUnpackObjectWrapper(isolate_, value);
-      // Excluding SMIs and stringrefs, every value received here, must have
-      // been wrapped. This is ensured by JSToWasmObject().
-      DCHECK_EQ(unpacked, !value->IsSmi() && !value->IsString());
-      USE(unpacked);  // Prevent nused warning if DCHECKs disabled.
-    }
-    WriteGlobalValue(global, WasmValue(value, global.type));
+    WriteGlobalValue(global, WasmValue(wasm_value, global.type));
     return true;
   }
 
@@ -2011,8 +2000,7 @@ void InstanceBuilder::SetTableInitialValues(
         for (uint32_t entry_index = 0; entry_index < table.initial_size;
              entry_index++) {
           WasmTableObject::Set(isolate_, table_object, entry_index,
-                               to_value(result).to_ref(),
-                               WasmTableObject::kWasm);
+                               to_value(result).to_ref());
         }
       }
     }
@@ -2061,7 +2049,7 @@ base::Optional<MessageTemplate> LoadElemSegmentImpl(
           zone, entry, elem_segment.type, isolate, instance);
       if (is_error(result)) return to_error(result);
       WasmTableObject::Set(isolate, table_object, entry_index,
-                           to_value(result).to_ref(), WasmTableObject::kWasm);
+                           to_value(result).to_ref());
     }
   }
   return {};

src/wasm/wasm-objects-inl.h

@@ -166,33 +166,8 @@ void WasmGlobalObject::SetF64(double value) {
   base::WriteUnalignedValue(address(), value);
 }
 
-void WasmGlobalObject::SetExternRef(Handle<Object> value) {
-  DCHECK(type().is_reference_to(wasm::HeapType::kExtern));
-  tagged_buffer().set(offset(), *value);
-}
-
-void WasmGlobalObject::SetAnyRef(Handle<Object> value) {
-  DCHECK(type().is_reference_to(wasm::HeapType::kAny) ||
-         type().is_reference_to(wasm::HeapType::kEq) ||
-         type().is_reference_to(wasm::HeapType::kData) ||
-         type().is_reference_to(wasm::HeapType::kArray) ||
-         type().is_reference_to(wasm::HeapType::kI31));
-  tagged_buffer().set(offset(), *value);
-}
-
-bool WasmGlobalObject::SetFuncRef(Isolate* isolate, Handle<Object> value) {
-  DCHECK_EQ(type(), wasm::kWasmFuncRef);
-  if (value->IsNull() ||
-      WasmInternalFunction::FromExternal(value, isolate).ToHandle(&value)) {
-    tagged_buffer().set(offset(), *value);
-    return true;
-  }
-  return false;
-}
-
-void WasmGlobalObject::SetStringRef(Handle<Object> value) {
-  DCHECK_EQ(type(), wasm::kWasmStringRef);
-  DCHECK(value->IsNull() || value->IsString());
+void WasmGlobalObject::SetRef(Handle<Object> value) {
+  DCHECK(type().is_object_reference());
   tagged_buffer().set(offset(), *value);
 }
 

src/wasm/wasm-objects.h

@@ -176,12 +176,9 @@ class WasmTableObject
  public:
   inline wasm::ValueType type();
 
-  enum ValueRepr { kJS, kWasm };
-
   V8_EXPORT_PRIVATE static int Grow(Isolate* isolate,
                                     Handle<WasmTableObject> table,
-                                    uint32_t count, Handle<Object> init_value,
-                                    ValueRepr entry_repr);
+                                    uint32_t count, Handle<Object> init_value);
 
   V8_EXPORT_PRIVATE static Handle<WasmTableObject> New(
       Isolate* isolate, Handle<WasmInstanceObject> instance,
@@ -196,18 +193,21 @@ class WasmTableObject
   static bool IsInBounds(Isolate* isolate, Handle<WasmTableObject> table,
                          uint32_t entry_index);
 
-  static bool IsValidJSElement(Isolate* isolate, Handle<WasmTableObject> table,
-                               Handle<Object> entry);
+  // Thin wrapper around {JsToWasmObject}.
+  static MaybeHandle<Object> JSToWasmElement(Isolate* isolate,
+                                             Handle<WasmTableObject> table,
+                                             Handle<Object> entry,
+                                             const char** error_message);
 
+  // This function will not handle JS objects; i.e., {entry} needs to be in wasm
+  // representation.
   V8_EXPORT_PRIVATE static void Set(Isolate* isolate,
                                     Handle<WasmTableObject> table,
-                                    uint32_t index, Handle<Object> entry,
-                                    ValueRepr entry_repr);
+                                    uint32_t index, Handle<Object> entry);
 
   V8_EXPORT_PRIVATE static Handle<Object> Get(Isolate* isolate,
                                               Handle<WasmTableObject> table,
-                                              uint32_t index,
-                                              ValueRepr as_repr);
+                                              uint32_t index);
 
   V8_EXPORT_PRIVATE static void Fill(Isolate* isolate,
                                      Handle<WasmTableObject> table,
@@ -249,7 +249,7 @@ class WasmTableObject
   static void SetFunctionTableEntry(Isolate* isolate,
                                     Handle<WasmTableObject> table,
                                     Handle<FixedArray> entries, int entry_index,
-                                    Handle<Object> entry, ValueRepr entry_repr);
+                                    Handle<Object> entry);
 
   TQ_OBJECT_CONSTRUCTORS(WasmTableObject)
 };
@@ -314,10 +314,8 @@ class WasmGlobalObject
   inline void SetI64(int64_t value);
   inline void SetF32(float value);
   inline void SetF64(double value);
-  inline void SetExternRef(Handle<Object> value);
-  inline void SetAnyRef(Handle<Object> value);
-  inline bool SetFuncRef(Isolate* isolate, Handle<Object> value);
-  inline void SetStringRef(Handle<Object> value);
+  // {value} must be an object in Wasm representation.
+  inline void SetRef(Handle<Object> value);
 
  private:
   // This function returns the address of the global's data in the
@@ -1075,11 +1073,6 @@ namespace wasm {
 MaybeHandle<Object> JSToWasmObject(Isolate* isolate, const WasmModule* module,
                                    Handle<Object> value, ValueType expected,
                                    const char** error_message);
-
-// If {in_out_value} is a wrapped wasm struct/array, it gets unwrapped in-place
-// and this returns {true}. Otherwise, the value remains unchanged and this
-// returns {false}.
-bool TryUnpackObjectWrapper(Isolate* isolate, Handle<Object>& in_out_value);
 }  // namespace wasm
 
 }  // namespace internal

test/cctest/wasm/test-run-wasm-bulk-memory.cc

@@ -377,8 +377,7 @@ void CheckTable(Isolate* isolate, Handle<WasmTableObject> table, Args... args) {
   CHECK_EQ(table->current_length(), args_length);
   Handle<Object> handles[] = {args...};
   for (uint32_t i = 0; i < args_length; ++i) {
-    CHECK(WasmTableObject::Get(isolate, table, i, WasmTableObject::kWasm)
-              .is_identical_to(handles[i]));
+    CHECK(WasmTableObject::Get(isolate, table, i).is_identical_to(handles[i]));
   }
 }
 
@@ -573,11 +572,11 @@ void TestTableCopyElems(TestExecutionTier execution_tier, int table_dst,
                  r.builder().instance_object()->tables().get(table_dst)),
              isolate);
   r.CheckCallViaJS(0, 0, 0, kTableSize);
-  auto f0 = WasmTableObject::Get(isolate, table, 0, WasmTableObject::kWasm);
-  auto f1 = WasmTableObject::Get(isolate, table, 1, WasmTableObject::kWasm);
-  auto f2 = WasmTableObject::Get(isolate, table, 2, WasmTableObject::kWasm);
-  auto f3 = WasmTableObject::Get(isolate, table, 3, WasmTableObject::kWasm);
-  auto f4 = WasmTableObject::Get(isolate, table, 4, WasmTableObject::kWasm);
+  auto f0 = WasmTableObject::Get(isolate, table, 0);
+  auto f1 = WasmTableObject::Get(isolate, table, 1);
+  auto f2 = WasmTableObject::Get(isolate, table, 2);
+  auto f3 = WasmTableObject::Get(isolate, table, 3);
+  auto f4 = WasmTableObject::Get(isolate, table, 4);
 
   if (table_dst == table_src) {
     CheckTable(isolate, table, f0, f1, f2, f3, f4);
@@ -723,11 +722,11 @@ void TestTableCopyOobWrites(TestExecutionTier execution_tier, int table_dst,
              isolate);
   // Fill the dst table with values from the src table, to make checks easier.
   r.CheckCallViaJS(0, 0, 0, kTableSize);
-  auto f0 = WasmTableObject::Get(isolate, table, 0, WasmTableObject::kWasm);
-  auto f1 = WasmTableObject::Get(isolate, table, 1, WasmTableObject::kWasm);
-  auto f2 = WasmTableObject::Get(isolate, table, 2, WasmTableObject::kWasm);
-  auto f3 = WasmTableObject::Get(isolate, table, 3, WasmTableObject::kWasm);
-  auto f4 = WasmTableObject::Get(isolate, table, 4, WasmTableObject::kWasm);
+  auto f0 = WasmTableObject::Get(isolate, table, 0);
+  auto f1 = WasmTableObject::Get(isolate, table, 1);
+  auto f2 = WasmTableObject::Get(isolate, table, 2);
+  auto f3 = WasmTableObject::Get(isolate, table, 3);
+  auto f4 = WasmTableObject::Get(isolate, table, 4);
 
   CheckTable(isolate, table, f0, f1, f2, f3, f4);
 

test/cctest/wasm/test-run-wasm-wrappers.cc

@@ -336,8 +336,8 @@ TEST(WrapperReplacement_IndirectExport) {
     Handle<WasmTableObject> table(
         WasmTableObject::cast(instance->tables().get(table_index)), isolate);
     // Get the Wasm function through the exported table.
-    Handle<Object> function = WasmTableObject::Get(
-        isolate, table, function_index, WasmTableObject::kWasm);
+    Handle<Object> function =
+        WasmTableObject::Get(isolate, table, function_index);
     Handle<WasmExportedFunction> indirect_function(
         WasmExportedFunction::cast(
             WasmInternalFunction::cast(*function).external()),

test/common/wasm/wasm-interpreter.cc

@@ -1942,8 +1942,7 @@ class WasmInterpreterInternals {
             isolate_);
         auto delta = Pop().to<uint32_t>();
         auto value = Pop().to_ref();
-        int32_t result = WasmTableObject::Grow(isolate_, table, delta, value,
-                                               WasmTableObject::kWasm);
+        int32_t result = WasmTableObject::Grow(isolate_, table, delta, value);
         Push(WasmValue(result));
         *len += imm.length;
         return true;
@@ -3703,8 +3702,8 @@ class WasmInterpreterInternals {
           if (entry_index >= table_size) {
             return DoTrap(kTrapTableOutOfBounds, pc);
           }
-          Handle<Object> value = WasmTableObject::Get(
-              isolate_, table, entry_index, WasmTableObject::kWasm);
+          Handle<Object> value =
+              WasmTableObject::Get(isolate_, table, entry_index);
           Push(WasmValue(value, table->type()));
           len = 1 + imm.length;
           break;
@@ -3722,8 +3721,7 @@ class WasmInterpreterInternals {
           if (entry_index >= table_size) {
             return DoTrap(kTrapTableOutOfBounds, pc);
           }
-          WasmTableObject::Set(isolate_, table, entry_index, value,
-                               WasmTableObject::kWasm);
+          WasmTableObject::Set(isolate_, table, entry_index, value);
           len = 1 + imm.length;
           break;
         }

test/mjsunit/wasm/js-api.js

@@ -715,18 +715,19 @@ assertThrows(
   /must be convertible to a valid number/);
 assertThrows(
     () => set.call(tbl1, 0, undefined), TypeError,
-    /Argument 1 is invalid for table of type funcref/);
+    /Argument 1 is invalid for table: /);
 assertThrows(
     () => set.call(tbl1, undefined, undefined), TypeError,
     /must be convertible to a valid number/);
 assertThrows(
     () => set.call(tbl1, 0, {}), TypeError,
-    /Argument 1 is invalid for table of type funcref/);
-assertThrows(() => set.call(tbl1, 0, function() {}),
-             TypeError, /Argument 1 is invalid for table of type funcref/);
+    /Argument 1 is invalid for table:.*null.*or a Wasm function object/);
+assertThrows(
+    () => set.call(tbl1, 0, function() {}), TypeError,
+    /Argument 1 is invalid for table:.*null.*or a Wasm function object/);
 assertThrows(
     () => set.call(tbl1, 0, Math.sin), TypeError,
-    /Argument 1 is invalid for table of type funcref/);
+    /Argument 1 is invalid for table:.*null.*or a Wasm function object/);
 assertThrows(
     () => set.call(tbl1, {valueOf() { throw Error('hai') }}, null), Error,
     'hai');

test/mjsunit/wasm/reference-tables.js

@@ -99,7 +99,7 @@ d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
   assertThrows(
       () => instance.exports.table.set(0, exporting_instance.exports.addition),
       TypeError,
-      /Argument 1 is invalid for table of type \(ref null 0\)/);
+      /Argument 1 is invalid for table: assigned exported function has to be a subtype of the expected type/);
 })();
 
 (function TestNonNullableTables() {

test/mjsunit/wasm/stringrefs-js.js

@@ -197,10 +197,10 @@ function assertInvalid(fn, message) {
                  TypeError, msg);
   }
 
-  assertThrows(()=>new WebAssembly.Global({ value: 'stringref' }),
-               TypeError,
-               "WebAssembly.Global(): " +
-               "Missing initial value when creating stringref global");
+  // String with default initializer.
+  // TODO(12868): Is this the intended behavior?
+  let null_str = new WebAssembly.Global({ value: 'stringref' });
+  assertEquals(null, null_str.value);
 
   let kSig_w_v = makeSig([], [kWasmStringRef]);
   let kSig_v_w = makeSig([kWasmStringRef], []);
@@ -302,8 +302,8 @@ function assertInvalid(fn, message) {
     let unsupportedGetMessage =
         `WebAssembly.Table.get(): ${type} has no JS representation`;
     let unsupportedSetMessage =
-        'WebAssembly.Table.set(): Argument 1 is invalid for table of type '
-        + `${type}ref`;
+        'WebAssembly.Table.set(): Argument 1 is invalid for table: '
+        + `${type} has no JS representation`;
     assertThrows(()=>table.get(0), TypeError, unsupportedGetMessage);
     assertThrows(()=>{table.set(0, null);}, TypeError, unsupportedSetMessage);
   }