[PATCH] D100375: [yaml2obj] Enable support for parsing 64-bit XCOFF.

Fri Apr 30 02:07:54 PDT 2021

jhenderson added a comment.

The basic code looks good, but I have no concept of whether it is correct or not.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:68
+  // For XCOFF64: The symbol name is always in string table or .debug section.
+  return (SymbolName.size() > XCOFF::NameSize) || is64Bit;
+}
----------------
If I'm following it correctly, XCOFF32 can use a string table, and XCOFF64 always does?

If that's the case it would be a good idea to split this change in two. The first adds string table support to XCOFF32 and the second adds XCOFF64 support. It also ensures we have testing for XCOFF32 + using a string table (which I don't think this change currently has, if I'm not mistaken).

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:350
+    // Write the string table.
+    Strings.write(W.OS);
+  }
----------------
I don't know if this makes sense, but do we need a test case where there are no symbols, and therefore no string table?

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml:89
+
+#SYMBOLS64:       Symbols [
+#SYMBOLS64-NEXT:    Symbol {
----------------
Nit here and below.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100375