This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] Enable support for parsing 64-bit XCOFF.
ClosedPublic

Authored by Esme on Apr 13 2021, 3:24 AM.

Details

Summary

Add support for yaml2obj to parse 64-bit XCOFF.

Diff Detail

Event Timeline

Esme created this revision.Apr 13 2021, 3:24 AM
Esme requested review of this revision.Apr 13 2021, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 3:24 AM
Higuoxing added inline comments.Apr 13 2021, 6:10 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
269

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;
};
292

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
40

StringRef is small and pervasive enough in LLVM that it should always be passed by value. (https://bcain-llvm.readthedocs.io/projects/llvm/en/latest/ProgrammersManual/#the-stringref-class)

231

It looks that char Padding[4]; isn't used here?

Esme updated this revision to Diff 338422.Apr 18 2021, 8:29 PM

Addressed Higuoxing's comments.

llvm/include/llvm/BinaryFormat/XCOFF.h
269

It seems we could remove these structs and define size constants directly.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
231

Here we padded 4 bytes in the end of SectionsHeader64, which is a reserved field.

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

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
68

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).

350

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.

Esme updated this revision to Diff 343565.May 6 2021, 7:41 PM

Addressed James's comments.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
350

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.

Esme updated this revision to Diff 350606.Jun 8 2021, 8:25 AM

Rebase.

Esme updated this revision to Diff 350771.Jun 8 2021, 8:16 PM

Rebase.

jhenderson added inline comments.Jun 9 2021, 12:37 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
278–279

Didn't notice this before when reviewing the 32-bit implementation: why is this check here? Same comment for other such checks.

350

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.

Esme added a comment.EditedJun 9 2021, 1:48 AM

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
278–279

As Zheng's comment in D95505

We don't have to call write_zeros if PaddingSize is 0

jhenderson added inline comments.Jun 9 2021, 1:54 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
278–279

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.

sfertile added inline comments.Jun 14 2021, 6:08 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
68

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.

135

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.

shchenz added inline comments.Jun 14 2021, 9:43 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
278–279

Either way is ok to me. Both ways have their pros and cons.
We used to add a guard for the same cases(write_zeros() for padding) in XCOFFObjectWriter.cpp.

Esme added inline comments.Jun 15 2021, 8:26 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
135

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.

Esme added inline comments.Jun 15 2021, 8:33 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
135

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.

Esme updated this revision to Diff 356863.Jul 6 2021, 9:45 PM
Esme edited the summary of this revision. (Show Details)

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
65–71
Esme added a comment.Jul 7 2021, 12:46 AM

Thanks! @jhenderson
Hi, @sfertile, could you help to review the patch? thx.

shchenz added inline comments.Jul 27 2021, 7:57 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
193

number of entries in symbol table should be after SymbolTableOffset. This should align with XCOFF32?

300–301

delete the check to align with others

328

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?

Esme updated this revision to Diff 362272.Jul 27 2021, 9:07 PM

Address comments.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
193

We should not change the order.
In XCOFF32, the order of fields is SymbolTableOffset, NumberOfSymTableEntries, AuxHeaderSize, Flags.
In XCOFF64, the order is SymbolTableOffset, AuxHeaderSize, Flags, NumberOfSymTableEntries.

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.

jhenderson added inline comments.Jul 28 2021, 12:01 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
65–71

Ping - this comment hasn't been addressed.

Esme updated this revision to Diff 362289.Jul 28 2021, 12:06 AM

Addressed.

shchenz accepted this revision.Jul 28 2021, 12:08 AM

LGTM. Thanks for doing this.

This revision is now accepted and ready to land.Jul 28 2021, 12:08 AM
This revision was landed with ongoing or failed builds.Jul 29 2021, 7:06 PM
This revision was automatically updated to reflect the committed changes.