[PATCH] D13274: [WinEH] Clone funclets with multiple parents

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3039-3053
@@ +3038,17 @@
+      }
+    } else if (auto *CRI = dyn_cast<CleanupReturnInst>(Terminator)) {
+      // If we're cloning a cleanup pad and the uncloned cleanup pad unwinds
+      // to a block in the clone parent, we need to replace the cleanupret with
+      // an unreachable instruction.
+      BasicBlock *UnwindDest = CRI->getUnwindDest();
+      if (UnwindDest && FuncletBlocks[Parent].count(UnwindDest)) {
+        CRI->eraseFromParent();
+        new UnreachableInst(BB->getContext(), BB);
+        // Loop over all of the successors, removing BB's entry from any PHI
+        // nodes.
+        for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE;
+             ++SI)
+          (*SI)->removePredecessor(BB);
+      }
+    }
+  }
----------------
andrew.w.kaylor wrote:
> JosephTremoulet wrote:
> > andrew.w.kaylor wrote:
> > > JosephTremoulet wrote:
> > > > Cleanupret targets a sibling, not a parent.  So,
> > > >  1. I don't think this code should be here.
> > > >  2. I guess you should have code somewhere that updates these sibling->sibling edges, though you probably can't do that until both siblings have been cloned...
> > > > 
> > > > Also, whatever you do about this with cleanupret, I think cleanupendpad should get the same treatment
> > > I see what you're saying, but cleanupret doesn't always unwind to a sibling.  If it's the last cleanup inside a catch it can unwind to the parent (as it does in most of the current test cases).
> > > 
> > > I'll add some test cases involving sibling and then see if I can work out a good way to handle them.
> > Oh, yeah, huh, good point.
> > 
> > Thinking about that, I think that given a cleanupret/cleanupendpad:
> >  - if it unwinds to a catchendpad, it's an edge to the grandparent (we give the catchendpad the color of the parent of the catch, and the catch it's exiting is the parent of the cleanup)
> >  - if it unwinds to a cleanupendpad, it's an edge to the parent (we give the cleanupendpad the color of its cleanup, so this is a child cleanup unwinding to its parent cleanup)
> >  - if it unwinds to a catchpad or a cleanup pad or a terminatepad, it's an edge to a sibling
> > 
> > Does that sound right?
> That sounds right for IR generated in the expected ways.  I'm not certain that it isn't possible to construct legal IR that does other things (like cleanret returning to an arbitrary block in the parent).  I don't know why it would ever happen, but do our written rules disallow it?
1) Regarding

> if it unwinds to a catchendpad, it's an edge to the grandparent (we give the catchendpad the color of the parent of the catch, and the catch it's exiting is the parent of the cleanup)

I think that since the langref [specifies](http://llvm.org/docs/LangRef.html#catchendpad-instruction):

> It is undefined behavior to execute a catchendpad if, after the most recent execution of the normal successor edge of any catchpad chained to it, any other catchpad or cleanuppad has been executed but has not had a corresponding catchret/cleanupret/catchendpad/cleanupendpad executed

then it would be UB if the catchendpad were not ending a parent catch, and therefore having the grandparent color.


2) Regarding

> if it unwinds to a cleanupendpad, it's an edge to the parent (we give the cleanupendpad the color of its cleanup, so this is a child cleanup unwinding to its parent cleanup)

I think that since the langref [specifies](http://llvm.org/docs/LangRef.html#cleanupendpad-instruction)

> It is undefined behavior to execute a cleanupendpad if, after the most recent execution of its cleanuppad, any other cleanuppad or catchpad has been executed but has not had a corresponding cleanupret/catchret/cleanupendpad/catchendpad executed

then it would be UB if the cleanupendpad were not ending a parent cleanup, and therefore having the parent color.

3) Regarding

>if it unwinds to a catchpad or a cleanup pad or a terminatepad, it's an edge to a sibling

This is a direct consequence of the coloring algorithm.  For these sibling edges, I can't see why we would want to do anything other than translate them to the corresponding cloned-sibling edges between children of the cloned node.

4) Regarding

> like cleanret returning to an arbitrary block in the parent

 - A `cleanupret` successor edge is an unwind edge, and so can only target an EH pad.  I actually am not seeing this in the langref, but it's certainly [there in the verifier](http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?revision=246752&view=markup#l3018).
 - A `catchret` targeting a block in the middle of the "wrong" handler is an interesting case.  I think you're right that the langref doesn't preclude it, so I guess either we should find a wording that would preclude it or continue cloning at the point it targets?  Continuing cloning would complicate the cycle detection -- the UB rules in the langref indicate that you can't enter a pad twice (execute the foopad twice without an intervening fooret/fooendpad) and that you can't return from a pad twice (execute the catchret twice without an intervening catchpad), and currently you're building the graph/tree/paths with the assumption that any node entered is entered via its entry pad and so detecting cycles just on the pad...






Repository:
  rL LLVM

http://reviews.llvm.org/D13274