1308056 - WebAssembly: syntax changes for inline imports/exports

Status: RESOLVED FIXED

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

Description

[Alon Zakai (:azakai)]

Attachment: b.out.wast

Attached are

b.out.wast - a wast from emcc (with function bodies emptied)
b.byn.wasm - the wasm binaryen emits
b.wabt.wasm - the wasm wabt emits

binaryen and wabt emit almost the same binary, and appear compatible with each other (wabt can load the binaryen wasm, etc.). However SpiderMonkey errors on both binaries,

CompileError: at offset 805: unsupported import kind

on byn.wast, and

CompileError: at offset 889: unsupported import kind

on wabt.wasm.

Looking in the binaryen debug of writing the binary, offset 805 is actually in the *exports* section, not imports. Imports is 57-578, exports is 789-1052. (My first guess was a typo in the error message in SpiderMonkey, but that's not it.)

Comment 1

[Alon Zakai (:azakai)]

Attachment: b.byn.wasm

Comment 2

[Alon Zakai (:azakai)]

Attachment: b.wabt.wasm

Comment 3

[Alon Zakai (:azakai)]

Attachment: empty module

Trying to reduce this further is hard. When I remove stuff I see different errors, I can't seem to get to a good baseline. I also see an error on an empty binaryen/wabt module (attached):

CompileError: at offset 16: currently, every memory/table must be declared default

Comment 4

[Alon Zakai (:azakai)]

One option here is to wait for sm to support the new import/export syntax in wasts, then we could diff the sm binary with binaryen/wabt's.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Looking at this right now with the other syntax changes.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

As a reminder to myself: the default flag on table/memory isn't a thing anymore.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.limits.patch

This renames ResizableLimits to Limits, which is more concise and match the spec description.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.anyfunc-table-inline.patch

This introduces the anyfunc syntax and adds inline imports / exports for tables. It also modifies a bunch of tests to just match this new syntax, except of having special cases.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.func-inline.patch

This adds inline imports and exports syntax for functions.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.globals-inline-immutable-by-default.patch

Make globals great again^W^W immutable by default and add inline import/export syntax.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.owner-index.patch

Adds optional syntax to reference owning table/memory.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6-memory-inline.patch

This implements syntax for inline import/export for memories.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7-default-flag.patch

This removes the default flag and ResizableFlags (since it's just a boolean to say whether we have a maximum amount or not).

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 8-spec-tests.patch

This enhances the wast interpreter to support the following:
- (get) syntax for retrieving global exports
- (assert_unlinkable) for modules we can compile but not link
- optional module names, when using the wast interpreter builtins like invoke/get.

Also re-enables a few tests that were disabled mostly because of the new syntax.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: folded.patch

Folded patch. With this one, the wast file in this bug can be parsed, as well as the AngryBots as proposed in https://github.com/WebAssembly/webassembly.github.io/pull/28

There are still a few issues with the binary though, and I will probably get to them first thing next week.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8798910 [details] [diff] [review]
1.limits.patch

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

Agreed!

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +4092,5 @@
>      return true;
>  }
>  
>  static bool
> +EncodeResizableTable(Encoder& e, const Limits& resizable)

EncodeTableLimits / EncodeLimits?

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8798912 [details] [diff] [review]
2.anyfunc-table-inline.patch

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

Ah, nice to see 'resizable' go; that was ugly, who did that anyway ;)

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8798913 [details] [diff] [review]
3.func-inline.patch

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

Nice!

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8798917 [details] [diff] [review]
7-default-flag.patch

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

Ah, good to have this fixed!

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8798918 [details] [diff] [review]
8-spec-tests.patch

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

Really great work on this whole stack.  Nice-looking code and much appreciated!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6996ca3c1ef6
Rename ResizableLimits to Limits; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a39a87c6bb9
wasm: add anyfunc and inline import/export for tables; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/c417fb206f6c
wasm: add syntax for inline import/export in functions; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/913ccaec26d5
wasm: add syntax for inline import/export in globals; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/27bec108f660
Allow to index memory/table owner in elem/data sections; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aaf29ab8c30
wasm: add syntax for inline import/export in memories; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/8030f70a5130
wasm: enable a few spec test cases; r=luke

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

I haven't pushed the default memory flag patch, because it would have broken the WebAssembly demo on webassembly.org.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows bustage and spidermonkey test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35590eb61756f1f36569a19740bfdc34e3bb190b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d77072059582f13fe62477717836cec397dbc70b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced9e3a01e45e1595cae4133993be10114260c55
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b61cc39e92fb28f7d5d946c4a78123db56970a
https://hg.mozilla.org/integration/mozilla-inbound/rev/089d15228b41bf259e5b809fca406f0e5fed40ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/206a77f3fffbc168474cfc3acc13630d22658faf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8276f7f3413f821e07bbea0f77842d807b951815


Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8030f70a51308de1e8448e538817b86b7315721e

Failure log Windows XP build: https://treeherder.mozilla.org/logviewer.html#?job_id=37381412&repo=mozilla-inbound

05:11:13     INFO -  c:/mozilla-build/python27/python2.7.EXE c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/sccache/sccache.py c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/vs2015u3/VC/bin/amd64_x86/cl.EXE -FoUnified_cpp_js_src33.obj -c   -DDEBUG=1 -DTRACING=1 -D_CRT_RAND_S -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES '-DDLL_PREFIX=""' '-DDLL_SUFFIX=".dll"' -DFFI_BUILDING -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/js/src -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/js/src -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/js/src/ctypes/libffi/include -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/js/src/ctypes/libffi/src/x86 -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/dist/include  -Ic:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/dist/include/nspr         -MD -FI c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/js/src/js-confdefs.h -DMOZILLA_CLIENT -deps.deps/Unified_cpp_js_src33.obj.pp  -TP -nologo -wd4345 -wd4351 -wd4800 -wd4819 -wd4595 -D_CRT_SECURE_NO_WARNINGS -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:threadSafeInit- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -FS -Gw -wd4244 -wd4267 -wd4251 -we4553 -GR-  -Z7 -O2 -Oy- -WX -wd4805 -wd4661 -we4067 -we4258 -we4275 -wd4146 -wd4577 -wd4312   c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/obj-firefox/js/src/Unified_cpp_js_src33.cpp
05:11:19     INFO -  Unified_cpp_js_src27.cpp
05:11:19     INFO -  Unified_cpp_js_src3.cpp
05:11:19     INFO -  c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\asmjs\wasmtexttobinary.cpp(2778) : error C2220: warning treated as error - no 'object' file generated
05:11:19     INFO -  Warning: C4700 in c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\asmjs\wasmtexttobinary.cpp: uninitialized local variable 'openParen' used
05:11:19     INFO -  c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\asmjs\wasmtexttobinary.cpp(2778) : warning C4700: uninitialized local variable 'openParen' used
05:11:20     INFO -  c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/config/rules.mk:961: recipe for target 'Unified_cpp_js_src3.obj' failed
05:11:20     INFO -  mozmake.EXE[5]: *** [Unified_cpp_js_src3.obj] Error 2


Failure log sm-plain: https://treeherder.mozilla.org/logviewer.html#?job_id=37381370&repo=mozilla-inbound

[task 2016-10-11T12:01:01.936891Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/f64_cmp.wast.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2016-10-11T12:01:01.969871Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/float_literals.wast.js | Success (code 0, args "--wasm-always-baseline")
[task 2016-10-11T12:01:01.977522Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/float_literals.wast.js | Success (code 0, args "--baseline-eager")
[task 2016-10-11T12:01:01.997525Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/float_literals.wast.js | Success (code 0, args "--no-baseline --no-ion")
[task 2016-10-11T12:01:02.002592Z] TEST-PASS | js/src/jit-test/tests/wasm/spec/float_literals.wast.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2016-10-11T12:01:02.019874Z] /home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18 SyntaxError: wasm text error: parsing wasm text at 1:4294663681
[task 2016-10-11T12:01:02.019945Z] Stack:
[task 2016-10-11T12:01:02.019983Z]   wasmEvalText@/home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18
[task 2016-10-11T12:01:02.020025Z]   exec@/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:298:22
[task 2016-10-11T12:01:02.020071Z]   @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:491:13
[task 2016-10-11T12:01:02.020125Z]   @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/float_memory.wast.js:2:43
[task 2016-10-11T12:01:02.020176Z] Exit code: 3
[task 2016-10-11T12:01:02.020234Z] FAIL - wasm/spec/float_memory.wast.js
[task 2016-10-11T12:01:02.020364Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/spec/float_memory.wast.js | /home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18 SyntaxError: wasm text error: parsing wasm text at 1:4294663681 (code 3, args "")
[task 2016-10-11T12:01:02.020418Z] INFO exit-status     : 3
[task 2016-10-11T12:01:02.020479Z] INFO timed-out       : False
[task 2016-10-11T12:01:02.020564Z] INFO stderr         2> /home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18 SyntaxError: wasm text error: parsing wasm text at 1:4294663681
[task 2016-10-11T12:01:02.020617Z] INFO stderr         2> Stack:
[task 2016-10-11T12:01:02.020694Z] INFO stderr         2> wasmEvalText@/home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18
[task 2016-10-11T12:01:02.020767Z] INFO stderr         2> exec@/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:298:22
[task 2016-10-11T12:01:02.020844Z] INFO stderr         2> @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:491:13
[task 2016-10-11T12:01:02.020922Z] INFO stderr         2> @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/float_memory.wast.js:2:43
[task 2016-10-11T12:01:02.048499Z] /home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18 SyntaxError: wasm text error: parsing wasm text at 1:86588929
[task 2016-10-11T12:01:02.048573Z] Stack:
[task 2016-10-11T12:01:02.048611Z]   wasmEvalText@/home/worker/workspace/build/src/js/src/jit-test/lib/wasm.js:7:18
[task 2016-10-11T12:01:02.048652Z]   exec@/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:298:22
[task 2016-10-11T12:01:02.048692Z]   @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/../spec.js:491:13
[task 2016-10-11T12:01:02.048745Z]   @/home/worker/workspace/build/src/js/src/jit-test/tests/wasm/spec/float_memory.wast.js:2:43
[task 2016-10-11T12:01:02.048777Z] Exit code: 3

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8798916 [details] [diff] [review]
6-memory-inline.patch

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

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +2751,5 @@
>  {
> +    AstName name = c.ts.getIfName();
> +
> +    WasmToken openParen;
> +    if (c.ts.getIf(WasmToken::OpenParen)) {

Don't you mean to pass &openParen as a second arg here?

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Great catch, thanks!

I've changed the default ctor of WasmToken to initialize the kind_ to Kind(-1), with a MOZ_ASSERT in the kind() getter, which would have caught this issue.

Very strange that gcc didn't catch this uninitialized use... But MSVC did:
05:11:19 INFO - Warning: C4700 in c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\js\src\asmjs\wasmtexttobinary.cpp: uninitialized local variable 'openParen' used

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/555ab2415874
Rename ResizableLimits to Limits; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b0d3332ae4
wasm: add anyfunc and inline import/export for tables; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d375dc220475
wasm: add syntax for inline import/export in functions; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/968f741d2686
wasm: add syntax for inline import/export in globals; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/65c5f806efd3
Allow to index memory/table owner in elem/data sections; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8b3e369a53
wasm: add syntax for inline import/export in memories; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/9128754c56f4
wasm: enable a few spec test cases; r=luke

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/555ab2415874
https://hg.mozilla.org/mozilla-central/rev/36b0d3332ae4
https://hg.mozilla.org/mozilla-central/rev/d375dc220475
https://hg.mozilla.org/mozilla-central/rev/968f741d2686
https://hg.mozilla.org/mozilla-central/rev/65c5f806efd3
https://hg.mozilla.org/mozilla-central/rev/5a8b3e369a53
https://hg.mozilla.org/mozilla-central/rev/9128754c56f4

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: update-binary-getset-global.patch

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: workarounds.patch

Not for review: a set of workarounds to make all the given examples compile under spidermonkey.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee09d521307
wasm: remove default memory/table option and simplify flags; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0611e12d2cb
Update binary encoding of GetGlobal/SetGlobal; r=luke

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ee09d521307
https://hg.mozilla.org/mozilla-central/rev/d0611e12d2cb