Implement the core wasm exception handling instructions in the Baseline compiler
Categories
(Core :: JavaScript: WebAssembly, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: away, Assigned: idimitriou)
References
(Blocks 1 open bug)
Details
(Keywords: perf-alert)
Attachments
(22 files, 10 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
Comment 4•8 years ago
|
||
Comment 5•7 years ago
|
||
Updated•7 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Comment 6•5 years ago
|
||
Patches probably stale but exception handling is happening, so I will try to resurrect and see if any of this code is salvageable.
Comment 7•5 years ago
|
||
Text->binary translation, and hooks in all the large switches for new opcodes.
Comment 8•5 years ago
|
||
Part 1 (comment 7) extracts all the useful content from the original patches except binary parsing (ie the bodies of the WasmOpIter methods). Exception handling has changed a lot since the draft code was written, as have other aspects of the engine, so we'll need to write that from scratch, although part 3 of the original patch set contains useful sketches.
Updated•5 years ago
|
Comment 9•5 years ago
|
||
Been reading the current spec overview, https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md. Mostly straightforward. Some notes.
There's a global space of opaque exception tags, these identify exceptions globally. If a module has a private exception E it has a tag that distinguishes it from all other exns in the system. If a module exports an exception E and some other module imports it, then the exported and imported exns have the same tag. For practical purposes, a tag could be a uint32_t acting as an index into a global table, or it could be a pointer to a descriptor in ditto table.
New exception type 'exnref' <: 'anyref' is added but is otherwise opaque; this represents a package of the values created by the throw along with the tag of the exception.
throw has two parts: box up the arguments from the stack and push an exception object, then effectively perform rethrow. Initially the box is most reasonably some JSObject*, as this allows for efficient representation as anyref, even if we might use less storage with other representations. One JSObject class for each signature of exception, probably.
br_on_exn is probably a simple comparison; if tags are ints then all sorts of techniques apply if we want to optimize a cluster of discriminants. (A sequence of br_on_exn instructions is basically a wordily encoded br_table.) Since the exn representation is opaque, loading of exception values at the branch target can be inlined from some optimized representation (though of course the loading happens along the branch edge so there's an intermediate jump island to handle the unboxing).
rethrow has two cases: if the try block is lexically nesting in the same function then it amounts to a branch; if not, it amounts to finding a nesting function call with a try block, unwinding to that function, and then branching. For a first cut, the latter case probably is something to handle in C++.
The only really difficult thing then is the unwinding logic for the hard part of rethrow, and the metadata we create for making that fast.
Updated•5 years ago
|
Comment 10•5 years ago
|
||
This handles events, mostly.
Depends on D52625
Comment 11•5 years ago
|
||
Depends on D53939
Comment 12•5 years ago
|
||
Depends on D55430
Comment 13•5 years ago
|
||
Depends on D56520
Comment 14•5 years ago
|
||
Needs some ifdeffery
Depends on D56521
Comment 15•5 years ago
|
||
More ifdeffery wanted
Depends on D56522
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
There's an open spec issue (I opened it): is exnref nullable or not? Currently I assume it is not, but this assumption is not deeply baked into current code, and it is left as a TODO comment where the assumption may be depended on, if only in a comment.
My current patch set implements infrastructure, the exnref type, parsing of the new instructions and the events, and has a framework for validation and compilation with MOZ_CRASH("NYI") at the obvious leaves - in code generation functions and in the WasmOpIter readers notably. There are some test cases. There are probably bugs.
The main work items remaining are at least these:
- solidifying the vague parts of the specification, I suspect there are numerous open issues both internal to wasm and in the JS API
- validation
- code generation for the individual instructions
- designing a data representation for Exception event objects
- designing a representation for unwind maps / the unwind logic
- handling throws that propagate wasm -> JS and JS -> wasm, which may entail boxing/unboxing the wasm Exception in a JS exception
As far as the data representation of the Exception is concerned, I was thinking I would initially just use a JSObject with expandos since this would be simple, but there are certainly more efficient representations.
One might initially perhaps just implement exceptions that aren't allowed to cross the JS/wasm boundary, for simplicity.
Comment 17•5 years ago
|
||
(Patches updated now.)
Comment 18•5 years ago
|
||
One more thing about the patches: I decided to completely #ifdef this feature in all its aspects[*] because it is still very early stage; if we end up landing any of it, it's good to have the ifdefs as flags pointing us to the code that the feature touches. It would be nice to keep this up until the feature is late in stage 2 or so.
[*] With one exception: the text->binary tokenizer. Really this could be ifdeffed too.
Comment 19•5 years ago
|
||
(Patches updated now.)
Comment 20•5 years ago
|
||
Note an upcoming change to the order of sections: https://github.com/WebAssembly/exception-handling/issues/98
Comment 21•5 years ago
|
||
Not currently working on this; Ioanna from Igaila is taking it over.
Assignee | ||
Comment 22•5 years ago
|
||
Originally added by Lars T. Hansen <lhansen@mozilla.com>
Assignee | ||
Comment 23•5 years ago
|
||
Originally added by Lars T. Hansen <lhansen@mozilla.com>,
with modifications especially in js/src/wasm/WasmInstance.cpp
to accommodate changes introduced by the multi-value proposal additions.
Depends on D70304
Assignee | ||
Comment 24•5 years ago
|
||
Originally added by Lars T. Hansen <lhansen@mozilla.com>
Needs some ifdeffery
Depends on D70305
Assignee | ||
Comment 25•5 years ago
|
||
Originally added by Lars T. Hansen <lhansen@mozilla.com>
Modification by Asumu Takikawa <asumu@igalia.com>:
Adjust syntax test for new wat
parser
Depends on D70306
Assignee | ||
Comment 26•5 years ago
|
||
Co-authored with Asumu Takikawa (asumu@igalia.com)
Adds validation and tests for wasm exception handling.
This patch:
- fixes: event validation event section validation, exported event validation.
(The funcTypeIndex is not a FuncTypeWithId as it does not necessarily correspond to a function.) - adds: validation of instructions throw, rethrow, try-catch, br_on_exn
- moves event section between the memory and global sections.
- tests for all of the above
Depends on D70307
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 27•5 years ago
|
||
Comment 28•5 years ago
|
||
Comment 29•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 30•5 years ago
|
||
Updated patches 1-5 to current stand of mozilla-central.
Comment 31•4 years ago
|
||
Adds compile-time and run-time flags for wasm exception handling.
Comment 32•4 years ago
|
||
Adds support for event sections in wasm modules.
Depends on D96680
Comment 33•4 years ago
|
||
Add the WebAssembly.Exception object that is used for event import
and export between wasm modules.
Depends on D96681
Comment 34•4 years ago
|
||
Adds support for exporting events, and generate corresponding runtime
exception tags.
Depends on D97048
Comment 35•4 years ago
|
||
Comment 36•4 years ago
|
||
Depends on D97221
Comment 37•4 years ago
|
||
Comment 38•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a83dc07778d3
https://hg.mozilla.org/mozilla-central/rev/41738b7fe24e
https://hg.mozilla.org/mozilla-central/rev/944cf753ae68
https://hg.mozilla.org/mozilla-central/rev/55f9150b55ff
Comment 39•4 years ago
|
||
Asumu, should this perhaps have had a leave-open flag on it?
Comment 40•4 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #39)
Asumu, should this perhaps have had a leave-open flag on it?
Whoops, sorry you are right. If it could be reopened and the tag added that would be great (I can't actually edit this bug). Thanks.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 41•4 years ago
|
||
Comment 42•4 years ago
|
||
bugherder |
Comment 43•4 years ago
|
||
Comment 44•4 years ago
|
||
Comment 45•4 years ago
|
||
bugherder |
Comment 46•4 years ago
|
||
This patch adds a WasmTryNote data structure that is similar to
JS try notes, in that they note the code ranges that correspond
to try blocks. These are used in a subsequent patch to find catch
handler code to throw to.
The Assembler additions are there primarily to allow the code
generator in Ion to easily access the try notes in a future
patch.
Comment 47•4 years ago
|
||
This patch extends ErrorObject so that errors that originated from
Wasm traps can be distinguished in the VM from other kinds of errors.
This is used to correctly implement the catching of JS exceptions in
Wasm code.
Depends on D101132
Comment 48•4 years ago
|
||
This patch adds the WebAssembly.RuntimeException class in order to
represent exceptions thrown from Wasm. When the JS API is finalized,
this class may allow access to the thrown values from JS.
Depends on D101133
Comment 49•4 years ago
|
||
Adds a WasmExceptionReg register definition in Assembler headers.
This register is used to coordinate between exception throw
and catch sites in Wasm.
Depends on D101134
Comment 50•4 years ago
|
||
This patch adds support for walking the stack in order to find
Wasm exception handlers to HandleThrow and adjusts support in
JitFrames.cpp and WasmStubs.cpp to use this functionality.
It also adds a wrapper class for wrapping JS values that get thrown
to Wasm handlers (they are unwrapped on a Wasm throw
).
Depends on D101135
Comment 51•4 years ago
|
||
Add Wasm instance methods that handle exception object allocation,
preparing to throw an exception, and finding the index corresponding
to a thrown exception object.
Depends on D101136
Comment 52•4 years ago
|
||
Implement compilation for the core Wasm exception instruction
in the Baseline compiler. Also adds evaluation tests.
Depends on D101137
Comment 53•4 years ago
|
||
Disallows exception types containing a reference type for the throw
and catch instructions temporarily until support for this is added.
Updated•4 years ago
|
Comment 54•4 years ago
|
||
Comment 55•4 years ago
|
||
Comment 56•4 years ago
|
||
Comment 57•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e708372efade
https://hg.mozilla.org/mozilla-central/rev/0b260a20b583
https://hg.mozilla.org/mozilla-central/rev/201aaf2eb7e0
https://hg.mozilla.org/mozilla-central/rev/81bb9db5e139
https://hg.mozilla.org/mozilla-central/rev/0d79003d2bab
https://hg.mozilla.org/mozilla-central/rev/3659f7375613
https://hg.mozilla.org/mozilla-central/rev/3c0b8d1a9c6a
https://hg.mozilla.org/mozilla-central/rev/33f7edb2c121
Comment 58•4 years ago
|
||
This enables the checking of validation test error messages,
which wasn't possible before the exception instructions were
implemented in at least the Baseline compiler.
Comment 59•4 years ago
|
||
Parts 8-15 broke asm.js/wasm jit-tests on Linux x86 (32-bit). It crashes in generated code, one of the registers contains 0xbad
. Can you take a look?
Comment 60•4 years ago
|
||
The regressor is https://phabricator.services.mozilla.com/D101136. Reproduce via passing --target=i686-pc-linux-gnu
to configure, and building and running jit-tests as usual.
Comment 61•4 years ago
|
||
Bug 1690790 is adding a Linux32 x86 jit-test job to catch this in the future.
Comment 62•4 years ago
|
||
The bustage was from some incorrect parameter passing that was added in these patches in the throw stub code. It triggered only on x86 as it was just the stack parameter case that was broken.
I have a patch that fixes it that I'll upload soon after testing more thoroughly.
Comment 63•4 years ago
|
||
Fixes incorrect parameter passing on the stack for a call to
WasmHandleThrow in the wasm throw stub.
Updated•4 years ago
|
Comment 64•4 years ago
|
||
Comment 65•4 years ago
|
||
bugherder |
Comment 66•4 years ago
|
||
== Change summary for alert #28626 (as of Thu, 04 Feb 2021 14:17:44 GMT) ==
Regressions:
Ratio | Suite | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|---|
15% | booking | fcp | android-hw-p2-8-0-android-aarch64-shippable | nocondprof warm webrender | 202.46 -> 233.25 |
12% | booking | FirstVisualChange | android-hw-p2-8-0-android-aarch64-shippable | nocondprof warm webrender | 237.71 -> 267.00 |
11% | booking | ContentfulSpeedIndex | android-hw-p2-8-0-android-aarch64-shippable | nocondprof warm webrender | 268.88 -> 299.58 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28626
Updated•4 years ago
|
Comment 67•4 years ago
|
||
I just put the check-in needed tag on https://phabricator.services.mozilla.com/D103965 which is the last patch on this initial bug for exception handling. I am thinking of renaming this bug as was suggested in this comment to something like "Implement the core exception handling instructions in the Baseline compiler" and closing it. Then I'd open a new meta-bug for exception handling in general (which would be a dependency for the new bugs for follow-up exception patches).
Please let me know if anyone's opposed to that reorganization or if there's a better way I should do it. Cheers.
Comment 68•4 years ago
|
||
Comment 69•4 years ago
|
||
bugherder |
Comment 70•4 years ago
|
||
(In reply to Asumu Takikawa from comment #67)
I just put the check-in needed tag on https://phabricator.services.mozilla.com/D103965 which is the last patch on this initial bug for exception handling. I am thinking of renaming this bug as was suggested in this comment to something like "Implement the core exception handling instructions in the Baseline compiler" and closing it. Then I'd open a new meta-bug for exception handling in general (which would be a dependency for the new bugs for follow-up exception patches).
Please let me know if anyone's opposed to that reorganization or if there's a better way I should do it. Cheers.
Sounds like a good idea. Go for it.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Description
•