Add invoke of llvm.donothing to outlined catch and cleanup handlers to identify their personality

[andrew.w.kaylor]

Summary

This is a patch we talked about Friday on IRC to make it easier to identify the personality function associated with a catch or cleanup handler that has been outlined for native Windows exception handling.

[rnk]

lgtm

lib/CodeGen/WinEHPrepare.cpp

762

Do you think more work is necessary here? Unreachable seems like a reasonable thing to do.

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

762

No, that was a note to myself before I finished this function. Sorry about that.

[andrew.w.kaylor]

The only changes from the previously reviewed version are that I merged with top of trunk and I revised the WinEHCleanupDirector::handleEndCatch implementation.

After merging with trunk the cppeh-alloca-sink.ll test was asserting with this patch.

The problem is that nested "cleanup" handlers were being populated with an unreachable instruction because they begin with a call to llvm.eh.endcatch. In earlier tests I had been manually editing the IR to change invokes with effectively empty unwind destinations to calls, but this problem will continue to show up because the front end is still generating code like this. Also, I think we would have incorrectly handled nested cleanup of non-empty landing pads.

I believe that the cleanup cloning code will never find calls to llvm.eh.endcatch that aren't part of nested landing pads because it stops cloning when it sees llvm.eh.begincatch and doesn't follow that block's children.

I expect that there will be problems if a catch-all handler requires cleanup and both the cleanup and the catch handler are in the same block. This is not a new problem, so I just added a FIXME comment to note it.

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

1198

After thinking about this a little more, I think this comment is misplaced. I believe the cleanup code will always precede the catch code if they are in the same block, and the cleanup handler will stop cloning when it sees eh.begincatch. The cleanup code may be duplicated in the catch handler.

In any event, I'm going to delete this FIXME comment and see if I can create a test case.

[andrew.w.kaylor]

This was implemented in r234360.

lib/CodeGen/WinEHPrepare.cpp

private:
   void promoteLandingPadValues(LandingPadInst *LPad);
   void completeNestedLandingPad(Function *ParentFn,
                                 LandingPadInst *OutlinedLPad,
                                 const LandingPadInst *OriginalLPad,
                                 FrameVarInfoMap &VarInfo);
   bool outlineHandler(ActionHandler *Action, Function *SrcFn,
                       LandingPadInst *LPad, BasicBlock *StartBB,
                       FrameVarInfoMap &VarInfo);
+  void addStubInvokeToHandlerIfNeeded(Function *Handler, Value *PersonalityFn);
 
   void mapLandingPadBlocks(LandingPadInst *LPad, LandingPadActions &Actions);
   CatchHandler *findCatchHandler(BasicBlock *BB, BasicBlock *&NextBB,
                                  VisitedBlockSet &VisitedBlocks);
   CleanupHandler *findCleanupHandler(BasicBlock *StartBB, BasicBlock *EndBB);
 
   void processSEHCatchHandler(CatchHandler *Handler, BasicBlock *StartBB);
 
if (Pred == CmpInst::ICMP_NE) {
     CatchHandler = FBB;
     NextBB = TBB;
     return true;
   }
 
   return false;
 }
 
+static BasicBlock *createStubLandingPad(Function *Handler,
+                                        Value *PersonalityFn) {
+  // FIXME: Finish this!
+  LLVMContext &Context = Handler->getContext();
+  BasicBlock *StubBB = BasicBlock::Create(Context, "stub");
+  Handler->getBasicBlockList().push_back(StubBB);
+  IRBuilder<> Builder(StubBB);
+  LandingPadInst *LPad = Builder.CreateLandingPad(
+      llvm::StructType::get(Type::getInt8PtrTy(Context),
+                            Type::getInt32Ty(Context), nullptr),
+      PersonalityFn, 0);
+  LPad->setCleanup(true);
+  Builder.CreateUnreachable();
+  return StubBB;
+}
+
+// Cycles through the blocks in an outlined handler function looking for an
+// invoke instruction and inserts an invoke of llvm.donothing with an empty
+// landing pad if none is found.  The code that generates the .xdata tables for
+// the handler needs at least one landing pad to identify the parent function's
+// personality.
+void WinEHPrepare::addStubInvokeToHandlerIfNeeded(Function *Handler,
+                                                  Value *PersonalityFn) {
+  ReturnInst *Ret = nullptr;
+  for (BasicBlock &BB : *Handler) {
+    TerminatorInst *Terminator = BB.getTerminator();
+    // If we find an invoke, there is nothing to be done.
+    auto *II = dyn_cast<InvokeInst>(Terminator);
+    if (II)
+      return;
+    // If we've already recorded a return instruction, keep looking for invokes.
+    if (Ret)
+      continue;
+    // If we haven't recorded a return instruction yet, try this terminator.
+    Ret = dyn_cast<ReturnInst>(Terminator);
+  }
+
+  // If we got this far, the handler contains no invokes.  We should have seen
+  // at least one return.  We'll insert an invoke of llvm.donothing ahead of
+  // that return.
+  assert(Ret);
+  BasicBlock *OldRetBB = Ret->getParent();
+  BasicBlock *NewRetBB = SplitBlock(OldRetBB, Ret);
+  // SplitBlock adds an unconditional branch instruction at the end of the
+  // parent block.  We want to replace that with an invoke call, so we can
+  // erase it now.
+  OldRetBB->getTerminator()->eraseFromParent();
+  BasicBlock *StubLandingPad = createStubLandingPad(Handler, PersonalityFn);
+  Function *F =
+      Intrinsic::getDeclaration(Handler->getParent(), Intrinsic::donothing);
+  InvokeInst::Create(F, NewRetBB, StubLandingPad, None, "", OldRetBB);
+}
+
 bool WinEHPrepare::outlineHandler(ActionHandler *Action, Function *SrcFn,
                                   LandingPadInst *LPad, BasicBlock *StartBB,
                                   FrameVarInfoMap &VarInfo) {
   Module *M = SrcFn->getParent();
   LLVMContext &Context = M->getContext();
 
   // Create a new function to receive the handler contents.
   Type *Int8PtrType = Type::getInt8PtrTy(Context);
CloneAndPruneIntoFromInst(Handler, SrcFn, II, VMap,
                             /*ModuleLevelChanges=*/false, Returns, "",
                             &OutlinedFunctionInfo, Director.get());
 
   // Move all the instructions in the first cloned block into our entry block.
   BasicBlock *FirstClonedBB = std::next(Function::iterator(Entry));
   Entry->getInstList().splice(Entry->end(), FirstClonedBB->getInstList());
   FirstClonedBB->eraseFromParent();
 
+  // Make sure we can identify the handler's personality later.
+  addStubInvokeToHandlerIfNeeded(Handler, LPad->getPersonalityFn());
+
   if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
     WinEHCatchDirector *CatchDirector =
         reinterpret_cast<WinEHCatchDirector *>(Director.get());
     CatchAction->setExceptionVar(CatchDirector->getExceptionVar());
     CatchAction->setReturnTargets(CatchDirector->getReturnTargets());
 
     // Look for blocks that are not part of the landing pad that we just
     // outlined but terminate with a call to llvm.eh.endcatch and a
CloningDirector::CloningAction WinEHCleanupDirector::handleBeginCatch(
   // before the cloning process is complete.
   BasicBlock::InstListType &InstList = NewBB->getInstList();
   InstList.push_back(new UnreachableInst(NewBB->getContext()));
   return CloningDirector::StopCloningBB;
 }
 
 CloningDirector::CloningAction WinEHCleanupDirector::handleEndCatch(
     ValueToValueMapTy &VMap, const Instruction *Inst, BasicBlock *NewBB) {
-  // Catch blocks within cleanup handlers will always be unreachable.
-  // We'll insert an unreachable instruction now, but it will be pruned
-  // before the cloning process is complete.
-  BasicBlock::InstListType &InstList = NewBB->getInstList();
-  InstList.push_back(new UnreachableInst(NewBB->getContext()));
-  return CloningDirector::StopCloningBB;
+  // Cleanup handlers nested within catch handlers may begin with a call to
+  // eh.endcatch.  We can just ignore that instruction.
+  // FIXME: There will be a problem with a catch-all handler and cleanup
+  // code in the same block.
+  return CloningDirector::SkipInstruction;
 }
 
 CloningDirector::CloningAction WinEHCleanupDirector::handleTypeIdFor(
     ValueToValueMapTy &VMap, const Instruction *Inst, BasicBlock *NewBB) {
   // If we encounter a selector comparison while cloning a cleanup handler,
   // we want to stop cloning immediately.  Anything after the dispatch
   // will be outlined into a different handler.
   BasicBlock *CatchHandler;
while (BB) {
     if (Branch && Branch->isConditional()) {
       // Look for the selector dispatch.
       //   %2 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIf to i8*))
       //   %matches = icmp eq i32 %sel, %2
       //   br i1 %matches, label %catch14, label %eh.resume
       CmpInst *Compare = dyn_cast<CmpInst>(Branch->getCondition());
       if (!Compare || !Compare->isEquality())
         return createCleanupHandler(CleanupHandlerMap, BB);
-      for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(),
-        IE = BB->end();
+      for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(), IE = BB->end();
            II != IE; ++II) {
         Instruction *Inst = II;
         if (LPadMap && LPadMap->isLandingPadSpecificInst(Inst))
           continue;
         if (Inst == Compare || Inst == Branch)
           continue;
         if (match(Inst, m_Intrinsic<Intrinsic::eh_typeid_for>()))
           continue;
         return createCleanupHandler(CleanupHandlerMap, BB);
       }
       // The selector dispatch block should always terminate our search.
       assert(BB == EndBB);
       return nullptr;
     }
 
     // Anything else is either a catch block or interesting cleanup code.
-    for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(),
-      IE = BB->end();
+    for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(), IE = BB->end();
          II != IE; ++II) {
       Instruction *Inst = II;
       if (LPadMap && LPadMap->isLandingPadSpecificInst(Inst))
         continue;
       // Unconditional branches fall through to this loop.
       if (Inst == Branch)
         continue;
       // If this is a catch block, there is no cleanup code to be found.
       if (match(Inst, m_Intrinsic<Intrinsic::eh_begincatch>()))

 }
 
 // This is a public function, declared in WinEHFuncInfo.h and is also
 // referenced by WinEHNumbering in FunctionLoweringInfo.cpp.
 void llvm::parseEHActions(const IntrinsicInst *II,
                           SmallVectorImpl<ActionHandler *> &Actions) {
   for (unsigned I = 0, E = II->getNumArgOperands(); I != E;) {
     uint64_t ActionKind =
         cast<ConstantInt>(II->getArgOperand(I))->getZExtValue();
     if (ActionKind == /*catch=*/1) {
       auto *Selector = cast<Constant>(II->getArgOperand(I + 1));
       ConstantInt *EHObjIndex = cast<ConstantInt>(II->getArgOperand(I + 2));
       int64_t EHObjIndexVal = EHObjIndex->getSExtValue();
       Constant *Handler = cast<Constant>(II->getArgOperand(I + 3));
       I += 4;
       auto *CH = new CatchHandler(/*BB=*/nullptr, Selector, /*NextBB=*/nullptr);
       CH->setHandlerBlockOrFunc(Handler);
       CH->setExceptionVarIndex(EHObjIndexVal);
       Actions.push_back(CH);
     } else if (ActionKind == 0) {
       Constant *Handler = cast<Constant>(II->getArgOperand(I + 1));
       I += 2;
       auto *CH = new CleanupHandler(/*BB=*/nullptr);
       CH->setHandlerBlockOrFunc(Handler);
       Actions.push_back(CH);
     } else {
       llvm_unreachable("Expected either a catch or cleanup handler!");
     }
   }
   std::reverse(Actions.begin(), Actions.end());
 }
-