Add support for yaml2obj to parse 64-bit XCOFF.
Details
- Reviewers
shchenz jhenderson Higuoxing MaskRay jasonliu sfertile - Group Reviewers
Restricted Project - Commits
- rG8011fc195383: [yaml2obj] Enable support for parsing 64-bit XCOFF.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
263 | It looks that FileHeader32 and FileHeader64 are very similar. Instead of creating a seperate struct, can we change the type of SymbolTableFileOffset to uint64_t in FileHeader32, and rename it to FileHeader? struct FileHeader { uint16_t Magic; uint16_t NumberOfSections; int32_t TimeStamp; uint64_t SymbolTableFileOffset; int32_t NumberOfSymbolTableEntries; uint16_t AuxiliaryHeaderSize; uint16_t Flags; }; | |
263 | Ditto. struct SectionHeader { char Name[XCOFF::NameSize]; uint64_t PhysicalAddress; uint64_t VirtualAddress; uint64_t Size; uint64_t FileOffsetToData; uint64_t FileOffsetToRelocations; uint64_t FileOffsetToLineNumbers; uint32_t NumberOfRelocations; uint32_t NumberOfLineNumbers; int32_t Flags; char Padding[4]; }; | |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
41 |
| |
229 | It looks that char Padding[4]; isn't used here? |
The basic code looks good, but I have no concept of whether it is correct or not.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
69 | 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). | |
348 | 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? | |
llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml | ||
90 | Nit here and below. |
Addressed James's comments.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
348 | The string table contains only long symbol names, so it's reasonable that there would be no string table without symbols. |
Thanks for working on this. I'm excited to finally replace all the precompiled objects we used for testing the XCOFF tools implementation. I've only got one initial comment since I'm not real familiar with this part fo the code yet, but will spend some more time going over this tomorrow.
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
29 | Please put the replacement of FileHeader32/SectionHeader32 structs by FileHeaderSize32/SectionHeaderSize32 constants into an NFC patch and commit it separately. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
276–277 | Didn't notice this before when reviewing the 32-bit implementation: why is this check here? Same comment for other such checks. | |
348 | 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. | |
llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml | ||
23 | 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. |
As for the need of testing the string table, I will add such test after llvm-readobj --string-table is implemented.
For now, I can use the system tool dump under AIX to verify the results.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
276–277 | As Zheng's comment in D95505
|
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
276–277 | You're right that we don't have to, but by adding the guard, you complicate the logic. Calling it is harmless. For a similar case, you don't put an if (!x.empty()) around a loop which iterates over the elements of x, for example. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
69 | 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. | |
134 | 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. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
276–277 | Either way is ok to me. Both ways have their pros and cons. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
134 | Most of the key-values are optional in YAML, which allows users to write test inputs with more flexibility. For some optional fields, we have to calculated the default values that are derived from the contents of the YAML, like InitFileHdr.NumberOfSymTableEntries. And we have an option to override that value when writing to object file, that is, use the derived value when the field is omitted, otherwise keep the value that defined in YAML. The MagicNumber in FileHeader is optional, and we use XCOFF::XCOFF32 as a default value if user did not define the key-value in YAML. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
134 | In addition, for some values such as offset, they must be finalized in advance to write a correct object file, regardless of whether the value is omitted. |
Looks good barring a nit, from my point of view, but you should get someone who knows the XCOFF file format better to review this to ensure the content is correct and tested satisfactorily.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
68–69 |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
191 | number of entries in symbol table should be after SymbolTableOffset. This should align with XCOFF32? | |
298–299 | delete the check to align with others | |
326 | Would it be more sense to use the defined const SymbolTableEntrySize for 18? | |
llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml | ||
74 | The physicaladdress and the virtualaddress should be not 0 as the previous section's size is 8? |
Address comments.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
191 | We should not change the order. | |
llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml | ||
74 | It's a non-data/text/bss section, so the address is set to 0 as the spec required. I will change the section type to bss to check the address value. thx. |
llvm/lib/ObjectYAML/XCOFFEmitter.cpp | ||
---|---|---|
68–69 | Ping - this comment hasn't been addressed. |
Please put the replacement of FileHeader32/SectionHeader32 structs by FileHeaderSize32/SectionHeaderSize32 constants into an NFC patch and commit it separately.