[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR
Andy Kaylor via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 7 13:11:11 PST 2015
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
More information about the llvm-commits
mailing list