[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added inline comments.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:169
@@ +168,3 @@
+        dyn_cast<FuncletPadInst>(FuncletEntryBB->getFirstNonPHI());
+    if (!FuncletPad)
+      FuncletUnwindDest = nullptr;
----------------
Am I right in thinking that you could assert(FuncletPad || FuncletEntryBB == &Fn->getEntryBlock()) here?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:215
@@ -250,10 +214,3 @@
 
-static void calculateExplicitCXXStateNumbers(WinEHFuncInfo &FuncInfo,
-                                             const BasicBlock &BB,
-                                             int ParentState) {
-  assert(BB.isEHPad());
-  const Instruction *FirstNonPHI = BB.getFirstNonPHI();
-  // All catchpad instructions will be handled when we process their
-  // respective catchendpad instruction.
-  if (isa<CatchPadInst>(FirstNonPHI))
-    return;
+static void calculateCXXStateNumbers(WinEHFuncInfo &FuncInfo,
+                                     const Instruction *FirstNonPHI,
----------------
This code could really use some comments.  I had to build it and step through some examples in the debugger to understand how it works.  It would be nice to have a single large comment block somewhere explaining the whole process of state numbering for a C++ function in addition to a few remarks sprinkled throughout this function.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:219
@@ +218,3 @@
+  const BasicBlock *BB = FirstNonPHI->getParent();
+  assert(BB->isEHPad() && "no a funclet!");
+
----------------
s/no/not

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:243
@@ +242,3 @@
+    for (const auto *CatchPad : Handlers) {
+      FuncInfo.FuncletBaseStateMap[CatchPad] = CatchLow;
+      for (const User *U : CatchPad->users()) {
----------------
This assigns the same base state to all catchpads within a catchswitch.  That seems sketchy to me.  Consider the following example:
```
void foo() {
  try {
    f(1);
  } catch (int) {
    f(2);
    try {
      f(3);
    } catch (...) { }
  } catch (...) {
    f(4);
    try {
      f(5);
    } catch (...) {}
  }
}
```
Because both outer-level catches get the same base state (1), this state will be assigned to both f(2) and f(4).  I think it's possible that __CxxFrameHandler3 will correctly deal with the same state appearing in two different funclets, but I'm not confident of that.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:261
@@ -290,1 +260,3 @@
+    // cleanupret instructions.
+    if (FuncInfo.EHPadStateMap.count(CleanupPad))
       return;
----------------
With the new implementation, I don't think it is possible to get here twice for the same cleanup pad.  So this could be made an assertion, right?

================
Comment at: lib/IR/Verifier.cpp:3011
@@ -3037,1 +3010,3 @@
+         OuterScope);
 
+  visitTerminatorInst(CatchSwitch);
----------------
Shouldn't this be verifying the unwind destination?

================
Comment at: lib/IR/Verifier.cpp:3022
@@ -3043,3 +3021,3 @@
     Instruction *I = UnwindDest->getFirstNonPHI();
     Assert(I->isEHPad() && !isa<LandingPadInst>(I),
            "CleanupReturnInst must unwind to an EH block which is not a "
----------------
This should also exclude CatchPadInst.

================
Comment at: lib/IR/Verifier.cpp:3048
@@ -3069,3 +3047,3 @@
     Instruction *I = UnwindDest->getFirstNonPHI();
     Assert(I->isEHPad() && !isa<LandingPadInst>(I),
            "TerminatePadInst must unwind to an EH block which is not a "
----------------
This should also exclude CatchPadInst.


http://reviews.llvm.org/D15139