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

Reid Kleckner rnk at google.com
Tue Feb 24 12:57:18 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()) {
----------------
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.

================
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) {
----------------
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.

================
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;
+}
----------------
Any reason not to map llvm.eh.begincatch to unreachable in a cleanup?

================
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;
+}
----------------
Similarly, I'd map this to unreachable. It isn't tested yet anyway.

http://reviews.llvm.org/D7865

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






More information about the llvm-commits mailing list