Page MenuHomePhabricator

[WebAssembly] CFG stackify support for exception handling
ClosedPublic

Authored by aheejin on Jun 18 2018, 4:16 AM.

Details

Summary

This adds support for exception handling to CFGStackify pass. This only
adds TRY / END_TRY markers and DOES NOT yet fix unwind mismatches that
can be created by the linearization of the CFG into the structural wasm
format. The mismatch fix will be added by following patches.

In detail, this patch

  • Added support for TRY / END_TRY markers to support EH
  • Changed many static functions into class member functions as they take too many arguments now
  • Added several more bookeeping data structures
  • Refactored routines that decide where to insert markers, because without refactoring this got too complicated as we added support for new kinds of markers (TRY/END_TRY).
  • Rewrote rethrow instructions' BB arguments to relative depths in EH pad stack

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Jun 18 2018, 4:16 AM
aheejin edited the summary of this revision. (Show Details)Jun 18 2018, 4:19 AM
aheejin edited the summary of this revision. (Show Details)
aheejin updated this revision to Diff 160221.Aug 10 2018, 6:43 PM

Add markers to an existing test case

dschuff added inline comments.Aug 14 2018, 4:27 PM
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
126 ↗(On Diff #160221)

I think this idea of BeforeSet and AfterSet makes sense given the symmetry, but it was unclear on the first reading that only the BeforeSet is actually used other than for debugging (despite the ifdef and comment below). So this comment could maybe just have an additional ", but is only used for sanity checking" at the end.

294 ↗(On Diff #160221)

why is this only for debugging? edit: nevermind I figured it out. see comment abo.ve

307 ↗(On Diff #160221)

I know LLVM style allows this complete lack of braces, but IMO using this many lines (control structures, etc) is just asking for bugs. Plus I find it difficult to read. Could we use the braces anyway in cases where the body is a nested expression?

539 ↗(On Diff #160221)

Add braces here too.

670 ↗(On Diff #160221)

Update comment.

dschuff added inline comments.Aug 14 2018, 4:52 PM
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
443 ↗(On Diff #160221)

The fact that so many sections of this function are the same as or almost the same as placeBlockMarker kind of makes me wonder if we can do better about sharing the logic, although there are enough differences to make that challenging too. Probably worth thinking about, especially if we end up thinking of going toward a more explicit or IR-like representation of wasm control structures.

aheejin updated this revision to Diff 160915.Aug 15 2018, 1:55 PM
aheejin marked 4 inline comments as done.
  • Address comments
  • Add more braces
aheejin added inline comments.Aug 15 2018, 1:55 PM
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
307 ↗(On Diff #160221)

Yeah I used to place braces in this kind of cases too, but someone (not you) once told me to remove them in code review, and I guess that's when I started removing them :$ But I also think braces make code easy to read and less bug prone. Anyway I added the braces back.

443 ↗(On Diff #160221)

Hmm. Yes. In the current structure I also find it difficult to share the logics. Further, when determining BeforeSet and AfterSet, there are even more differences, partly because marker kinds are different, but also partly try/end_try marker placement is done lastly so it has to make sure the placement makes sense with respect to all previously placed block/end_block/loop/end_loop markers.

dschuff accepted this revision.Aug 16 2018, 1:12 PM
This revision is now accepted and ready to land.Aug 16 2018, 1:12 PM

@sunfish Do you have any other concerns?

sunfish accepted this revision.Aug 16 2018, 3:58 PM

This looks ok to land now.

This revision was automatically updated to reflect the committed changes.