[WinEH] Update CoreCLR EH state numbering

[JosephTremoulet]

Summary

Fix the CLR state numbering to generate correct tables, and update the lit
test to verify them.

The CLR numbering assigns one state number to each catchpad and
cleanuppad.

It also computes two tree-like relations over states:

1. Each state has a "HandlerParentState", which is the state of the next outer handler enclosing this state's handler (same as nearest ancestor per the ParentPad linkage on EH pads, but skipping over catchswitches).
2. Each state has a "TryParentState", which: a) for a catchpad that's not the last handler on its catchswitch, is the state of the next catchpad on that catchswitch. b) for all other pads, is the state of the pad whose try region is the next outer try region enclosing this state's try region. The "try regions are not present as such in the IR, but will be inferred based on the placement of invokes and pads which reach each other by exceptional exits.

Catchswitches do not get their own states, but each gets mapped to the
state of its first catchpad.

Table generation requires each state's "unwind dest" state to have a lower
state number than the given state.

Since HandlerParentState can be computed as a function of a pad's
ParentPad, and TryParentState can be computed as a function of its unwind
dest and the TryParentStates of its children, the CLR state numbering
algorithm first computes HandlerParentState in a top-down pass, then
computes TryParentState in a bottom-up pass.

Also reword some comments/names in the CLR EH table generation to make the
distinction between the different kinds of "parent" clear.

[JosephTremoulet]

• Update worklist seeding to include all pads that unwind to caller (or are cleanuppads with no cleanupret)

[JosephTremoulet]

• fix wineh-coreclr test to report proper outer scope for nested fault cleanuppad
• adjust getEHPadFromPredecessor and calls to it for CLR EH so it behaves correctly for nested CLR funclets

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

534

I have to admit that I'm not familiar enough with Core CLR to know what's possible and what isn't, so maybe this isn't an issue, but it strikes me here that a nested cleanup pad that never returns will be treated like a top level cleanup pad. I'm thinking of something like this:

try {
  try {
    f()
  } catch (...) {
    try {
      g()
    } finally {
      throw() // noreturn
    }
  }
} catch (...) {
}

The throw call here would unwind to the outer catch handler, but I think CPI->getCleanupRetUnwindDest() would return null.

538

I'm probably not understanding this correctly. Can you talk me through how a case like the following would be handled?

try {
  try {
  } catch { // inner catch
  }
} catch { // outer catch
  try {
  } catch { // catch catch
  }
}

If I have everything straight, both "outer catch" and "catch catch" will have null unwind destinations. So that would make the parent state of "catch catch" -1, I think. Is that what you want?

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

534

Yes, good catch, that's a case that wouldn't be handled correctly as written. We've discussed a plan, which I think we now need for cases like this and the irreducible loop, and which I am working on, to add "bundles" to calls and invokes in EH handlers indicating their handler membership (i.e. "color"), analogous to the "within" annotation that D15139 adds to the pads themselves. In that world, the IR for your example would look like:

invoke void @f()
  to label %whatever unwind label %inner_catch_switch
inner_catch_switch:
  %ics = catchswitch within none [label %inner_catch] unwind label %outer_catch_switch
inner_catch:
  %ic = catchpad within %ics [...]
  invoke void @g() ["within" token %ic]
    to label %whatever2 unwind label %finally
finally:
  %f = cleanuppad within %ic []
  invoke void @throw() ["within" token %f]
    to label %unreachable unwind label %outer_catch_switch

I'd expect this code to be updated to look through uses of the cleanuppad (%f) for not just cleanuprets but also "within" bundles on calls/invokes, and find the "unwind label %outer_catch_switch" on the invoke.

As far as staging the changes, I was thinking it makes sense to go ahead and check in this code that gets us from "completely broken" to "correct except in cleanups that have lost their cleanupret", once D15139 lands, since that will be enough to unblock downstreaming changes into LLILC while the bundle change iterates. Though I probably should at least include a FIXME here in this change.

It would of course be possible to hunt for invokes in the cleanup without the bundles making it easy, but since we seem to need the bundles for exotic-but-theoretically-possible cases where the correct color is irresolvably ambiguous, I'd rather not bother with that just to rewrite it to use the token bundles; but I would still like to checkpoint this step.

And, of course, I should be more concrete about "cases where I think we need the bundles". Here's an example (courtesy @majnemer): Suppose we started with:

...
 } finally {
    if (_) {
      left();
      __unreachable;
    } else {
      right();
      __unreachable;
    }
  }
...
left();
try {
  foo();
} catch { // catch 1
  /* stuff */
}
...
right();
try {
  bar();
} catch { // catch 2
  /* stuff */
}

which would become IR like

finally:
  %fin = cleanuppad []
  br i1 %_, label %left_unreachable, label %right_unreachable
left_unreachable:
  call void @left()
  unreachable
right_unreachable:
  call void @right()
  unreachable
...
left_bait:
  call void @left()
  invoke void @foo()
    to label %whatever unwind label %catchswitch_1
...
right_bait:
  call void @right()
  invoke void @bar()
    to label %whatever_else unwind label %catchswitch_2

then, although I'm pretty sure we don't actually have this transformation, it seems reasonable to expect to be able to rewrite that to:

finally:
  %fin = cleanuppad []
  br i1 %_, label %left_bait, label %right_bait
...
left_bait:
  call void @left()
  invoke void @foo()
    to label %whatever unwind label %catchswitch_1
...
right_bait:
  call void @right()
  invoke void @bar()
    to label %whatever_else unwind label %catchswitch_2

Then the "finally" cleanup has no cleanupret, and if you try to figure out where the cleanup's "unwind dest" is by scanning its invokes, you'll find both %catchswitch_1 and %catchswitch_2, and picking either of them would lead you to generate bad code if dynamically the other path is taken. Having the front-end tag the calls/invokes in a cleanup with the token of the cleanup they're in makes it unambiguous when you want to infer a cleanup's "unwind destination" from an invoke -- the inference is valid if and only if the invoke has a matching token.

Of course, this code would still have to deal with the case that there isn't a cleanupret and aren't any invokes either. I've convinced myself that in that case it's correct to treat the cleanup as at the topmost level inside its enclosing funclet.

538

Yes, that's what I want. The relationship I need is:

• If a catchpad is not the first on its catchswitch, then the immediately prior catchpad on its same catchswitch is its parent
• otherwise, if the try region for EH pad A is the innermost try region enclosing the try region for EH pad B, then A is the parent of B

Since the try region for "outer catch" is disjoint with the try region for "catch catch", neither is an ancestor of the other; since no try region encloses the try region for "catch catch", its parent is -1.

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

538

Oops, got the first rule reversed. It's:

• If a catchpad is not the last on its catchswitch, then the immediately following catchpad on its same catchswitch is its parent
• otherwise, if the try region for EH pad A is the innermost try region enclosing the try region for EH pad B, then A is the parent of B

[JosephTremoulet]

• refactor toplevel test into isToplevelForCLR, give a fuller implementation
• rebase

[JosephTremoulet]

• expand comments in response to @AndyAyers's feedback at https://github.com/Microsoft/llvm/pull/123

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

450

This comment makes me nervous. Besides indicating where exceptions that occur inside the catchpads unwind the unwind clause of a catchswitch also indicates where an exception will go if it is not caught by any of the handlers in the catchswitch. So in order to change this clause from some EH pad to "unwinds to caller" we'd need to know both that none of the catch pads ever throws an exception and that all exceptions that come to this catch switch are caught by one of the catch pads (and even then, it seems like a bad idea). It doesn't seem like any optimization outside of the WinEH-specific handling should be able to confirm this second condition since it depends on the personality function. Is this really happening?

464

Does the phrase "exceptions propogating out of it" include both exceptions that cause the cleanup pad to be executed but reach the cleanupret instruction and exceptions that occur within the cleanup handler? It seems from the code that follows that it does, but it wasn't entirely clear to me.

477

This doesn't seem quite right. Consider the following scenario:

cleanup:
  %cl = cleanuppad within none []
  invoke void @g()
    to label %cleanup.cont unwind label %cleanup.cs
cleanup.cs:
  %cs = catchswitch within %cl [label %cleanup.catch] unwind label %nested.cleanup
cleanup.catch:
  %cp = catchpad within %cs []
  catchret from %cp to label %cleanup.cont
nested.cleanup:
  %nc = cleanuppad within %cl []
  call void @h()
  cleanupret from %nc unwind to caller
cleanup.cont:
  cleanupret from %cl unwind to caller

This case may return either true or false, depending on whether it encounters the nested catchswitch or the outer cleanupret first in the user list.

492

Shouldn't this be "if (!UselessCleanups.count(CPI))"?

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

450

The only place I'm aware of making this transformation is in SimplifyCFG's SimplifyUnreachable -- if the catchswitch (and invokes etc within the catches) unwinds to a cleanuppad that immediately its unreachable, we'll removeUnwindEdge each of those preds -- so in that case,
a) the inference is coming from the unwind destination, not analysis of the switch clauses, and
b) the same transformation will get applied to the invokes and such in the catches
which may be of some comfort.

Point well taken that preserving the agreement between unwind destinations in catches w.r.t. each other and the catchswitch in general requires careful non-local analysis.

464

Yeah, it needs to mean both, I'll try to make that more clear.

477

Yes, this was supposed to check if the catchswitch dest's parent is also the cleanup and continue to the next use if so. Good catch, thanks, I'll fix and expand the test.

492

Whoops, yes, of course. I'll maybe just remove this loop, though -- when the invokes are tagged with the cleanup, getting to this point means no exceptions escape the cleanup that aren't supposed to reach all the way up to caller, and so we can report it as top-level by the same reasoning that we use for faux-unwinds to caller catchswitches on line 454.

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

450

OK. I got the impression from this comment that this was happening if nothing was unwinding from within the catch switch.

[JosephTremoulet]

• Rebase on top of funclet bundle changes
• Extend to handle missing cases; rework as a top-down pass and then a bottom-up pass instead of using the IsToplevel/getEHPred scheme
• Split ClrEHUnwindMapEntry's Parent field into HandlerParentState and TryParentState; update comments and naming to keep the distinction clear
• Expand lit test to cover several of the corner cases

[majnemer]

lib/CodeGen/WinEHPrepare.cpp

585–586

Is calculateStateNumbersForInvokes correct for your personality? I'm surprised it went through untouched. Specifically, the bit where we use getCleanupRetUnwindDest seems a little sketchy...

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

585–586

Yes, it works out, although we're not really making use of its logic. This bit is the part that's irrelevant for CLR EH:

BasicBlock *FuncletUnwindDest;
auto *FuncletPad =
    dyn_cast<FuncletPadInst>(FuncletEntryBB->getFirstNonPHI());
assert(FuncletPad || FuncletEntryBB == &Fn->getEntryBlock());
if (!FuncletPad)
  FuncletUnwindDest = nullptr;
else if (auto *CatchPad = dyn_cast<CatchPadInst>(FuncletPad))
  FuncletUnwindDest = CatchPad->getCatchSwitch()->getUnwindDest();
else if (auto *CleanupPad = dyn_cast<CleanupPadInst>(FuncletPad))
  FuncletUnwindDest = getCleanupRetUnwindDest(CleanupPad);
else
  llvm_unreachable("unexpected funclet pad!");

BasicBlock *InvokeUnwindDest = II->getUnwindDest();
int BaseState = -1;
if (FuncletUnwindDest == InvokeUnwindDest) {
  auto BaseStateI = FuncInfo.FuncletBaseStateMap.find(FuncletPad);
  if (BaseStateI != FuncInfo.FuncletBaseStateMap.end())
    BaseState = BaseStateI->second;
}

You're right that FuncletUnwindDest will take on a meaningless value for cleanupret-less cleanups, but since we never populate FuncletBaseStateMap when targeting CLR, we never reach BaseState = BaseStateI->second, regardless of whether FuncletUnwindDest happens to match InvokeUnwindDest.

[JosephTremoulet]

rebase

[majnemer]

LGTM

lib/CodeGen/WinEHPrepare.cpp

if (BaseState != -1) {
       Instruction *PadInst = InvokeUnwindDest->getFirstNonPHI();
       assert(FuncInfo.EHPadStateMap.count(PadInst) && "EH Pad has no state!");
       FuncInfo.InvokeStateMap[II] = FuncInfo.EHPadStateMap[PadInst];
     }
   }
 }
 
 // Given BB which ends in an unwind edge, return the EHPad that this BB belongs
-// to. If the unwind edge came from an invoke, return null.
+// to. If the unwind edge came from an invoke, return null.  If ParentPad is
+// non-null, EHPads that do not have it as their ParentPad will also be ignored
+// (returning null).
 static const BasicBlock *getEHPadFromPredecessor(const BasicBlock *BB,
                                                  Value *ParentPad) {
   const TerminatorInst *TI = BB->getTerminator();
   if (isa<InvokeInst>(TI))
     return nullptr;
   if (auto *CatchSwitch = dyn_cast<CatchSwitchInst>(TI)) {
-    if (CatchSwitch->getParentPad() != ParentPad)
+    if (ParentPad && CatchSwitch->getParentPad() != ParentPad)
       return nullptr;
     return BB;
   }
   assert(!TI->isEHPad() && "unexpected EHPad!");
   auto *CleanupPad = cast<CleanupReturnInst>(TI)->getCleanupPad();
-  if (CleanupPad->getParentPad() != ParentPad)
+  if (ParentPad && CleanupPad->getParentPad() != ParentPad)
     return nullptr;
   return CleanupPad->getParent();
 }
 
 static void calculateCXXStateNumbers(WinEHFuncInfo &FuncInfo,
                                      const Instruction *FirstNonPHI,
                                      int ParentState) {
   const BasicBlock *BB = FirstNonPHI->getParent();
static int addClrEHHandler(WinEHFuncInfo &FuncInfo, int ParentState,
   Entry.Parent = ParentState;
   Entry.Handler = Handler;
   Entry.HandlerType = HandlerType;
   Entry.TypeToken = TypeToken;
   FuncInfo.ClrEHUnwindMap.push_back(Entry);
   return FuncInfo.ClrEHUnwindMap.size() - 1;
 }
 
+static bool isTopLevelPadForCLR(const Instruction *EHPad) {
+  // For the CLR, a pad is "top-level" if it is not a catchpad and exceptions
+  // that propagate out of it continue straight up to the function's caller.
+
+  // A catchswitch is directly annotated as unwinding to caller or not.
+  // Note that upstream code might mark a catchswitch as "unwinds to caller"
+  // when really what it proved is that the catchswitch never takes its unwind
+  // edge (e.g. SimplifyUnreachable).  If we hit that case here, we will end up
+  // reporting such a catchswitch as top-level -- i.e. not inside any protected
+  // region.  The "missing" entries in the EH table should be benign, as they
+  // would describe an unwind that can never happen.
+  if (const auto *CSI = dyn_cast<CatchSwitchInst>(EHPad))
+    return CSI->unwindsToCaller();
+
+  // Rather than redundantly mark catchpads and their enclosing catchswitches
+  // as toplevel when both unwind to caller, just rely on visiting the catchpad
+  // as a consequence of visiting its catchswitch.
+  if (isa<CatchPadInst>(EHPad))
+    return false;
+
+  // The pad is a cleanuppad, which is the tricky case because what we want
+  // to know is if exceptions propagating out of it escape to the caller or
+  // not, but don't have direct linkage to that information.  The information
+  // will be available on any cleanuprets for the cleanuppad, as well as any
+  // nested catchpads.  Likewise for nested cleanuppads, which makes the
+  // problem recursive, so use a worklist, seeded with the given pad.
+  SmallVector<const CleanupPadInst *, 4> Worklist;
+  SmallPtrSet<const CleanupPadInst *, 4> UselessCleanups;
+  do {
+    Worklist.push_back(cast<CleanupPadInst>(EHPad));
+    do {
+      const CleanupPadInst *Cleanup = Worklist.pop_back_val();
+      for (const User *U : Cleanup->users()) {
+        if (const auto *CRI = dyn_cast<CleanupReturnInst>(U))
+          return CRI->unwindsToCaller();
+        if (const auto *CSI = dyn_cast<CatchSwitchInst>(U)) {
+          if (CSI->hasUnwindDest())
+            return false;
+          // This catchswitch is annotated as "unwinds to caller", but we can't
+          // trust that means what it says (e.g. SimplifyUnreachable might have
+          // rewritten it that way because it initially unwound to an inner
+          // cleanuppad containing unreachable).  So just ignore this user.
+          continue;
+        }
+        // FIXME: This should also be checking for invokes in the cleanup; their
+        // unwind dest must also agree.  Calls in the cleanup have the same
+        // issue as catchswitch, that they might not necessarily imply unwinding
+        // to caller.
+        if (const auto *CPI = dyn_cast<CleanupPadInst>(U))
+          if (UselessCleanups.count(CPI))
+            Worklist.push_back(CPI);
+      }
+    } while (!Worklist.empty());
+    // Couldn't find any unwind edges within the cleanup.  We have to keep it
+    // within its parent funclet, but can assume that it is not in any try
+    // regions within the parent, so try to find the parent's unwind dest.
+    const Value *Parent = cast<CleanupPadInst>(EHPad)->getParentPad();
+    // Check for the "none" sentinel that indicates this cleanup isn't nested
+    // in another funclet, which implies it is top-level.
+    if (isa<ConstantTokenNone>(Parent))
+      return true;
+    // As above, catchpads are never considered toplevel, because their
+    // catchswitches may be and they will be visited from their catchswitch.
+    if (isa<CatchPadInst>(Parent))
+      return false;
+    // Parent must be a cleanuppad; report this one as useless so we don't
+    // revisit it, and recurse up to the parent.
+    UselessCleanups.insert(cast<CleanupPadInst>(EHPad));
+    EHPad = cast<Instruction>(Parent);
+  } while (true);
+}
+
 void llvm::calculateClrEHStateNumbers(const Function *Fn,
                                       WinEHFuncInfo &FuncInfo) {
   // Return if it's already been done.
   if (!FuncInfo.EHPadStateMap.empty())
     return;
 
   SmallVector<std::pair<const Instruction *, int>, 8> Worklist;
 
   // Each pad needs to be able to refer to its parent, so scan the function
   // looking for top-level handlers and seed the worklist with them.
   for (const BasicBlock &BB : *Fn) {
     if (!BB.isEHPad())
       continue;
     if (BB.isLandingPad())
       report_fatal_error("CoreCLR EH cannot use landingpads");
     const Instruction *FirstNonPHI = BB.getFirstNonPHI();
-    if (!isTopLevelPadForMSVC(FirstNonPHI))
+    if (!isTopLevelPadForCLR(FirstNonPHI))
       continue;
     // queue this with sentinel parent state -1 to mean unwind to caller.
     Worklist.emplace_back(FirstNonPHI, -1);
   }
 
   while (!Worklist.empty()) {
     const Instruction *Pad;
     int ParentState;
     std::tie(Pad, ParentState) = Worklist.pop_back_val();
 
-    Value *ParentPad;
-    int PredState;
+    int SelfState;
     if (const CleanupPadInst *Cleanup = dyn_cast<CleanupPadInst>(Pad)) {
       // A cleanup can have multiple exits; don't re-process after the first.
       if (FuncInfo.EHPadStateMap.count(Cleanup))
         continue;
       // CoreCLR personality uses arity to distinguish faults from finallies.
       const BasicBlock *PadBlock = Cleanup->getParent();
       ClrHandlerType HandlerType =
-          (Cleanup->getNumOperands() ? ClrHandlerType::Fault
+          (Cleanup->getNumArgOperands() ? ClrHandlerType::Fault
                                         : ClrHandlerType::Finally);
-      int NewState =
+      SelfState =
           addClrEHHandler(FuncInfo, ParentState, HandlerType, 0, PadBlock);
-      FuncInfo.EHPadStateMap[Cleanup] = NewState;
-      // Propagate the new state to all preds of the cleanup
-      ParentPad = Cleanup->getParentPad();
-      PredState = NewState;
     } else if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(Pad)) {
-      SmallVector<const CatchPadInst *, 1> Handlers;
-      for (const BasicBlock *CatchPadBB : CatchSwitch->handlers()) {
-        const auto *Catch = cast<CatchPadInst>(CatchPadBB->getFirstNonPHI());
-        Handlers.push_back(Catch);
-      }
-      FuncInfo.EHPadStateMap[CatchSwitch] = ParentState;
+      // Walk the catchpads in reverse order since the early ones are reported
+      // like descendants of the later ones in the EH tables.
       int NewState = ParentState;
-      for (auto HandlerI = Handlers.rbegin(), HandlerE = Handlers.rend();
-           HandlerI != HandlerE; ++HandlerI) {
-        const CatchPadInst *Catch = *HandlerI;
-        const BasicBlock *PadBlock = Catch->getParent();
+      SmallVector<const BasicBlock *, 4> CatchBlocks(CatchSwitch->handlers());
+      for (auto CBI = CatchBlocks.rbegin(), CBE = CatchBlocks.rend();
+           CBI != CBE; ++CBI) {
+        const auto *Catch = cast<CatchPadInst>((*CBI)->getFirstNonPHI());
         uint32_t TypeToken = static_cast<uint32_t>(
             cast<ConstantInt>(Catch->getArgOperand(0))->getZExtValue());
         NewState = addClrEHHandler(FuncInfo, NewState, ClrHandlerType::Catch,
-                                   TypeToken, PadBlock);
+                                   TypeToken, *CBI);
         FuncInfo.EHPadStateMap[Catch] = NewState;
       }
-      for (const auto *CatchPad : Handlers) {
-        for (const User *U : CatchPad->users()) {
-          const auto *UserI = cast<Instruction>(U);
-          if (UserI->isEHPad())
-            Worklist.emplace_back(UserI, ParentState);
-        }
-      }
-      PredState = NewState;
-      ParentPad = CatchSwitch->getParentPad();
+      // The catchswitch uses the same state number as the first catch (or, if
+      // we ever see an empty catchswitch, the state number of its successor).
+      SelfState = NewState;
     } else {
       llvm_unreachable("Unexpected EH pad");
     }
 
+    // Record this pad's state.
+    FuncInfo.EHPadStateMap[Pad] = SelfState;
+
     // Queue all predecessors with the given state
     for (const BasicBlock *Pred : predecessors(Pad->getParent())) {
-      if ((Pred = getEHPadFromPredecessor(Pred, ParentPad)))
-        Worklist.emplace_back(Pred->getFirstNonPHI(), PredState);
+      if ((Pred = getEHPadFromPredecessor(Pred, nullptr)))
+        Worklist.emplace_back(Pred->getFirstNonPHI(), SelfState);
     }
   }
 
   calculateStateNumbersForInvokes(Fn, FuncInfo);
 }
 
 void WinEHPrepare::replaceTerminatePadWithCleanup(Function &F) {
   if (Personality != EHPersonality::MSVC_CXX)
     return;
   for (BasicBlock &BB : F) {
     Instruction *First = BB.getFirstNonPHI();
     auto *TPI = dyn_cast<TerminatePadInst>(First);
     if (!TPI)

test/CodeGen/X86/wineh-coreclr.ll

 ; CHECK: leaq [[FPOffset:[0-9]+]](%rsp), %rbp
 ; CHECK: .seh_endprologue
 ; CHECK: movq %rsp, [[PSPSymOffset:[0-9]+]](%rsp)
 ; CHECK: [[L_before_f1:.+]]:
 ; CHECK-NEXT: movl $1, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f1:.+]]:
   invoke void @f(i32 1)
-    to label %inner_try unwind label %finally.pad
+    to label %inner_try unwind label %finally
 inner_try:
 ; CHECK: # %inner_try
 ; CHECK: [[L_before_f2:.+]]:
 ; CHECK-NEXT: movl $2, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f2:.+]]:
   invoke void @f(i32 2)
-    to label %finally.clone unwind label %catch1.pad
-catch1.pad:
-  %cs1 = catchswitch within none [label %catch1.body, label %catch2.body] unwind label %finally.pad
-catch1.body:
-  %catch1 = catchpad within %cs1 [i32 1]
+    to label %finally.clone unwind label %exn.dispatch
+exn.dispatch:
+  %catchswitch = catchswitch within none [label %catch1, label %catch2] unwind label %finally
+catch1:
+  %catch.pad1 = catchpad within %catchswitch [i32 1]
 ; CHECK: .seh_proc [[L_catch1:[^ ]+]]
 ; CHECK: .seh_stackalloc [[FuncletFrameSize:[0-9]+]]
 ;                        ^ all funclets use the same frame size
 ; CHECK: movq [[PSPSymOffset]](%rcx), %rcx
 ;                              ^ establisher frame pointer passed in rcx
 ; CHECK: movq %rcx, [[PSPSymOffset]](%rsp)
 ; CHECK: leaq [[FPOffset]](%rcx), %rbp
 ; CHECK: .seh_endprologue
 ; CHECK: movq %rdx, %rcx
 ;             ^ exception pointer passed in rdx
 ; CHECK-NEXT: callq g
-  %exn1 = call i8 addrspace(1)* @llvm.eh.exceptionpointer.p1i8(token %catch1)
+  %exn1 = call i8 addrspace(1)* @llvm.eh.exceptionpointer.p1i8(token %catch.pad1)
   call void @g(i8 addrspace(1)* %exn1)
 ; CHECK: [[L_before_f3:.+]]:
 ; CHECK-NEXT: movl $3, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f3:.+]]:
   invoke void @f(i32 3)
-    to label %catch1.ret unwind label %finally.pad
+    to label %catch1.ret unwind label %finally
 catch1.ret:
-  catchret from %catch1 to label %finally.clone
-catch2.body:
-  %catch2 = catchpad within %cs1 [i32 2]
+  catchret from %catch.pad1 to label %finally.clone
+catch2:
+  %catch.pad2 = catchpad within %catchswitch [i32 2]
 ; CHECK: .seh_proc [[L_catch2:[^ ]+]]
 ; CHECK: .seh_stackalloc [[FuncletFrameSize:[0-9]+]]
 ;                        ^ all funclets use the same frame size
 ; CHECK: movq [[PSPSymOffset]](%rcx), %rcx
 ;                              ^ establisher frame pointer passed in rcx
 ; CHECK: movq %rcx, [[PSPSymOffset]](%rsp)
 ; CHECK: leaq [[FPOffset]](%rcx), %rbp
 ; CHECK: .seh_endprologue
 ; CHECK: movq %rdx, %rcx
 ;             ^ exception pointer passed in rdx
 ; CHECK-NEXT: callq g
-  %exn2 = call i8 addrspace(1)* @llvm.eh.exceptionpointer.p1i8(token %catch2)
+  %exn2 = call i8 addrspace(1)* @llvm.eh.exceptionpointer.p1i8(token %catch.pad2)
   call void @g(i8 addrspace(1)* %exn2)
 ; CHECK: [[L_before_f4:.+]]:
 ; CHECK-NEXT: movl $4, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f4:.+]]:
   invoke void @f(i32 4)
-    to label %try_in_catch unwind label %finally.pad
+    to label %try_in_catch unwind label %finally
 try_in_catch:
 ; CHECK: # %try_in_catch
 ; CHECK: [[L_before_f5:.+]]:
 ; CHECK-NEXT: movl $5, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f5:.+]]:
   invoke void @f(i32 5)
-    to label %catch2.ret unwind label %fault.pad
-fault.pad:
+    to label %catch2.ret unwind label %fault
+fault:
 ; CHECK: .seh_proc [[L_fault:[^ ]+]]
-  %fault = cleanuppad within none [i32 undef]
+  %fault.pad = cleanuppad within %catch.pad2 [i32 undef]
 ; CHECK: .seh_stackalloc [[FuncletFrameSize:[0-9]+]]
 ;                        ^ all funclets use the same frame size
 ; CHECK: movq [[PSPSymOffset]](%rcx), %rcx
 ;                              ^ establisher frame pointer passed in rcx
 ; CHECK: movq %rcx, [[PSPSymOffset]](%rsp)
 ; CHECK: leaq [[FPOffset]](%rcx), %rbp
 ; CHECK: .seh_endprologue
 ; CHECK: [[L_before_f6:.+]]:
 ; CHECK-NEXT: movl $6, %ecx
 ; CHECK-NEXT: callq f
 ; CHECK-NEXT: [[L_after_f6:.+]]:
   invoke void @f(i32 6)
-    to label %fault.ret unwind label %finally.pad
+    to label %fault.ret unwind label %finally
 fault.ret:
-  cleanupret from %fault unwind label %finally.pad
+  cleanupret from %fault.pad unwind label %finally
 catch2.ret:
-  catchret from %catch2 to label %finally.clone
+  catchret from %catch.pad2 to label %finally.clone
 finally.clone:
   call void @f(i32 7)
   br label %tail
-finally.pad:
+finally:
 ; CHECK: .seh_proc [[L_finally:[^ ]+]]
-  %finally = cleanuppad within none []
+  %finally.pad = cleanuppad within none []
 ; CHECK: .seh_stackalloc [[FuncletFrameSize:[0-9]+]]
 ;                        ^ all funclets use the same frame size
 ; CHECK: movq [[PSPSymOffset]](%rcx), %rcx
 ;                              ^ establisher frame pointer passed in rcx
 ; CHECK: movq %rcx, [[PSPSymOffset]](%rsp)
 ; CHECK: leaq [[FPOffset]](%rcx), %rbp
 ; CHECK: .seh_endprologue
 ; CHECK-NEXT: movl $7, %ecx
 ; CHECK-NEXT: callq f
   call void @f(i32 7)
-  cleanupret from %finally unwind to caller
+  cleanupret from %finally.pad unwind to caller
 tail:
   call void @f(i32 8)
   ret void
 ; CHECK: [[L_end:.*func_end.*]]:
 }
 
-; FIXME: Verify that the new clauses are correct and re-enable these checks.
-
 ; Now check for EH table in xdata (following standard xdata)
-; CHECKX-LABEL: .section .xdata
+; CHECK-LABEL: .section .xdata
 ; standard xdata comes here
-; CHECKX:      .long 4{{$}}
+; CHECK:      .long 4{{$}}
 ;                   ^ number of funclets
-; CHECKX-NEXT: .long [[L_catch1]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_catch1]]-[[L_begin]]
 ;                   ^ offset from L_begin to start of 1st funclet
-; CHECKX-NEXT: .long [[L_catch2]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_catch2]]-[[L_begin]]
 ;                   ^ offset from L_begin to start of 2nd funclet
-; CHECKX-NEXT: .long [[L_fault]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_fault]]-[[L_begin]]
 ;                   ^ offset from L_begin to start of 3rd funclet
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset from L_begin to start of 4th funclet
-; CHECKX-NEXT: .long [[L_end]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_end]]-[[L_begin]]
 ;                   ^ offset from L_begin to end of last funclet
-; CHECKX-NEXT: .long 7
+; CHECK-NEXT: .long 7
 ;                   ^ number of EH clauses
 ; Clause 1: call f(2) is guarded by catch1
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ flags (0 => catch handler)
-; CHECKX-NEXT: .long ([[L_before_f2]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f2]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_catch1]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_catch1]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_catch2]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_catch2]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 1
+; CHECK-NEXT: .long 1
 ;                   ^ type token of catch (from catchpad)
 ; Clause 2: call f(2) is also guarded by catch2
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ flags (0 => catch handler)
-; CHECKX-NEXT: .long ([[L_before_f2]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f2]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_catch2]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_catch2]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_fault]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_fault]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 2
+; CHECK-NEXT: .long 2
 ;                   ^ type token of catch (from catchpad)
 ; Clause 3: calls f(1) and f(2) are guarded by finally
-; CHECKX-NEXT: .long 2
+; CHECK-NEXT: .long 2
 ;                   ^ flags (2 => finally handler)
-; CHECKX-NEXT: .long ([[L_before_f1]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f1]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f2]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_end]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_end]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ type token slot (null for finally)
 ; Clause 4: call f(3) is guarded by finally
 ;           This is a "duplicate" because the protected range (f(3))
 ;           is in funclet catch1 but the finally's immediate parent
 ;           is the main function, not that funclet.
-; CHECKX-NEXT: .long 10
+; CHECK-NEXT: .long 10
 ;                   ^ flags (2 => finally handler | 8 => duplicate)
-; CHECKX-NEXT: .long ([[L_before_f3]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f3]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f3]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f3]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_end]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_end]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ type token slot (null for finally)
 ; Clause 5: call f(5) is guarded by fault
-; CHECKX-NEXT: .long 4
+; CHECK-NEXT: .long 4
 ;                   ^ flags (4 => fault handler)
-; CHECKX-NEXT: .long ([[L_before_f5]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f5]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f5]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f5]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_fault]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_fault]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ type token slot (null for fault)
 ; Clause 6: calls f(4) and f(5) are guarded by finally
 ;           This is a "duplicate" because the protected range (f(4)-f(5))
 ;           is in funclet catch2 but the finally's immediate parent
 ;           is the main function, not that funclet.
-; CHECKX-NEXT: .long 10
+; CHECK-NEXT: .long 10
 ;                   ^ flags (2 => finally handler | 8 => duplicate)
-; CHECKX-NEXT: .long ([[L_before_f4]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f4]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f5]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f5]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_end]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_end]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ type token slot (null for finally)
 ; Clause 7: call f(6) is guarded by finally
 ;           This is a "duplicate" because the protected range (f(3))
 ;           is in funclet catch1 but the finally's immediate parent
 ;           is the main function, not that funclet.
-; CHECKX-NEXT: .long 10
+; CHECK-NEXT: .long 10
 ;                   ^ flags (2 => finally handler | 8 => duplicate)
-; CHECKX-NEXT: .long ([[L_before_f6]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_before_f6]]-[[L_begin]])+1
 ;                   ^ offset of start of clause
-; CHECKX-NEXT: .long ([[L_after_f6]]-[[L_begin]])+1
+; CHECK-NEXT: .long ([[L_after_f6]]-[[L_begin]])+1
 ;                   ^ offset of end of clause
-; CHECKX-NEXT: .long [[L_finally]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_finally]]-[[L_begin]]
 ;                   ^ offset of start of handler
-; CHECKX-NEXT: .long [[L_end]]-[[L_begin]]
+; CHECK-NEXT: .long [[L_end]]-[[L_begin]]
 ;                   ^ offset of end of handler
-; CHECKX-NEXT: .long 0
+; CHECK-NEXT: .long 0
 ;                   ^ type token slot (null for finally)