[WinEH] Split blocks at calls to llvm.eh.begincatch.

[andrew.w.kaylor]

Summary

This patch introduces code to split basic blocks before calls to llvm.eh.begincatch (and any adjacent operands) in order to allow catch handlers and cleanup code to be more easily distinguished during later processing.

The patch also replaces all calls to BasicBlock::splitBasicBlock() with a call to llvm::SplitBlock() because the latter is able to preserve the DominatorTree whereas the former does not. This causes a change in block names, because llvm::SplitBlock() doesn't provide a parameter to name the split block. Obviously we could rename the block after SplitBlock returns, but that didn't seem necessary to me.

[rnk]

lgtm

Yeah, I was worrying about DominatorTree preservation when I added those splitBasicBlock calls. Nothing seemed to break, so I went with it, which is always dangerous. :)

lib/CodeGen/WinEHPrepare.cpp

409–411 ↗

(On Diff #24516)

Hm, I guess this funny business is needed to allow bitcasts and extractvalue? I guess it prevents us from demoting too much stuff.

418 ↗

(On Diff #24516)

You should check I != nullptr in this condition because Op0 and Op1 could be null if the catch parameter is null. Null is not an instruction, so the dyn_cast will fail, and we might call getPrevNode() on null.

llvm/trunk/lib/CodeGen/WinEHPrepare.cpp

public:
   }
 
 private:
   bool prepareExceptionHandlers(Function &F,
                                 SmallVectorImpl<LandingPadInst *> &LPads);
   void promoteLandingPadValues(LandingPadInst *LPad);
   void demoteValuesLiveAcrossHandlers(Function &F,
                                       SmallVectorImpl<LandingPadInst *> &LPads);
+  void findSEHEHReturnPoints(Function &F,
+                             SetVector<BasicBlock *> &EHReturnBlocks);
+  void findCXXEHReturnPoints(Function &F,
+                             SetVector<BasicBlock *> &EHReturnBlocks);
   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);
if (auto *II = dyn_cast<InvokeInst>(BB->getTerminator())) {
       continue;
     }
 
     // Otherwise, follow all successors.
     Worklist.insert(succ_begin(BB), succ_end(BB));
   }
 }
 
+// Attempt to find an instruction where a block can be split before
+// a call to llvm.eh.begincatch and its operands.  If the block
+// begins with the begincatch call or one of its adjacent operands
+// the block will not be split.
+static Instruction *findBeginCatchSplitPoint(BasicBlock *BB,
+                                             IntrinsicInst *II) {
+  // If the begincatch call is already the first instruction in the block,
+  // don't split.
+  Instruction *FirstNonPHI = BB->getFirstNonPHI();
+  if (II == FirstNonPHI)
+    return nullptr;
+
+  // If either operand is in the same basic block as the instruction and
+  // isn't used by another instruction before the begincatch call, include it
+  // in the split block.
+  auto *Op0 = dyn_cast<Instruction>(II->getOperand(0));
+  auto *Op1 = dyn_cast<Instruction>(II->getOperand(1));
+
+  Instruction *I = II->getPrevNode();
+  Instruction *LastI = II;
+
+  while (I == Op0 || I == Op1) {
+    // If the block begins with one of the operands and there are no other
+    // instructions between the operand and the begincatch call, don't split.
+    if (I == FirstNonPHI)
+      return nullptr;
+
+    LastI = I;
+    I = I->getPrevNode();
+  }
+
+  // If there is at least one instruction in the block before the begincatch
+  // call and its operands, split the block at either the begincatch or
+  // its operand.
+  return LastI;
+}
+
 /// Find all points where exceptional control rejoins normal control flow via
 /// llvm.eh.endcatch. Add them to the normal bb reachability worklist.
-static void findCXXEHReturnPoints(Function &F,
-                                  SetVector<BasicBlock *> &EHReturnBlocks) {
+void WinEHPrepare::findCXXEHReturnPoints(
+    Function &F, SetVector<BasicBlock *> &EHReturnBlocks) {
   for (auto BBI = F.begin(), BBE = F.end(); BBI != BBE; ++BBI) {
     BasicBlock *BB = BBI;
     for (Instruction &I : *BB) {
+      if (match(&I, m_Intrinsic<Intrinsic::eh_begincatch>())) {
+        Instruction *SplitPt = 
+            findBeginCatchSplitPoint(BB, cast<IntrinsicInst>(&I));
+        if (SplitPt) {
+          // Split the block before the llvm.eh.begincatch call to allow
+          // cleanup and catch code to be distinguished later.
+          // Do not update BBI because we still need to process the
+          // portion of the block that we are splitting off.
+          SplitBlock(BB, &I, DT);
+          break;
+        }
+      }
       if (match(&I, m_Intrinsic<Intrinsic::eh_endcatch>())) {
         // Split the block after the call to llvm.eh.endcatch if there is
         // anything other than an unconditional branch, or if the successor
         // starts with a phi.
         auto *Br = dyn_cast<BranchInst>(I.getNextNode());
         if (!Br || !Br->isUnconditional() ||
             isa<PHINode>(Br->getSuccessor(0)->begin())) {
           DEBUG(dbgs() << "splitting block " << BB->getName()
                        << " with llvm.eh.endcatch\n");
-          BBI = BB->splitBasicBlock(I.getNextNode(), "ehreturn");
+          BBI = SplitBlock(BB, I.getNextNode(), DT);
         }
         // The next BB is normal control flow.
         EHReturnBlocks.insert(BB->getTerminator()->getSuccessor(0));
         break;
       }
     }
   }
 }
 
 static bool isCatchAllLandingPad(const BasicBlock *BB) {
   const LandingPadInst *LP = BB->getLandingPadInst();
   if (!LP)
     return false;
   unsigned N = LP->getNumClauses();
   return (N > 0 && LP->isCatch(N - 1) &&
           isa<ConstantPointerNull>(LP->getClause(N - 1)));
 }
 
 /// Find all points where exceptions control rejoins normal control flow via
 /// selector dispatch.
-static void findSEHEHReturnPoints(Function &F,
-                                  SetVector<BasicBlock *> &EHReturnBlocks) {
+void WinEHPrepare::findSEHEHReturnPoints(
+    Function &F, SetVector<BasicBlock *> &EHReturnBlocks) {
   for (auto BBI = F.begin(), BBE = F.end(); BBI != BBE; ++BBI) {
     BasicBlock *BB = BBI;
     // If the landingpad is a catch-all, treat the whole lpad as if it is
     // reachable from normal control flow.
     // FIXME: This is imprecise. We need a better way of identifying where a
     // catch-all starts and cleanups stop. As far as LLVM is concerned, there
     // is no difference.
     if (isCatchAllLandingPad(BB)) {
for (ActionHandler *Action : Actions) {
       }
 
       outlineHandler(Action, &F, LPad, StartBB, FrameVarInfo);
     }
 
     // Split the block after the landingpad instruction so that it is just a
     // call to llvm.eh.actions followed by indirectbr.
     assert(!isa<PHINode>(LPadBB->begin()) && "lpad phi not removed");
-    LPadBB->splitBasicBlock(LPad->getNextNode(),
-                            LPadBB->getName() + ".prepsplit");
+    SplitBlock(LPadBB, LPad->getNextNode(), DT);
     // Erase the branch inserted by the split so we can insert indirectbr.
     LPadBB->getTerminator()->eraseFromParent();
 
     // Replace all extracted values with undef and ultimately replace the
     // landingpad with undef.
     SmallVector<Instruction *, 4> SEHCodeUses;
     SmallVector<Instruction *, 4> EHUndefs;
     for (User *U : LPad->users()) {
void WinEHPrepare::addStubInvokeToHandlerIfNeeded(Function *Handler,
   // llvm.donothing ahead of that instruction.
   assert(Ret || Unreached);
   TerminatorInst *Term;
   if (Ret)
     Term = Ret;
   else
     Term = Unreached;
   BasicBlock *OldRetBB = Term->getParent();
-  BasicBlock *NewRetBB = SplitBlock(OldRetBB, Term);
+  BasicBlock *NewRetBB = SplitBlock(OldRetBB, Term, DT);
   // 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);
if (Res) {
     // block.
     // FIXME: Handle instructions in the dispatch block. Currently we drop them,
     // leading to crashes if some optimization hoists stuff here.
     assert(CatchAction->getSelector() && HandlerBB &&
            "expected catch EH dispatch");
   } else {
     // This must be a catch-all. Split the block after the landingpad.
     assert(CatchAction->getSelector()->isNullValue() && "expected catch-all");
-    HandlerBB =
-        StartBB->splitBasicBlock(StartBB->getFirstInsertionPt(), "catch.all");
+    HandlerBB = SplitBlock(StartBB, StartBB->getFirstInsertionPt(), DT);
   }
   IRBuilder<> Builder(HandlerBB->getFirstInsertionPt());
   Function *EHCodeFn = Intrinsic::getDeclaration(
       StartBB->getParent()->getParent(), Intrinsic::eh_exceptioncode);
   Value *Code = Builder.CreateCall(EHCodeFn, "sehcode");
   Code = Builder.CreateIntToPtr(Code, SEHExceptionCodeSlot->getAllocatedType());
   Builder.CreateStore(Code, SEHExceptionCodeSlot);
   CatchAction->setHandlerBlockOrFunc(BlockAddress::get(HandlerBB));
if (isa<ConstantPointerNull>(ExpectedSelector)) {
       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 = nullptr;
       if (CatchHandlerMap.count(BB) && CatchHandlerMap[BB] != nullptr) {
         // If the CatchHandlerMap already has an entry for this BB, re-use it.
         Action = CatchHandlerMap[BB];
         assert(Action->getSelector() == ExpectedSelector);
       } else {
-        // 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.
+        // We don't expect a selector dispatch, but there may be a call to
+        // llvm.eh.begincatch, which separates catch handling code from
+        // cleanup code in the same control flow.  This call looks for the
+        // begincatch intrinsic.
+        Action = findCatchHandler(BB, NextBB, VisitedBlocks);
+        if (Action) {
+          // 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);
+        } else {
+          // If an action was not found, it means that the control flows
+          // directly into the catch-all handler and there is no cleanup code.
+          // That's an expected situation and we must create a catch action.
+          // 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.
           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;
     }
if (isAsynchronousEHPersonality(Personality)) {
         DEBUG(dbgs() << "  Found frontend-outlined finally call to "
                      << Fin->getName() << " in block "
                      << Action->getStartBlock()->getName() << "\n");
 
         // Split the block if there were more interesting instructions and look
         // for finally calls in the normal successor block.
         BasicBlock *SuccBB = BB;
         if (FinallyCall.getInstruction() != BB->getTerminator() &&
-            FinallyCall.getInstruction()->getNextNode() != BB->getTerminator()) {
-          SuccBB = BB->splitBasicBlock(FinallyCall.getInstruction()->getNextNode());
+            FinallyCall.getInstruction()->getNextNode() !=
+                BB->getTerminator()) {
+          SuccBB =
+              SplitBlock(BB, FinallyCall.getInstruction()->getNextNode(), DT);
         } else {
           if (FinallyCall.isInvoke()) {
-            SuccBB = cast<InvokeInst>(FinallyCall.getInstruction())->getNormalDest();
+            SuccBB =
+                cast<InvokeInst>(FinallyCall.getInstruction())->getNormalDest();
           } else {
             SuccBB = BB->getUniqueSuccessor();
             assert(SuccBB && "splitOutlinedFinallyCalls didn't insert a branch");
           }
         }
         BB = SuccBB;
         if (BB == EndBB)
           return;

llvm/trunk/test/CodeGen/WinEH/cppeh-demote-liveout.ll

 }
 
 ; CHECK-LABEL: define i32 @liveout_catch(i32 %p)
 ; CHECK: %val.entry = add i32 %p, 1
 ; CHECK-NEXT: store i32 %val.entry, i32* %val.entry.reg2mem
 ; CHECK: invoke void @might_throw()
 ;
 ; CHECK: landingpad
-; CHECK: indirectbr i8* {{.*}}, [label %ehreturn]
+; CHECK: indirectbr i8* {{.*}}, [label %catchit.split]
 ;
-; CHECK: ehreturn:
+; CHECK: catchit.split:
 ; CHECK: load i32, i32* %val.lpad.reg2mem
 ; CHECK: br label %ret
 ;
 ; CHECK: ret:
-; CHECK: %rv = phi i32 [ {{.*}}, %entry ], [ {{.*}}, %ehreturn ]
+; CHECK: %rv = phi i32 [ {{.*}}, %entry ], [ {{.*}}, %catchit.split ]
 ; CHECK: ret i32
 
 ; CHECK-LABEL: define internal i8* @liveout_catch.catch(i8*, i8*)
 ; CHECK: %[[val:[^ ]*]] = load i32, i32*
 ; CHECK-NEXT: %[[val_lpad:[^ ]*]] = add i32 %[[val]], 1
 ; CHECK-NEXT: store i32 %[[val_lpad]], i32*
-; CHECK: ret i8* blockaddress(@liveout_catch, %ehreturn)
+; CHECK: ret i8* blockaddress(@liveout_catch, %catchit.split)

llvm/trunk/test/CodeGen/WinEH/cppeh-mixed-catch-and-cleanup.ll

+; RUN: opt -mtriple=x86_64-pc-windows-msvc -winehprepare -S -o - < %s | FileCheck %s
+
+; This test is based on the following code:
+;
+; void test()
+; {
+;   try {
+;     Obj o;
+;     may_throw();
+;   } catch (...) {
+;   }
+; }
+;
+; The purpose of this test is to verify that we create separate catch and
+; cleanup handlers.  When compiling for the C++ 11 standard, this isn't
+; strictly necessary, since calling the destructor from the catch handler
+; would be logically equivalent to calling it from a cleanup handler.
+; However, if the -std=c++98 option is used, an exception in the cleanup
+; code should terminate the process (the MSVCRT runtime will do that) but
+; if the destructor is called from the catch handler, it wouldn't terminate
+; the process
+
+
+; ModuleID = 'cppeh-mixed-catch-and-cleanup.cpp'
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+%class.Obj = type { i8 }
+
+; This just verifies that the function was processed by WinEHPrepare.
+;
+; CHECK-LABEL: define void @"\01?test@@YAXXZ"()
+; CHECK: entry:
+; CHECK:   call void (...) @llvm.frameescape
+; CHECK: }
+
+; Function Attrs: nounwind uwtable
+define void @"\01?test@@YAXXZ"() #0 {
+entry:
+  %o = alloca %class.Obj, align 1
+  %exn.slot = alloca i8*
+  %ehselector.slot = alloca i32
+  invoke void @"\01?may_throw@@YAXXZ"()
+          to label %invoke.cont unwind label %lpad
+
+invoke.cont:                                      ; preds = %entry
+  call void @"\01??1Obj@@QEAA@XZ"(%class.Obj* %o) #3
+  br label %try.cont
+
+lpad:                                             ; preds = %entry
+  %0 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
+          catch i8* null
+  %1 = extractvalue { i8*, i32 } %0, 0
+  store i8* %1, i8** %exn.slot
+  %2 = extractvalue { i8*, i32 } %0, 1
+  store i32 %2, i32* %ehselector.slot
+  call void @"\01??1Obj@@QEAA@XZ"(%class.Obj* %o) #3
+  %exn = load i8*, i8** %exn.slot
+  call void @llvm.eh.begincatch(i8* %exn, i8* null) #3
+  call void @llvm.eh.endcatch() #3
+  br label %try.cont
+
+try.cont:                                         ; preds = %catch, %invoke.cont
+  ret void
+}
+
+; Verify that a cleanup handler was created and that it calls ~Obj().
+; CHECK-LABEL: define internal void @"\01?test@@YAXXZ.cleanup"(i8*, i8*)
+; CHECK: entry:
+; CHECK:   @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1, i32 0)
+; CHECK:   call void @"\01??1Obj@@QEAA@XZ"
+; CHECK:   ret void
+; CHECK: }
+
+; Verify that a catch handler was created and that it does not call ~Obj().
+; CHECK-LABEL: define internal i8* @"\01?test@@YAXXZ.catch"(i8*, i8*)
+; CHECK: entry:
+; CHECK-NOT:  call void @"\01??1Obj@@QEAA@XZ"
+; CHECK:   ret i8* blockaddress(@"\01?test@@YAXXZ", %try.cont)
+; CHECK: }
+
+
+
+declare void @"\01?may_throw@@YAXXZ"() #1
+
+declare i32 @__CxxFrameHandler3(...)
+
+; Function Attrs: nounwind
+declare void @"\01??1Obj@@QEAA@XZ"(%class.Obj*) #2
+
+; Function Attrs: nounwind
+declare void @llvm.eh.begincatch(i8* nocapture, i8* nocapture) #3
+
+; Function Attrs: nounwind
+declare void @llvm.eh.endcatch() #3
+
+attributes #0 = { nounwind uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #3 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.7.0 (trunk 235779) (llvm/trunk 235769)"}

llvm/trunk/test/CodeGen/WinEH/cppeh-shared-empty-catch.ll

lpad:                                             ; preds = %entry
   br label %catch2
 
 ; Note: Even though this landing pad has two catch clauses, it only has one action because both
 ;       handlers do the same thing.
 ; CHECK: [[LPAD1_LABEL]]:
 ; CHECK:   landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
 ; CHECK-NEXT:           catch %eh.CatchHandlerType* @llvm.eh.handlertype.H.0
 ; CHECK-NEXT:           catch i8* null
-; CHECK-NEXT:   [[RECOVER:\%.+]] = call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* (i8*, i8*)* @"\01?f@@YAXXZ.catch1")
+; CHECK-NEXT:   [[RECOVER:\%.+]] = call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* (i8*, i8*)* @"\01?f@@YAXXZ.catch")
 ; CHECK-NEXT:   indirectbr i8* [[RECOVER]], [label %try.cont4]
 
 lpad1:                                            ; preds = %invoke.cont
   %2 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
           catch %eh.CatchHandlerType* @llvm.eh.handlertype.H.0
           catch i8* null
   %3 = extractvalue { i8*, i32 } %2, 0
   br label %catch2

llvm/trunk/test/CodeGen/WinEH/seh-catch-all.ll

 
 __try.cont:                                       ; preds = %__except, %invoke.cont
   ret void
 }
 
 ; CHECK-LABEL: define void @seh_catch_all()
 ; CHECK: landingpad
 ; CHECK-NEXT: catch i8* null
-; CHECK-NEXT: call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* blockaddress(@seh_catch_all, %catch.all))
+; CHECK-NEXT: call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* blockaddress(@seh_catch_all, %lpad.split))
 ; CHECK-NEXT: indirectbr
 ;
-; CHECK: catch.all:
+; CHECK: lpad.split:
 ; CHECK-NOT: extractvalue
 ; CHECK: call i32 @puts

llvm/trunk/test/CodeGen/WinEH/seh-simple.ll

 return:
   %r = load i32, i32* %retval
   ret i32 %r
 }
 
 ; CHECK-LABEL: define i32 @catch_all()
 ; CHECK: landingpad { i8*, i32 }
 ; CHECK-NEXT: catch i8* null
-; CHECK-NEXT: call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* blockaddress(@catch_all, %catch.all))
-; CHECK-NEXT: indirectbr {{.*}} [label %catch.all]
+; CHECK-NEXT: call i8* (...) @llvm.eh.actions(i32 1, i8* null, i32 -1, i8* blockaddress(@catch_all, %lpad.split))
+; CHECK-NEXT: indirectbr {{.*}} [label %lpad.split]
 ;
-; CHECK: catch.all:
+; CHECK: lpad.split:
 ; CHECK: store i32 1, i32* %retval
 
 
 define i32 @except_phi() {
 entry:
   invoke void @might_crash()
           to label %return unwind label %lpad