[webkit-reviews] review granted: [Bug 197969] [WASM-References] Add support for Anyref in parameters and return types, Ref.null and Ref.is_null for Anyref values. : [Attachment 370091] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 197969: [WASM-References] Add support for Anyref in parameters and return
types, Ref.null and Ref.is_null for Anyref values.
https://bugs.webkit.org/show_bug.cgi?id=197969

Attachment 370091: Patch

https://bugs.webkit.org/attachment.cgi?id=370091&action=review




--- Comment #3 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 370091
  --> https://bugs.webkit.org/attachment.cgi?id=370091
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370091&action=review

r=me

> Source/JavaScriptCore/wasm/WasmValidate.cpp:177
> +    WASM_VALIDATOR_FAIL_IF(Type::Anyref != value, "ref.is_null to type ",
value, " expected ", Type::Anyref);

You can define a helper function like, `isReferenceType()` instead of directly
checking AnyRef, and in the future, you can extend this function to accept
funcref too.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:261
> +	   isNullref.link(&jit);

Discussed with Justin, at this point, we can just use JSValue representation
directly.
We could change this after revisiting thread proposal.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:262
> +			   if (!buffer[argNum])
> +			       arg = jsNull();
> +			   else
> +			       arg = JSValue::decode(buffer[argNum]);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:300
> +		       if (result.isNull())
> +			   realResult = 0;
> +		       else
> +			   realResult = JSValue::encode(result);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:427
> +		   // If 0, this is nullref so it becomes null. Otherwise, it
is already a JSValue.
> +		   auto isNullref = jit.branchIfNotEmpty(gprReg);
> +		   jit.moveTrustedValue(jsNull(), JSValueRegs { gprReg });
> +		   isNullref.link(&jit);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:598
> +	   isNullref.link(&jit);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:90
> +	   case Wasm::Anyref:
> +	       // All JSValues are valid, but null becomes 0.
> +	       if (arg.isNull())
> +		   arg = JSValue::decode(0);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:465
> +	   isNullref.link(&jit);

Ditto.