This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix ExceptionInfo grouping again
ClosedPublic

Authored by aheejin on Mar 4 2021, 3:49 AM.

Details

Summary

This is a case D97677 missed. When taking out remaining BBs that are
reachable from already-taken-out exceptions (because they are not
subexcptions but unwind destinations), I assumed the remaining BBs are
not EH pads, but they can be. For example,

try {
  try {
    throw 0;
  } catch (int) { // (a)
  }
} catch (int) {   // (b)
}
try {
  foo();
} catch (int) {   // (c)
}

In this code, (b) is the unwind destination of (a) so its exception is
taken out of (a)'s exception, But even though the next try-catch is not
inside the first two-level try-catches, because the first try always
throws, its continuation BB is unreachable and the whole rest of the
function is dominated by EH pad (a), including EH pad (c). So after we
take out of (b)'s exception out of (a)'s, we also need to take out (c)'s
exception out of (a)'s, because (c) is reachable from (b).

This adds one more step before what we did for remaining BBs in D97677;
it traverses EH pads first to take subexceptions out of their incorrect
parent exception. It's the same thing as D97677, but because we can do
this before we add BBs to exceptions' sets, we don't need to fix sets
and only need to fix parent exception pointers.

Other changes are variable name changes (I changed WE -> SrcWE,
UnwindWE -> DstWE for clarity), some comment changes, and a drive-by
fix in a bug in a LLVM_DEBUG print statement.

Fixes https://github.com/emscripten-core/emscripten/issues/13588.

Diff Detail

Event Timeline

aheejin created this revision.Mar 4 2021, 3:49 AM
aheejin requested review of this revision.Mar 4 2021, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 3:49 AM
aheejin edited the summary of this revision. (Show Details)Mar 4 2021, 4:00 AM
dschuff added inline comments.Mar 4 2021, 9:55 AM
llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
1560

I don't see any CHECKs here. I guess the test is just to ensure there are no assert failures?

dschuff accepted this revision.Mar 4 2021, 11:18 AM

LGTM assuming the test is what you intended.

This revision is now accepted and ready to land.Mar 4 2021, 11:18 AM
aheejin added inline comments.Mar 4 2021, 3:03 PM
llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
1560

Yes these regression tests are mostly to check they don't crash. Checking specific CFGStackify output is not very informative or not easy to verify by eyes. Maybe one more informative thing to dump or check is WebAssemblyExceptionInfo::dump().. Maybe we can dump that info and check that.

This revision was landed with ongoing or failed builds.Mar 4 2021, 3:06 PM
This revision was automatically updated to reflect the committed changes.
dschuff added inline comments.Mar 4 2021, 3:14 PM
llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
1560

that sounds like it could be useful. One thing to watch out for on that, is that (IIRC) some dumping methods get compiled away in release builds. I'm not sure what the common practice is for using those in tests.