Support the integrated assembler for XCOFF 64 bit, and update the related tests.
The patch is based on D85774 revision
Details
- Reviewers
jhenderson nemanjai jasonliu sfertile Esme daltenty hubert.reinterpretcast - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please be sure to add reviewers from IBM that are actively developing this area of the PowerPC back end on your patches.
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. |
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.
Lots of stylistic comments from me. Someone else will need to review functionality.
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
263 | 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. | |
630 | ||
631 | 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? | |
644 | ||
645 | As above - clang-format and do you need the cast? | |
646 | Please end all your comments with . as per the style guide and the existing code. | |
648 | ||
689–690 | Same comments as above re. typos/grammar/clang-format/casting. | |
703–708 | Ditto. | |
918–922 | Don't use braces for single-statement if/else clauses (see the style guide). | |
1018–1024 | 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 | ||
383 | Use upper-case variable names. | |
741 | 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. |
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
263 | 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. | |
450 | 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? | |
456 | 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. |
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
263 | 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)); |
llvm/include/llvm/Object/XCOFFObjectFile.h | ||
---|---|---|
263 | 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. |
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.
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.
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). | |
450 | +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. | |
456 | Name your inptu parameter. | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
615 | 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"). | |
719–724 | Drop the braces for simple if-else statements, as per the Coding Standards. | |
734 | You don't need to cahnge this comment. It's clear from the context that this is for 32 bits. | |
741 | Put this comment inside the if and drop the "64 bits" bit of it. | |
1025–1026 | 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. | |
384 | ||
529–530 | 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–173 | 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. |
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 |
| |
580 | nameShouldBeInStringTable() will never be called if the TargetObjectWriter->is64Bit() is true, right? Maybe we can just remove the or condition. | |
757–758 | 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. |
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.
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
580 | Actually it's used in XCOFFObjectWriter::executePostLayoutBinding() even for 64 bit. |
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. |
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.