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

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dmitry]

[WASM-References] Support imm for ref.null

[Dmitry]

Created attachment 413682 [details]
Patch

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Dmitry]

Created attachment 413779 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 413779 [details]
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

[Dmitry]

Created attachment 413917 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 413917 [details]
Patch

Nice! r=me if EWS gets green.

[EWS]

Committed r269729: <https://trac.webkit.org/changeset/269729>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413917 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71321783>