Page MenuHomePhabricator

[Inliner/WinEH] Honor implicit nounwinds
ClosedPublic

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

Details

Summary

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
accordingly.

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
lib/Transforms/Utils/InlineFunction.cpp
359

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

392–409

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

443–451

I think this might be easier with #ifndef NDEBUG

447–450

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

615–624

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

629

patern -> parent?

address feedback

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

Updated per feedback.

lib/Transforms/Utils/InlineFunction.cpp
447–450

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.

615–624

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
...
sibling:
  %sib = cleanuppad within %parent []
  unreachable
...
  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!

lib/Transforms/Utils/InlineFunction.cpp
615–624

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
lib/Transforms/Utils/InlineFunction.cpp
615–624

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).