Closed Bug 1612534 Opened 4 years ago Closed 4 years ago

Replace guts of wasmTextToBinary with the wat subsystem

Categories

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

Product:
Core
Component:
JavaScript: WebAssembly
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla76
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: lth, Assigned: rhunt)

References

Details

Attachments

(16 files)

Bug 1612534 - Use 'wat' crate for wasmTextToBinary. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Include vendored files. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch to (result) type parameter for block. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch to (result) type parameter for if. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch call_indirect to (type). r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch to explicit (export (func)). r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch (export memory). r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch (export table). r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch to (result) type parameter for loop. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Switch to (result) type parameter for block's with labels. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix wasm/ jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix 'wasm/bigint/' jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix 'wasm/gc/' jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix 'wasm/regress/' jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix 'asm.js/' jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review
Bug 1612534 - Manually fix 'debug/' jit-tests. r?lth
47 bytes, text/x-phabricator-request
Details | Review

According to Alex, the wat crate (https://crates.io/crates/wat) strives to have everything that wabt has and he takes a liberal view on accepting extensions (eg for our experimental GC stuff). With that in mind, it's plausible that we can just replace the guts of wasmTextToBinary with the wat crate and thus get rid of our non-standard C++-based parser without impacting our velocity. This would then in principle resolve a lot of open bugs (attaching a partial list of blocked bugs, but there are more, follow the bug trees) and reduce effort on future maintenance.

Depends on: 1613945

I've got a prototype of this hooked up here [1]. I've tested locally that it can compile (module), and give error messages for failures.

Beyond that, this try run will probably be very red due to syntax misalignment.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7517c9e0ff88bc90a95be0c008bfea70e74c33f

Assignee: nobody → rhunt

(In reply to Ryan Hunt [:rhunt] from comment #2)

Fix for unused build errors. Now we can see the syntax errors.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b88fb8575f0d97d05e74a97adacf7360e48ffdad

I think "the first 1% of the syntax errors" is probably more like it, judging from the output... Great start though!

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

(In reply to Ryan Hunt [:rhunt] from comment #2)

Fix for unused build errors. Now we can see the syntax errors.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b88fb8575f0d97d05e74a97adacf7360e48ffdad

I think "the first 1% of the syntax errors" is probably more like it, judging from the output... Great start though!

I've now gotten all non-gc jit-tests passing. It was indeed just the first 1%.

I've tried to fix most issues with regex, and then did a manual pass on the rest.

These are significant areas of change:

  1. Different 'inf' vs 'infinity'
  2. Different float constant overflow behavior (wat fails, we overflow to infinity)
  3. 'Block, loop, if' type syntax
  4. call_indirect type syntax
  5. import defaulting to 'func' or not
  6. export defaulting to 'func' or not
  7. imports allowed after other definitions or not
  8. (result) allowed before (param) or not
  9. (local) allowed before (result)/(param) or not

Overall, wat seems to do an excellent job here. There were multiple tests where the behavior changed to be more spec-compliant in subtle ways. There were a couple issues, but Alex quickly resolved them [1] [2] [3].

[1] https://github.com/bytecodealliance/wat/issues/59
[2] https://github.com/bytecodealliance/wat/issues/60
[3] https://github.com/bytecodealliance/wat/issues/62

Finally have all tests passing, including the GC ones. There's another PR with some misc fixes on the 'wat' crate that will need to be merged first. Other than that, the only outstanding issue is the 'withOffsets' parameter to wasmTextToBinary.

This flag changes wasmTextToBinary to record an offsets array of the offset of the start of each function's code. It's only used in a couple places as far as I can tell, mostly debugging tests. Somehow, returning an empty offsets array still allows them to pass so I didn't notice until late. The wat crate will probably need to get support added somehow for this.

Try run here [1]. It may just burn and crash with a build error if I didn't import 'wat' correctly. Couldn't use the normal mach vendor as there are some fixes upstream that haven't been released.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=786874c04b959810e70f5e64ab5e9e8cc5550059

This commit removes the old WasmTextToBinary implementation and replaces
it with a stub that calls into the rust wat implementation.

The old wasmTextToBinary function took an optional boolean parameter to
indicate that additional metadata was desired. This extra metadata was
a list of offsets of instructions in the code section. I'm not sure if
this was intentional or not, but it looks like it only includes root
expressions of nested s-exprs.

These offsets were used in two tests, debug/wasm-breakpoints.js and a
wasm/regress test. Adding this functionality into 'wat' proved to be a large
change for a function that didn't seem to have much use cases. A new
wasmCodeOffsets function was added to replicate this feature. It's implemented
in rust using the 'wasmparser' crate that cranelift uses. When cranelift is
compiled, this shouldn't result in an increased binary size. If the compile
time or binary size proves to be an issue in non-cranelift builds, I think
we can make this a conditional feature for JS-shells only.

This commit uses a regex to automatically replace uses of
block valtype
with
block (result valtype)

Depends on D67246

This commit uses a regex to automatically replace uses of
if valtype
with
if (result valtype)

Depends on D67247

This commit uses a regex to automatically switch call_indirect type signature
immediates to use an explicit '(type $id)' wrapper.

This has a negative interaction with the gc/ tests which use an identifier
immediately after call_indirect to specify the table and not the signature. A
later commit fixes this.

Depends on D67248

This commit uses a regex to automatically change exports of indices
to specify that they are exporting functions.

Depends on D67249

This commit uses a regex to automatically switch the export syntax
for memory.

Depends on D67250

This commit uses a regex to automatically switch the export syntax for table.

Depends on D67251

This commit uses a regex to automatically switch
(loop valtype ...
with
(loop (result valtype ...

Depends on D67252

This commit uses a regex to automatically switch
(block $label valtype ...
with
(block $label (result valtype ...

This should have been combined with the earlier 'block' commit, but was not
realised until too late.

Depends on D67253

This commit includes all the manual changes needed to get 'wasm/' jit-tests
passing.

Depends on D67254

This commit includes all the manual changes needed to get 'wasm/bigint'
jit-tests passing.

Depends on D67255

This commit includes all the changes necessary to get 'wasm/gc' jit-tests
passing.

Depends on D67256

This commit includes all the changes needed to get 'wasm/regress/' passing
jit-tests.

Depends on D67257

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/d2f2c94dca9e
Use 'wat' crate for wasmTextToBinary. r=lth
https://hg.mozilla.org/integration/autoland/rev/f1950c23749c
Include vendored files. r=lth
https://hg.mozilla.org/integration/autoland/rev/3fc498a2bb3c
Switch to (result) type parameter for block. r=lth
https://hg.mozilla.org/integration/autoland/rev/3af7b2e5ef1f
Switch to (result) type parameter for if. r=lth
https://hg.mozilla.org/integration/autoland/rev/a72c64deac11
Switch call_indirect to (type). r=lth
https://hg.mozilla.org/integration/autoland/rev/7e2f866b6e94
Switch to explicit (export (func)). r=lth
https://hg.mozilla.org/integration/autoland/rev/6ad16f01837d
Switch (export memory). r=lth
https://hg.mozilla.org/integration/autoland/rev/c550399af2a8
Switch (export table). r=lth
https://hg.mozilla.org/integration/autoland/rev/d7e96dba0889
Switch to (result) type parameter for loop. r=lth
https://hg.mozilla.org/integration/autoland/rev/3724851a0720
Switch to (result) type parameter for block's with labels. r=lth
https://hg.mozilla.org/integration/autoland/rev/22c65750be93
Manually fix wasm/ jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/1f671f8eb9e4
Manually fix 'wasm/bigint/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/812aeb578b38
Manually fix 'wasm/gc/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/10b08f0d8d1c
Manually fix 'wasm/regress/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/96a3dca92600
Manually fix 'asm.js/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/65b06190327e
Manually fix 'debug/' jit-tests. r=lth
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68b6c7af6a45
Backed out 16 changesets for spidermonkey build bustages in /builds/worker/workspace/build/src/js/src/wasm/WasmTesting.h CLOSED TREE

Missed a test and some non-unified build issues.

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/128b94d489da
Use 'wat' crate for wasmTextToBinary. r=lth
https://hg.mozilla.org/integration/autoland/rev/3adfac0ba298
Include vendored files. r=lth
https://hg.mozilla.org/integration/autoland/rev/d198f3622b66
Switch to (result) type parameter for block. r=lth
https://hg.mozilla.org/integration/autoland/rev/13e0fa81800d
Switch to (result) type parameter for if. r=lth
https://hg.mozilla.org/integration/autoland/rev/a9b43b521c6b
Switch call_indirect to (type). r=lth
https://hg.mozilla.org/integration/autoland/rev/a783089c764d
Switch to explicit (export (func)). r=lth
https://hg.mozilla.org/integration/autoland/rev/15f0a5c7902a
Switch (export memory). r=lth
https://hg.mozilla.org/integration/autoland/rev/5ab5b2df1a51
Switch (export table). r=lth
https://hg.mozilla.org/integration/autoland/rev/61b698328174
Switch to (result) type parameter for loop. r=lth
https://hg.mozilla.org/integration/autoland/rev/168bb2aa8012
Switch to (result) type parameter for block's with labels. r=lth
https://hg.mozilla.org/integration/autoland/rev/79f172cf28fa
Manually fix wasm/ jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/32755d255aa0
Manually fix 'wasm/bigint/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/bd9126f1eab9
Manually fix 'wasm/gc/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/9bcb3a201d6b
Manually fix 'wasm/regress/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/6cb283dacbb9
Manually fix 'asm.js/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/c5950b8fed9f
Manually fix 'debug/' jit-tests. r=lth

More tests to fix that I should've known of.

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/7636e6d6990c
Use 'wat' crate for wasmTextToBinary. r=lth
https://hg.mozilla.org/integration/autoland/rev/b5923a09f47b
Include vendored files. r=lth
https://hg.mozilla.org/integration/autoland/rev/a6f189bdd87c
Switch to (result) type parameter for block. r=lth
https://hg.mozilla.org/integration/autoland/rev/c84bc5841266
Switch to (result) type parameter for if. r=lth
https://hg.mozilla.org/integration/autoland/rev/d0807ab80319
Switch call_indirect to (type). r=lth
https://hg.mozilla.org/integration/autoland/rev/bfdc8a02829b
Switch to explicit (export (func)). r=lth
https://hg.mozilla.org/integration/autoland/rev/20a7f0d29c6f
Switch (export memory). r=lth
https://hg.mozilla.org/integration/autoland/rev/583d043bfeac
Switch (export table). r=lth
https://hg.mozilla.org/integration/autoland/rev/2eec4b8552ad
Switch to (result) type parameter for loop. r=lth
https://hg.mozilla.org/integration/autoland/rev/9426dc357014
Switch to (result) type parameter for block's with labels. r=lth
https://hg.mozilla.org/integration/autoland/rev/757afe060150
Manually fix wasm/ jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/b6bcf6dfed53
Manually fix 'wasm/bigint/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/dddb970d1a04
Manually fix 'wasm/gc/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/8eefe5980f50
Manually fix 'wasm/regress/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/e2151aa71196
Manually fix 'asm.js/' jit-tests. r=lth
https://hg.mozilla.org/integration/autoland/rev/16f2eded7e00
Manually fix 'debug/' jit-tests. r=lth
Regressions: 1624592
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: