Fix WinEHPrepare bug with multiple catch handlers

[andrew.w.kaylor]

Summary

This fixes the bug with multiple catch handlers at the same level. The test case looks like this:

void f() {
  try {
    may_throw();
  }
  catch (int) {
  }
  catch (float) {
  }
}

With this IR:

; Function Attrs: uwtable
define void @"\01?f@@YAXXZ"() #0 {
entry:
  %exn.slot = alloca i8*
  %ehselector.slot = alloca i32
  %0 = alloca float, align 4
  %1 = alloca i32, align 4
  invoke void @"\01?may_throw@@YAXXZ"()
          to label %invoke.cont unwind label %lpad

invoke.cont:                                      ; preds = %entry
  br label %try.cont

lpad:                                             ; preds = %entry
  %2 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.H
          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.M
  %3 = extractvalue { i8*, i32 } %2, 0
  store i8* %3, i8** %exn.slot
  %4 = extractvalue { i8*, i32 } %2, 1
  store i32 %4, i32* %ehselector.slot
  br label %catch.dispatch

catch.dispatch:                                   ; preds = %lpad
  %sel = load i32, i32* %ehselector.slot
  %5 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.H to i8*)) #3
  %matches = icmp eq i32 %sel, %5
  br i1 %matches, label %catch2, label %catch.fallthrough

catch2:                                           ; preds = %catch.dispatch
  %exn3 = load i8*, i8** %exn.slot
  %6 = bitcast i32* %1 to i8*
  call void @llvm.eh.begincatch(i8* %exn3, i8* %6) #3
  call void @llvm.eh.endcatch() #3
  br label %try.cont

try.cont:                                         ; preds = %catch2, %catch, %invoke.cont
  ret void

catch.fallthrough:                                ; preds = %catch.dispatch
  %7 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.M to i8*)) #3
  %matches1 = icmp eq i32 %sel, %7
  br i1 %matches1, label %catch, label %eh.resume

catch:                                            ; preds = %catch.fallthrough
  %exn = load i8*, i8** %exn.slot
  %8 = bitcast float* %0 to i8*
  call void @llvm.eh.begincatch(i8* %exn, i8* %8) #3
  call void @llvm.eh.endcatch() #3
  br label %try.cont

eh.resume:                                        ; preds = %catch.fallthrough
  %exn4 = load i8*, i8** %exn.slot
  %sel5 = load i32, i32* %ehselector.slot
  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn4, 0
  %lpad.val6 = insertvalue { i8*, i32 } %lpad.val, i32 %sel5, 1
  resume { i8*, i32 } %lpad.val6
}

It's a small fix, but I thought it was worth pausing to talk about because the code involved seems potentially brittle.

The problem in this case was that the landing pad selector value was being loaded in one catch dispatch block but being referenced in two blocks. Neither the landing pad block mapping code nor the catch handling code were expecting this. For this case it was fairly simple to extend the code to recognize what was happening, but it raises a concern in my mind that the selector value, which we must be able to recognize for this pass to work properly, can be shuffled around through any manner of transformation. I can't see why it would be manipulated, beyond the basic optimizations to avoid storing and reloading it, but it can happen.

Do you think this needs to be more robust?

[rnk]

Test case?

I was on vacation Thurs/Fri, so I'm a little slow today.

[andrew.w.kaylor]

I've got mixed feelings about test cases for the kind of bugs that are showing up at this stage of the implementation. I tested this with the case shown in the comments, but I'm not sure we want to clutter the test suite with a large variety of trivial cases. I think that it would be better to have a more robust suite of tests later that would include things like this. That said, I'm willing to add the trivial test cases if the consensus is that we should do it that way..

[andrew.w.kaylor]

Adding a fix for an unrelated problem where an llvm.eh.endcatch call is not immediately followed by an unconditional branch instruction:

define void @f() {
entry:
  invoke void @f() to label %try.cont unwind label %lpad

lpad:                                             ; preds = %entry
  %0 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
          catch i8* null
  %1 = extractvalue { i8*, i32 } %0, 0
  tail call void @llvm.eh.begincatch(i8* %1, i8* null) #0
  tail call void @llvm.eh.endcatch() #0
  ret void

try.cont:                                         ; preds = %entry
  ret void
}

I considered splitting the block in the mapLandingPadBlocks code, but that involved extra searching for the endcatch calls, while this code already has the needed location.

[rnk]

I understand why we need to split the block after llvm.eh.endcatch, but I don't really see why we should be treating icmps specially. Maybe we should pre-populate the value map with all dominating EH value stores instead? I'm looking at the broken test case and looking at how to fix it.

Feel free to commit the basic block splitting separately.

[rnk]

I agree we shouldn't clutter the test suite with too many trivial cases, but I think a single try with multiple catches is an interesting basic case that we should handle.

[andrew.w.kaylor]

That's pretty much why I put this up for review. I'm not entirely happy with this solution to the original problem (referencing a loaded EH selector value in a block whose cloning director didn't see it being loaded). The problem arises from the decision not to start cloning at the original landing pad block. The mapLandingPadUsers code tries to handle this sort of thing by looking at all the places where the original values are stored, but if there's an efficient way to find all of the places where it is loaded again, I'm not aware of it.

The way that the cloning works, the first time we're forced to deal with the unrecognized %sel value would be in the materializeValueFor() method and by then it's too late (unless we shared the mapped landing pad values with the materializer and that seems very ugly).

Of course, if we want to be entirely bullet-proof the problem is even worse. You can imagine all sorts of ways that a selector value could be manipulated such that the handler cloning code isn't even looking at a load of the original stored value. In practice, the only things that I would ever expect to happen to the selector value are (1) extract from the aggregate, (2) insert into another aggregate value, (3) stored to a memory location, (4) loaded from a memory location, (5) compared with a result of llvm.eh.typeid.for(). I'm just not sure it's reasonable to impose this as a limitation.

[rnk]

Here's an idea. What if before outlining, we run mem2reg (see llvm::PromoteMemToReg) on all EH value objects? This should greatly simplify the existing preparation code, because we won't need to track stores of selectors and EH pointers anymore. Even at -O0, these allocas are not visible to the user, so removing them is useful. If the frontend produces IR that fails to promote, I think it's totally reasonable for WinEHPrepare to assert.

[andrew.w.kaylor]

That sounds pretty reasonable. I'll give it a try and add test cases.

[andrew.w.kaylor]

Added code to promote extracted landing pad values to registers.
Removed code which was made obsolete by the above change.
Added a test case.

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

1418 ↗

(On Diff #23017)

Oops. This shouldn't be here. I forgot to delete it when testing proved it wasn't needed.

1449 ↗

(On Diff #23017)

Again, ignore this.

[rnk]

LGTM with the #if 0 code removed. The promotion seems like a nice simplification. :)

lib/CodeGen/WinEHPrepare.cpp

371 ↗

(On Diff #23017)

My understanding is that you are also required to change the INITIALIZE_TM_PASS code to use INITIALIZE_TM_PASS_BEGIN / END and INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass). See DwarfEHPrepare. I could be wrong, the pass registry stuff is all ugly goo that will hopefully be removed when the new pass manager happens.

[rnk]

(Setting the "accept revision" phab bit this time...)

One other thing worth thinking about is remapping phis of EH values in situations like this:

void might_throw();
void f() {
  try {
    try { might_throw(); } catch (int) { }
    try { might_throw(); } catch (int) { }
  } catch (int) {
  }
}

In this situation, there should be two landingpads, and the outermost EH dispatch block will use a phi of the two lpad selector values.

We can handle this later by recursively adding phis of selector values to ExtractedSelectors.

llvm/trunk/lib/CodeGen/WinEHPrepare.cpp

 
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Analysis/LibCallSemantics.h"
 #include "llvm/CodeGen/WinEHFuncInfo.h"
+#include "llvm/IR/Dominators.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/Support/raw_ostream.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Local.h"
+#include "llvm/Transforms/Utils/PromoteMemToReg.h"
 #include <memory>
 
 using namespace llvm;
 using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "winehprepare"
 
 namespace {

 
 typedef DenseMap<const BasicBlock *, CatchHandler *> CatchHandlerMapTy;
 typedef DenseMap<const BasicBlock *, CleanupHandler *> CleanupHandlerMapTy;
 
 class WinEHPrepare : public FunctionPass {
 public:
   static char ID; // Pass identification, replacement for typeid.
   WinEHPrepare(const TargetMachine *TM = nullptr)
-      : FunctionPass(ID) {}
+      : FunctionPass(ID), DT(nullptr) {}
 
   bool runOnFunction(Function &Fn) override;
 
   bool doFinalization(Module &M) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   const char *getPassName() const override {
     return "Windows exception handling preparation";
   }
 
 private:
   bool prepareExceptionHandlers(Function &F,
                                 SmallVectorImpl<LandingPadInst *> &LPads);
+  void promoteLandingPadValues(LandingPadInst *LPad);
   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);
 
   // All fields are reset by runOnFunction.
+  DominatorTree *DT;
   EHPersonality Personality;
   CatchHandlerMapTy CatchHandlerMap;
   CleanupHandlerMapTy CleanupHandlerMap;
-  DenseMap<const LandingPadInst *, LandingPadMap>  LPadMaps;
+  DenseMap<const LandingPadInst *, LandingPadMap> LPadMaps;
 };
 
 class WinEHFrameVariableMaterializer : public ValueMaterializer {
 public:
   WinEHFrameVariableMaterializer(Function *OutlinedFn,
                                  FrameVarInfoMap &FrameVarInfo);
   ~WinEHFrameVariableMaterializer() {}
 
   virtual Value *materializeValueFor(Value *V) override;
 
 private:
   FrameVarInfoMap &FrameVarInfo;
   IRBuilder<> Builder;
 };
 
 class LandingPadMap {
 public:
   LandingPadMap() : OriginLPad(nullptr) {}
   void mapLandingPad(const LandingPadInst *LPad);
 
   bool isInitialized() { return OriginLPad != nullptr; }
 
-  bool mapIfEHPtrLoad(const LoadInst *Load) {
-    return mapIfEHLoad(Load, EHPtrStores, EHPtrStoreAddrs);
-  }
-  bool mapIfSelectorLoad(const LoadInst *Load) {
-    return mapIfEHLoad(Load, SelectorStores, SelectorStoreAddrs);
-  }
-
   bool isOriginLandingPadBlock(const BasicBlock *BB) const;
   bool isLandingPadSpecificInst(const Instruction *Inst) const;
 
-  void remapSelector(ValueToValueMapTy &VMap, Value *MappedValue) const;
+  void remapEHValues(ValueToValueMapTy &VMap, Value *EHPtrValue,
+                     Value *SelectorValue) const;
 
 private:
-  bool mapIfEHLoad(const LoadInst *Load,
-                   SmallVectorImpl<const StoreInst *> &Stores,
-                   SmallVectorImpl<const Value *> &StoreAddrs);
-
   const LandingPadInst *OriginLPad;
   // We will normally only see one of each of these instructions, but
   // if more than one occurs for some reason we can handle that.
   TinyPtrVector<const ExtractValueInst *> ExtractedEHPtrs;
   TinyPtrVector<const ExtractValueInst *> ExtractedSelectors;
-
-  // In optimized code, there will typically be at most one instance of
-  // each of the following, but in unoptimized IR it is not uncommon
-  // for the values to be stored, loaded and then stored again.  In that
-  // case we will create a second entry for each store and store address.
-  SmallVector<const StoreInst *, 2> EHPtrStores;
-  SmallVector<const StoreInst *, 2> SelectorStores;
-  SmallVector<const Value *, 2> EHPtrStoreAddrs;
-  SmallVector<const Value *, 2> SelectorStoreAddrs;
 };
 
 class WinEHCloningDirectorBase : public CloningDirector {
 public:
   WinEHCloningDirectorBase(Function *HandlerFn,
                            FrameVarInfoMap &VarInfo,
                            LandingPadMap &LPadMap)
       : Materializer(HandlerFn, VarInfo),
bool WinEHPrepare::runOnFunction(Function &Fn) {
 
   // Classify the personality to see what kind of preparation we need.
   Personality = classifyEHPersonality(LPads.back()->getPersonalityFn());
 
   // Do nothing if this is not an MSVC personality.
   if (!isMSVCEHPersonality(Personality))
     return false;
 
+  DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
+
   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 {}
+void WinEHPrepare::getAnalysisUsage(AnalysisUsage &AU) const {
+  AU.addRequired<DominatorTreeWrapperPass>();
+}
 
 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;
 
for (Instruction &Inst : *LPadBB) {
       }
     }
 
     // If we've already outlined the handlers for this landingpad,
     // there's nothing more to do here.
     if (LPadHasActionList)
       continue;
 
+    // If either of the values in the aggregate returned by the landing pad is
+    // extracted and stored to memory, promote the stored value to a register.
+    promoteLandingPadValues(LPad);
+
     LandingPadActions Actions;
     mapLandingPadBlocks(LPad, Actions);
 
     for (ActionHandler *Action : Actions) {
       if (Action->hasBeenProcessed())
         continue;
       BasicBlock *StartBB = Action->getStartBlock();
 
bool WinEHPrepare::prepareExceptionHandlers(
   DeleteContainerSeconds(CatchHandlerMap);
   CatchHandlerMap.clear();
   DeleteContainerSeconds(CleanupHandlerMap);
   CleanupHandlerMap.clear();
 
   return HandlersOutlined;
 }
 
+void WinEHPrepare::promoteLandingPadValues(LandingPadInst *LPad) {
+  // If the return values of the landing pad instruction are extracted and
+  // stored to memory, we want to promote the store locations to reg values.
+  SmallVector<AllocaInst *, 2> EHAllocas;
+
+  // The landingpad instruction returns an aggregate value.  Typically, its
+  // value will be passed to a pair of extract value instructions and the
+  // results of those extracts are often passed to store instructions.
+  // In unoptimized code the stored value will often be loaded and then stored
+  // again.
+  for (auto *U : LPad->users()) {
+    ExtractValueInst *Extract = dyn_cast<ExtractValueInst>(U);
+    if (!Extract)
+      continue;
+
+    for (auto *EU : Extract->users()) {
+      if (auto *Store = dyn_cast<StoreInst>(EU)) {
+        auto *AV = cast<AllocaInst>(Store->getPointerOperand());
+        EHAllocas.push_back(AV);
+      }
+    }
+  }
+
+  // We can't do this without a dominator tree.
+  assert(DT);
+
+  if (!EHAllocas.empty()) {
+    PromoteMemToReg(EHAllocas, *DT);
+    EHAllocas.clear();
+  }
+}
+
 // This function examines a block to determine whether the block ends with a
 // conditional branch to a catch handler based on a selector comparison.
 // This function is used both by the WinEHPrepare::findSelectorComparison() and
 // WinEHCleanupDirector::handleTypeIdFor().
 static bool isSelectorDispatch(BasicBlock *BB, BasicBlock *&CatchHandler,
                                Constant *&Selector, BasicBlock *&NextBB) {
   ICmpInst::Predicate Pred;
   BasicBlock *TBB, *FBB;
bool WinEHPrepare::outlineHandler(ActionHandler *Action, Function *SrcFn,
   ValueToValueMapTy VMap;
 
   LandingPadMap &LPadMap = LPadMaps[LPad];
   if (!LPadMap.isInitialized())
     LPadMap.mapLandingPad(LPad);
   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));
+    LPadMap.remapEHValues(VMap, UndefValue::get(Int8PtrType),
+                          ConstantInt::get(Type::getInt32Ty(Context), 1));
   } else {
     Director.reset(new WinEHCleanupDirector(Handler, VarInfo, LPadMap));
+    LPadMap.remapEHValues(VMap, UndefValue::get(Int8PtrType),
+                          UndefValue::get(Type::getInt32Ty(Context)));
   }
 
   SmallVector<ReturnInst *, 8> Returns;
   ClonedCodeInfo OutlinedFunctionInfo;
 
   // If the start block contains PHI nodes, we need to map them.
   BasicBlock::iterator II = StartBB->begin();
   while (auto *PN = dyn_cast<PHINode>(II)) {
void LandingPadMap::mapLandingPad(const LandingPadInst *LPad) {
   // If the landing pad has already been mapped, there's nothing more to do.
   if (OriginLPad == LPad)
     return;
 
   OriginLPad = LPad;
 
   // The landingpad instruction returns an aggregate value.  Typically, its
   // value will be passed to a pair of extract value instructions and the
-  // results of those extracts are often passed to store instructions.
-  // In unoptimized code the stored value will often be loaded and then stored
-  // again.
+  // results of those extracts will have been promoted to reg values before
+  // this routine is called.
   for (auto *U : LPad->users()) {
     const ExtractValueInst *Extract = dyn_cast<ExtractValueInst>(U);
     if (!Extract)
       continue;
     assert(Extract->getNumIndices() == 1 &&
            "Unexpected operation: extracting both landing pad values");
     unsigned int Idx = *(Extract->idx_begin());
     assert((Idx == 0 || Idx == 1) &&
            "Unexpected operation: extracting an unknown landing pad element");
     if (Idx == 0) {
-      // Element 0 doesn't directly corresponds to anything in the WinEH
-      // scheme.
-      // It will be stored to a memory location, then later loaded and finally
-      // the loaded value will be used as the argument to an
-      // llvm.eh.begincatch
-      // call.  We're tracking it here so that we can skip the store and load.
       ExtractedEHPtrs.push_back(Extract);
     } else if (Idx == 1) {
-      // Element 1 corresponds to the filter selector.  We'll map it to 1 for
-      // matching purposes, but it will also probably be stored to memory and
-      // reloaded, so we need to track the instuction so that we can map the
-      // loaded value too.
       ExtractedSelectors.push_back(Extract);
     }
-
-    // Look for stores of the extracted values.
-    for (auto *EU : Extract->users()) {
-      if (auto *Store = dyn_cast<StoreInst>(EU)) {
-        if (Idx == 1) {
-          SelectorStores.push_back(Store);
-          SelectorStoreAddrs.push_back(Store->getPointerOperand());
-        } else {
-          EHPtrStores.push_back(Store);
-          EHPtrStoreAddrs.push_back(Store->getPointerOperand());
-        }
-      }
-    }
   }
 }
 
 bool LandingPadMap::isOriginLandingPadBlock(const BasicBlock *BB) const {
   return BB->getLandingPadInst() == OriginLPad;
 }
 
 bool LandingPadMap::isLandingPadSpecificInst(const Instruction *Inst) const {
   if (Inst == OriginLPad)
     return true;
   for (auto *Extract : ExtractedEHPtrs) {
     if (Inst == Extract)
       return true;
   }
   for (auto *Extract : ExtractedSelectors) {
     if (Inst == Extract)
       return true;
   }
-  for (auto *Store : EHPtrStores) {
-    if (Inst == Store)
-      return true;
-  }
-  for (auto *Store : SelectorStores) {
-    if (Inst == Store)
-      return true;
-  }
-
   return false;
 }
 
-void LandingPadMap::remapSelector(ValueToValueMapTy &VMap,
-                                     Value *MappedValue) const {
-  // Remap all selector extract instructions to the specified value.
+void LandingPadMap::remapEHValues(ValueToValueMapTy &VMap, Value *EHPtrValue,
+                                  Value *SelectorValue) const {
+  // Remap all landing pad extract instructions to the specified values.
+  for (auto *Extract : ExtractedEHPtrs)
+    VMap[Extract] = EHPtrValue;
   for (auto *Extract : ExtractedSelectors)
-    VMap[Extract] = MappedValue;
-}
-
-bool LandingPadMap::mapIfEHLoad(const LoadInst *Load,
-                                   SmallVectorImpl<const StoreInst *> &Stores,
-                                   SmallVectorImpl<const Value *> &StoreAddrs) {
-  // This makes the assumption that a store we've previously seen dominates
-  // this load instruction.  That might seem like a rather huge assumption,
-  // but given the way that landingpads are constructed its fairly safe.
-  // FIXME: Add debug/assert code that verifies this.
-  const Value *LoadAddr = Load->getPointerOperand();
-  for (auto *StoreAddr : StoreAddrs) {
-    if (LoadAddr == StoreAddr) {
-      // Handle the common debug scenario where this loaded value is stored
-      // to a different location.
-      for (auto *U : Load->users()) {
-        if (auto *Store = dyn_cast<StoreInst>(U)) {
-          Stores.push_back(Store);
-          StoreAddrs.push_back(Store->getPointerOperand());
-        }
-      }
-      return true;
-    }
-  }
-  return false;
+    VMap[Extract] = SelectorValue;
 }
 
 CloningDirector::CloningAction WinEHCloningDirectorBase::handleInstruction(
     ValueToValueMapTy &VMap, const Instruction *Inst, BasicBlock *NewBB) {
   // If this is one of the boilerplate landing pad instructions, skip it.
   // The instruction will have already been remapped in VMap.
   if (LPadMap.isLandingPadSpecificInst(Inst))
     return CloningDirector::SkipInstruction;
 
-  if (auto *Load = dyn_cast<LoadInst>(Inst)) {
-    // Look for loads of (previously suppressed) landingpad values.
-    // The EHPtr load can be mapped to an undef value as it should only be used
-    // as an argument to llvm.eh.begincatch, but the selector value needs to be
-    // mapped to a constant value of 1.  This value will be used to simplify the
-    // branching to always flow to the current handler.
-    if (LPadMap.mapIfSelectorLoad(Load)) {
-      VMap[Inst] = ConstantInt::get(SelectorIDType, 1);
-      return CloningDirector::SkipInstruction;
-    }
-    if (LPadMap.mapIfEHPtrLoad(Load)) {
-      VMap[Inst] = UndefValue::get(Int8PtrType);
-      return CloningDirector::SkipInstruction;
-    }
-
-    // Any other loads just get cloned.
-    return CloningDirector::CloneInstruction;
-  }
-
   // Nested landing pads will be cloned as stubs, with just the
   // landingpad instruction and an unreachable instruction. When
   // all landingpads have been outlined, we'll replace this with the
   // llvm.eh.actions call and indirect branch created when the
   // landing pad was outlined.
   if (auto *NestedLPad = dyn_cast<LandingPadInst>(Inst)) {
     Instruction *NewInst = NestedLPad->clone();
     if (NestedLPad->hasName())
WinEHCatchDirector::handleEndCatch(ValueToValueMapTy &VMap,
   // or at the end of the catch block.  However, a catch-all handler may call
   // end catch from the original landing pad.  If the call occurs in a nested
   // landing pad block, we must skip it and continue so that the landing pad
   // gets cloned.
   auto *ParentBB = IntrinCall->getParent();
   if (ParentBB->isLandingPad() && !LPadMap.isOriginLandingPadBlock(ParentBB))
     return CloningDirector::SkipInstruction;
 
-  // If an end catch occurs anywhere else the next instruction should be an
-  // unconditional branch instruction that we want to replace with a return
-  // to the the address of the branch target.
-  const BasicBlock *EndCatchBB = IntrinCall->getParent();
-  const TerminatorInst *Terminator = EndCatchBB->getTerminator();
-  const BranchInst *Branch = dyn_cast<BranchInst>(Terminator);
-  assert(Branch && Branch->isUnconditional());
-  assert(std::next(BasicBlock::const_iterator(IntrinCall)) ==
-         BasicBlock::const_iterator(Branch));
+  // If an end catch occurs anywhere else we want to terminate the handler
+  // with a return to the code that follows the endcatch call.  If the
+  // next instruction is not an unconditional branch, we need to split the
+  // block to provide a clear target for the return instruction.
+  BasicBlock *ContinueBB;
+  auto Next = std::next(BasicBlock::const_iterator(IntrinCall));
+  const BranchInst *Branch = dyn_cast<BranchInst>(Next);
+  if (!Branch || !Branch->isUnconditional()) {
+    // We're interrupting the cloning process at this location, so the
+    // const_cast we're doing here will not cause a problem.
+    ContinueBB = SplitBlock(const_cast<BasicBlock *>(ParentBB),
+                            const_cast<Instruction *>(cast<Instruction>(Next)));
+  } else {
+    ContinueBB = Branch->getSuccessor(0);
+  }
 
-  BasicBlock *ContinueLabel = Branch->getSuccessor(0);
-  ReturnInst::Create(NewBB->getContext(), BlockAddress::get(ContinueLabel),
-                     NewBB);
-  ReturnTargets.push_back(ContinueLabel);
+  ReturnInst::Create(NewBB->getContext(), BlockAddress::get(ContinueBB), NewBB);
+  ReturnTargets.push_back(ContinueBB);
 
   // 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 WinEHCatchDirector::handleTypeIdFor(
while (BB) {
     if (BB->isLandingPad()) {
       LandingPadInst *LPad = BB->getLandingPadInst();
       LPadMap = &LPadMaps[LPad];
       if (!LPadMap->isInitialized())
         LPadMap->mapLandingPad(LPad);
     }
 
     // Look for the bare resume pattern:
-    //   %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
+    //   %lpad.val1 = insertvalue { i8*, i32 } undef, i8* %exn, 0
+    //   %lpad.val2 = insertvalue { i8*, i32 } %lpad.val1, i32 %sel, 1
     //   resume { i8*, i32 } %lpad.val2
     if (auto *Resume = dyn_cast<ResumeInst>(Terminator)) {
       InsertValueInst *Insert1 = nullptr;
       InsertValueInst *Insert2 = nullptr;
       Value *ResumeVal = Resume->getOperand(0);
       // If there is only one landingpad, we may use the lpad directly with no
       // insertions.
       if (isa<LandingPadInst>(ResumeVal))
if (auto *Resume = dyn_cast<ResumeInst>(Terminator)) {
             (Inst->user_back() != Insert1 && Inst->user_back() != Insert2)) {
           return createCleanupHandler(CleanupHandlerMap, BB);
         }
       }
       return nullptr;
     }
 
     BranchInst *Branch = dyn_cast<BranchInst>(Terminator);
-    if (Branch) {
-      if (Branch->isConditional()) {
-        // Look for the selector dispatch.
-        //   %sel = load i32* %ehselector.slot
-        //   %2 = call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIf to i8*))
-        //   %matches = icmp eq i32 %sel12, %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();
-             II != IE; ++II) {
-          Instruction *Inst = II;
-          if (LPadMap && LPadMap->isLandingPadSpecificInst(Inst))
-            continue;
-          if (Inst == Compare || Inst == Branch)
-            continue;
-          if (!Inst->hasOneUse() || (Inst->user_back() != Compare))
-            return createCleanupHandler(CleanupHandlerMap, BB);
-          if (match(Inst, m_Intrinsic<Intrinsic::eh_typeid_for>()))
-            continue;
-          if (!isa<LoadInst>(Inst))
-            return createCleanupHandler(CleanupHandlerMap, BB);
-        }
-        // The selector dispatch block should always terminate our search.
-        assert(BB == EndBB);
-        return nullptr;
-      } else {
-        // Look for empty blocks with unconditional branches.
-        for (BasicBlock::iterator II = BB->getFirstNonPHIOrDbg(),
-                                  IE = BB->end();
-             II != IE; ++II) {
-          Instruction *Inst = II;
-          if (LPadMap && LPadMap->isLandingPadSpecificInst(Inst))
-            continue;
-          if (Inst == Branch)
-            continue;
-          // This can happen with a catch-all handler.
-          if (match(Inst, m_Intrinsic<Intrinsic::eh_begincatch>()))
-            return nullptr;
-          if (match(Inst, m_Intrinsic<Intrinsic::eh_endcatch>()))
-            continue;
-          // Anything else makes this interesting cleanup code.
-          return createCleanupHandler(CleanupHandlerMap, BB);
-        }
-        if (BB == EndBB)
-          return nullptr;
-        // The branch was unconditional.
-        BB = Branch->getSuccessor(0);
-        continue;
-      } // End else of if branch was conditional
-    }   // End if Branch
-
-    // Anything else makes this interesting cleanup code.
-    return createCleanupHandler(CleanupHandlerMap, BB);
+    if (Branch && Branch->isConditional()) {
+      //   %matches = icmp eq i32 %sel, %2
+    }
+
+    // Anything else is either a catch block or interesting cleanup code.
+      // Unconditional branches fall through to this loop.
+      // If this is a catch block, there is no cleanup code to be found.
+
+    // Only unconditional branches in empty blocks should get this far.
+    assert(Branch && Branch->isUnconditional());
   }
   return nullptr;
 }

llvm/trunk/test/CodeGen/WinEH/cppeh-multi-catch.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 {
+;     may_throw();
+;   } catch (int i) {
+;     handle_int(i);
+;   } catch (long long ll) {
+;     handle_long_long(ll);
+;   } catch (SomeClass &obj) {
+;     handle_obj(&obj);
+;   } catch (...) {
+;     handle_exception();
+;   }
+; }
+;
+; The catch handlers were edited to insert 'ret void' after the endcatch call.
+
+; ModuleID = 'catch-with-type.cpp'
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+%rtti.TypeDescriptor2 = type { i8**, i8*, [3 x i8] }
+%eh.HandlerMapEntry = type { i32, i32 }
+%rtti.TypeDescriptor3 = type { i8**, i8*, [4 x i8] }
+%rtti.TypeDescriptor15 = type { i8**, i8*, [16 x i8] }
+%class.SomeClass = type { i8 }
+
+$"\01??_R0H@8" = comdat any
+
+$"\01??_R0_J@8" = comdat any
+
+$"\01??_R0?AVSomeClass@@@8" = comdat any
+
+@"\01??_7type_info@@6B@" = external constant i8*
+@"\01??_R0H@8" = linkonce_odr global %rtti.TypeDescriptor2 { i8** @"\01??_7type_info@@6B@", i8* null, [3 x i8] c".H\00" }, comdat
+@__ImageBase = external constant i8
[e-mail].H = private unnamed_addr constant %eh.HandlerMapEntry { i32 0, i32 trunc (i64 sub nuw nsw (i64 ptrtoint (%rtti.TypeDescriptor2* @"\01??_R0H@8" to i64), i64 ptrtoint (i8* @__ImageBase to i64)) to i32) }, section "llvm.metadata"
+@"\01??_R0_J@8" = linkonce_odr global %rtti.TypeDescriptor3 { i8** @"\01??_7type_info@@6B@", i8* null, [4 x i8] c"._J\00" }, comdat
[e-mail]._J = private unnamed_addr constant %eh.HandlerMapEntry { i32 0, i32 trunc (i64 sub nuw nsw (i64 ptrtoint (%rtti.TypeDescriptor3* @"\01??_R0_J@8" to i64), i64 ptrtoint (i8* @__ImageBase to i64)) to i32) }, section "llvm.metadata"
+@"\01??_R0?AVSomeClass@@@8" = linkonce_odr global %rtti.TypeDescriptor15 { i8** @"\01??_7type_info@@6B@", i8* null, [16 x i8] c".?AVSomeClass@@\00" }, comdat
+@"llvm.eh.handlermapentry.reference.?AVSomeClass@@" = private unnamed_addr constant %eh.HandlerMapEntry { i32 8, i32 trunc (i64 sub nuw nsw (i64 ptrtoint (%rtti.TypeDescriptor15* @"\01??_R0?AVSomeClass@@@8" to i64), i64 ptrtoint (i8* @__ImageBase to i64)) to i32) }, section "llvm.metadata"
+
+
+; CHECK: define void @"\01?test@@YAXXZ"() #0 {
+; CHECK: entry:
+; CHECK:   [[UNWINDHELP:\%.+]] = alloca i64
+; CHECK:   [[OBJ_PTR:\%.+]] = alloca %class.SomeClass*, align 8
+; CHECK:   [[LL_PTR:\%.+]] = alloca i64, align 8
+; CHECK:   [[I_PTR:\%.+]] = alloca i32, align 4
+; CHECK:   call void (...)* @llvm.frameescape(i32* [[I_PTR]], i64* [[LL_PTR]], %class.SomeClass** [[OBJ_PTR]])
+; CHECK:   store volatile i64 -2, i64* [[UNWINDHELP]]
+; CHECK:   [[TMP:\%.+]] = bitcast i64* [[UNWINDHELP]] to i8*
+; CHECK:   call void @llvm.eh.unwindhelp(i8* [[TMP]])
+; CHECK:   invoke void @"\01?may_throw@@YAXXZ"()
+; CHECK:           to label %invoke.cont unwind label %[[LPAD_LABEL:lpad[0-9]+]]
+
+; Function Attrs: uwtable
+define void @"\01?test@@YAXXZ"() #0 {
+entry:
+  %exn.slot = alloca i8*
+  %ehselector.slot = alloca i32
+  %obj = alloca %class.SomeClass*, align 8
+  %ll = alloca i64, align 8
+  %i = alloca i32, align 4
+  invoke void @"\01?may_throw@@YAXXZ"()
+          to label %invoke.cont unwind label %lpad
+
+invoke.cont:                                      ; preds = %entry
+  br label %try.cont
+
+; CHECK: [[LPAD_LABEL]]:{{[ ]+}}; preds = %entry
+; CHECK:   landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
+; CHECK-NEXT:           catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.H
+; CHECK-NEXT:           catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry._J
+; CHECK-NEXT:           catch %eh.HandlerMapEntry* @"llvm.eh.handlermapentry.reference.?AVSomeClass@@"
+; CHECK-NEXT:           catch i8* null
+; CHECK-NEXT:   [[RECOVER:\%.+]] = call i8* (...)* @llvm.eh.actions(i32 1, i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.H to i8*), i32* %i, i8* (i8*, i8*)* @"\01?test@@YAXXZ.catch", i32 1, i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry._J to i8*), i64* %ll, i8* (i8*, i8*)* @"\01?test@@YAXXZ.catch1", i32 1, i8* bitcast (%eh.HandlerMapEntry* @"llvm.eh.handlermapentry.reference.?AVSomeClass@@" to i8*), %class.SomeClass** %obj, i8* (i8*, i8*)* @"\01?test@@YAXXZ.catch2", i32 1, i8* null, i8* null, i8* (i8*, i8*)* @"\01?test@@YAXXZ.catch3")
+; CHECK-NEXT:   indirectbr i8* [[RECOVER]], [label %catch14.split, label %catch10.split, label %catch6.split, label %catch.split]
+
+lpad:                                             ; preds = %entry
+  %0 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
+          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.H
+          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry._J
+          catch %eh.HandlerMapEntry* @"llvm.eh.handlermapentry.reference.?AVSomeClass@@"
+          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
+  br label %catch.dispatch
+
+; CHECK-NOT: catch.dispatch:
+catch.dispatch:                                   ; preds = %lpad
+  %sel = load i32, i32* %ehselector.slot
+  %3 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.H to i8*)) #3
+  %matches = icmp eq i32 %sel, %3
+  br i1 %matches, label %catch14, label %catch.fallthrough
+
+; CHECK-NOT: catch14:
+; CHECK: catch14.split:
+; CHECK-NEXT:   ret void
+catch14:                                          ; preds = %catch.dispatch
+  %exn15 = load i8*, i8** %exn.slot
+  %4 = bitcast i32* %i to i8*
+  call void @llvm.eh.begincatch(i8* %exn15, i8* %4) #3
+  %5 = load i32, i32* %i, align 4
+  call void @"\01?handle_int@@YAXH@Z"(i32 %5)
+  call void @llvm.eh.endcatch() #3
+  ret void
+
+try.cont:                                         ; preds = %invoke.cont
+  ret void
+
+; CHECK-NOT: catch.fallthrough:
+catch.fallthrough:                                ; preds = %catch.dispatch
+  %6 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry._J to i8*)) #3
+  %matches1 = icmp eq i32 %sel, %6
+  br i1 %matches1, label %catch10, label %catch.fallthrough2
+
+; CHECK-NOT: catch10:
+; CHECK: catch10.split:
+; CHECK-NEXT:   ret void
+catch10:                                          ; preds = %catch.fallthrough
+  %exn11 = load i8*, i8** %exn.slot
+  %7 = bitcast i64* %ll to i8*
+  call void @llvm.eh.begincatch(i8* %exn11, i8* %7) #3
+  %8 = load i64, i64* %ll, align 8
+  call void @"\01?handle_long_long@@YAX_J@Z"(i64 %8)
+  call void @llvm.eh.endcatch() #3
+  ret void
+
+; CHECK-NOT: catch.fallthrough2:
+catch.fallthrough2:                               ; preds = %catch.fallthrough
+  %9 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @"llvm.eh.handlermapentry.reference.?AVSomeClass@@" to i8*)) #3
+  %matches3 = icmp eq i32 %sel, %9
+  br i1 %matches3, label %catch6, label %catch
+
+; CHECK-NOT: catch6:
+; CHECK: catch6.split:
+; CHECK-NEXT:   ret void
+catch6:                                           ; preds = %catch.fallthrough2
+  %exn7 = load i8*, i8** %exn.slot
+  %10 = bitcast %class.SomeClass** %obj to i8*
+  call void @llvm.eh.begincatch(i8* %exn7, i8* %10) #3
+  %11 = load %class.SomeClass*, %class.SomeClass** %obj, align 8
+  call void @"\01?handle_obj@@YAXPEAVSomeClass@@@Z"(%class.SomeClass* %11)
+  call void @llvm.eh.endcatch() #3
+  ret void
+
+; CHECK-NOT: catch:
+; CHECK: catch.split:
+; CHECK-NEXT:   ret void
+catch:                                            ; preds = %catch.fallthrough2
+  %exn = load i8*, i8** %exn.slot
+  call void @llvm.eh.begincatch(i8* %exn, i8* null) #3
+  call void @"\01?handle_exception@@YAXXZ"()  call void @llvm.eh.endcatch() #3
+  ret void
+; CHECK: }
+}
+
+; CHECK-LABEL: define internal i8* @"\01?test@@YAXXZ.catch"(i8*, i8*)
+; CHECK: entry:
+; CHECK:   [[RECOVER_I:\%.+]] = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1, i32 0)
+; CHECK:   [[I_PTR:\%.+]] = bitcast i8* [[RECOVER_I]] to i32*
+; CHECK:   [[TMP1:\%.+]] = load i32, i32* [[I_PTR]], align 4
+; CHECK:   call void @"\01?handle_int@@YAXH@Z"(i32 [[TMP1]])
+; CHECK:   ret i8* blockaddress(@"\01?test@@YAXXZ", %catch14.split)
+; CHECK: }
+
+; CHECK-LABEL: define internal i8* @"\01?test@@YAXXZ.catch1"(i8*, i8*)
+; CHECK: entry:
+; CHECK:   [[RECOVER_LL:\%.+]] = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1, i32 1)
+; CHECK:   [[LL_PTR:\%.+]] = bitcast i8* [[RECOVER_LL]] to i64*
+; CHECK:   [[TMP2:\%.+]] = load i64, i64* [[LL_PTR]], align 8
+; CHECK:   call void @"\01?handle_long_long@@YAX_J@Z"(i64 [[TMP2]])
+; CHECK:   ret i8* blockaddress(@"\01?test@@YAXXZ", %catch10.split)
+; CHECK: }
+
+; CHECK-LABEL: define internal i8* @"\01?test@@YAXXZ.catch2"(i8*, i8*)
+; CHECK: entry:
+; CHECK:   [[RECOVER_OBJ:\%.+]] = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?test@@YAXXZ" to i8*), i8* %1, i32 2)
+; CHECK:   [[OBJ_PTR:\%.+]] = bitcast i8* [[RECOVER_OBJ]] to %class.SomeClass**
+; CHECK:   [[TMP3:\%.+]] = load %class.SomeClass*, %class.SomeClass** [[OBJ_PTR]], align 8
+; CHECK:   call void @"\01?handle_obj@@YAXPEAVSomeClass@@@Z"(%class.SomeClass* [[TMP3]])
+; CHECK:   ret i8* blockaddress(@"\01?test@@YAXXZ", %catch6.split)
+; CHECK: }
+
+; CHECK-LABEL: define internal i8* @"\01?test@@YAXXZ.catch3"(i8*, i8*)
+; CHECK: entry:
+; CHECK:   call void @"\01?handle_exception@@YAXXZ"()
+; CHECK:   ret i8* blockaddress(@"\01?test@@YAXXZ", %catch.split)
+; CHECK: }
+
+
+declare void @"\01?may_throw@@YAXXZ"() #1
+
+declare i32 @__CxxFrameHandler3(...)
+
+; Function Attrs: nounwind readnone
+declare i32 @llvm.eh.typeid.for(i8*) #2
+
+; Function Attrs: nounwind
+declare void @llvm.eh.begincatch(i8* nocapture, i8* nocapture) #3
+
+declare void @"\01?handle_exception@@YAXXZ"() #1
+
+; Function Attrs: nounwind
+declare void @llvm.eh.endcatch() #3
+
+declare void @"\01?handle_obj@@YAXPEAVSomeClass@@@Z"(%class.SomeClass*) #1
+
+declare void @"\01?handle_long_long@@YAX_J@Z"(i64) #1
+
+declare void @"\01?handle_int@@YAXH@Z"(i32) #1
+
+attributes #0 = { 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" "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" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind readnone }
+attributes #3 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.7.0 (trunk 233155) (llvm/trunk 233153)"}