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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:145
   // The default format of the object file is XCOFF32.
   InitFileHdr.Magic = XCOFF::XCOFF32;
   InitFileHdr.NumberOfSections = Obj.Sections.size();
----------------
Shouldn't this be updated to assign `XCOFF64` magic number when `Is64Bit` is true? It seems odd not to, although looking at the other parts of the code I think it will still work. I missed the previous review, so can you help me understand why we need an `InitFileHdr` and the file header that's a member of Obj? The mixture of duplication, and sometimes having the relevant/correct info on Obj.Header and sometimes on InitFileHdr is rather cumbersome to follow.


================
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;
+}
----------------
jhenderson wrote:
> 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).
Yes, 32-bit XCOFF can store the name directly in the symbol table entry if it fits (8 bytes or less), otherwise it gets placed into the string table and then the first 4 bytes of the field are zeros followed by a 4-byte offset into the string table. The 64-bit format avoids the extra complexity and always places the name in the string table.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100375