This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF][AIX] Relocation support for SymB
ClosedPublic

Authored by jasonliu on Apr 3 2020, 12:00 PM.

Details

Summary

This patch intends to provide relocation support for the expression contains two unpaired relocatable terms with opposite signs.

Diff Detail

Event Timeline

jasonliu created this revision.Apr 3 2020, 12:00 PM
DiggerLin added inline comments.Apr 7 2020, 10:30 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
421

what about use const auto ?

jasonliu marked an inline comment as done.Apr 7 2020, 11:57 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
421

I'm guessing the reason you mention about "const auto" is that maybe it would help when we have 64 bit.
However, I don't think we made any decision on how to implement 64 bit yet. So I would like to only focus on how this code would work on 32 bit and worry about 64 bit later. So in that case, I don't think we should use auto here, as it would make it very unclear what type we are getting here.

DiggerLin added inline comments.Apr 7 2020, 2:33 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
462

I have question on the "SymbolA - SymbolB + imm64"

for example: in the assembly of the test case

       .csect .text[PR] 
.foo:
# %bb.0:                                # %entry
        li 4, 0
        cmplwi  3, 3
        stw 3, -8(1)
        stw 4, -12(1)
        bgt     0, LBB0_5
# %bb.1:                                # %entry
        lwz 4, LC0(2)
        slwi 3, 3, 2
        lwzx 3, 3, 4
        add 3, 3, 4
        mtctr 3
        bctr
LBB0_2:                                 # %sw.bb1
        li 3, 1
        b LBB0_6

.......
........
        .csect .rodata[RO]
        .align  2
.LJTI0_0: (symb)
        .long   LBB0_2-.LJTI0_0

the address of LBB0_2 is 44 (0x2C) .

the value in the
00000058 <.rodata>:

58:   ff ff ff d4     .long 0xffffffd4   (-0x2C)
              58: R_POS       .text
              58: R_NEG       .rodata-0x58

the jump address
.text+imm64 (-0x2C) - (0x58) =0x84 + LC0(R2). it not correct

I think the content of in the address of the first bytes .csect .rodata[RO] after link in should be
.text - imm64(-0X2C)-(0X58) = -0X2C + LC0(R2) ?

463

what about use XCOFF::RelocationType type instead of uint8_t ?

DiggerLin added inline comments.Apr 8 2020, 5:44 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
462

please ignore above question. I got it . the value in the address 0x58 is .text+imm64-.rodata.
So imm64 = value in the address 0x58 + .rodata -.text
when relocation.

jasonliu marked an inline comment as done.Apr 8 2020, 12:27 PM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
463

At this stage, we are already lowering the high level representation to the actual binary level representation. XCOFFRelocation uses uint8_t to store the XCOFF::RelocationType, so I prefer to matches them here.

llvm/lib/MC/XCOFFObjectWriter.cpp
400

On the assembly path, I don't see a label for function descriptor definitions or for common definitions.

401

Typo: s/containting/containing/;

406

If this is not being changed, then—given the size of the function—using const MCSymbol *const SymA could help readability.

jasonliu marked an inline comment as done.Apr 10 2020, 11:10 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
400

We don't have a label for function descriptor definitions or common definitions. So those symbols won't even get here. Could you elaborate your concern?

jasonliu updated this revision to Diff 256666.Apr 10 2020, 2:21 PM

Address comments.

jasonliu marked 3 inline comments as done.Apr 10 2020, 2:22 PM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
400

Discussed offline, updated the misleading comments.

LGTM with a request for more input.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.ll
38 ↗(On Diff #256666)

I'd like a second opinion over whether this is too high-level.

This revision is now accepted and ready to land.Apr 12 2020, 8:35 PM
jasonliu marked an inline comment as done.Apr 13 2020, 7:07 AM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.ll
38 ↗(On Diff #256666)

I see your concern. The better way to write the test is through assembly. As you can easly write assembly like
.csect A[PR]
A:
...
.csect B[RO]
B:
.long A - B
to trigger the relocation emition.
But I doubt the asm parser for AIX works right now.
And I'm not sure if there are other way to get a better test case for it.

jasonliu updated this revision to Diff 257471.Apr 14 2020, 1:28 PM

Updated test case to use mir in order to be as low level as possible.

jasonliu marked an inline comment as done.Apr 14 2020, 2:21 PM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.mir
50

I will subsitute index in this line and line 68, so that we don't use hardcoded value.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.mir
24

I'm a bit disturbed by the weirdness of this function. I suggest the following.

---
name:            foo
alignment:       16
tracksRegLiveness: true
jumpTable:
  kind:            label-difference32
  entries:
    - id:              0
      blocks:          [ '%bb.0' ]
body:             |
  bb.0:
    successors: %bb.0(0x20000000)
    liveins: $r2
    renamable $r3 = LWZtoc %jump-table.0, $r2 :: (load 4 from got)
    BLR implicit $lr, implicit $rm, implicit killed $r3
...
jasonliu updated this revision to Diff 257509.Apr 14 2020, 2:58 PM

Updated test case to a much readable version.

jasonliu marked an inline comment as done.Apr 14 2020, 2:59 PM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.mir
24

Thanks. It is much better.

This revision was automatically updated to reflect the committed changes.