This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix unwind destination mismatches in CFG stackify
ClosedPublic

Authored by aheejin on Jun 19 2018, 6:31 PM.

Details

Summary

Linearing the control flow by placing try/end_try markers can create
mismatches in unwind destinations. This patch resolves these mismatches
by wrapping those instructions with an incorrect unwind destination with
a nested try/catch/end_try and branching to the right destination
within the new catch block.

Diff Detail

Event Timeline

aheejin created this revision.Jun 19 2018, 6:31 PM

Continuing the discussion started in the email:

I think it's better not to split this pass as a separate pass. The reasons are:

  1. Now CFGStackify pass maintains several internal maps (ScopeTops, BeginToEnd, EndToBegin, TryToEHPad, EHPadToTry, and BeginToBottom) to keep track of start/end markers, and entries are added to these maps and queried both in CFGStackify and namely, FixUnwindMismatch pass. So these maps have to be maintained in both passes, and also, we have to either carry that information using auxiliary data structure or we have to recalculate these maps at the end of FixUnwindMatch, which is more work.
  1. Now CFGStackify also has several helper functions (registerScope(), registerTryScope(), and unregisterScope()) that help add and delete entries to various maps I mentioned in 1. We need these utility function in both passes.
  1. rewriteDepthImmediate() and fixEndsAtEndOfFunction() functions run at the end of CFGStackify. But because we change CFG in FixUnwindMatch pass as well, they should rather run at the end of FixUnwindMatch pass now. This looks a bit weird bcause FixUnwindMatch pass should only run for functions that have a personality function (or an EH pad). And also intuitively rewriting depth immediates is not a part of FixUnwindMatch but CFGStackify.

Also, if we can bear with CFGStackify file getting a bit longer, I think the two parts are kind of well divided and factorized into functions.

aheejin updated this revision to Diff 177300.Dec 7 2018, 1:50 PM
  • Add unregisterScope() deleted in rL348647
aheejin planned changes to this revision.Jan 23 2019, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 4:03 AM
aheejin updated this revision to Diff 191350.Mar 19 2019, 10:43 AM

Reimplementation based on the new proposal

aheejin retitled this revision from [WebAssembly] Fix unwind destination mismatches in CFG stackify (WIP) to [WebAssembly] Fix unwind destination mismatches in CFG stackify.Mar 19 2019, 10:44 AM
aheejin edited the summary of this revision. (Show Details)
aheejin updated this revision to Diff 191352.Mar 19 2019, 10:53 AM

Add comments in tests

dschuff added inline comments.Mar 19 2019, 3:22 PM
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
83

This comment is pretty vague. I guess this is one block for each function, and is now shared for creating a correct signature for fallthrough returns, and to use as a target for rethrows that need to unwind to the caller, but are trapped inside an outer try/catch?

654

s/Linearing/Linearizing

690

s/boy/body

724

remove the period after "block"

794

It seems unfortunate that we have to care about whether explicit locals is turned on. Is that just because this pass runs after createWebAssemblyExplicitLocals? Does the order matter? e.g. would it be simpler to run it after CfgStackify? Or, could we just run it again if this pass changes anything?

829

continue here will just skip to the next instruction, but the comment says it will skip the rest of the BB (which makes sense), should this be break instead? Or do we need to keep going to find the try/catch?

903

s/don't/don't have

972

these could be else if?

test/CodeGen/WebAssembly/cfg-stackify-eh-mismatch.ll
41 ↗(On Diff #191352)

Any weird corner cases it would be worthwhile to test for? Mixing both kinds of mismatches? A case with uwind-to-caller and block signature needed? Would multiple nesting of mismatches affect what happens?

aheejin updated this revision to Diff 191718.Mar 21 2019, 9:24 AM
aheejin marked 9 inline comments as done.

Addressed simple comments first.
Things I haven't addressed yet:

  • Add more test cases
  • Switch ExplicitLocal pass order?
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
83

Yes. Updated the comments.

829

Right. It can't be break because we still need to process this part and update EHPadStack.

if (MI.getOpcode() == WebAssembly::TRY)
  EHPadStack.pop_back();
else if (MI.getOpcode() == WebAssembly::CATCH)
  EHPadStack.push_back(MI.getParent());
972

Changed to switch-case.

aheejin marked 2 inline comments as done.Mar 21 2019, 10:03 AM
aheejin added inline comments.
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
794

Actually I don't think the order matters. Thanks for the idea. Created D59652.

aheejin planned changes to this revision.Mar 21 2019, 10:40 AM
aheejin marked an inline comment as done.

Now after D59652 we don't need the separate code (and separate test commands) for when -wasm-disable-explicit-locals is set and it is not. Will fix it soon.

aheejin updated this revision to Diff 192136.Mar 25 2019, 10:13 AM
  • Deleted all logics related to -wasm-disable-explicit-locals thanks to D59652
  • Fix several bugs
  • Add more tests
aheejin edited the summary of this revision. (Show Details)Mar 25 2019, 10:14 AM
aheejin marked 2 inline comments as done.Mar 25 2019, 10:19 AM
aheejin added inline comments.
test/CodeGen/WebAssembly/cfg-stackify-eh-mismatch.ll
41 ↗(On Diff #191352)

Add some more tests.

  • Mixing both kinds of mismatches: Done (test8)
  • A case with uwind-to-caller and block signature needed: it's not possible because of newly added rethrow. Added a test that shows why it's not possible. (test7)
  • Multiple nesting of mismatches: test8 kind of covers this too.

Note that -wasm-disable-ehpad-sort is used to generate maximum # of mismatches for those mismatch tests.

aheejin marked 2 inline comments as done.Mar 25 2019, 10:20 AM
aheejin added inline comments.
test/CodeGen/WebAssembly/cfg-stackify-eh-mismatch.ll
41 ↗(On Diff #191352)

Oh and please also note that I deleted this file and moved all tests to cfg-stackify-eh.ll.

dschuff accepted this revision.Mar 26 2019, 12:38 PM
This revision is now accepted and ready to land.Mar 26 2019, 12:38 PM
This revision was automatically updated to reflect the committed changes.