Page MenuHomePhabricator

[RISCV] Allow parsing of bare symbols with offsets
ClosedPublic

Authored by lewis-revill on Jan 28 2019, 8:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Jan 28 2019, 8:19 AM

Should this only support constant offsets to a symbol? It seems sensible to only support offsets to the symbol, IE add/sub rather than more complex expressions, but the RHS could be any expression at the moment.

asb added a comment.Jun 18 2019, 9:28 PM

Is this being used in the wild and does it work sensibly with binutils? I'm seeing that binutils seems to be associating the addend with both the emitted R_RISCV_PCREL_HI20 relocation and R_RISCV_RELAX relocation. Maybe this is just a binutils bug and the addend is ignored by the linker but this does seem surprising...

$ cat t.s 
lla a5, a_symbol + (0xFF << 3)
$ ./riscv32-unknown-elf-as t.s
$ ./riscv32-unknown-elf-readelf -r a.out 

Relocation section '.rela.text' at offset 0xec contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000617 R_RISCV_PCREL_HI2 00000000   a_symbol + 7f8
00000000  00000033 R_RISCV_RELAX                7f8
00000004  00000418 R_RISCV_PCREL_LO1 00000000   .L0  + 0
00000004  00000033 R_RISCV_RELAX                0
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 9:28 PM
Herald added subscribers: Jim, benna, psnobl. · View Herald Transcript
In D57332#1549661, @asb wrote:

Is this being used in the wild and does it work sensibly with binutils? I'm seeing that binutils seems to be associating the addend with both the emitted R_RISCV_PCREL_HI20 relocation and R_RISCV_RELAX relocation. Maybe this is just a binutils bug and the addend is ignored by the linker but this does seem surprising...

$ cat t.s 
lla a5, a_symbol + (0xFF << 3)
$ ./riscv32-unknown-elf-as t.s
$ ./riscv32-unknown-elf-readelf -r a.out 

Relocation section '.rela.text' at offset 0xec contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000617 R_RISCV_PCREL_HI2 00000000   a_symbol + 7f8
00000000  00000033 R_RISCV_RELAX                7f8
00000004  00000418 R_RISCV_PCREL_LO1 00000000   .L0  + 0
00000004  00000033 R_RISCV_RELAX                0

Yes, FreeRTOS at least uses it, and we've had to manually patch the source to refer to just the bare symbol and follow it with an addi in order to support LLVM. The RELAX addend is probably a bug but I don't think it actually ever gets read.

asb added a comment.Jul 7 2019, 10:24 PM

This patch seems good, but needs rebasing. Thanks!

@asb are we happy for this to land? You were happy with it without explicitly approving it.

asb accepted this revision.Aug 1 2019, 5:25 AM

Adding explicit LGTM in the Phabrictor UI. Thanks again Lewis!

This revision is now accepted and ready to land.Aug 1 2019, 5:25 AM
This revision was automatically updated to reflect the committed changes.
lenary added a subscriber: hans.Aug 16 2019, 6:12 AM

@hans please can you backport rL369097 to the 9.0 branch?

hans added a comment.Aug 20 2019, 12:44 AM

@hans please can you backport rL369097 to the 9.0 branch?

Merged in r369338.