[PATCH] Outline cleanup handlers for native Windows C++ exception handling

Andy Kaylor andrew.kaylor at intel.com
Tue Feb 24 13:24:32 PST 2015


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:285-287
@@ +284,5 @@
+
+    // FIXME: We need to handle the case where code has been moved into the
+    //        landing pad but the landing pad has not been marked as needing
+    //        cleanup.  That will happen after the block analysis is in place.
+    if (LPad->isCleanup()) {
----------------
rnk wrote:
> Now that I think about it, I'm not sure we actually need to handle that case. All optimizations have to assume that landingpads can be entered for reasons other than those that are listed. Consider this case where we might hoist a store:
>   void g();
>   static int r;
>   int f() {
>     try { g(); }
>     catch (int) { r = 42; }
>     catch (float) { r = 42; }
>     return r;
>   }
> 
> An optimization that hoisted the r = 42 store into the landingpad block would be invalid, because the landingpad can be entered for other reasons. In particular, if we did that hoist and then inlined f into some other function with cleanups, now the store would be unconditional.
Well, I'd be happy to never see the hoisting case arise, but I don't think it's actually going to be very difficult to handle if we wanted to.  It might be better to have code to handle it that never gets executed than to have code to flags it as a fatal error if it does happen.  Either way, I can take this comment out for now and we can decide when we get to the block analysis review.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:288-292
@@ +287,7 @@
+    //        cleanup.  That will happen after the block analysis is in place.
+    if (LPad->isCleanup()) {
+      CallInst *EHAlloc = nullptr;
+      AllocaInst *IgnoreEHObjPtr = nullptr;
+      bool Outlined = outlineHandler(Cleanup, &F, nullptr, LPad, EHAlloc,
+                                     IgnoreEHObjPtr, FrameVarInfo);
+      if (Outlined) {
----------------
rnk wrote:
> I'm not sure we can handle catches inside cleanups this way. Are you planning to switch to a more up front analysis in subsequent changes? I'm thinking of this case:
>   void g();
>   struct MyRAII { ~MyRAII(); };
>   void runs_before_dtor();
>   void f() {
>     MyRAII x;
>     try { g(); }
>     catch (int) { runs_before_dtor(); }
>   }
> 
> We need to make sure runs_before_dtor() runs first, but I guess we haven't gotten to building eh.actions.
Yeah, this only handles a single cleanup per landing pad and doesn't handle the case where a block of cleanup code is shared between landing pads.  This is basically here as a place holder to move things along.  I can add a comment explaining that.

To handle cases like the one you describe I need the block analysis stuff in place.  The actually outlining works the same way except that it gets a specific starting block and has different termination conditions.  I have that working in my sandbox, but it seemed like a bit much to put into this review.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:702-703
@@ +701,4 @@
+  // from the EH alloc block, so we can just map to that here.
+  VMap[Inst] = UndefValue::get(Inst->getType());
+  return CloningDirector::SkipInstruction;
+}
----------------
rnk wrote:
> Any reason not to map llvm.eh.begincatch to unreachable in a cleanup?
That's a good idea.  I'll change it to that.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:708-737
@@ +707,32 @@
+    ValueToValueMapTy &VMap, const Instruction *Inst, BasicBlock *NewBB) {
+  // It might be interesting to track whether or not we are inside a catch
+  // function, but that might make the algorithm more brittle than it needs
+  // to be.
+
+  // The end catch call can occur in one of two places: either in a landingpad
+  // block that is part of the catch handlers exception mechanism, or at the
+  // end of the catch block.  If it occurs in a landing pad, we must skip it
+  // and continue so that the landing pad gets cloned.
+  // FIXME: This case isn't fully supported yet and shouldn't turn up in any
+  //        of the test cases until it is.
+  if (Inst->getParent()->isLandingPad())
+    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 = Inst->getParent();
+  const TerminatorInst *Terminator = EndCatchBB->getTerminator();
+  const BranchInst *Branch = dyn_cast<BranchInst>(Terminator);
+  assert(Branch && Branch->isUnconditional());
+  assert(std::next(BasicBlock::const_iterator(Inst)) ==
+         BasicBlock::const_iterator(Branch));
+
+  ReturnInst::Create(NewBB->getContext(),
+                     BlockAddress::get(Branch->getSuccessor(0)), 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;
+}
----------------
rnk wrote:
> Similarly, I'd map this to unreachable. It isn't tested yet anyway.
Agreed.

http://reviews.llvm.org/D7865

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list