[webkit-reviews] review denied: [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]

Saam Barati <sbarati at apple.com> has denied  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 #4 from Saam Barati <sbarati 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

Everything looks great! r- because I think I found a bug in the stub that we
use to call from JS to Wasm. Let's add a test for that and get this landed

> Source/JavaScriptCore/ChangeLog:7
> +	   Add support for Anyref in parameters and return types of Wasm
functions.
> +	   Add Ref.null and Ref.is_null for Anyref values. Support for these
functions with funcrefs is out of scope.

style nit: this should go below "Reviewed by ..."

Also, you should talk about how you implemented this here. What from JS maps to
null in Wasm? etc

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:258
> +	   // If 0, this is nullref so it becomes null. Otherwise, it is
already a JSValue.

not sure this comment is adding much

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:183
> +	       case Anyref: {

Can this just be part of the I32 case above, and the call to zeroExtend can
branch on Anyref vs I32?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:424
> +		   // If 0, this is nullref so it becomes null. Otherwise, it
is already a JSValue.

comment isn't adding much that the code doesn't tell you.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:425
> +		   auto isNullref = jit.branchIfNotEmpty(gprReg);

this variable is named wrong. Should be something like "not null ref"

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:595
> +	   // Any JSValue, Ref.func or Ref.null is valid here, and it is
impossible to create any other kind of ref from within wasm.
> +	   // Hence, there are no exception checks.
> +
> +	   // If null, this is nullref so it becomes 0. Otherwise, it is still
a JSValue.

same about comment not being helpful.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:596
> +	   auto isNullref = jit.branchIfNotNull(GPRInfo::returnValueGPR);

nit: Wrong name. This is JSValue null, so should be named something like
"isNull"

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:88
> +	       // All JSValues are valid, but null becomes 0.

ditto about comment

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:461
> +    case Wasm::Anyref: {

You're not handling this as a parameter in this JS to Wasm stub. I think that's
a bug. Please add a test.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:462
> +	   // If 0, this is nullref so it becomes null. Otherwise, it is
already a JSValue.

ditto

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:463
> +	   auto isNullref = jit.branchIfNotEmpty(GPRInfo::returnValueGPR);

nit: name should be inverted to "not null ref"