Page MenuHomePhabricator

[XCOFF][AIX] Add support for XCOFF 64 bit Object files
Needs ReviewPublic

Authored by EGuesnet on Jun 4 2021, 5:52 AM.

Details

Reviewers
jhenderson
nemanjai
jasonliu
sfertile
Esme
daltenty
hubert.reinterpretcast
Group Reviewers
Restricted Project
Summary

Support the integrated assembler for XCOFF 64 bit, and update the related tests.
The patch is based on D85774 revision

Diff Detail

Event Timeline

EGuesnet created this revision.Jun 4 2021, 5:52 AM
EGuesnet requested review of this revision.Jun 4 2021, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 5:52 AM
MaryamBen edited the summary of this revision. (Show Details)Jun 4 2021, 6:13 AM
MaryamBen added a subscriber: MaryamBen.
MaryamBen edited the summary of this revision. (Show Details)Jun 4 2021, 6:18 AM
nemanjai added reviewers: nemanjai, Restricted Project.Jun 4 2021, 6:52 AM

Please be sure to add reviewers from IBM that are actively developing this area of the PowerPC back end on your patches.

jsji added a reviewer: Esme.Jun 4 2021, 7:10 AM
shchenz added a subscriber: shchenz.Jun 4 2021, 4:49 PM

Thanks for working on this. My first suggestion is we need to break up this patch to make it smaller and easier to review. The natural place to partition it is the llvm-readobj functionality. I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
108

Please remove the unreachable instead of commenting it out.

jsji added a subscriber: jsji.Jun 7 2021, 10:45 AM

I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

https://reviews.llvm.org/D100375 is ready for review, so I would suggest we use yam2obj base on that .
It would be great if we can help review and test D100375.

I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

https://reviews.llvm.org/D100375 is ready for review, so I would suggest we use yam2obj base on that .
It would be great if we can help review and test D100375.

Awesome, I'm happy to help review it. Thanks Jinsong.

EGuesnet updated this revision to Diff 350515.Jun 8 2021, 12:40 AM
EGuesnet edited the summary of this revision. (Show Details)

Remove the unreachable
@sfertile thank you

Lots of stylistic comments from me. Someone else will need to review functionality.

llvm/include/llvm/Object/XCOFFObjectFile.h
264

It would be preferable if we didn't duplicate nearly the entire contents of the 32-bit version of this struct. Could you have a single generic version which stores the virtual address in the larger field, and then just write it out to the right size at the appropriate time?

llvm/lib/MC/XCOFFObjectWriter.cpp
347–348
603

I'm not sure this comment gives us anything, since the is64Bit check is right before it. Same below for the 32-bit version.

606–607

Please ensure comments are complete sentences, as per the style guide. Additionally, why is Zeros capitalised and plural? In fact, I'm not sure you need to go into this much detail - that belongs in the underlying function. Here, I'd just specify "Name or string table offset", if you feel you really need to, although really the function name for writeSymbolName should be descriptive enough that the comment is superfluous.

627
628

This doesn't look to be clang-formatted to me. Additionally, do you actually need the mask and cast? Does the write function not already do that conversion implicitly?

636
637

As above - clang-format and do you need the cast?

638

Please end all your comments with . as per the style guide and the existing code.

640
682–683

Same comments as above re. typos/grammar/clang-format/casting.

691–696

Ditto.

908–912

Don't use braces for single-statement if/else clauses (see the style guide).

1009–1015

Don't use braces (as above). Also, please run clang-format on all your changed code before uploading for review.

llvm/lib/Object/XCOFFObjectFile.cpp
370

Use upper-case variable names.

728

Don't have blank lines at the start of functions.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
169
170–172

Is there a particular need for this? Whilst the spec may say that only these section types have relocations, if I follow things correctly, there's nothing actually stopping XCOFF emitters from generating files with relocations for other section types.

173

Prefer to report warnings rather than errors, so that llvm-readobj doesn't bail out on malformed inputs. This helps diagnose more issues.

See reportUniqueWarning for the preferred method.

sfertile added inline comments.Jun 8 2021, 7:06 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
264

As an alternate, why don't we take the same approach as the section headers. For Relocations we would have a base class that can implement everything, and simply have a template parameter for the type of the VirtualAddress field. That will simplify the llvm-readobj XCOFF dumper changes in this patch nicely, and keeps the relocation implementation consistent with the other parts of this class.

425–428

No need to disambiguate by adding '32' on this since this interface only makes sense with 32-bit XCOFF files. But we might want to rethink the naming here. It would be useful to have the same function name, overloaded by the section argument type for both 64-bit and 32-bit that returns the 'real' count of relocations. @hubert.reinterpretcast Do you have any. thoughts on this?

431

Do we need the '64' vs '32' the disambiguate for relocations? We have it for the section header tables but that's because we don't have an argument to use for overloading with those.

MaryamBen added inline comments.Jun 9 2021, 12:41 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
264

Yeah, it would be great if I were able to do it, especially that the only difference is the size of VirtualAddress (32-bit/64-bit).

I have done this because in function relocation32 they read relocations from memory and save them into an array of XCOFFRelocation32, so the structure must match the content of memory.

static_assert(
      sizeof(XCOFFRelocation32) == XCOFF::RelocationSerializationSize32, "");
  auto RelocationOrErr =
      getObject<XCOFFRelocation32>(Data, reinterpret_cast<void *>(RelocAddr),
                                   NumRelocEntries * sizeof(XCOFFRelocation32));
EGuesnet updated this revision to Diff 350823.Jun 9 2021, 2:06 AM

@jhenderson thank you for your comments. It was very helpful.

sfertile added inline comments.Jun 9 2021, 11:07 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
264

Yep, I believe the only difference between the 32-bit and 64-bit relocation structures is the VirtualAddress field. Cribing from the section header implementation, something like:

template <typename VirtualAddress_t>
struct XCOFFReloc {
  static constexpr uint8_t XR_SIGN_INDICATOR_MASK = 0x80;
  static constexpr uint8_t XR_FIXUP_MASK = 0x40;
  static constexpr uint8_t XR_BIASED_LENGTH_MASK = 0x3f;

  VirtualAddress_t         VirtualAddress;
  llvm::support::ubig32_t  SymbolIndex;
  uint8_t                  Info;
  uint8_t                  Type;

  bool isRelocationSigned() const;
  bool isFixupIndicated() const;

  uint8_t getRelocatedLength() const;
};

struct XCOFFReloc32;
struct XCOFFReloc64;

extern template struct XCOFFReloc<llvm::support::ubig32_t>;
extern template struct XCOFFReloc<llvm::support::ubig64_t>;

struct XCOFFReloc32 : XCOFFReloc<llvm::support::ubig32_t> {};
struct XCOFFReloc64 : XCOFFReloc<llvm::support::ubig64_t> {};

allows us to implement all the shared functionality in the base class, while having the correct size and layout for each of the derived types with minimal boiler plate.

EGuesnet updated this revision to Diff 351835.Jun 14 2021, 5:25 AM

Implement XCOFFRelocation32 and XCOFFRelocation64 properly

Hello,
For you information, we have had trouble with arcanist configuration, and my account is used to push to review. Patch has been written by @MaryamBen, as confirmed by commit author.
Comments in push message are also made @MaryamBen.
Regards.

Hello,
For you information, we have had trouble with arcanist configuration, and my account is used to push to review. Patch has been written by @MaryamBen, as confirmed by commit author.
Comments in push message are also made @MaryamBen.
Regards.

I don't really use archanist so I can't comment on how to fix any problems with it, but now that the review is posted @MaryamBen can commander the revision from you. Also you can upload diffs using the 'edit revision` button with out needing to use arcanist.

My first suggestion is we need to break up this patch to make it smaller and easier to review. The natural place to partition it is the llvm-readobj functionality. I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

Reiterating this, as I think it is really important. This patch is too large to review as is. I understand breaking it up seems like extra work but it is going to go much faster/smoother if we can partition this into some smaller changes. The first patch I think should be the addition of Relocation64 to the XCOFF implementation in the Object library (ie include/llvm/Object/XCOFFObjectFile.h and lib/ObjectXCOFFObjectFile.cpp), any code needed to support that (for example if additions need to be made to include/llvm/BinaryFormat/XCOFF.h) and the 64-bit relocation reading support added to the llvm-readobj XCOFFDumper to exercises the newly added functionality.

jhenderson added inline comments.Jun 15 2021, 1:10 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
280

Let's keep the 32 bit and 64 bit versions of each of these structs next to each other, so the order will be: FileHeader32, FileHeader64, SectionHeader32, SectionHeader64.

llvm/include/llvm/Object/XCOFFObjectFile.h
231

Drop the _t. Optionally, change this to VirtualAddressType or probably more simply just AddressType or similar. We don't name our types and variables with prefix/suffixes like this typically (exceptions are some enums - see the Coding Standards for details).

425–428

+1 to something like this. For ELF we often use templates for these functions, and then just explicitly instantiate them in the .cpp. Take a look throughout the LLD source code for example. You'd end up with something like:

template<typename XCOFFT> Expected<uint32_t>
  getNumberOfRelocationEntries(const XOCFFT::XCOFFSectionHeader &Sec) const;

For this to work, you'd create an equivalent of the ElfTraits structs, which contains typedefs for the 32-bit and 64-bit types.

431

Name your inptu parameter.

llvm/lib/MC/XCOFFObjectWriter.cpp
609

As previously stated, this comment doesn't really give us anything. At least drop the "32 bits", since you're inside the`!is64Bit` check. (Also "Symbole" -> "Symbol").

707–711

Drop the braces for simple if-else statements, as per the Coding Standards.

722

You don't need to cahnge this comment. It's clear from the context that this is for 32 bits.

729

Put this comment inside the if and drop the "64 bits" bit of it.

1011–1012

If you were to switch to the ElfTraits model suggested above, you could avoid this if and just do something like:

RawPointer = sizeof(XCOFFTraits::FileHeader) + auxiliaryHeaderSize() +
             SectionCount * sizeof(XCOFFTraits::SectionHeader);
llvm/lib/Object/XCOFFObjectFile.cpp
73

Same comment as above re _t. Also goes for the following functions.

371
515–516

Unrelated formatting changes should be applied in a different commit.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
61

The changes in this file doesn't seem related to the rest of the patch. Should they be a separate patch?

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
335

Delete second blank line.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
137

Don't use auto where the type isn't obvious (except in certain cases to do with iterators). See the Coding Standards.

143

Ditto. Don't use auto here. Also should this be const &?

162

Do you plan to add support for this? I think it would make more sense to just ignore the option, if there's no strong motivation to crash in this case... (even if you do want a warning/error, use the reportUniqueWarning/reportError functions).

167

Don't use auto.

169

This and the following comments haven't been addressed.

Esme added inline comments.Jun 15 2021, 7:48 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
280

Please refer to patch D103901, we have removed these structures as we only use their size. It looks like this patch also only use their sizeof, so it's reasonable to replace the two structs with the constants:

constexpr size_t FileHeaderSize64 = 24;
constexpr size_t SectionHeaderSize64 = 72;
llvm/lib/MC/XCOFFObjectWriter.cpp
206

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)

580

nameShouldBeInStringTable() will never be called if the TargetObjectWriter->is64Bit() is true, right? Maybe we can just remove the or condition.

745

Move this comment to the appropriate code, ie. inside the if.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
336

Add some space to keep alignment. Same as below.

llvm/test/CodeGen/PowerPC/aix-return55.ll
94
llvm/tools/llvm-readobj/XCOFFDumper.cpp
47

I think you should split these part to another patch about implementation of printing relocations64 in llvm-readobj, and add corresponding test cases.

My first suggestion is we need to break up this patch to make it smaller and easier to review. The natural place to partition it is the llvm-readobj functionality. I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

Reiterating this, as I think it is really important. This patch is too large to review as is. I understand breaking it up seems like extra work but it is going to go much faster/smoother if we can partition this into some smaller changes. The first patch I think should be the addition of Relocation64 to the XCOFF implementation in the Object library (ie include/llvm/Object/XCOFFObjectFile.h and lib/ObjectXCOFFObjectFile.cpp), any code needed to support that (for example if additions need to be made to include/llvm/BinaryFormat/XCOFF.h) and the 64-bit relocation reading support added to the llvm-readobj XCOFFDumper to exercises the newly added functionality.

You are right, I will break it into two parts.
The first patch for the objectWriter ( PPCXCOFFObjectWriter.cpp, XCOFFObjectWriter.cpp, XCOFF.h and the related tests), the second part for 64-bit relocation support.

MaryamBen added inline comments.Jun 16 2021, 5:18 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
580

Actually it's used in XCOFFObjectWriter::executePostLayoutBinding() even for 64 bit.

MaryamBen added inline comments.Jun 16 2021, 5:31 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
162

I have no idea why this condition needs to be checked, as I just adapted the existing code for 64-bit, but since it is better to use reportUniqueWarning, I will do so.