Closed Bug 1545761 Opened 4 months ago Closed 4 months ago

WasmTextToBinary: global initial value requires parentheses

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: volker.berlin, Assigned: lth, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

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.

Blocks: 1527871
Type: defect → enhancement

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!

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Summary: WasmTextToBinary: global initial value not parsed → WasmTextToBinary: global initial value requires parentheses
Mentor: lhansen
Keywords: good-first-bug

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.

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

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

Assignee: nobody → lhansen
Status: NEW → ASSIGNED

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.

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f69959d8a88
Accept paren-less initializer syntax for globals. r=jseward
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Works for me now.

You need to log in before you can comment on or make changes to this bug.