Page MenuHomePhabricator

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

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

Details

Reviewers
shchenz
jhenderson
Higuoxing
MaskRay
jasonliu
Group Reviewers
Restricted Project
Summary

Add support for yaml2obj to parse 64-bit XCOFF.
The patch depends on D95505, which initializes 32-bit support, and D85774, as the patch provides 64-bit llvm-readobj for the test case.

Diff Detail

Event Timeline

Esme created this revision.Tue, Apr 13, 3:24 AM
Esme requested review of this revision.Tue, Apr 13, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 13, 3:24 AM
Higuoxing added inline comments.Tue, Apr 13, 6:10 PM
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

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)

229

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

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

Addressed Higuoxing's comments.

llvm/include/llvm/BinaryFormat/XCOFF.h
263

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

llvm/lib/ObjectYAML/XCOFFEmitter.cpp
229

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

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

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.