Use WinEHPrepare to outline SEH finally blocks

[rnk]

Summary

No outlining is necessary for SEH catch blocks. Use the blockaddr of the
handler in place of the usual outlined function.

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

94

We should probably add comments to say which of these fields (I guess all of them currently) are reset with each Function the pass is run on.

465

What exactly does this do? I've seen some cases recently where cleanup outlining is choking because of PHINodes that are referring to the return value of the landingpad instruction. This is too late to handle that (which just needs a few more VMap entries before outlining) but it raises the question in my mind of general handling of PHINodes which use values that are being erased from the original landing pads here.

I think that all of the code in question will be pruned as part of what we're doing here (it was resume blocks using either the landingpad value or values extracted from it). I'm just raising the issue to see if you can think of anything else that might have a similar issue.

474

This should be 1 according to your recent documentation.

482

This should be 0.

728

In the case of "__except(EXCEPTION_EXECUTE_HANDLER)" I'm seeing the landing pad branch unconditionally to the catch handler.

[rnk]

I'm going to take a shot at using StartBB instead of using HandlerBB like I did in this patch.

lib/CodeGen/WinEHPrepare.cpp

94

Yeah, all of em. :)

465

I was trying to handle except_phi test case:

define i32 @except_phi() {
entry:
  invoke void @might_crash()
          to label %return unwind label %lpad

lpad:
  %ehvals = landingpad { i8*, i32 } personality i32 (...)* @__C_specific_handler
          catch i32 ()* @filt
  %sel = extractvalue { i8*, i32 } %ehvals, 1
  %filt_sel = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i32 ()* @filt to i8*))
  %matches = icmp eq i32 %sel, %filt_sel
  br i1 %matches, label %return, label %eh.resume

return:
  %r = phi i32 [0, %entry], [1, %lpad]
  ret i32 %r

eh.resume:
  resume { i8*, i32 } %ehvals
}

I needed to update the %r phi node. Maybe what I should do instead is split the landingpad block directly, because we might have this kind of code:

define i32 @except_phi(i32 %a) {
entry:
  invoke void @might_crash()
          to label %return unwind label %lpad

lpad:
  %ehvals = landingpad { i8*, i32 } personality i32 (...)* @__C_specific_handler
          catch i32 ()* @filt
  %sel = extractvalue { i8*, i32 } %ehvals, 1
  %a1 = add i32 %a, 1 ; live SSA value will become unreachable.
  %filt_sel = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i32 ()* @filt to i8*))
  %matches = icmp eq i32 %sel, %filt_sel
  br i1 %matches, label %return, label %eh.resume

return:
  %r = phi i32 [%a, %entry], [%a1, %lpad]
  ret i32 %r

eh.resume:
  resume { i8*, i32 } %ehvals
}

The pass as is will probably crash. I'm OK with that for now, honestly.

474

I'd rather make that a separate change.

728

Right, any catch-all block will ignore the selector and unconditionally branch into the recovery. I haven't handled that yet. I think a better approach might be to keep StartBB and simply discard all EH related instructions. This would also fix the phi case.

[rnk]

I added support for catch-all, but I didn't end up handling the case where the catch BB contains interesting code. My reasoning was that EH preparation should be identifying that as a cleanup, and we'll need to do something to handle it for C++.

[andrew.w.kaylor]

I see just one problem with this.

llvm/trunk/lib/CodeGen/WinEHPrepare.cpp

1194 ↗

(On Diff #22206)

This is wrong for the C++ EH case, which can have cleanup code to be executed after a catch-all handler. Setting BB to NULL will prevent use from finding the cleanup in that scenario.

[rnk]

llvm/trunk/lib/CodeGen/WinEHPrepare.cpp

1194 ↗

(On Diff #22206)

Can you elaborate? Is this a cleanup inside the catch, or outside the catch? I don't think it's possible to have exceptional cleanups after a catchall:

void f() {
  HasDtor o;
  try { g(); }
  catch (...) { g(); }
}

[andrew.w.kaylor]

Actually, I think I was wrong about the clean-up after catch-all. I thought I had a case that would do it, but it doesn't and I think I've convinced myself that it can't happen.

lib/CodeGen/WinEHPrepare.cpp

 #include "llvm/Analysis/LibCallSemantics.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Pass.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <memory>
 
 using namespace llvm;
 using namespace llvm::PatternMatch;
public:
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   const char *getPassName() const override {
     return "Windows exception handling preparation";
   }
 
 private:
-  bool prepareCPPEHHandlers(Function &F,
+  bool prepareExceptionHandlers(Function &F,
                                 SmallVectorImpl<LandingPadInst *> &LPads);
   bool outlineHandler(ActionHandler *Action, Function *SrcFn,
                       LandingPadInst *LPad, BasicBlock *StartBB,
                       FrameVarInfoMap &VarInfo);
 
   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);
+
+  EHPersonality Personality;
   CatchHandlerMapTy CatchHandlerMap;
   CleanupHandlerMapTy CleanupHandlerMap;
   DenseMap<const LandingPadInst *, LandingPadMap>  LPadMaps;
 };
 
 class WinEHFrameVariableMaterializer : public ValueMaterializer {
 public:
   WinEHFrameVariableMaterializer(Function *OutlinedFn,
CloningAction handleInvoke(ValueToValueMapTy &VMap, const InvokeInst *Invoke,
                              BasicBlock *NewBB) override;
   CloningAction handleResume(ValueToValueMapTy &VMap, const ResumeInst *Resume,
                              BasicBlock *NewBB) override;
 };
 
 class ActionHandler {
 public:
   ActionHandler(BasicBlock *BB, ActionType Type)
-      : StartBB(BB), Type(Type), OutlinedFn(nullptr) {}
+      : StartBB(BB), Type(Type), HandlerBlockOrFunc(nullptr) {}
 
   ActionType getType() const { return Type; }
   BasicBlock *getStartBlock() const { return StartBB; }
 
-  bool hasBeenOutlined() { return OutlinedFn != nullptr; }
+  bool hasBeenProcessed() { return HandlerBlockOrFunc != nullptr; }
 
-  void setOutlinedFunction(Function *F) { OutlinedFn = F; }
-  Function *getOutlinedFunction() { return OutlinedFn; }
+  void setHandlerBlockOrFunc(Constant *F) { HandlerBlockOrFunc = F; }
+  Constant *getHandlerBlockOrFunc() { return HandlerBlockOrFunc; }
 
 private:
   BasicBlock *StartBB;
   ActionType Type;
-  Function *OutlinedFn;
+
+  // Can be either a BlockAddress or a Function depending on the EH personality.
+  Constant *HandlerBlockOrFunc;
 };
 
 class CatchHandler : public ActionHandler {
 public:
   CatchHandler(BasicBlock *BB, Constant *Selector, BasicBlock *NextBB)
       : ActionHandler(BB, ActionType::Catch), Selector(Selector),
         NextBB(NextBB), ExceptionObjectVar(nullptr) {}
 

 char WinEHPrepare::ID = 0;
 INITIALIZE_TM_PASS(WinEHPrepare, "winehprepare", "Prepare Windows exceptions",
                    false, false)
 
 FunctionPass *llvm::createWinEHPass(const TargetMachine *TM) {
   return new WinEHPrepare(TM);
 }
 
+// FIXME: Remove this once the backend can handle the prepared IR.
+static cl::opt<bool>
+SEHPrepare("sehprepare", cl::Hidden,
+           cl::desc("Prepare functions with SEH personalities"));
+
 bool WinEHPrepare::runOnFunction(Function &Fn) {
   SmallVector<LandingPadInst *, 4> LPads;
   SmallVector<ResumeInst *, 4> Resumes;
   for (BasicBlock &BB : Fn) {
     if (auto *LP = BB.getLandingPadInst())
       LPads.push_back(LP);
     if (auto *Resume = dyn_cast<ResumeInst>(BB.getTerminator()))
       Resumes.push_back(Resume);
   }
 
   // No need to prepare functions that lack landing pads.
   if (LPads.empty())
     return false;
 
   // Classify the personality to see what kind of preparation we need.
-  EHPersonality Pers = classifyEHPersonality(LPads.back()->getPersonalityFn());
+  Personality = classifyEHPersonality(LPads.back()->getPersonalityFn());
 
   // Do nothing if this is not an MSVC personality.
-  if (!isMSVCEHPersonality(Pers))
-    return false;
-
-  // FIXME: This only returns true if the C++ EH handlers were outlined.
-  //        When that code is complete, it should always return whatever
-  //        prepareCPPEHHandlers returns.
-  if (Pers == EHPersonality::MSVC_CXX && prepareCPPEHHandlers(Fn, LPads))
-    return true;
-
-  // FIXME: SEH Cleanups are unimplemented. Replace them with unreachable.
-  if (Resumes.empty())
+  if (!isMSVCEHPersonality(Personality))
     return false;
 
+  if (isAsynchronousEHPersonality(Personality) && !SEHPrepare) {
+    // Replace all resume instructions with unreachable.
+    // FIXME: Remove this once the backend can handle the prepared IR.
     for (ResumeInst *Resume : Resumes) {
       IRBuilder<>(Resume).CreateUnreachable();
       Resume->eraseFromParent();
     }
+    return true;
+  }
 
+  // If there were any landing pads, prepareExceptionHandlers will make changes.
+  prepareExceptionHandlers(Fn, LPads);
   return true;
 }
 
 bool WinEHPrepare::doFinalization(Module &M) {
   return false;
 }
 
 void WinEHPrepare::getAnalysisUsage(AnalysisUsage &AU) const {}
 
-bool WinEHPrepare::prepareCPPEHHandlers(
+bool WinEHPrepare::prepareExceptionHandlers(
     Function &F, SmallVectorImpl<LandingPadInst *> &LPads) {
   // These containers are used to re-map frame variables that are used in
   // outlined catch and cleanup handlers.  They will be populated as the
   // handlers are outlined.
   FrameVarInfoMap FrameVarInfo;
 
   bool HandlersOutlined = false;
 
for (LandingPadInst *LPad : LPads) {
     // there's nothing more to do here.
     if (LPadHasActionList)
       continue;
 
     LandingPadActions Actions;
     mapLandingPadBlocks(LPad, Actions);
 
     for (ActionHandler *Action : Actions) {
-      if (Action->hasBeenOutlined())
+      if (Action->hasBeenProcessed())
         continue;
       BasicBlock *StartBB = Action->getStartBlock();
+
+      // SEH doesn't do any outlining for catches. Instead, pass the handler
+      // basic block addr to llvm.eh.actions and list the block as a return
+      // target.
+      if (isAsynchronousEHPersonality(Personality)) {
+        if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
+          processSEHCatchHandler(CatchAction, StartBB);
+          HandlersOutlined = true;
+          continue;
+        }
+      }
+
       if (outlineHandler(Action, &F, LPad, StartBB, FrameVarInfo)) {
         HandlersOutlined = true;
       }
     } // End for each Action
 
     // FIXME: We need a guard against partially outlined functions.
     if (!HandlersOutlined)
       continue;
 
     // Replace the landing pad with a new llvm.eh.action based landing pad.
     BasicBlock *NewLPadBB = BasicBlock::Create(Context, "lpad", &F, LPadBB);
     assert(!isa<PHINode>(LPadBB->begin()));
     Instruction *NewLPad = LPad->clone();
     NewLPadBB->getInstList().push_back(NewLPad);
     while (!pred_empty(LPadBB)) {
       auto *pred = *pred_begin(LPadBB);
       InvokeInst *Invoke = cast<InvokeInst>(pred->getTerminator());
       Invoke->setUnwindDest(NewLPadBB);
     }
 
+    // Replace uses of the old lpad in phis with this block and delete the old
+    // block.
+    LPadBB->replaceSuccessorsPhiUsesWith(NewLPadBB);
+    LPadBB->getTerminator()->eraseFromParent();
+    new UnreachableInst(LPadBB->getContext(), LPadBB);
+
     // Add a call to describe the actions for this landing pad.
     std::vector<Value *> ActionArgs;
     ActionArgs.push_back(NewLPad);
     for (ActionHandler *Action : Actions) {
       if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
         ActionArgs.push_back(ConstantInt::get(Int32Type, 0));
         ActionArgs.push_back(CatchAction->getSelector());
         Value *EHObj = const_cast<Value *>(CatchAction->getExceptionVar());
         if (EHObj)
           ActionArgs.push_back(EHObj);
         else
           ActionArgs.push_back(ConstantPointerNull::get(Int8PtrType));
       } else {
         ActionArgs.push_back(ConstantInt::get(Int32Type, 1));
       }
-      Constant *HandlerPtr =
-          ConstantExpr::getBitCast(Action->getOutlinedFunction(), Int8PtrType);
+      Constant *HandlerPtr = ConstantExpr::getBitCast(
+          Action->getHandlerBlockOrFunc(), Int8PtrType);
       ActionArgs.push_back(HandlerPtr);
     }
     CallInst *Recover =
         CallInst::Create(ActionIntrin, ActionArgs, "recover", NewLPadBB);
 
     // Add an indirect branch listing possible successors of the catch handlers.
     IndirectBrInst *Branch = IndirectBrInst::Create(Recover, 0, NewLPadBB);
     for (ActionHandler *Action : Actions) {
bool WinEHPrepare::outlineHandler(ActionHandler *Action, Function *SrcFn,
 
   std::unique_ptr<WinEHCloningDirectorBase> Director;
 
   ValueToValueMapTy VMap;
 
   LandingPadMap &LPadMap = LPadMaps[LPad];
   if (!LPadMap.isInitialized())
     LPadMap.mapLandingPad(LPad);
-  if (Action->getType() == Catch) {
-    Constant *SelectorType = cast<CatchHandler>(Action)->getSelector();
-    Director.reset(
-        new WinEHCatchDirector(Handler, SelectorType, VarInfo, LPadMap));
-    LPadMap.remapSelector(VMap, ConstantInt::get( Type::getInt32Ty(Context), 1));
+  if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
+    Constant *Sel = CatchAction->getSelector();
+    Director.reset(new WinEHCatchDirector(Handler, Sel, VarInfo, LPadMap));
+    LPadMap.remapSelector(VMap, ConstantInt::get(Type::getInt32Ty(Context), 1));
   } else {
     Director.reset(new WinEHCleanupDirector(Handler, VarInfo, LPadMap));
   }
 
   SmallVector<ReturnInst *, 8> Returns;
   ClonedCodeInfo OutlinedFunctionInfo;
 
   // Skip over PHIs and, if applicable, landingpad instructions.
bool WinEHPrepare::outlineHandler(ActionHandler *Action, Function *SrcFn,
 
   if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
     WinEHCatchDirector *CatchDirector =
         reinterpret_cast<WinEHCatchDirector *>(Director.get());
     CatchAction->setExceptionVar(CatchDirector->getExceptionVar());
     CatchAction->setReturnTargets(CatchDirector->getReturnTargets());
   }
 
-  Action->setOutlinedFunction(Handler);
+  Action->setHandlerBlockOrFunc(Handler);
 
   return true;
 }
 
+/// This BB must end in a selector dispatch. All we need to do is pass the
+/// handler block to llvm.eh.actions and list it as a possible indirectbr
+/// target.
+void WinEHPrepare::processSEHCatchHandler(CatchHandler *CatchAction,
+                                          BasicBlock *StartBB) {
+  BasicBlock *HandlerBB;
+  BasicBlock *NextBB;
+  Constant *Selector;
+  bool Res = isSelectorDispatch(StartBB, HandlerBB, Selector, NextBB);
+  (void)Res;
+  assert(Res && HandlerBB && "mis-identified catch block?");
+  CatchAction->setHandlerBlockOrFunc(BlockAddress::get(HandlerBB));
+  TinyPtrVector<BasicBlock *> Targets(HandlerBB);
+  CatchAction->setReturnTargets(Targets);
+}
+
 void LandingPadMap::mapLandingPad(const LandingPadInst *LPad) {
   // Each instance of this class should only ever be used to map a single
   // landing pad.
   assert(OriginLPad == nullptr || OriginLPad == LPad);
 
   // If the landing pad has already been mapped, there's nothing more to do.
   if (OriginLPad == LPad)
     return;
while (BB) {
     //   %exn2 = load i8** %exn.slot
     //   %sel2 = load i32* %ehselector.slot
     //   %lpad.val1 = insertvalue { i8*, i32 } undef, i8* %exn2, 0
     //   %lpad.val2 = insertvalue { i8*, i32 } %lpad.val1, i32 %sel2, 1
     //   resume { i8*, i32 } %lpad.val2
     if (auto *Resume = dyn_cast<ResumeInst>(Terminator)) {
       InsertValueInst *Insert1 = nullptr;
       InsertValueInst *Insert2 = nullptr;
-      if (!isa<PHINode>(Resume->getOperand(0))) {
-        Insert2 = dyn_cast<InsertValueInst>(Resume->getOperand(0));
+      Value *ResumeVal = Resume->getOperand(0);
+      // If there is only one landingpad, we may use the lpad directly with no
+      // insertions.
+      if (isa<LandingPadInst>(ResumeVal))
+        return nullptr;
+      if (!isa<PHINode>(ResumeVal)) {
+        Insert2 = dyn_cast<InsertValueInst>(ResumeVal);
         if (!Insert2)
           return createCleanupHandler(CleanupHandlerMap, BB);
         Insert1 = dyn_cast<InsertValueInst>(Insert2->getAggregateOperand());
         if (!Insert1)
           return createCleanupHandler(CleanupHandlerMap, BB);
       }
       for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(), IE = BB->end();
            II != IE; ++II) {

test/CodeGen/WinEH/seh-simple.ll

+; RUN: opt -S -winehprepare -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare void @cleanup()
+declare i32 @filt()
+declare void @might_crash()
+declare i32 @__C_specific_handler(...)
+declare i32 @llvm.eh.typeid.for(i8*)
+
+define i32 @simple_except_store() {
+entry:
+  %retval = alloca i32
+  store i32 0, i32* %retval
+  invoke void @might_crash()
+          to label %return unwind label %lpad
+
+lpad:
+  %ehvals = landingpad { i8*, i32 } personality i32 (...)* @__C_specific_handler
+          catch i32 ()* @filt
+  %sel = extractvalue { i8*, i32 } %ehvals, 1
+  %filt_sel = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i32 ()* @filt to i8*))
+  %matches = icmp eq i32 %sel, %filt_sel
+  br i1 %matches, label %__except, label %eh.resume
+
+__except:
+  store i32 1, i32* %retval
+  br label %return
+
+return:
+  %r = load i32, i32* %retval
+  ret i32 %r
+
+eh.resume:
+  resume { i8*, i32 } %ehvals
+}
+
+; CHECK-LABEL: define i32 @simple_except_store()
+; CHECK: landingpad { i8*, i32 }
+; CHECK-NEXT: catch i32 ()* @filt
+; CHECK-NEXT: call i8* (...)* @llvm.eh.actions({{.*}}, i32 0, i8* bitcast (i32 ()* @filt to i8*), i8* null, i8* blockaddress(@simple_except_store, %__except))
+; CHECK-NEXT: indirectbr {{.*}} [label %__except]
+
+define i32 @except_phi() {
+entry:
+  invoke void @might_crash()
+          to label %return unwind label %lpad
+
+lpad:
+  %ehvals = landingpad { i8*, i32 } personality i32 (...)* @__C_specific_handler
+          catch i32 ()* @filt
+  %sel = extractvalue { i8*, i32 } %ehvals, 1
+  %filt_sel = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i32 ()* @filt to i8*))
+  %matches = icmp eq i32 %sel, %filt_sel
+  br i1 %matches, label %return, label %eh.resume
+
+return:
+  %r = phi i32 [0, %entry], [1, %lpad]
+  ret i32 %r
+
+eh.resume:
+  resume { i8*, i32 } %ehvals
+}
+
+; CHECK-LABEL: define i32 @except_phi()
+; CHECK: landingpad { i8*, i32 }
+; CHECK-NEXT: catch i32 ()* @filt
+; CHECK-NEXT: call i8* (...)* @llvm.eh.actions({{.*}}, i32 0, i8* bitcast (i32 ()* @filt to i8*), i8* null, i8* blockaddress(@except_phi, %return))
+; CHECK-NEXT: indirectbr {{.*}} [label %return]
+;
+; CHECK: return:
+; CHECK-NEXT: %r = phi i32 [ 0, %entry ], [ 1, %lpad1 ]
+; CHECK-NEXT: ret i32 %r
+
+define i32 @cleanup_and_except() {
+entry:
+  invoke void @might_crash()
+          to label %return unwind label %lpad
+
+lpad:
+  %ehvals = landingpad { i8*, i32 } personality i32 (...)* @__C_specific_handler
+    cleanup
+          catch i32 ()* @filt
+  call void @cleanup()
+  %sel = extractvalue { i8*, i32 } %ehvals, 1
+  %filt_sel = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i32 ()* @filt to i8*))
+  %matches = icmp eq i32 %sel, %filt_sel
+  br i1 %matches, label %return, label %eh.resume
+
+return:
+  %r = phi i32 [0, %entry], [1, %lpad]
+  ret i32 %r
+
+eh.resume:
+  resume { i8*, i32 } %ehvals
+}
+
+; CHECK-LABEL: define i32 @cleanup_and_except()
+; CHECK: landingpad { i8*, i32 }
+; CHECK-NEXT: cleanup
+; CHECK-NEXT: catch i32 ()* @filt
+; CHECK-NEXT: call i8* (...)* @llvm.eh.actions(
+; CHECK: i32 1, i8* bitcast (void (i8*, i8*)* @cleanup_and_except.cleanup to i8*),
+; CHECK: i32 0, i8* bitcast (i32 ()* @filt to i8*), i8* null, i8* blockaddress(@cleanup_and_except, %return))
+; CHECK-NEXT: indirectbr {{.*}} [label %return]
+;
+; CHECK: return:
+; CHECK-NEXT: %r = phi i32 [ 0, %entry ], [ 1, %lpad1 ]
+; CHECK-NEXT: ret i32 %r