1335652 - Implement the core wasm exception handling instructions in the Baseline compiler

Status: RESOLVED FIXED

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

Description

[(Away)]

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.

Comment 1

[(Away)]

Attachment: Part 1 - stubs for various future functions

This is roughly based on all the places where we have code for `growMemory`.

Comment 2

[(Away)]

Attachment: Part 2 - throw keyword

This works reasonably well and includes some tests.

Comment 3

[(Away)]

Attachment: Part 3 - try and catch (incomplete, buggy)

Comment 4

[Hannes Verschore [:h4writer]]

Not sure what the deadline is. Marking as P3. Feel free to update.

Comment 5

[BMO Automation]

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.

Comment 6

[Lars T Hansen [:lth]]

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

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exception handling, part 1. WIP

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

Comment 8

[Lars T Hansen [:lth]]

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.

Comment 9

[Lars T Hansen [:lth]]

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.

Comment 10

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exception handling, part 2. WIP

This handles events, mostly.

Depends on D52625

Comment 11

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - opiter + exnref. WIP

Depends on D53939

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exceptions part 1: configuration

Depends on D55430

Comment 13

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exceptions part 2: exnref

Depends on D56520

Comment 14

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exceptions part 3: parsing and scaffolding

Needs some ifdeffery

Depends on D56521

Comment 15

[Lars T Hansen [:lth]]

Attachment: Bug 1335652 - wasm exceptions part 4: events

More ifdeffery wanted

Depends on D56522

Comment 16

[Lars T Hansen [:lth]]

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

[Lars T Hansen [:lth]]

(Patches updated now.)

Comment 18

[Lars T Hansen [:lth]]

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

[Lars T Hansen [:lth]]

(Patches updated now.)

Comment 20

[Lars T Hansen [:lth]]

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

Comment 21

[Lars T Hansen [:lth]]

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

Comment 22

[Ioanna M. Dimitriou]

Attachment: Bug 1335652 - wasm exceptions part 1: configuration

Originally added by Lars T. Hansen <[e-mail]>

Comment 23

[Ioanna M. Dimitriou]

Attachment: Bug 1335652 - wasm exceptions part 2: exnref

Originally added by Lars T. Hansen <[e-mail]>,
with modifications especially in js/src/wasm/WasmInstance.cpp
to accommodate changes introduced by the multi-value proposal additions.

Depends on D70304

Comment 24

[Ioanna M. Dimitriou]

Attachment: Bug 1335652 - wasm exceptions part 3: parsing and scaffolding

Originally added by Lars T. Hansen <[e-mail]>
Needs some ifdeffery

Depends on D70305

Comment 25

[Ioanna M. Dimitriou]

Attachment: Bug 1335652 - wasm exceptions part 4: events

Originally added by Lars T. Hansen <[e-mail]>

Modification by Asumu Takikawa <[e-mail]>:
Adjust syntax test for new wat parser

Depends on D70306

Comment 26

[Ioanna M. Dimitriou]

Attachment: Bug 1335652 - wasm exceptions part 5: validation

Co-authored with Asumu Takikawa ([e-mail])
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

Comment 27

[Lars T Hansen [:lth]]

Comment on attachment 8832340 [details] [diff] [review]
Part 1 - stubs for various future functions

Superseded by Ioanna's patches.

Comment 28

[Lars T Hansen [:lth]]

Comment on attachment 8832341 [details] [diff] [review]
Part 2 - throw keyword

Superseded by Ioanna's patches.

Comment 29

[Lars T Hansen [:lth]]

Comment on attachment 8832342 [details] [diff] [review]
Part 3 - try and catch (incomplete, buggy)

Superseded by Ioanna's patches.

Comment 30

[Ioanna M. Dimitriou]

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

Comment 31

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 1: configuration & flags

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

Comment 32

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 2: events

Adds support for event sections in wasm modules.

Depends on D96680

Comment 33

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 3: add WebAssembly.Exception

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

Depends on D96681

Comment 34

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 4: make events exportable and generate tags

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

Depends on D97048

Comment 35

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 5: scaffolding for instructions

Comment 36

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 6: validation

Depends on D97221

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Bogdan Tara[:bogdan_tara | bogdant]]

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

[Lars T Hansen [:lth]]

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

Comment 40

[Asumu Takikawa]

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7f3f2d66bca3
https://hg.mozilla.org/mozilla-central/rev/16e8936af123

Comment 43

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 7: vendor the latest wat

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9fc476c3142
wasm exceptions part 7: vendor the latest wat r=rhunt

Comment 45

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b9fc476c3142

Comment 46

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 8: add try notes

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 9: adjust ErrorObject to allow detection of traps.

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 10: add WebAssembly.RuntimeException

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 11: add WasmExceptionReg.

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 12: stack walking for exceptions

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 13: add instance methods for exception handling

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 15: Baseline implementation

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

Depends on D101137

Comment 53

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 14: disallow exns with reftypes for now

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

Comment 54

[Pulsebot]

Pushed by [e-mail]:
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

Comment 55

[Pulsebot]

Pushed by [e-mail]:
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

Comment 56

[Pulsebot]

Pushed by [e-mail]:
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

Comment 57

[Sandor Molnar[:smolnar]]

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 17: refine validation tests

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

[Jan de Mooij [:jandem]]

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

[Andy Wingo [:wingo]]

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

[Jan de Mooij [:jandem]]

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

Comment 62

[Asumu Takikawa]

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

[Asumu Takikawa]

Attachment: Bug 1335652 - wasm exceptions part 16: fix x86 crashes in wasm throw stub

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

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ece6be486a7
wasm exceptions part 16: fix x86 crashes in wasm throw stub r=rhunt

Comment 65

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0ece6be486a7

Comment 66

[Florin Strugariu [:Bebe]]

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

Comment 67

[Asumu Takikawa]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/376a5c3973da
wasm exceptions part 17: refine validation tests r=rhunt

Comment 69

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/376a5c3973da

Comment 70

[Lars T Hansen [:lth]]

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