[WinEH] Fixes for a few cppeh failures

[andrew.w.kaylor]

Summary

This patch fixes three groups of failures I've come across with C++ exception handling.

1. Under some circumstances a cleanup handler will call terminate and go to an unreachable instruction rather than returning. This caused an assertion in the code that tries to insert a stub landing pad. There are a couple of problems here that I didn't attempt to address. I don't think the front end should be inserting a landing pad with a terminate call, since the runtime takes care of that on Windows. Also, the IR is generated as a landing pad with a catch-all clause but no begin/end catch. As a result we are interpreting it as cleanup code. In any event, it seems useful, at least in the short term, to handle the case of an outlined handler that flows to an unreachable instruction but not a return instruction.

2. Under some circumstances the front end will chain together a series of catch dispatch blocks wherein some selectors are repeated and do not correspond to clauses in the parent landing pad. This happens when similar block sequences are combined. It should work because the earlier dispatch will logically catch the exception and make the later dispatch unreachable for that code path. However, our handler block mapping wasn't paying attention to which selector comparisons were being done and so it thinks it is done mapping blocks before it actually is. The "fix" I have for that is intended as a stop-gap measure to get some tests passing until we can decide how this should be redesigned.

3. When blocks are combined for multiple landing pads the selector values can reach the dispatch comparison through a PHI node and thus confuse the cloning code. The cloning code tries to do constant folding to avoid cloning blocks it doesn't need, but in this case the PHI resolution doesn't happen until all blocks have been cloned and so we hit an assertion when a second begincatch call is reached. I worked around this by treating all compare instructions which have an llvm.eh.typeid.for result as an operand as if the other operand is the selector value (which I believe will always be true). This effectively emulates the suggestion I made of combining the llvm.eh.typeid.for+icmp instructions into a single intrinsic. I still think adding that intrinsic is a better solution but, again, this gets some things working in the meantime.

I don't really think that any of the changes in this patch are the right long term solution, but I think they are worth committing to establish a baseline of correct results.

[rnk]

lgtm

[andrew.w.kaylor]

This was submitting in r235239.

lib/CodeGen/WinEHPrepare.cpp

virtual CloningAction handleTypeIdFor(ValueToValueMapTy &VMap,
                                         const Instruction *Inst,
                                         BasicBlock *NewBB) = 0;
   virtual CloningAction handleInvoke(ValueToValueMapTy &VMap,
                                      const InvokeInst *Invoke,
                                      BasicBlock *NewBB) = 0;
   virtual CloningAction handleResume(ValueToValueMapTy &VMap,
                                      const ResumeInst *Resume,
                                      BasicBlock *NewBB) = 0;
+  virtual CloningAction handleCompare(ValueToValueMapTy &VMap,
+                                      const CmpInst *Compare,
+                                      BasicBlock *NewBB) = 0;
   virtual CloningAction handleLandingPad(ValueToValueMapTy &VMap,
                                          const LandingPadInst *LPad,
                                          BasicBlock *NewBB) = 0;
 
   ValueMaterializer *getValueMaterializer() override { return &Materializer; }
 
 protected:
   WinEHFrameVariableMaterializer Materializer;
CloningAction handleEndCatch(ValueToValueMapTy &VMap, const Instruction *Inst,
                                BasicBlock *NewBB) override;
   CloningAction handleTypeIdFor(ValueToValueMapTy &VMap,
                                 const Instruction *Inst,
                                 BasicBlock *NewBB) override;
   CloningAction handleInvoke(ValueToValueMapTy &VMap, const InvokeInst *Invoke,
                              BasicBlock *NewBB) override;
   CloningAction handleResume(ValueToValueMapTy &VMap, const ResumeInst *Resume,
                              BasicBlock *NewBB) override;
+  CloningAction handleCompare(ValueToValueMapTy &VMap,
+                              const CmpInst *Compare, BasicBlock *NewBB) override;
   CloningAction handleLandingPad(ValueToValueMapTy &VMap,
                                  const LandingPadInst *LPad,
                                  BasicBlock *NewBB) override;
 
   Value *getExceptionVar() { return ExceptionObjectVar; }
   TinyPtrVector<BasicBlock *> &getReturnTargets() { return ReturnTargets; }
 
 private:
CloningAction handleEndCatch(ValueToValueMapTy &VMap, const Instruction *Inst,
                                BasicBlock *NewBB) override;
   CloningAction handleTypeIdFor(ValueToValueMapTy &VMap,
                                 const Instruction *Inst,
                                 BasicBlock *NewBB) override;
   CloningAction handleInvoke(ValueToValueMapTy &VMap, const InvokeInst *Invoke,
                              BasicBlock *NewBB) override;
   CloningAction handleResume(ValueToValueMapTy &VMap, const ResumeInst *Resume,
                              BasicBlock *NewBB) override;
+  CloningAction handleCompare(ValueToValueMapTy &VMap,
+                              const CmpInst *Compare, BasicBlock *NewBB) override;
   CloningAction handleLandingPad(ValueToValueMapTy &VMap,
                                  const LandingPadInst *LPad,
                                  BasicBlock *NewBB) override;
 };
 
 class LandingPadActions {
 public:
   LandingPadActions() : HasCleanupHandlers(false) {}

 // 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;
+  UnreachableInst *Unreached = 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.
+    if (!Ret)
       Ret = dyn_cast<ReturnInst>(Terminator);
+    // If we haven't recorded an unreachable instruction, try this terminator.
+    if (!Unreached)
+      Unreached = dyn_cast<UnreachableInst>(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);
+  assert(Ret || Unreached);
+  TerminatorInst *Term;
+  if (Ret)
+    Term = Ret;
+  else
+    Term = Unreached;
+  BasicBlock *OldRetBB = Term->getParent();
+  BasicBlock *NewRetBB = SplitBlock(OldRetBB, Term);
   // 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);
CloningDirector::CloningAction WinEHCloningDirectorBase::handleInstruction(
   }
 
   if (auto *Invoke = dyn_cast<InvokeInst>(Inst))
     return handleInvoke(VMap, Invoke, NewBB);
 
   if (auto *Resume = dyn_cast<ResumeInst>(Inst))
     return handleResume(VMap, Resume, NewBB);
 
+  if (auto *Cmp = dyn_cast<CmpInst>(Inst))
+    return handleCompare(VMap, Cmp, NewBB);
+
   if (match(Inst, m_Intrinsic<Intrinsic::eh_begincatch>()))
     return handleBeginCatch(VMap, Inst, NewBB);
   if (match(Inst, m_Intrinsic<Intrinsic::eh_endcatch>()))
     return handleEndCatch(VMap, Inst, NewBB);
   if (match(Inst, m_Intrinsic<Intrinsic::eh_typeid_for>()))
     return handleTypeIdFor(VMap, Inst, NewBB);
 
   // Continue with the default cloning behavior.
WinEHCatchDirector::handleResume(ValueToValueMapTy &VMap,
                                  const ResumeInst *Resume, BasicBlock *NewBB) {
   // Resume instructions shouldn't be reachable from catch handlers.
   // We still need to handle it, but it will be pruned.
   BasicBlock::InstListType &InstList = NewBB->getInstList();
   InstList.push_back(new UnreachableInst(NewBB->getContext()));
   return CloningDirector::StopCloningBB;
 }
 
+CloningDirector::CloningAction
+WinEHCatchDirector::handleCompare(ValueToValueMapTy &VMap,
+                                  const CmpInst *Compare, BasicBlock *NewBB) {
+  const IntrinsicInst *IntrinCall = nullptr;
+  if (match(Compare->getOperand(0), m_Intrinsic<Intrinsic::eh_typeid_for>())) {
+    IntrinCall = dyn_cast<IntrinsicInst>(Compare->getOperand(0));
+  } else if (match(Compare->getOperand(1), m_Intrinsic<Intrinsic::eh_typeid_for>())) {
+    IntrinCall = dyn_cast<IntrinsicInst>(Compare->getOperand(1));
+  }
+  if (IntrinCall) {
+    Value *Selector = IntrinCall->getArgOperand(0)->stripPointerCasts();
+    // This causes a replacement that will collapse the landing pad CFG based
+    // on the filter function we intend to match.
+    if (Selector == CurrentSelector->stripPointerCasts()) {
+      VMap[Compare] = ConstantInt::get(SelectorIDType, 1);
+    }
+    else {
+      VMap[Compare] = ConstantInt::get(SelectorIDType, 0);
+    }
+    return CloningDirector::SkipInstruction;
+  }
+  return CloningDirector::CloneInstruction;
+}
+
 CloningDirector::CloningAction WinEHCleanupDirector::handleLandingPad(
     ValueToValueMapTy &VMap, const LandingPadInst *LPad, BasicBlock *NewBB) {
   // The MS runtime will terminate the process if an exception occurs in a
   // cleanup handler, so we shouldn't encounter landing pads in the actual
   // cleanup code, but they may appear in catch blocks.  Depending on where
   // we started cloning we may see one, but it will get dropped during dead
   // block pruning.
   Instruction *NewInst = new UnreachableInst(NewBB->getContext());
   VMap[LPad] = NewInst;
   BasicBlock::InstListType &InstList = NewBB->getInstList();
   InstList.push_back(NewInst);
   return CloningDirector::StopCloningBB;
 }
 
 CloningDirector::CloningAction WinEHCleanupDirector::handleBeginCatch(
     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()));
+  // Cleanup code may flow into catch blocks or the catch block may be part
+  // of a branch that will be optimized away.  We'll insert a return
+  // instruction now, but it may be pruned before the cloning process is
+  // complete.
+  ReturnInst::Create(NewBB->getContext(), nullptr, NewBB);
   return CloningDirector::StopCloningBB;
 }
 
 CloningDirector::CloningAction WinEHCleanupDirector::handleEndCatch(
     ValueToValueMapTy &VMap, const Instruction *Inst, BasicBlock *NewBB) {
   // Cleanup handlers nested within catch handlers may begin with a call to
   // eh.endcatch.  We can just ignore that instruction.
   return CloningDirector::SkipInstruction;
CloningDirector::CloningAction WinEHCleanupDirector::handleResume(
   ReturnInst::Create(NewBB->getContext(), nullptr, NewBB);
 
   // We just added a terminator to the cloned block.
   // Tell the caller to stop processing the current basic block so that
   // the branch instruction will be skipped.
   return CloningDirector::StopCloningBB;
 }
 
+CloningDirector::CloningAction
+WinEHCleanupDirector::handleCompare(ValueToValueMapTy &VMap,
+                                    const CmpInst *Compare, BasicBlock *NewBB) {
+  const IntrinsicInst *IntrinCall = nullptr;
+  if (match(Compare->getOperand(0), m_Intrinsic<Intrinsic::eh_typeid_for>()) ||
+      match(Compare->getOperand(1), m_Intrinsic<Intrinsic::eh_typeid_for>())) {
+    VMap[Compare] = ConstantInt::get(SelectorIDType, 1);
+    return CloningDirector::SkipInstruction;
+  }
+  return CloningDirector::CloneInstruction;
+
+}
+
 WinEHFrameVariableMaterializer::WinEHFrameVariableMaterializer(
     Function *OutlinedFn, FrameVarInfoMap &FrameVarInfo)
     : FrameVarInfo(FrameVarInfo), Builder(OutlinedFn->getContext()) {
   BasicBlock *EntryBB = &OutlinedFn->getEntryBlock();
   Builder.SetInsertPoint(EntryBB, EntryBB->getFirstInsertionPt());
 }
 
 Value *WinEHFrameVariableMaterializer::materializeValueFor(Value *V) {
void WinEHPrepare::mapLandingPadBlocks(LandingPadInst *LPad,
 
   VisitedBlockSet VisitedBlocks;
 
   while (HandlersFound != NumClauses) {
     BasicBlock *NextBB = nullptr;
 
     // See if the clause we're looking for is a catch-all.
     // If so, the catch begins immediately.
-    if (isa<ConstantPointerNull>(LPad->getClause(HandlersFound))) {
+    Constant *ExpectedSelector = LPad->getClause(HandlersFound)->stripPointerCasts();
+    if (isa<ConstantPointerNull>(ExpectedSelector)) {
       // The catch all must occur last.
       assert(HandlersFound == NumClauses - 1);
 
+      // There can be additional selector dispatches in the call chain that we
+      // need to ignore.
+      BasicBlock *CatchBlock = nullptr;
+      Constant *Selector;
+      while (BB && isSelectorDispatch(BB, CatchBlock, Selector, NextBB)) {
+        DEBUG(dbgs() << "  Found extra catch dispatch in block "
+          << CatchBlock->getName() << "\n");
+        BB = NextBB;
+      }
+
       // For C++ EH, check if there is any interesting cleanup code before we
       // begin the catch. This is important because cleanups cannot rethrow
       // exceptions but code called from catches can. For SEH, it isn't
       // important if some finally code before a catch-all is executed out of
       // line or after recovering from the exception.
       if (Personality == EHPersonality::MSVC_CXX)
         findCleanupHandlers(Actions, BB, BB);
 
       // Add the catch handler to the action list.
-      CatchHandler *Action =
-          new CatchHandler(BB, LPad->getClause(HandlersFound), nullptr);
+      // Since this is a catch-all handler, the selector won't actually appear
+      // in the code anywhere.  ExpectedSelector here is the constant null ptr
+      // that we got from the landing pad instruction.
+      CatchHandler *Action = new CatchHandler(BB, ExpectedSelector, nullptr);
       CatchHandlerMap[BB] = Action;
       Actions.insertCatchHandler(Action);
       DEBUG(dbgs() << "  Catch all handler at block " << BB->getName() << "\n");
       ++HandlersFound;
 
       // Once we reach a catch-all, don't expect to hit a resume instruction.
       BB = nullptr;
       break;
     }
 
     CatchHandler *CatchAction = findCatchHandler(BB, NextBB, VisitedBlocks);
     // See if there is any interesting code executed before the dispatch.
     findCleanupHandlers(Actions, BB, CatchAction->getStartBlock());
 
     assert(CatchAction);
+
+    // When the source program contains multiple nested try blocks the catch
+    // handlers can get strung together in such a way that we can encounter
+    // a dispatch for a selector that we've already had a handler for.
+    if (CatchAction->getSelector()->stripPointerCasts() == ExpectedSelector) {
       ++HandlersFound;
 
       // Add the catch handler to the action list.
-    Actions.insertCatchHandler(CatchAction);
       DEBUG(dbgs() << "  Found catch dispatch in block "
                    << CatchAction->getStartBlock()->getName() << "\n");
+      Actions.insertCatchHandler(CatchAction);
+    } else {
+      DEBUG(dbgs() << "  Found extra catch dispatch in block "
+                   << CatchAction->getStartBlock()->getName() << "\n");
+    }
 
     // Move on to the block after the catch handler.
     BB = NextBB;
   }
 
   // If we didn't wind up in a catch-all, see if there is any interesting code
   // executed before the resume.
   findCleanupHandlers(Actions, BB, BB);