[Webkit-unassigned] [Bug 218744] [WASM-References] Support imm for ref.null

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=218744

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysuzuki at apple.com
 Attachment #413779|                            |review-
              Flags|                            |

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

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

Nice, the design of the patch looks good!

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Can you describe the explanation of changes in ChangeLog?
And can you also add a link to the spec of this change in the ChangeLog?

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:497
> +        WASM_PARSER_FAIL_IF(!parseValueType(typeOfNull), "can't get ref.null's type");

This is checking whether the value is ValueType. I think we need to validate whether this is RefType.
Can you add a test to ensure that module validation fails if the input is not a retype?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:478
> +        WASM_PARSER_FAIL_IF(!parseValueType(typeOfNull), "can't get ref.null's type");

Ditto.

> Source/JavaScriptCore/wasm/wasm.json:62
> +        "ref.null":            { "category": "special",    "value": 208, "return": ["externref", "funcref"],         "parameter": [],                             "immediate": [{"name": "reftype",        "type": "elem_type"}],                                             "description": "a constant null reference" },

Ditto.

> JSTests/wasm/Builder.js:311
> +            assert.truthy(WASM.isValidElemType(imms[idx]), `Invalid elem type on ${op}: "${imms[idx]}"`);

Let's rename ElemType to RefType.

> JSTests/wasm/LowLevelBinary.js:206
> +    elem_type(v) {
> +        if (!WASM.isValidElemType(v))

Ditto. ref_type and RefType.

> JSTests/wasm/WASM.js:45
> +const _elemTypeSet = new Set(description.elem_type);
> +export const isValidElemType = v => _elemTypeSet.has(v);

Ditto.

> JSTests/wasm/wasm.json:62
> +        "ref.null":            { "category": "special",    "value": 208, "return": ["externref", "funcref"],         "parameter": [],                             "immediate": [{"name": "reftype",        "type": "elem_type"}],                                             "description": "a constant null reference" },

Can you rename this "elem_type" to "ref_type"?
https://webassembly.github.io/reference-types/core/binary/instructions.html#reference-instructions

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201111/26342b38/attachment-0001.htm>