Bug 165886 – Wasm should decode constants correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Wasm should decode constants correctly

[Keith Miller]

Created attachment 297156 [details]
Patch

[WebKit Commit Bot]

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".

[JF Bastien]

Comment on attachment 297156 [details]
Patch

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

A few comments, after lgtm.

> JSTests/wasm/LowLevelBinary.js:139
> +            throw new RangeError("We cannot write NaNs yet");

Other messages use "unimplemented" so it's easy to grep (here and above).

> JSTests/wasm/LowLevelBinary.js:140
> +        // Unfortunately, we cannot just view the actual buffer as a Float32Array since it needs to be 4 byte aligned

You mean Float64 here.

[Saam Barati]

Comment on attachment 297156 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:535
> +        return m_currentBlock->appendNew<ConstDoubleValue>(m_proc, Origin(), bitwise_cast<double>(static_cast<int64_t>(value)));

Why is this needed? value is already uint64_t

> Source/JavaScriptCore/wasm/wasm.json:65
> +        "f64.const":           { "category": "special",    "value":  68, "return": ["f64"],      "parameter": [],                       "immediate": [{"name": "value",          "type": "double"}],                                             "description": "a constant value interpreted as f64" },
> +        "f32.const":           { "category": "special",    "value":  67, "return": ["f32"],      "parameter": [],                       "immediate": [{"name": "value",          "type": "float"}],                                             "description": "a constant value interpreted as f32" },
> +        "get_local":           { "category": "special",    "value":  32, "return": ["any"],    "parameter": [],                       "immediate": [{"name": "local_index",    "type": "varuint32"}],                                          "description": "read a local variable or parameter" },
> +        "set_local":           { "category": "special",    "value":  33, "return": [],           "parameter": ["any"],                "immediate": [{"name": "local_index",    "type": "varuint32"}],                                          "description": "write a local variable or parameter" },
> +        "tee_local":           { "category": "special",    "value":  34, "return": ["any"],     "parameter": ["any"],                  "immediate": [{"name": "local_index",    "type": "varuint32"}],                                          "description": "write a local variable or parameter and return the same value" },
> +        "get_global":          { "category": "special",    "value":  35, "return": ["any"],   "parameter": [],                       "immediate": [{"name": "global_index",   "type": "varuint32"}],                                          "description": "read a global variable" },
> +        "set_global":          { "category": "special",    "value":  36, "return": [],         "parameter": ["any"],               "immediate": [{"name": "global_index",   "type": "varuint32"}],                                          "description": "write a global variable" },

Should you make these changes in the other wasm.json too?

[Keith Miller]

Committed r209852: <http://trac.webkit.org/changeset/209852>