Closed Bug 1612534 Opened 5 years ago Closed 5 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

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

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

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

Depends on D67245

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

Depends on D67259

Depends on D67260

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)
Blocks: 1624363
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: