[PATCH] D13451: [WinEH] Add CoreCLR EH table emission

[Joseph Tremoulet via llvm-commits]

JosephTremoulet marked 8 inline comments as done.
JosephTremoulet added a comment.

Thanks for the review!  I believe I've incorporated everything.


================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1011
@@ +1010,3 @@
+  assert(NumStates > 0 && "Don't need exception table!");
+  DenseMap<const MachineBasicBlock *, int> HandlerStates;
+  for (int State = 0; State < NumStates; ++State) {
----------------
Yes.  I've expanded the comment to be more verbose.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1031
@@ +1030,3 @@
+  // 1. Emit a list of the offsets to each handler entry, in lexical order.
+  // 2. Compute a map (EndSymbolMap) from each funclet to the symbol at its end.
+  // 3. Compute the list of ClrClauses, in the required order (inner before
----------------
Part of the complication here is that nothing in the CLR docs/code has this notion of "EH states".  I was referring to these things as states because it's the logical counterpart of what gets computed in ComputeStateNumbering for the other personalities.  Really the "states" as they're used here are identifiers for the handlers and/or for the funclets (handlers and funclets have a 1:1 correspondence for CLR EH), and specifically they're the index of the corresponding entry in the unwind map.  I've updated the comments throughout this function to be a bit more careful/verbose about that, let me know if you think further editing would be helpful.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1041
@@ +1040,3 @@
+  //    top-level function contains a try clause targeting the key handler.
+
+  // PendingStartLabel is the EH label preceding the most recently visited
----------------
Hopefully this is clear given the above.  "Low state" doesn't really mean anything to me.  I'm talking about MBB membership here, so "the handler" means the funclet that this MBB is a (top-level) member of.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1053
@@ +1052,3 @@
+  // PendingEndLabel is the EH label following the most recently visited
+  // invoke.
+  MCSymbol *PendingEndLabel = nullptr;
----------------
Sure.  I've added a check above that parent states are strictly less than child states, which will guarantee this loop terminates, and added an explicit assertion that PendingState never reaches NullState to be more easily diagnosable than the bounds assertion in the vector access.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1128
@@ +1127,3 @@
+        // This is a call which may unwind to the caller; indicate this by
+        // identifying its handler as the toplevel NullState.
+        NewState = NullState;
----------------
This one I didn't change, since the PendingState we're walking to here was set to AncestorState in the call to RewindPendingTo a few lines above, which in turn was computed by `getAncestor(..., NewState)`.  So if we jumped over PendingState here it would be a bug in getAncestor, not bad input.  I think the implicit check in the vector indexer is sufficient.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.h:68
@@ -65,1 +67,3 @@
+  const MCExpr *getOffset(const MCSymbol *OffsetOf, const MCSymbol *OffsetFrom);
+  const MCExpr *getOffset(const MCExpr *OffsetOf, const MCSymbol *OffsetFrom);
 
----------------
They can and should be.  Fixed, thanks.


http://reviews.llvm.org/D13451