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

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:980
@@ +979,3 @@
+  struct ClrClause {
+    MCSymbol *StartLabel; // Start of protected regino
+    MCSymbol *EndLabel;   // End of protected region
----------------
typo - "regino"

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:997
@@ +996,3 @@
+
+  // Write out a sentinel indicating the end of the xdata proper.
+  OS.EmitIntValue(0xffffffff, 4);
----------------
What do you mean by "the xdata proper" and is this the same thing you are referring to as "the standard xdata" in the review description?

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1017
@@ +1016,3 @@
+  MCSymbol *PendingStartLabel = nullptr;
+  // PendingState is the state of the handler of the most recently visited
+  // invoke.
----------------
I've found that we've been kind of sloppy in the terminology we're using in comments to describe EH states.  When you say "the state of the handler" do you mean the EH state of the code inside the handler or the EH state which is handled by that handler at the most recent invoke?

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1027
@@ +1026,3 @@
+  MCSymbol *PendingEndLabel = nullptr;
+  // EnclosingState is the state corresponding to the function/funclet
+  // that the current MBB belongs to.
----------------
So this is the EH state of code inside the handler?  The low state if the handler has multiple states?

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1039
@@ +1038,3 @@
+  auto RewindPendingTo = [&](int AncestorState) {
+    while (PendingState != AncestorState) {
+      // Close the pending clause
----------------
Can you add some sort of debug check to account for the possibility that a poorly constructed ClrEHUnwindMap skips past AncestorState (i.e. state 2 unwinds to state 0 but AncestorState was 1)?

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1093
@@ +1092,3 @@
+                 !callToNoUnwindFunction(&MI)) {
+        // We don't have a labels handy for this call, but that's ok because
+        // we don't need them.
----------------
typo - "a labels"

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1114
@@ +1113,3 @@
+        for (int EnteredState = NewState; EnteredState != PendingState;
+             EnteredState = FuncInfo.ClrEHUnwindMap[EnteredState].Parent) {
+          int &MinEnclosingState = MinClauseMap[EnteredState];
----------------
Again, it seems like there is a possibility here of jumping over PendingState.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.h:68
@@ -65,1 +67,3 @@
+  const MCExpr *getOffset(MCSymbol *OffsetOf, MCSymbol *OffsetFrom);
+  const MCExpr *getOffset(const MCExpr *OffsetOf, MCSymbol *OffsetFrom);
 
----------------
Is there a reason only one of these parameters (for the two functions above) is const?  It seems like they all could be.


http://reviews.llvm.org/D13451