[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
bugzilla-daemon at webkit.org
Thu May 16 18:01:03 PDT 2019
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"
More information about the webkit-reviews
mailing list