[PATCH] D63076: [ELF][RISCV] Support PLT, GOT, copy and relative relocations

Thu Jun 13 19:08:24 PDT 2019

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.

================
Comment at: test/ELF/riscv-plt.s:72
+  call foo
+  call bar
+  call bar at plt
----------------
jrtc27 wrote:
> MaskRay wrote:
> > jrtc27 wrote:
> > > jrtc27 wrote:
> > > > MaskRay wrote:
> > > > > jrtc27 wrote:
> > > > > > This should never use a PLT stub.
> > > > > > This should never use a PLT stub.
> > > > > 
> > > > > Which function should not use a PLT stub? I think the behavior is consistent with ld.bfd, foo doesn't get a PLT but weak and bar get PLT stubs.
> > > > Yes, and I'm 99% sure that's an unintended bug. If you delete the `call foo at plt`, then the `call bar` *doesn't* use a PLT stub.
> > > I mean delete `call bar at plt`, of course...
> > > Yes, and I'm 99% sure that's an unintended bug. If you delete the call bar at plt, then the call bar *doesn't* use a PLT stub.
> > 
> > I think you misread it..
> > 
> > bar at plt will be created, even if I delete `call bar at plt`. The behavior is consistent with ld.bfd.
> > 
> > Because bar is not local, a PLT has to be created.
> No, the whole point is that by *not* adding `@plt` you forcefully bypass the PLT and don't allow bar to be preempted in this instance:
> 
> ```
> burn:snippets jrtc4% cat call-plt.s
> .global test, foo, bar, baz
> 
> test:
>     call foo
>     call bar
>     call bar at plt
>     call baz at plt
> burn:snippets jrtc4% riscv64-linux-gnu-gcc -o call-plt-pic.so -shared -fPIC call-plt.s -nostdlib
> burn:snippets jrtc4% riscv64-linux-gnu-objdump -d call-plt-pic.so                               
> 
> call-plt-pic.so:     file format elf64-littleriscv
> 
> 
> Disassembly of section .plt:
> 
> 00000000000002f0 <.plt>:
>  2f0:   00002397            auipc   t2,0x2
>  2f4:   41c30333            sub t1,t1,t3
>  2f8:   d103be03            ld  t3,-752(t2) # 2000 <.got>
>  2fc:   fd430313            addi    t1,t1,-44
>  300:   d1038293            addi    t0,t2,-752
>  304:   00135313            srli    t1,t1,0x1
>  308:   0082b283            ld  t0,8(t0)
>  30c:   000e0067            jr  t3
> 
> 0000000000000310 <baz at plt>:
>  310:   00002e17            auipc   t3,0x2
>  314:   d00e3e03            ld  t3,-768(t3) # 2010 <baz>
>  318:   000e0367            jalr    t1,t3
>  31c:   00000013            nop
> 
> 0000000000000320 <bar at plt>:
>  320:   00002e17            auipc   t3,0x2
>  324:   cf8e3e03            ld  t3,-776(t3) # 2018 <bar>
>  328:   000e0367            jalr    t1,t3
>  32c:   00000013            nop
> 
> Disassembly of section .text:
> 
> 0000000000000330 <test>:
>  330:   00000097            auipc   ra,0x0
>  334:   cd0080e7            jalr    -816(ra) # 0 <_PROCEDURE_LINKAGE_TABLE_-0x2f0>
>  338:   fe9ff0ef            jal ra,320 <bar at plt>
>  33c:   fe5ff0ef            jal ra,320 <bar at plt>
>  340:   fd1ff0ef            jal ra,310 <baz at plt>
> ```
> 
> `foo` and `baz` behave as expected, with the former not using a PLT stub but the latter using a PLT stub (based on whether or not they were suffixed with `@plt`). However, *both* calls to `bar` end up using a PLT stub, despite only one of them having a `@plt` suffix. This is the BFD bug I was talking about.
`call bar` (`R_RISCV_CALL`) and `call bar at plt` (`R_RISCV_CALL_PLT`) can both appear in an object file as a result of ld -r.

Making `call bar` and `call bar at plt` resolve to different targets (`R_RISCV_CALL`->`__ehdr_start`; `R_RISCV_CALL_PLT`->plt stub) is nonsensical. The preemptability is the property of the symbol, not the property of the relocation type. The preemptability decides whether a PLT stub should be the target. This is both simple and consistent.

Preemptability, as a property of the symbol, is mergeable. If you have an undefined reference to foo in a.o and a defined foo in b.o, after ld -r, you get a defined foo, which is non-preemptable if you eventually use it in a -no-pie/-pie link. In contrast, preemptability on relocation types is not mergeable. If you place the preemptability information on the relocation types, you can get things that aren't consistent after ld -r.

I don't think `R_RISCV_CALL` is meaningful so I filed https://github.com/riscv/riscv-elf-psabi-doc/issues/98 The examples I raised (`R_X86_64_PLT32` `R_PPC_LOCAL24PC` `R_PPC_PLTREL24`) were to demonstrate how the PLT relocation types evolved on other architectures.

@jrtc27 Have said all that I've no objections to make them distinct in the future if RISC-V folks can convince me the distinction between `R_RISCV_CALL` and `R_RISCV_CALL_PLT` is useful :) Does this patch look good from your perspective minus this difference from ld.bfd?

Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63076/new/

https://reviews.llvm.org/D63076