Page MenuHomePhabricator

[Inliner/WinEH] Honor implicit nounwinds

Authored by JosephTremoulet on Jan 19 2016, 7:25 AM.



Funclet EH tables require that a given funclet have only one unwind
destination for exceptional exits. The verifier will therefore reject
e.g. two cleanuprets with different unwind dests for the same cleanup, or
two invokes exiting the same funclet but to different unwind dests.
Because catchswitch has no 'nounwind' variant, and because IR producers
are not *required* to annotate calls which will not unwind as 'nounwind',
it is legal to nest a call or an "unwind to caller" catchswitch within a
funclet pad that has an unwind destination other than caller; it is
undefined behavior for such a call or catchswitch to unwind.

Normally when inlining an invoke, calls in the inlined sequence are
rewritten to invokes that unwind to the callsite invoke's unwind
destination, and "unwind to caller" catchswitches in the inlined sequence
are rewritten to unwind to the callsite invoke's unwind destination.
However, if such a call or "unwind to caller" catchswitch is located in a
callee funclet that has another exceptional exit with an unwind
destination within the callee, applying the normal transformation would
give that callee funclet multiple unwind destinations for its exceptional
exits. There would be no way for EH table generation to determine which
is the "true" exit, and the verifier would reject the function

Add logic to the inliner to detect these cases and leave such calls and
"unwind to caller" catchswitches as calls and "unwind to caller"
catchswitches in the inlined sequence.

This fixes PR26147.

Diff Detail

Event Timeline

JosephTremoulet retitled this revision from to [Inliner/WinEH] Honor implicit nounwinds.
JosephTremoulet updated this object.
majnemer added inline comments.Jan 19 2016, 9:29 AM

I'd use logical not-equal instead of bit-wise xor


Would it be more idiomatic to start the worklist with UselessPad and switch the loop to something like while (!Worklist.empty())


I think this might be easier with #ifndef NDEBUG


If we are in this case, can we also assert that UnwindDestToken is not null?


Does this case only arise when you have an unwind-to-caller catchswitch nested inside an unwind-to-caller catchswitch/cleanuppad?


patern -> parent?

address feedback

JosephTremoulet marked 4 inline comments as done.Jan 19 2016, 10:18 AM

Updated per feedback.


No, if the entire funclet tree has no invokes and consists entirely of cleanuppads that don't have cleanuprets and catchswitches that are "unwind to caller", then UnwindDestToken will be null here.


No, this happens when you have an unwind-to-caller catchswitch nested inside an outer pad which unwinds somewhere else in the inlinee. E.g. if you start with

define void inlinee() {
  %parent = cleanuppad within none []
  %current = catchswitch within %parent [label ...] unwind label %sibling
  %sib = cleanuppad within %parent []
  cleanupret from %parent unwind label %uncle

then SimplifyUnreachable could turn that into

define void inlinee() {
  %parent = cleanuppad within none []
  %current = catchswitch within %parent [label ...] unwind to caller
  cleanupret from %parent unwind label %uncle

and that's the case this code is dealing with.

majnemer accepted this revision.Jan 19 2016, 5:17 PM
majnemer edited edge metadata.

LGTM, thanks for fixing this!


Would we need to do this if we had catchswitches which could be annotated as nounwind?

This revision is now accepted and ready to land.Jan 19 2016, 5:17 PM

We would not. I think that would be a better solution long-term, though we'd still have to keep much of the rest of the logic in this patch unless we start requiring IR producers to explicitly annotate the analogous calls 'nounwind' (which might be a reasonable thing to require... fwiw, I've been going back and forth on whether I like that idea).