[PATCH] Fix WinEHPrepare bug with multiple catch handlers

[Andy 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.


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D8682

Files:
  lib/CodeGen/WinEHPrepare.cpp

Index: lib/CodeGen/WinEHPrepare.cpp
===================================================================
--- lib/CodeGen/WinEHPrepare.cpp
+++ lib/CodeGen/WinEHPrepare.cpp
@@ -925,6 +925,26 @@
     return CloningDirector::CloneInstruction;
   }
 
+  if (auto *Compare = dyn_cast<CmpInst>(Inst)) {
+    // Look for compare instructions that use selector values that were not
+    // defined in the current block.  A series of related catch dispatch blocks
+    // will share a loaded selector value, but after the first dispatch block
+    // we will have started outlining after the value is loaded.  We can
+    // spot this case by looking at the compare operands.
+    for (auto &U : Compare->operands()) {
+      // Ignore any operands we've already mapped.
+      if (VMap.count(U.get()))
+        continue;
+      if (auto *Load = dyn_cast<LoadInst>(U.get())) {
+        if (LPadMap.mapIfSelectorLoad(Load))
+          VMap[Load] = ConstantInt::get(SelectorIDType, 1);
+        break;
+      }
+    }
+    // Whether we mapped a selector load above or not, the compare gets 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
@@ -994,20 +1014,24 @@
   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));
-
-  BasicBlock *ContinueLabel = Branch->getSuccessor(0);
-  ReturnInst::Create(NewBB->getContext(), BlockAddress::get(ContinueLabel),
-                     NewBB);
-  ReturnTargets.push_back(ContinueLabel);
+  // 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<IntrinsicInst *>(IntrinCall));
+  } else {
+    ContinueBB = Branch->getSuccessor(0);
+  }
+
+  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
@@ -1461,12 +1485,19 @@
             continue;
           if (Inst == Compare || Inst == Branch)
             continue;
-          if (!Inst->hasOneUse() || (Inst->user_back() != Compare))
-            return createCleanupHandler(CleanupHandlerMap, BB);
+          // Loads of selector values may be used by multiple blocks, but if the
+          // loaded value is used in this block, it should be used by the
+          // compare instruction.
+          if (auto *Load = dyn_cast<LoadInst>(Inst)) {
+            for (auto *U : Load->users()) {
+              if (cast<Instruction>(U)->getParent() == BB && U != Compare)
+                return createCleanupHandler(CleanupHandlerMap, BB);
+            }
+            continue;
+          }
           if (match(Inst, m_Intrinsic<Intrinsic::eh_typeid_for>()))
             continue;
-          if (!isa<LoadInst>(Inst))
-            return createCleanupHandler(CleanupHandlerMap, BB);
+          return createCleanupHandler(CleanupHandlerMap, BB);
         }
         // The selector dispatch block should always terminate our search.
         assert(BB == EndBB);

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D8682.22903.patch
Type: text/x-patch
Size: 4524 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150330/6d2eecf1/attachment.bin>