This is the second patch to enable the XCOFF64 object writer.
Details
- Reviewers
jsji shchenz jhenderson DiggerLin - Group Reviewers
Restricted Project - Commits
- rG8d6e2c3e3db1: [XCOFF] support writing sections, relocations and symbols for XCOFF64.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
My only real comment on this is patch ordering. An XCOFF/AIX person should review too.
| llvm/lib/MC/XCOFFObjectWriter.cpp | ||
|---|---|---|
| 769 | The non-zero value plus "not supported yet" comment doesn't quite work. Perhaps "Only source file symbol supported"? | |
| 828–829 | I'd have thought implementing the symbol table before relocations makes more sense. | |
| 866–869 | Same comment as above: support the symtab first? | |
Thanks for your comments! @jhenderson
It seems inappropriate to support the symbol table before the sections and relocations, due to their offset order, or some other reasons. So I enabled all these fields in this patch and also modified related test cases.
| llvm/lib/MC/XCOFFObjectWriter.cpp | ||
|---|---|---|
| 203 | I do not think we will change ".file" . change to const StringRef SourceFileName = ".file" ? | |
| 771 | SymbolTableEntryCount+1 for ".file" symbol. | |
| 828–829 | agree with James | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
| 82 | I am not sure, whether I understand correct or not ?
in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__sua3i125jbau so for the {XCOFF::RelocationType::R_TOC, EncodedSignednessIndicator | 13 } | |
Addressed @DiggerLin 's comments.
| llvm/lib/MC/XCOFFObjectWriter.cpp | ||
|---|---|---|
| 203 |
Our current approach is to write a symbol without the auxiliary entry, with .file as the source file's name, but in fact we should write a symbol with the real source file's name. And we commented a todo for this. // FIXME: add the real source file's name. So I think the SourceFileName may be changed. | |
| 771 | void XCOFFObjectWriter::assignAddressesAndIndices(const MCAsmLayout &Layout) {
// The first symbol table entry (at index 0) is for the file name.
uint32_t SymbolTableIndex = 1;
...
SymbolTableEntryCount = SymbolTableIndex;
}The ".file" symbol has been counted. | |
| 828–829 | Thanks, since it's hard to separate the symbol table and relocation into 2 patches, I prefer to enable both of them in this patch. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
| 82 | Thanks for your catch! | |
Nothing much else to add apart from a stylistic comment.
| llvm/test/CodeGen/PowerPC/aix-extern-weak.ll | ||
|---|---|---|
| 2–4 | I have a personal preference for this syntax: from the first line, it makes it clear that the next line is a new command, whilst from the second line, the indentation followed by an executable name makes it clear that this is a continuation of the previous line. Same applies elsewhere. | |
| llvm/lib/MC/XCOFFObjectWriter.cpp | ||
|---|---|---|
| 203 | thanks. | |
| 723 | maybe this is more readable , even there is duplication code of writeWord(NumberOfRelocEnt); if (!is64Bit()) {
W.OS.write_zeros(4); // Reserved
writeWord(NumberOfRelocEnt);
W.OS.write_zeros(6); // Reserved
else {
writeWord(NumberOfRelocEnt);
W.OS.write_zeros(1); // Reserved
W.write<uint8_t>(XCOFF::AUX_SECT);
} | |
| 771 | thanks. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
| 82 | PPC::fixup_ppc_half16dq: is a A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for instrs like 'lxv'. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 82 | I am not a expert on the relocation, I just put my question and we may be need someone who is expert on the relocation to review the code. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 82 | /// A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for /// instrs like 'lxv'. Produces the same relocation as fixup_ppc_half16ds. PPC::fixup_ppc_half16dq Thanks, I thinks I need more investigation about this, the comment says it produces the same relocation as fixup_ppc_half16ds. | |
Hi @DiggerLin, Sorry for the late update, I have verified that the SignAndSize value should be 15 for both fixup_ppc_half16ds and fixup_ppc_half16dq. Although there are zeros being implied, the totally size should be a 16-bit fixup. I confirmed this by testing LIT and test-suite, and the test-suite passes only if the value is 15, otherwise there will be an error like this
Relocation overflow in reference to: L..CPI168_7 (entry 3199)
Address: 0x00082c12; RLD type: R_TOC; RLD length: 14| llvm/test/CodeGen/PowerPC/basic-toc-data-def.ll | ||
|---|---|---|
| 4–10 | FWIW, I'd actually keep the old style here, as it makes it clear which commands the options apply to, whilst still indicating FileCheck is a continuation. Same applies in similar cases. | |
Looks almost good to me. Thanks for adding this support.
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 74 | Is it possible to add some tests to verify the new relocation types added here for 64-bit objects, especially the ones the existing case changes do not contain? | |
| 82 | Hmm, yes, I also think this is a little confuse. From 64bit OpenPower ABI, section 3.5.1, it says:
I think it clearly indicates that fixup_ppc_half16ds and fixup_ppc_half16dq should use same relocation infos. For the bit length, it does not give a clear answer, it says is similar to half16, but is really just 14 bits. I checked the source code of commercial XLC, it gives 15. So I think it should be good to use 15 here. | |
| 84 | Should we handle RelocationType::R_TOCU? I guess it may be generated in large code model(-mcmodel=large). | |
| llvm/test/CodeGen/PowerPC/aix-extern-weak.ll | ||
| 376 | Use relative index like above Index: [[#Index+25]]? | |
| llvm/test/CodeGen/PowerPC/aix-extern.ll | ||
| 510 | Same here. | |
Addressed comments and improved tests.
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 84 | fixup_ppc_half16ds and fixup_ppc_half16dq may only correspond to lo16 of instrs, not ha16, so we shouldn't deal with RelocationType::R_TOCU here. I have added the corresponding large code model test in llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-large.ll. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 74 | Tests in ppc64-abs-reloc.s, aix-xcoff-reloc.ll, aix-xcoff-reloc-large.ll etc. cover the new relocation types. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
|---|---|---|
| 84 | Yes, right. When large code model is in effect, the higher 16 bit is relocated by addis which will not use fixup kind fixup_ppc_half16dq or fixup_ppc_half16ds | |
| llvm/test/CodeGen/PowerPC/aix-available-externally-linkage.ll | ||
| 50 | Except this field Auxiliary Type, all other fields for the symbol table are the same with XCOFF32, should we reuse them? I prefer to reuse them as we can get a better view about the difference between XCOFF32/XCOFF64. And many others test files too. | |
| llvm/test/MC/PowerPC/ppc64-abs-reloc.s | ||
| 4 | triple should be 64 bit. | |
| 27 | Exactly the same with above checks, can we reuse? | |
| llvm/lib/MC/XCOFFObjectWriter.cpp | ||
|---|---|---|
| 723 | I think we also need a test case for dwarf 64 too. | |
LGTM! Thanks for adding this amazing support.
Maybe we can wait for some days in case @DiggerLin or @jhenderson has some further comments.
Actually, according to the pre-merge checks, one of the tests is failing, and I suspect it might be related to this patch?
I do not think we will change ".file" . change to const StringRef SourceFileName = ".file" ?