Closed Bug 1335652 Opened 4 years ago Closed 2 months ago

Implement the core wasm exception handling instructions in the Baseline compiler

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: away, Assigned: idimitriou)

References

(Blocks 2 open bugs)

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
WIP patches for parsing, AST, text, etc. of try/catch/throw operators.

Currently these are just structural placeholders with no actual exception handling wired up.

For now, `throw` is more or less a `drop` that only accepts I32; `try` is like an `if` with no condition; and `catch` is like an `else` but with an element of `getLocal` for the catch-parameter.
This is roughly based on all the places where we have code for `growMemory`.
Attached patch Part 2 - throw keyword (obsolete) — Splinter Review
This works reasonably well and includes some tests.
Attachment #8832342 - Attachment is patch: true
Not sure what the deadline is. Marking as P3. Feel free to update.
Priority: -- → P3
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Component: JavaScript Engine: JIT → Javascript: Web Assembly
Type: defect → enhancement
Summary: Wasm: Parsing for try/catch/throw → Exception handling: Parsing for try/catch/throw

Patches probably stale but exception handling is happening, so I will try to resurrect and see if any of this code is salvageable.

Assignee: nobody → lhansen

Text->binary translation, and hooks in all the large switches for new opcodes.

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.

Status: REOPENED → ASSIGNED

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.

Summary: Exception handling: Parsing for try/catch/throw → Exception handling

This handles events, mostly.

Depends on D52625

Attached file Bug 1335652 - opiter + exnref. WIP (obsolete) —

Depends on D53939

Depends on D55430

Depends on D56520

Needs some ifdeffery

Depends on D56521

More ifdeffery wanted

Depends on D56522

Attachment #9108024 - Attachment is obsolete: true
Attachment #9110199 - Attachment is obsolete: true
Attachment #9110200 - Attachment is obsolete: true
Depends on: 1603496
Depends on: 1604453

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.

(Patches updated now.)

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.

(Patches updated now.)

Note an upcoming change to the order of sections: https://github.com/WebAssembly/exception-handling/issues/98

Not currently working on this; Ioanna from Igaila is taking it over.

Assignee: lhansen → nobody
Status: ASSIGNED → NEW
OS: Unspecified → All
Hardware: Unspecified → All

Originally added by Lars T. Hansen <lhansen@mozilla.com>

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

Originally added by Lars T. Hansen <lhansen@mozilla.com>
Needs some ifdeffery

Depends on D70305

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

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

Attachment #9114821 - Attachment is obsolete: true
Attachment #9114822 - Attachment is obsolete: true
Attachment #9114823 - Attachment is obsolete: true
Attachment #9114824 - Attachment is obsolete: true
Comment on attachment 8832340 [details] [diff] [review]
Part 1 - stubs for various future functions

Superseded by Ioanna's patches.
Attachment #8832340 - Attachment is obsolete: true
Comment on attachment 8832341 [details] [diff] [review]
Part 2 - throw keyword

Superseded by Ioanna's patches.
Attachment #8832341 - Attachment is obsolete: true
Comment on attachment 8832342 [details] [diff] [review]
Part 3 - try and catch (incomplete, buggy)

Superseded by Ioanna's patches.
Attachment #8832342 - Attachment is obsolete: true
Assignee: nobody → idimitriou
Status: NEW → ASSIGNED

Updated patches 1-5 to current stand of mozilla-central.

Adds compile-time and run-time flags for wasm exception handling.

Adds support for event sections in wasm modules.

Depends on D96680

Add the WebAssembly.Exception object that is used for event import
and export between wasm modules.

Depends on D96681

Adds support for exporting events, and generate corresponding runtime
exception tags.

Depends on D97048

Depends on D97221

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a83dc07778d3
wasm exceptions part 1: configuration & flags r=rhunt
https://hg.mozilla.org/integration/autoland/rev/41738b7fe24e
wasm exceptions part 2: events r=rhunt
https://hg.mozilla.org/integration/autoland/rev/944cf753ae68
wasm exceptions part 3: add WebAssembly.Exception r=rhunt
https://hg.mozilla.org/integration/autoland/rev/55f9150b55ff
wasm exceptions part 4: make events exportable and generate tags r=rhunt

Asumu, should this perhaps have had a leave-open flag on it?

Flags: needinfo?(asumu)

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

Flags: needinfo?(asumu)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Keywords: leave-open
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f3f2d66bca3
wasm exceptions part 5: scaffolding for instructions r=rhunt
https://hg.mozilla.org/integration/autoland/rev/16e8936af123
wasm exceptions part 6: validation r=rhunt
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9fc476c3142
wasm exceptions part 7: vendor the latest wat r=rhunt

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.

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

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

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

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

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

Implement compilation for the core Wasm exception instruction
in the Baseline compiler. Also adds evaluation tests.

Depends on D101137

Disallows exception types containing a reference type for the throw
and catch instructions temporarily until support for this is added.

Attachment #9196030 - Attachment description: Bug 1335652 - wasm exceptions part 14: Baseline implementation → Bug 1335652 - wasm exceptions part 15: Baseline implementation
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e708372efade
wasm exceptions part 8: add try notes r=rhunt
https://hg.mozilla.org/integration/autoland/rev/0b260a20b583
wasm exceptions part 9: adjust ErrorObject to allow detection of traps. r=rhunt
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/201aaf2eb7e0
wasm exceptions part 10: add WebAssembly.RuntimeException r=rhunt
https://hg.mozilla.org/integration/autoland/rev/81bb9db5e139
wasm exceptions part 11: add WasmExceptionReg. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/0d79003d2bab
wasm exceptions part 12: stack walking for exceptions r=rhunt
https://hg.mozilla.org/integration/autoland/rev/3659f7375613
wasm exceptions part 13: add instance methods for exception handling r=rhunt
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c0b8d1a9c6a
wasm exceptions part 14: disallow exns with reftypes for now r=rhunt
https://hg.mozilla.org/integration/autoland/rev/33f7edb2c121
wasm exceptions part 15: Baseline implementation r=rhunt
Regressions: 1690527

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.

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?

Flags: needinfo?(asumu)

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.

Bug 1690790 is adding a Linux32 x86 jit-test job to catch this in the future.

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.

Flags: needinfo?(asumu)

Fixes incorrect parameter passing on the stack for a call to
WasmHandleThrow in the wasm throw stub.

Attachment #9201020 - Attachment description: Bug 1335652 - wasm exceptions part 16: refine validation tests → Bug 1335652 - wasm exceptions part 17: refine validation tests
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/0ece6be486a7
wasm exceptions part 16: fix x86 crashes in wasm throw stub r=rhunt

== 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

Keywords: perf-alert

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.

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/376a5c3973da
wasm exceptions part 17: refine validation tests r=rhunt

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago2 months ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Exception handling → Implement the core wasm exception handling instructions in the Baseline compiler
Target Milestone: 85 Branch → 88 Branch
Blocks: 1695715
You need to log in before you can comment on or make changes to this bug.