1419781 - wasmBinaryToText mistranslates a global export

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Lars T Hansen [:lth]]

Input:

(module
 (global (export "hi") i32 (i32.const 0)))

text->binary followed by binary->text:

(module
  (global $global0 i32 (i32.const 0))
  (export "hi" global0)
)

Observe the missing '$' in the export clause.

Comment 1

[Lars T Hansen [:lth]]

Something deeper is wrong here, maybe I don't understand the syntax properly.  This can be parsed by wasmTextToBinary even though I thought it should not be:

(module
  (global $global0 i32 (i32.const 0))
  (export "hi" global0))

This cannot be converted:

(module
  (global $global0 i32 (i32.const 0))
  (export "hi" $global0))

This cannot be converted:

(module
  (global $global33 i32 (i32.const 0))
  (export "hi" global33))

This can:

(module
  (global i32 (i32.const 0))
  (export "hi" global0))

That is: (a) the label on the global is completely ignored and (b) the reference in the export clause is syntax, not a name.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Congratulations, you've run into the "spaces are not meaningful" feature of our WAT parser! For instance, (module(globali32(i32.const0))(export"hi"global0)) is totally valid.

In the first example,  (export "hi" global0)) is actually equivalent to (export "hi" global 0)), which is correct since there's one global which is indexed at 0.

In the second example, (export) used without the kind keyword will default to exporting a function, and if there's no function with index $global0, it won't work.

In the third example,   (export "hi" global33)) is equivalent to   (export "hi" global 33)), and there aren't 33 globals. What you want here is (export "hi" global $global33)).

In the fourth example, global at index 0 is exported.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Congratulations, you've run into the "spaces are not meaningful" feature of
> our WAT parser! 

I feel honored.  Since you call it a "feature" I assume no bug has been filed?  :-}

Thanks for the explanation, it makes sense now.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: render.patch

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

> Since you call it a "feature" I assume no bug has been filed?  :-}

We noticed it in the past, but didn't consider it critical; this path is only used for testing.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8935034 [details] [diff] [review]
render.patch

Review of attachment 8935034 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, right.  Excellent idea.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5fced4aa8fd
Render a supplementary space when exporting a wasm global; r=lth

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c5fced4aa8fd