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

[Reid Kleckner via llvm-commits]

rnk added a comment.

Some comments. I'm mainly interested in sharing the logic around EH_LABEL before committing this.


================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:976-977
@@ +975,4 @@
+                                      const MCSymbol *OffsetFrom) {
+  return MCBinaryExpr::createSub(create32bitRef(OffsetOf),
+                                 create32bitRef(OffsetFrom), Asm->OutContext);
+}
----------------
Do you need the IMGREL relocations to compute a label difference? You should just need to create an MCSymbolRefExpr like this code from emitCSpecificHandlerTable:
    const MCExpr *LabelDiff =
        MCBinaryExpr::createSub(MCSymbolRefExpr::create(TableEnd, Ctx),
                                MCSymbolRefExpr::create(TableBegin, Ctx), Ctx);
I think the assembler will do the same thing either way, but the .s file will look cleaner.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1032-1035
@@ +1031,6 @@
+  // 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
+  //    outer, earlier before later; the order by which a forward scan with
+  //    early termination will find the innermost enclosing clause covering
+  //    a given address).
+  // 4. A map (MinClauseMap) from each handler index to the index of the
----------------
Hah, I think you actually solved the problem that I avoided solving for __C_specific_handler. :) I might want to share the code at some point so we can get smaller SEH tables.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1098
@@ +1097,3 @@
+
+    for (const auto &MI : MBB) {
+      MCSymbol *NewStartLabel;
----------------
Can you use or adapt `invoke_ranges` to work here? The EH_LABEL tracking state machine is pretty subtle, I'd rather write it just once.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:1220-1223
@@ +1219,6 @@
+
+    const MCExpr *ClauseBegin =
+        getOffset(getLabelPlusOne(Clause.StartLabel), FuncBeginSym);
+    const MCExpr *ClauseEnd =
+        getOffset(getLabelPlusOne(Clause.EndLabel), FuncBeginSym);
+
----------------
I guess using getLabelPlusOne requires keeping the IMGREL stuff inside the label difference.

================
Comment at: test/CodeGen/WinEH/wineh-coreclr.ll:129
@@ +128,3 @@
+; CHECK-NEXT: .long [[L_catch1]]@IMGREL-[[L_begin]]@IMGREL
+;                   ^ offset from L_begin to start of 1st funclet
+; CHECK-NEXT: .long [[L_catch2]]@IMGREL-[[L_begin]]@IMGREL
----------------
Interesting. I think these cross-funclet label differences will prohibit us from placing CLR funclets into .text$x, unless we tweak them until they are a perfect match for some relocation.


http://reviews.llvm.org/D13451