[PATCH] D92058: [XCOFF][AIX] Alternative path in EHStreamer for platforms do not have uleb128 support

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:510
 
+  // An alternative path of EmitTypeTableRefAndCallSiteTableEndRef.
+  // For some platforms, the system assembler does not accept the form of
----------------
s/of/to/; to express the actual relationship.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:512
+  // For some platforms, the system assembler does not accept the form of
+  // `.uleb128 label2 - label1`. In those situation, we would need to calculate
+  // the size between label1 and label2 manually.
----------------
Minor nit: grammar.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:514
+  // the size between label1 and label2 manually.
+  // In this case, we would need to calculate the LSDA size, and the call
+  // site table size.
----------------
Minor nit: no comma for two-element list.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:518
+    assert(CallSiteEncoding == dwarf::DW_EH_PE_udata4 && !HasULEB128Directive &&
+           "Target supports .uleb128 do not need to take this path.");
+    if (CallSiteRanges.size() > 1)
----------------
Minor nit: grammar.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:521
+      report_fatal_error("-fbasic-block-sections is not yet supported on "
+                         "platforms that do not have .uleb128 directive.");
+
----------------
Since AIX has a .uleb128 directive, just not one that is fully flexible:
s/.uleb128 directive/general LEB128 directive support/;


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:526
+    for (size_t CallSiteIdx = CSRange.CallSiteBeginIdx;
+         CallSiteIdx != CSRange.CallSiteEndIdx; ++CallSiteIdx) {
+      const CallSiteEntry &S = CallSites[CallSiteIdx];
----------------
Minor nit: this is a comparison on an index, this `<` can be used.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:528
+      const CallSiteEntry &S = CallSites[CallSiteIdx];
+      // Each call site entry is consist of 3 udata4 fields (12 bytes) and
+      // 1 uleb128 field.
----------------
Minor nit: grammar.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:529
+      // Each call site entry is consist of 3 udata4 fields (12 bytes) and
+      // 1 uleb128 field.
+      CallSiteTableSize += 12 + getULEB128Size(S.Action);
----------------
The DWARF specification is consistent in using all caps for the abbreviation of "little-endian base 128".


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:530
+      // 1 uleb128 field.
+      CallSiteTableSize += 12 + getULEB128Size(S.Action);
+    }
----------------
Consider implementing overflow checking here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92058/new/

https://reviews.llvm.org/D92058