This patch intends to support three most common relocation type on AIX: R_POS, R_TOC, R_RBR.
These three relocation type will be needed for object file generation on AIX for small code model.
We will have follow up patches to bring relocation support for large code model on AIX.
Details
Diff Detail
Event Timeline
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
347 |
if ( XCOFF::XTY_ER != MCSec->getCSectType() && nameShouldBeInStringTable(MCSec->getSectionName()))
CsectGroup &Group = getCsectGroup(MCSec); to |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
347 | I don't think we could do this under current design. As the 328 and 329 assert suggested, we do not register undefined csect, we only register its symbol. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
51 | can we use sizeof(XCOFFRelocation) when using SizeOfXCOFFRelocation32 ? | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
84 | The relocation encodes the bit length being relocated minus 1. Add back the 1 to get the actual length being relocated. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
55 | I just want to make sure we have a good reason (e.g., a sizable performance benefit) for having this in-memory version instead of reusing the llvm::object::XCOFFRelocation32 serialization version. | |
160 | s/MC Section/MCSection/; | |
160 | s/corrresponding/corresponding/; | |
165 | s/corrresponding/corresponding/; |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
---|---|---|
75 | Does the reference returned by DF->getContents() change between loop iterations? For that matter, does the value returned by size()? | |
76 | Does the reference returned by DF->getFixups() change between loop iterations? | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
51 | I believe that sizeof(llvm::object::XCOFFRelocation32) is 10 (and we should not hard-code 10 here). | |
163 | The east const here does not agree with line 196, which uses west const. | |
167 | Fix the east const here too. | |
254–256 | Insert "the" before "mappings". | |
616 | Blank line before the comment please. | |
619 | Blank line before the comment please. | |
637 | Please move the comment into the if. | |
684 | Please move the comment into the if. | |
697 | Add "the" before "file offset" and "relocation entries". | |
704 | Both the multiplication and the addition can overflow here. | |
707 | It seems there is no special alignment requirement for the symbol table. Alignment as low as 2-byte has been observed. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
22 | There's XR_SIGN_INDICATOR_MASK in the codebase already. | |
24 | Is there a reason here to override public member functions of a public base class as protected? | |
46 | Can we just return directly instead of assigning to a variable first? | |
69 | There is a chance that merging this with the previous function would improve the logic and reduce parallel maintenance. In particular, the "magic numbers" are easier to swallow if they are paired with the XCOFF relocation type. | |
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll | ||
126 | It would be better if there was a case where the TOC entry is at a non-zero offset from the TOC base. | |
141 | @DiggerLin, I think this makes a good case for printing the storage mapping class for other-than-label-definition symbols. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
263 | Comment needs updating. | |
393 | I believe we decided that the PascalCase for "csect" is Csect. | |
403 | Is this really about temporary symbols, or is this more to do with linkage? | |
408 | s/relaitve/relative/; However, I think you mean the "symbol's virtual address in this object file". | |
414 | The coding standards say:
|
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
---|---|---|
84 | And my suggestion is to use a constexpr instead of using number directly here. | |
84 | the bit length of the relocatable reference of PPC::fixup_ppc_br24 is 24 bits ? it should be 24bits -1 = 23. |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
---|---|---|
84 | Branches are 4 byte aligned, so the 24 bits we encode in the instruction actually represents a 26 bit offset (because the implication the bottom 2 bits are 0), so it is 26-1. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
411 | Consider: int x = 2, arr[100] = { 1 }; extern int *p = arr + 42; The raw data with this patch does not reflect the offset from arr that p points to (and neither does the relocation itself): 00000004 arr: 4: 00 00 00 01 <unknown> ... 00000194 p: 194: 00 00 00 04 <unknown> The raw data without this patch was correct. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
51 | Could we include headers from llvm/Object in MC components? |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
51 | Good point. I would suggest this hard-coded value belongs in BinaryFormat and llvm/Object should check against that value. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
55 | As the comment above suggested, we do not want to use the version in llvm/Object for object file generation. | |
403 | Whenever we have a temporary symbol, we would relocate its containing csect because temporary symbol would not be on the symbol table. | |
707 | I will remove the TODO. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
22 | Unfortunately that's in llvm/Object. And we should have put it in llvm/BinaryFormat. | |
69 | Suggestion taken. | |
84 | Regarding to your suggestion for using constexpr value here... I'm not sure if it really helps that much. As the magic number I'm using here actually have some relationship with the Fixup Kind, in my mind, it's actually easier for people to reason about why we use that number if we just put that number in directly. | |
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll | ||
126 | You are right. I added the test and discovered we also need to calculate the FixedValue for R_TOC relocation. |
Address some minor concerns Hubert have in the offline discussion:
- Get symbol table index using SymA directly, if we can't find it in the symbol table, then find its csect in the symbol table.
- return pairs using uniform init directly in the switch.
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
707 | These asserts are not meaningful, we are just going to wrap. |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
---|---|---|
74 | can be marked const |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
400 | No comma before "or" here. | |
401 | s/relocate its csect/have the relocation reference its csect/; | |
409 | Is SectionMap[SymASec]->Address a fancy way of saying 0 for undefined symbols? If so, can we move it into the ternary expression? | |
411 | As discussed off-list, the added test should cover this too. | |
421 | assert(TargetObjectWriter->is64Bit() || Fixup.getOffset() <= UINT32_MAX - Layout.getFragmentOffset(Fragment)); | |
706 | Size required for the relocation entries of the current section is too large for a 32-bit object file. | |
709 | Before the increment: | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
29 | Missing override. | |
46 | This could be done (if indeed the current condition is correct) as: const uint8_t EncodedSignednessIndicator = IsPCRel ? SignBitMask : 0u; | |
52 | Add "a" before "strong". | |
65 | This could be done as EncodedSignednessIndicator | 15. | |
71 | Same comment. | |
74 | Same comment. | |
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll | ||
158 | The column alignment is off here. | |
165 | As discussed off-list, we should want the symbol table dump to match this virtual address with the TOC entry for globalA, etc. |
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll | ||
---|---|---|
86 | This does not appear to be the case generally. While we may generate them at a strictly positive offset in the object file Thus if there are more than 8192 TOC entries (in 32-bit mode), the linker will place TC entries above the anchor and generate the appropriately negatively signed displacement. XLC also thus appears to generate these relocations as signed. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
409 | SectionMap[SymASec]->Address is not just for undefined symbols. If SymA is a label inside of csect, we need to get (the csect address + the symbol offset) to get the exact address of that label. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
409 | Had offline discussion on this, and I will change the code to this as suggested. FixedValue = (SymA.isDefined() ? SectionMap[SymASec]->Address + Layout.getSymbolOffset(SymA) : 0) + Target.getConstant(); | |
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll | ||
86 | You are right.
Based on that, here's what I'm planning to do in this patch:
|
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
611 | I do not think we need to add assert here, implement of XCOFFRelocation32 is packed alignment. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
22 | we maybe need to have NFC patch to put the XR_SIGN_INDICATOR_MASK, XR_FIXUP_INDICATOR_MASK, XR_BIASED_LENGTH_MASK into XCOFF.h , and shared with XCOFFObjectFile.h |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
611 | Yes, it is packed alignment now, the assertion here is to make sure it will always be packed alignment in the future. So I don't think adding assertion here is a bad thing. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
22 | I agree we should have a NFC patch to clean it up. I think we could do that after this lands. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
704 | When I rethink the assertions, I think we could remove
|
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
715 | Use the text for the assertion above. The actual issue is that the symbol table needs to start at a point where the offset value can be encoded. The file can exceed 4 GiB. |
can be marked const