1545761 - WasmTextToBinary: global initial value requires parentheses

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Volker Berlin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.103 Safari/537.36

Steps to reproduce:

I test with version JavaScript-C68.0a1 from https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/jsshell-win64.zip

Call the line:
wasmTextToBinary(read('test.wat'));

with the file content:

(module
  (global $condition (mut i32) i32.const 0)
)

Actual results:

SyntaxError: wasm text error: parsing wasm text at 2:33

The error position is the i32.const 0. I have no idea if any syntax will be work.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Our syntax just wants more parenthesis around the initial value:

(module
  (global $condition (mut i32) (i32.const 0))
)

If you're interested to provide a patch to accept both modes, I'd be happy to review it!

Comment 2

[Volker Berlin]

(i32.const 0)

Thanks for the suggestion.

If you're interested to provide a patch to accept both modes, I'd be happy to review it!

I am not able to write a patch.

Comment 3

[Lars T Hansen [:lth]]

From an engineering point of view we must also worry about whether the parenless expression is allowed in other initializers for which we share the parsing code: element segments and data segments.

Comment 4

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #3)

From an engineering point of view we must also worry about whether the parenless expression is allowed in other initializers for which we share the parsing code: element segments and data segments.

Officially, the syntax is actually (offset <const_expr>) -- we don't accept this either -- and in this case any parens around <const_expr> are optional, they're syntactic sugar. Spec:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/text/parser.mly#L569
https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/text/parser.mly#L490
https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/text/parser.mly#L343

However just plain <expr> is allowed as sugar, but in this case the parens are required:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/text/parser.mly#L427

For some examples, see here:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/test/core/elem.wast#L4

To completely confuse the issues, in a case like this:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/test/core/elem.wast#L11
the literal integer is not an offset, but a label, this is pretty weird. In the future we'll want a table index here too.

Comment 5

[Lars T Hansen [:lth]]

I'll take this and fix it in a local manner and then open other bugs re the offset thing.

Comment 6

[Lars T Hansen [:lth]]

Attachment: Bug 1545761 - Accept paren-less initializer syntax for globals. r?jseward

This is for alignment with the WAT standard (wabt / reference interpreter).
To simplify, we split this parsing out from the parsing used for the
initializers for elem/data because those have other complexities and
will be handled separately, see bug 1527871.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f69959d8a88
Accept paren-less initializer syntax for globals. r=jseward

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/2f69959d8a88

Comment 9

[Volker Berlin]

Works for me now.