This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix incorrect grouping and sorting of exceptions
ClosedPublic

Authored by aheejin on Feb 22 2021, 7:36 PM.

Details

Summary

This CL is not big but contains changes that span multiple analyses and
passes. This description is very long because it tries to explain basics
on what each pass/analysis does and why we need this change on top of
that. Please feel free to skip parts that are not necessary for your
understanding.


WasmEHFuncInfo contains the mapping of <EH pad, the EH pad's next
unwind destination>. The value (unwind dest) here is where an exception
should end up when it is not caught by the key (EH pad). We record this
info in WasmEHPrepare to fix catch mismatches, because the CFG itself
does not have this info. A CFG only contains BBs and
predecessor-successor relationship between them, but in WasmEHFuncInfo
the unwind destination BB is not necessarily a successor or the key EH
pad BB. Their relationship can be intuitively explained by this C++ code
snippet:

try {
  try {
    foo();
  } catch (int) { // EH pad
    ...
  }
} catch (...) {   // unwind destination
}

So when foo() throws, it goes to catch (int) first. But if it is not
caught by it, it ends up in the next unwind destination catch (...).
This unwind destination is what you see in catchswitch's
unwind label %bb part.


WebAssemblyExceptionInfo groups exceptions so that they can be sorted
continuously together in CFGSort, as we do for loops. What this analysis
does is very simple: it creates a single WebAssemblyException per EH
pad, and all BBs that are dominated by that EH pad are included in this
exception. We also identify subexception relationship in this way: if
EHPad A domiantes EHPad B, EHPad B's exception is a subexception of
EHPad A's exception.

This simple rule turns out to be incorrect in some cases. In
WasmEHFuncInfo, if EHPad A's unwind destination is EHPad B, it means
semantically EHPad B should not be included in EHPad A's exception,
because it does not make sense to rethrow/delegate to an inner scope.
This is what happened in CFGStackify as a result of this:

try
  try
  catch
    ...   <- %dest_bb is among here!
  end
delegate %dest_bb

So this patch adds a phase in WebAssemblyExceptionInfo::recalculate to
make sure excptions' unwind destinations are not subexceptions of
their unwind sources in WasmEHFuncInfo.

But this alone does not prevent dest_bb in the example above from
being sorted within the inner catch's exception, even if its exception
is not a subexception of that catch's exception anymore, because of
how CFGSort works, which will be explained below.


CFGSort places BBs within the same SortRegion (loop or exception)
continuously together so they can be demarcated with loop-end_loop
or catch-end_try in CFGStackify.

SortRegion is a wrapper for one of MachineLoop or
WebAssemblyException. SortRegionInfo already does some complicated
things because there discrepancies between those two data structures.
WebAssemblyException is what we control, and it is defined as an EH
pad as its header and BBs dominated by the header as its BBs (with a
newly added exception of unwind destinations explained in the previous
paragraph). But MachineLoop is an LLVM data structure and uses the
standard loop detection algorithm. So by the algorithm, BBs that are 1.
dominated by the loop header and 2. have a path back to its header.
Because of the second condition, many BBs that are dominated by the loop
header are not included in the loop. So BBs that contain return or
branches to outside of the loop are not technically included in
MachineLoop, but they can be sorted together with the loop with no
problem.

Maybe to relax the condition, in CFGSort, when we are in a SortRegion
we allow sorting of not only BBs that belong to the current innermost
region but also BBs that are dominated by the current region header.
(This was written this way from the first version written by Dan, when
only loops existed.) But now, we have cases in exceptions when EHPad B
is the unwind destination for EHPad A, even if EHPad B is dominated by
EHPad A it should not be included in EHPad A's exception, and should not
be sorted within EHPad A.

One way to make things work, at least correctly, is change dominates
condition to contains condition for SortRegion when sorting BBs, but
this will change compilation results for existing non-EH code and I
can't be sure it will not degrade performance or code size. I think it
will degrade performance because it will force many BBs dominated by a
loop, which don't have the path back to the header, to be placed after
the loop and it will likely to create more branches and blocks.

So this does a little hacky check when adding BBs to Preferred list:
(Preferred list is a ready list. CFGSort maintains ready list in two
priority queues: Preferred and Ready. I'm not very sure why, but it
was written that way from the beginning. BBs are first added to
Preferred list and then some of them are pushed to Ready list, so
here we only need to guard condition for Preferred list.)

When adding a BB to Preferred list, we check if that BB is an unwind
destination of another BB. To do this, this adds the reverse mapping,
UnwindDestToSrc, and getter methods to WasmEHFuncInfo. And if the BB
is an unwind destination, it checks if the current stack of regions
(Entries) contains its source BB by traversing the stack backwards. If
we find its unwind source in there, we add the BB to its Deferred
list, to make sure that unwind destination BB is added to Preferred
list only after that region with the unwind source BB is sorted and
popped from the stack.


This does not contain a new test that crashes because of this bug, but
this fix changes the result for one of existing test case. This test
case didn't crash because it fortunately didn't contain delegate to
the incorrectly placed unwind destination BB.

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

Diff Detail

Event Timeline

aheejin created this revision.Feb 22 2021, 7:36 PM
aheejin requested review of this revision.Feb 22 2021, 7:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 7:36 PM
aheejin edited the summary of this revision. (Show Details)Feb 22 2021, 7:38 PM
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)Feb 22 2021, 7:43 PM
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)Feb 23 2021, 11:51 AM
dschuff accepted this revision.Feb 23 2021, 11:52 AM
dschuff added inline comments.
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
343–350

does moveing from NewMap mean it can't be touched anymore at all? Or is calling some kind of initialization function on it OK?
I guess if we think it's not ok, we could always use swap instead.

llvm/lib/Target/WebAssembly/WebAssemblyExceptionInfo.cpp
74
This revision is now accepted and ready to land.Feb 23 2021, 11:52 AM

The description is great, and I think this change is the right thing to do for now. Once things stabilize, it might eventually make sense to think some more about how we might simplify the sorting algorithm (as you discussed in the description). And you're right that we'd want to have better performance/benchmarking in place to inform that.

aheejin edited the summary of this revision. (Show Details)Feb 23 2021, 12:01 PM
aheejin marked an inline comment as done.Feb 23 2021, 2:48 PM
aheejin added inline comments.
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
343–350

Yes, functions without preconditions can be used on the object after it was moved from. clear() doesn't assume anything so it's fine.

From https://en.cppreference.com/w/cpp/utility/move:

Unless otherwise specified, all standard library objects that have been moved from are placed in a valid but unspecified state. That is, only the functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from:

+1 This is an impressively thorough and deep explanation of the issue. Thanks for taking the time to write it up in addition to the time it must have taken to dive in and figure out what was happening. I certainly don't understand how all the CFG sorting works, so once things are working smoothly end-to-end, I would welcome a chance to understand it more deeply.

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
343–350

I took a look at the implementation of DenseMap(DenseMap&&) and it's implemented with a swap, so this should be fine.

aheejin added a comment.EditedFeb 23 2021, 2:53 PM

+1 This is an impressively thorough and deep explanation of the issue. Thanks for taking the time to write it up in addition to the time it must have taken to dive in and figure out what was happening. I certainly don't understand how all the CFG sorting works, so once things are working smoothly end-to-end, I would welcome a chance to understand it more deeply.

While I understand how CFGSort works, I also don't completely understand all the motivations behind the algorithms, such as why we have separate Preferred and Ready lists. I actually asked Dan once and he said he didn't remember 🙃 But I also have been cautious of changing existing algorithms because I'm not sure how it will affect the performance.

This revision was landed with ongoing or failed builds.Feb 23 2021, 2:55 PM
This revision was automatically updated to reflect the committed changes.