This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF][AIX] Support basic relocation type on AIX
ClosedPublic

Authored by jasonliu on Dec 30 2019, 7:55 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jasonliu created this revision.Dec 30 2019, 7:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2019, 7:55 PM
DiggerLin added inline comments.Jan 3 2020, 11:56 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
347
  1. Can We delete code line 350 ~ 354 ?
  1. and change line 333 to

if ( XCOFF::XTY_ER != MCSec->getCSectType() && nameShouldBeInStringTable(MCSec->getSectionName()))

  1. delete line 328 ,329 assert(XCOFF::XTY_ER != MCSec->getCSectType() && "An undefined csect should not get registered.");
  1. changed the line 336

CsectGroup &Group = getCsectGroup(MCSec);

to
CsectGroup &Group = (XCOFF::XTY_ER != MCSec->getCSectType() ? getCsectGroup(MCSec): UndefinedCsects);

jasonliu marked an inline comment as done.Jan 3 2020, 6:14 PM
jasonliu added inline comments.
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.

jasonliu edited the summary of this revision. (Show Details)Jan 6 2020, 12:31 PM
DiggerLin added inline comments.Jan 6 2020, 1:11 PM
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.
this one should be 23 ?

jasonliu marked 2 inline comments as done.Jan 6 2020, 1:49 PM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
51

No, as I'm trying to express in the comments above, the sizeof(XCOFFRelocation) is 12, but actual size is 10.

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

No, we have 25 in the actual object file.

llvm/lib/MC/XCOFFObjectWriter.cpp
49

Suggestion:
The in-memory 32-bit XCOFF relocation structure is 12 bytes long, but the trailing 2-byte padding is omitted in the binary format.

51

I think we can drop "XCOFF"; this is a .cpp file that is XCOFF-specific.

Suggestion: RelocationSerializationSize32.

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

618

Blank line before the comment please.

621

Blank line before the comment please.

639

Please move the comment into the if.

686

Please move the comment into the if.

699

Add "the" before "file offset" and "relocation entries".

706

Both the multiplication and the addition can overflow here.

709

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:

If you use a braced initializer list when initializing a variable, use an equals before the open curly brace: [ ... ]

DiggerLin added inline comments.Jan 7 2020, 7:16 AM
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 ?
if it is , according to xcoff document .
r_rsize : 0x3F(6 bits)
Specifies the bit length of the relocatable reference minus one. The current architecture allows for fields of up to 32 bits (XCOFF32) or 64 bits (XCOFF64) to be relocated.

it should be 24bits -1 = 23.

jasonliu marked an inline comment as done.Jan 7 2020, 8:03 AM
jasonliu added inline comments.
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.

hubert.reinterpretcast requested changes to this revision.Jan 7 2020, 8:46 AM
hubert.reinterpretcast added inline comments.
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.

This revision now requires changes to proceed.Jan 7 2020, 8:46 AM
jasonliu marked 3 inline comments as done.Jan 7 2020, 8:47 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
51

Could we include headers from llvm/Object in MC components?
It seems the current convention is to only use headers from llvm/BinaryFormat, and llvm/Object is more for tooling purpose(llvm-readobj, llvm-objdump) and not object generation.

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.

jasonliu updated this revision to Diff 236865.Jan 8 2020, 10:58 AM
jasonliu marked 31 inline comments as done.

Addressed first round of comments.

jasonliu added inline comments.Jan 8 2020, 11:00 AM
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.

709

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.

jasonliu updated this revision to Diff 237678.Jan 13 2020, 7:49 AM

Address some minor concerns Hubert have in the offline discussion:

  1. 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.
  2. return pairs using uniform init directly in the switch.
daltenty added inline comments.Jan 15 2020, 9:05 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
709

These asserts are not meaningful, we are just going to wrap.

daltenty added inline comments.Jan 15 2020, 10:56 AM
llvm/lib/MC/MCXCOFFStreamer.cpp
74

can be marked const

jasonliu updated this revision to Diff 238609.Jan 16 2020, 1:57 PM

Addressed David's comment.

  1. Added const.
  2. Make the assertions useful.
jasonliu marked 2 inline comments as done.Jan 17 2020, 7:02 AM
daltenty added inline comments.Jan 22 2020, 2:16 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
49

I'm not sure that using IsPCRel this way makes sense, see R_TOC note below.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
86

For the R_TOC relocation, shouldn't this be signed as this is a displacement?

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));

708

Size required for the relocation entries of the current section is too large for a 32-bit object file.

711

Before the increment:
assert(RawPointer <= UINT32_MAX - RelocationSizeInSec && "Offset following the relocation data cannot be represented in 32 bits.");

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.

jasonliu marked 3 inline comments as done.Jan 23 2020, 7:04 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
49

Addressed below.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
86

The relocation reference for toc entry should always be unsigned, because it is relative to the TOC anchor, and TOC entry has to come after TOC anchor.

daltenty added inline comments.Jan 23 2020, 8:12 AM
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
(possible more than can fit in the available displacement), the linker will combine and reorder them into a valid order.

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.

jasonliu marked 9 inline comments as done.Jan 27 2020, 10:49 AM
jasonliu added inline comments.
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.

jasonliu marked 2 inline comments as done.Jan 27 2020, 12:40 PM
jasonliu added inline comments.
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.
There are some offline discussion going on with @daltenty and also people from AIX OS team. I will try to summarize them here and also put into the comments.

  1. The linker could move TC entries above TOC anchor during linking. So they could have a negative offset.
  2. However, assembler generates unsigned bit for this TC entry relocation.
  3. Stephen from AIX OS says the sign bit of relocation is mostly ignored by the link-editor. What's important is the behavior of the hardware instruction.

Based on that, here's what I'm planning to do in this patch:

  1. Since the signness does not matter most of the time, let's just follow what assembler's behavior and use the IsPCRel to determine if it is signed or not.
  2. For this patch, let's disallow the overflow in the compiler. Report fatal_error when positive range runs out.
DiggerLin added inline comments.Jan 28 2020, 7:02 AM
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

jasonliu updated this revision to Diff 240890.Jan 28 2020, 8:04 AM
jasonliu marked 5 inline comments as done.

Addressed last round of comments from David and Hubert.

jasonliu marked 2 inline comments as done.Jan 28 2020, 8:08 AM
jasonliu added inline comments.
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.

jasonliu marked 8 inline comments as done.Jan 28 2020, 8:13 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
409

Just discovered why I could not do the way as suggested. Added rationale into the comments.

411

Added into the same test. See arr and p in the test below. Thanks.

daltenty accepted this revision.Jan 29 2020, 9:52 AM

Other than minor nit, LGTM

llvm/lib/MC/XCOFFObjectWriter.cpp
706

nit: shouldn't these exceeding 32-bit assertions be report_fatal_errors? The user isn't going to get back a usable object file if this is the case.

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

nit: decides

jasonliu marked 2 inline comments as done.Jan 29 2020, 2:12 PM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
706

When I rethink the assertions, I think we could remove

  1. assert(Sec->RelocationCount < (UINT32_MAX / XCOFF::RelocationSerializationSize32)) because RelocationCount is uint16_t, and XCOFF::RelocationSerializationSize32 is always 10. So it won't be able to overflow RelocationSizeInSec which is a uint32_t.
  2. assert(RawPointer <= UINT32_MAX - RelocationSizeInSec) because RawPointer gets its initial value from RelocationEntryOffset(uint32_t), so adding any uint32_t is not going to overflow RawPointer which is uint64_t.

    I will make "assert(RawPointer <= UINT32_MAX);" a fatal_error.
llvm/lib/MC/XCOFFObjectWriter.cpp
717

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.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 30 2020, 8:04 AM
This revision was automatically updated to reflect the committed changes.