[PATCH] D100375: [yaml2obj] Enable support for parsing 64-bit XCOFF.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 9 00:37:55 PDT 2021
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
More information about the llvm-commits
mailing list