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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:350
+    // Write the string table.
+    Strings.write(W.OS);
+  }
----------------
Esme wrote:
> jhenderson wrote:
> > 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?
> The string table contains only long symbol names, so it's reasonable that there would be no string table without symbols.
Right, but we should probably have a test for that case, i.e. show that a no-symbol object produces no string table or an empty string table as appropriate.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:287
       }
       if (PaddingSize > 0)
         W.OS.write_zeros(PaddingSize);
----------------
Didn't notice this before when reviewing the 32-bit implementation: why is this check here? Same comment for other such checks.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml:22
+Symbols:
+  - Name:            .file
+    Section:         N_DEBUG
----------------
Similar to my comments on the string table review - it would make sense to have a check in this test which shows all the symbol names are in the string table and not elsewhere, I think, by dumping the string table contents.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100375