[wasm-gc] Change signature/name of TypeCheckJSObject

Rename {TypeCheckJSObject} to {JSToWasmObject}. Change it to return
a MaybeHandle containing the typechecked object transformed to its wasm representation. Use the new function to simplify
{WasmWrapperGraphBuilder::FromJS}.

Bug: v8:7748
Change-Id: I036f3a1c197041d0b12e7338adca2bc10e66038b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3874931
Commit-Queue: Matthias Liedtke <mliedtke@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Matthias Liedtke <mliedtke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83002}

src/compiler/wasm-compiler.cc

@@ -6365,7 +6365,6 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
       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();
     }
   }
@@ -6383,50 +6382,6 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     kUnwrapWasmExternalFunctions = true,
     kLeaveFunctionsAlone = false
   };
-  // Assumes {input} has been checked for validity against the target wasm type.
-  // If {input} is a function, returns the WasmInternalFunction associated with
-  // it. If {input} has the {wasm_wrapped_object_symbol} property, returns the
-  // value of that property. Otherwise, returns {input}.
-  Node* BuildUnpackObjectWrapper(
-      Node* input, Node* context,
-      UnwrapExternalFunctions unwrap_wasm_external_functions) {
-    auto end = gasm_->MakeLabel(MachineRepresentation::kTaggedPointer);
-
-    if (unwrap_wasm_external_functions) {
-      auto not_a_function = gasm_->MakeLabel();
-      gasm_->GotoIf(IsSmi(input), &not_a_function);
-      gasm_->GotoIfNot(gasm_->HasInstanceType(input, JS_FUNCTION_TYPE),
-                       &not_a_function);
-
-      Node* function_data = gasm_->LoadFunctionDataFromJSFunction(input);
-
-      // Due to type checking, {function_data} will be a WasmFunctionData.
-      Node* internal = gasm_->LoadFromObject(
-          MachineType::TaggedPointer(), function_data,
-          wasm::ObjectAccess::ToTagged(WasmFunctionData::kInternalOffset));
-      gasm_->Goto(&end, internal);
-
-      gasm_->Bind(&not_a_function);
-    }
-    if (!v8_flags.wasm_gc_js_interop) {
-      Node* obj = gasm_->CallBuiltin(
-          Builtin::kWasmGetOwnProperty, Operator::kEliminatable, input,
-          LOAD_ROOT(wasm_wrapped_object_symbol, wasm_wrapped_object_symbol),
-          context);
-      // Invalid object wrappers (i.e. any other JS object that doesn't have the
-      // magic hidden property) will return {undefined}. Map that to {input}.
-      Node* is_undefined = gasm_->TaggedEqual(obj, UndefinedValue());
-      gasm_->GotoIf(is_undefined, &end, input);
-
-      gasm_->Goto(&end, obj);
-    } else {
-      gasm_->Goto(&end, input);
-    }
-
-    gasm_->Bind(&end);
-
-    return end.PhiAt(0);
-  }
 
   Node* BuildChangeInt64ToBigInt(Node* input) {
     Node* target;
@@ -6465,34 +6420,6 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
                                      target, input, context);
   }
 
-  enum class I31Check : bool { Invalid, Valid };
-
-  void BuildCheckValidRefValue(Node* input, Node* js_context,
-                               wasm::ValueType type, I31Check i31_check) {
-    // Make sure ValueType fits in a Smi.
-    static_assert(wasm::ValueType::kLastUsedBit + 1 <= kSmiValueSize);
-
-    auto done = gasm_->MakeLabel();
-    // The instance node is always defined: if an instance is not available, it
-    // is the undefined value.
-    Node* inputs[] = {GetInstance(), input,
-                      mcgraph()->IntPtrConstant(
-                          IntToSmi(static_cast<int>(type.raw_bit_field())))};
-
-    Node* check = gasm_->BuildChangeSmiToInt32(BuildCallToRuntimeWithContext(
-        Runtime::kWasmIsValidRefValue, js_context, inputs, 3));
-
-    gasm_->GotoIf(check, &done, BranchHint::kTrue);
-    if (i31_check == I31Check::Valid) {
-      Node* is_smi = IsSmi(input);
-      gasm_->GotoIf(is_smi, &done, BranchHint::kTrue);
-    }
-    BuildCallToRuntimeWithContext(Runtime::kWasmThrowJSTypeError, js_context,
-                                  nullptr, 0);
-    gasm_->Goto(&done);
-    gasm_->Bind(&done);
-  }
-
   Node* BuildCheckString(Node* input, Node* js_context, wasm::ValueType type) {
     auto done = gasm_->MakeLabel(MachineRepresentation::kTagged);
     auto type_error = gasm_->MakeLabel();
@@ -6518,27 +6445,10 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
       case wasm::kRef:
       case wasm::kRefNull: {
         switch (type.heap_representation()) {
+          // Fast paths for extern and string.
+          // TODO(7748): Add more/all fast paths?
           case wasm::HeapType::kExtern:
             return input;
-          case wasm::HeapType::kFunc:
-            BuildCheckValidRefValue(input, js_context, type, I31Check::Invalid);
-            return BuildUnpackObjectWrapper(input, js_context,
-                                            kUnwrapWasmExternalFunctions);
-          case wasm::HeapType::kData:
-          case wasm::HeapType::kArray:
-            // TODO(7748): Update this when JS interop has settled.
-            BuildCheckValidRefValue(input, js_context, type, I31Check::Invalid);
-            // 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, js_context,
-                                            kLeaveFunctionsAlone);
-          case wasm::HeapType::kEq:
-            // TODO(7748): Update this when JS interop has settled.
-            BuildCheckValidRefValue(input, js_context, type, I31Check::Valid);
-            // 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, js_context,
-                                            kLeaveFunctionsAlone);
           case wasm::HeapType::kString:
             return BuildCheckString(input, js_context, type);
           case wasm::HeapType::kNone:
@@ -6547,16 +6457,23 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
           case wasm::HeapType::kAny:
           case wasm::HeapType::kI31:
             UNREACHABLE();
-          default:
-            if (module_->has_signature(type.ref_index())) {
-              BuildCheckValidRefValue(input, js_context, type,
-                                      I31Check::Invalid);
-              return BuildUnpackObjectWrapper(input, js_context,
-                                              kUnwrapWasmExternalFunctions);
-            }
-            // If this is reached, then IsJSCompatibleSignature() is too
-            // permissive.
-            UNREACHABLE();
+          case wasm::HeapType::kFunc:
+          case wasm::HeapType::kData:
+          case wasm::HeapType::kArray:
+          case wasm::HeapType::kEq:
+          default: {
+            // Make sure ValueType fits in a Smi.
+            static_assert(wasm::ValueType::kLastUsedBit + 1 <= kSmiValueSize);
+
+            // The instance node is always defined: if an instance is not
+            // available, it is the undefined value.
+            Node* inputs[] = {GetInstance(), input,
+                              mcgraph()->IntPtrConstant(IntToSmi(
+                                  static_cast<int>(type.raw_bit_field())))};
+
+            return BuildCallToRuntimeWithContext(Runtime::kWasmJSToWasmObject,
+                                                 js_context, inputs, 3);
+          }
         }
       }
       case wasm::kF32:
@@ -6580,7 +6497,6 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
       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();
     }
   }

src/objects/shared-function-info-inl.h

@@ -709,6 +709,10 @@ bool SharedFunctionInfo::HasAsmWasmData() const {
   return function_data(kAcquireLoad).IsAsmWasmData();
 }
 
+bool SharedFunctionInfo::HasWasmFunctionData() const {
+  return function_data(kAcquireLoad).IsWasmFunctionData();
+}
+
 bool SharedFunctionInfo::HasWasmExportedFunctionData() const {
   return function_data(kAcquireLoad).IsWasmExportedFunctionData();
 }

src/objects/shared-function-info.cc

@@ -137,6 +137,11 @@ CodeT SharedFunctionInfo::GetCode() const {
 }
 
 #if V8_ENABLE_WEBASSEMBLY
+WasmFunctionData SharedFunctionInfo::wasm_function_data() const {
+  DCHECK(HasWasmFunctionData());
+  return WasmFunctionData::cast(function_data(kAcquireLoad));
+}
+
 WasmExportedFunctionData SharedFunctionInfo::wasm_exported_function_data()
     const {
   DCHECK(HasWasmExportedFunctionData());

src/objects/shared-function-info.h

@@ -37,6 +37,7 @@ class DebugInfo;
 class IsCompiledScope;
 template <typename>
 class Signature;
+class WasmFunctionData;
 class WasmCapiFunctionData;
 class WasmExportedFunctionData;
 class WasmJSFunctionData;
@@ -344,6 +345,7 @@ class SharedFunctionInfo
 
 #if V8_ENABLE_WEBASSEMBLY
   inline bool HasAsmWasmData() const;
+  inline bool HasWasmFunctionData() const;
   inline bool HasWasmExportedFunctionData() const;
   inline bool HasWasmJSFunctionData() const;
   inline bool HasWasmCapiFunctionData() const;
@@ -353,6 +355,7 @@ class SharedFunctionInfo
 
   V8_EXPORT_PRIVATE WasmExportedFunctionData
   wasm_exported_function_data() const;
+  WasmFunctionData wasm_function_data() const;
   WasmJSFunctionData wasm_js_function_data() const;
   WasmCapiFunctionData wasm_capi_function_data() const;
   WasmResumeData wasm_resume_data() const;

src/runtime/runtime-wasm.cc

@@ -117,7 +117,10 @@ Object ThrowWasmError(Isolate* isolate, MessageTemplate message,
 }
 }  // namespace
 
-RUNTIME_FUNCTION(Runtime_WasmIsValidRefValue) {
+// Takes a JS object and a wasm type as Smi. Type checks the object against the
+// type; if the check succeeds, returns the object in its wasm representation;
+// otherwise throws a type error.
+RUNTIME_FUNCTION(Runtime_WasmJSToWasmObject) {
   // This code is called from wrappers, so the "thread is wasm" flag is not set.
   DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(),
                  !trap_handler::IsThreadInWasm());
@@ -138,9 +141,13 @@ RUNTIME_FUNCTION(Runtime_WasmIsValidRefValue) {
   wasm::ValueType type = wasm::ValueType::FromRawBitField(raw_type);
   const char* error_message;
 
-  bool result = internal::wasm::TypecheckJSObject(isolate, module, value, type,
-                                                  &error_message);
-  return Smi::FromInt(result);
+  Handle<Object> result;
+  bool success = internal::wasm::JSToWasmObject(isolate, module, value, type,
+                                                &error_message)
+                     .ToHandle(&result);
+  if (success) return *result;
+  THROW_NEW_ERROR_RETURN_FAILURE(
+      isolate, NewTypeError(MessageTemplate::kWasmTrapJSTypeError));
 }
 
 RUNTIME_FUNCTION(Runtime_WasmMemoryGrow) {

src/runtime/runtime.h

@@ -611,7 +611,7 @@ namespace internal {
   F(WasmTableCopy, 6, 1)              \
   F(WasmTableGrow, 3, 1)              \
   F(WasmTableFill, 5, 1)              \
-  F(WasmIsValidRefValue, 3, 1)        \
+  F(WasmJSToWasmObject, 3, 1)         \
   F(WasmCompileLazy, 3, 1)            \
   F(WasmCompileWrapper, 2, 1)         \
   F(WasmTriggerTierUp, 1, 1)          \

src/wasm/module-instantiate.cc

@@ -1535,8 +1535,9 @@ bool InstanceBuilder::ProcessImportedGlobal(Handle<WasmInstanceObject> instance,
 
   if (global.type.is_reference()) {
     const char* error_message;
-    if (!wasm::TypecheckJSObject(isolate_, module_, value, global.type,
-                                 &error_message)) {
+    if (wasm::JSToWasmObject(isolate_, module_, value, global.type,
+                             &error_message)
+            .is_null()) {
       ReportLinkError(error_message, global_index, module_name, import_name);
       return false;
     }
@@ -1548,7 +1549,7 @@ bool InstanceBuilder::ProcessImportedGlobal(Handle<WasmInstanceObject> instance,
                !value->IsNull()) {
       bool unpacked = TryUnpackObjectWrapper(isolate_, value);
       // Excluding SMIs and stringrefs, every value received here, must have
-      // been wrapped. This is ensured by TypeCheckJSObject().
+      // been wrapped. This is ensured by JSToWasmObject().
       DCHECK_EQ(unpacked, !value->IsSmi() && !value->IsString());
       USE(unpacked);  // Prevent nused warning if DCHECKs disabled.
     }

src/wasm/wasm-js.cc

@@ -1445,8 +1445,9 @@ bool checkAndUnpackAnyRef(i::Isolate* i_isolate,
                           i::wasm::ValueType type, ErrorThrower& thrower) {
   const char* error_message = nullptr;
   auto module = nullptr;
-  bool valid = internal::wasm::TypecheckJSObject(
-      i_isolate, module, in_out_value, type, &error_message);
+  bool valid = !internal::wasm::JSToWasmObject(i_isolate, module, in_out_value,
+                                               type, &error_message)
+                    .is_null();
   if (!valid) {
     DCHECK(error_message != nullptr);
     thrower.TypeError("%s", error_message);

src/wasm/wasm-objects.cc

@@ -325,8 +325,9 @@ bool WasmTableObject::IsValidJSElement(Isolate* isolate,
       !table->instance().IsUndefined()
           ? WasmInstanceObject::cast(table->instance()).module()
           : nullptr;
-  return wasm::TypecheckJSObject(isolate, module, entry, table->type(),
-                                 &error_message);
+  return !wasm::JSToWasmObject(isolate, module, entry, table->type(),
+                               &error_message)
+              .is_null();
 }
 
 void WasmTableObject::SetFunctionTableEntry(
@@ -2346,11 +2347,9 @@ bool TryUnpackObjectWrapper(Isolate* isolate, Handle<Object>& in_out_value) {
   return true;
 }
 
-// TODO(7748): Unify this with transforming from JS to wasm representation, and
-// use it as the single interface for the JS->wasm boundary.
-bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
-                       Handle<Object> value, ValueType expected,
-                       const char** error_message) {
+MaybeHandle<Object> JSToWasmObject(Isolate* isolate, const WasmModule* module,
+                                   Handle<Object> value, ValueType expected,
+                                   const char** error_message) {
   DCHECK(expected.is_reference());
   switch (expected.kind()) {
     case kRefNull:
@@ -2359,15 +2358,15 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
         switch (repr) {
           case HeapType::kStringViewWtf8:
             *error_message = "stringview_wtf8 has no JS representation";
-            return false;
+            return {};
           case HeapType::kStringViewWtf16:
             *error_message = "stringview_wtf16 has no JS representation";
-            return false;
+            return {};
           case HeapType::kStringViewIter:
             *error_message = "stringview_iter has no JS representation";
-            return false;
+            return {};
           default:
-            return true;
+            return value;
         }
       }
       V8_FALLTHROUGH;
@@ -2384,79 +2383,85 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
             *error_message =
                 "function-typed object must be null (if nullable) or a Wasm "
                 "function object";
-            return false;
+            return {};
           }
-          return true;
+          return MaybeHandle<Object>(Handle<JSFunction>::cast(value)
+                                         ->shared()
+                                         .wasm_function_data()
+                                         .internal(),
+                                     isolate);
         }
-        case HeapType::kExtern:
-          if (!value->IsNull(isolate)) return true;
+        case HeapType::kExtern: {
+          if (!value->IsNull(isolate)) return value;
           *error_message = "null is not allowed for (ref extern)";
-          return false;
-        case HeapType::kAny:
+          return {};
+        }
+        case HeapType::kAny: {
           if (!v8_flags.wasm_gc_js_interop) {
             TryUnpackObjectWrapper(isolate, value);
           }
-          if (!value->IsNull(isolate)) return true;
+          if (!value->IsNull(isolate)) return value;
           *error_message = "null is not allowed for (ref any)";
-          return false;
+          return {};
+        }
         case HeapType::kData: {
           if (v8_flags.wasm_gc_js_interop
                   ? value->IsWasmStruct() || value->IsWasmArray()
                   : TryUnpackObjectWrapper(isolate, value)) {
-            return true;
+            return value;
           }
           *error_message =
               "dataref object must be null (if nullable) or a wasm "
               "struct/array";
-          return false;
+          return {};
         }
         case HeapType::kArray: {
           if ((v8_flags.wasm_gc_js_interop ||
                TryUnpackObjectWrapper(isolate, value)) &&
               value->IsWasmArray()) {
-            return true;
+            return value;
           }
           *error_message =
               "arrayref object must be null (if nullable) or a wasm array";
-          return false;
+          return {};
         }
         case HeapType::kEq: {
           if (value->IsSmi() ||
               (v8_flags.wasm_gc_js_interop
                    ? value->IsWasmStruct() || value->IsWasmArray()
                    : TryUnpackObjectWrapper(isolate, value))) {
-            return true;
+            return value;
           }
           *error_message =
               "eqref object must be null (if nullable) or a wasm "
               "i31/struct/array";
-          return false;
+          return {};
         }
         case HeapType::kI31: {
-          if (value->IsSmi()) return true;
+          if (value->IsSmi()) return value;
           *error_message =
               "i31ref object must be null (if nullable) or a wasm i31";
-          return false;
+          return {};
         }
         case HeapType::kString:
-          if (value->IsString()) return true;
+          if (value->IsString()) return value;
           *error_message = "wrong type (expected a string)";
-          return false;
+          return {};
         case HeapType::kStringViewWtf8:
           *error_message = "stringview_wtf8 has no JS representation";
-          return false;
+          return {};
         case HeapType::kStringViewWtf16:
           *error_message = "stringview_wtf16 has no JS representation";
-          return false;
+          return {};
         case HeapType::kStringViewIter:
           *error_message = "stringview_iter has no JS representation";
-          return false;
+          return {};
         default:
           if (module == nullptr) {
             *error_message =
                 "an object defined in JavaScript cannot be compatible with a "
                 "type defined in a Webassembly module";
-            return false;
+            return {};
           }
           DCHECK(module->has_type(expected.ref_index()));
           if (module->has_signature(expected.ref_index())) {
@@ -2471,12 +2476,9 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
                 *error_message =
                     "assigned exported function has to be a subtype of the "
                     "expected type";
-                return false;
+                return {};
               }
-              return true;
-            }
-
-            if (WasmJSFunction::IsWasmJSFunction(*value)) {
+            } else if (WasmJSFunction::IsWasmJSFunction(*value)) {
               // Since a WasmJSFunction cannot refer to indexed types (definable
               // only in a module), we do not need full function subtyping.
               // TODO(manoskouk): Change this if wasm types can be exported.
@@ -2485,12 +2487,9 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
                 *error_message =
                     "assigned WasmJSFunction has to be a subtype of the "
                     "expected type";
-                return false;
+                return {};
               }
-              return true;
-            }
-
-            if (WasmCapiFunction::IsWasmCapiFunction(*value)) {
+            } else if (WasmCapiFunction::IsWasmCapiFunction(*value)) {
               // Since a WasmCapiFunction cannot refer to indexed types
               // (definable only in a module), we do not need full function
               // subtyping.
@@ -2500,33 +2499,28 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
                 *error_message =
                     "assigned WasmCapiFunction has to be a subtype of the "
                     "expected type";
-                return false;
+                return {};
               }
-              return true;
+            } else {
+              *error_message =
+                  "function-typed object must be null (if nullable) or a Wasm "
+                  "function object";
+              return {};
             }
-
-            *error_message =
-                "function-typed object must be null (if nullable) or a Wasm "
-                "function object";
-
-            return false;
+            return MaybeHandle<Object>(Handle<JSFunction>::cast(value)
+                                           ->shared()
+                                           .wasm_function_data()
+                                           .internal(),
+                                       isolate);
           } else {
             // A struct or array type with index is expected.
             DCHECK(module->has_struct(expected.ref_index()) ||
                    module->has_array(expected.ref_index()));
-            if (value->IsNull()) {
-              if (expected.is_non_nullable()) {
-                *error_message =
-                    "invalid null value for non-nullable element type";
-                return false;
-              }
-              return true;
-            }
             if (v8_flags.wasm_gc_js_interop
                     ? !value->IsWasmStruct() && !value->IsWasmArray()
                     : !TryUnpackObjectWrapper(isolate, value)) {
               *error_message = "object incompatible with wasm type";
-              return false;
+              return {};
             }
             auto wasm_obj = Handle<WasmObject>::cast(value);
             WasmTypeInfo type_info = wasm_obj->map().wasm_type_info();
@@ -2535,9 +2529,9 @@ bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
             if (!IsHeapSubtypeOf(HeapType(actual_idx), expected.heap_type(),
                                  actual_module, module)) {
               *error_message = "object is not a subtype of element type";
-              return false;
+              return {};
             }
-            return true;
+            return value;
           }
       }
     }

src/wasm/wasm-objects.h

@@ -1067,9 +1067,16 @@ class WasmSuspenderObject
 #undef DECL_OPTIONAL_ACCESSORS
 
 namespace wasm {
-bool TypecheckJSObject(Isolate* isolate, const WasmModule* module,
-                       Handle<Object> value, ValueType expected,
-                       const char** error_message);
+// Takes a {value} in the JS representation and typechecks it according to
+// {expected}. If the typecheck succeeds, returns the wasm representation of the
+// object; otherwise, returns the empty handle.
+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