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

[Fangrui Song via Phabricator via llvm-commits]

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


================
Comment at: ELF/Arch/RISCV.cpp:71
+  CopyRel = R_RISCV_COPY;
+  GotRel = Config->Is64 ? R_RISCV_64 : R_RISCV_32;
+  NoneRel = R_RISCV_NONE;
----------------
ruiu wrote:
> Does this really support R_RISCV_32?
This should be correct. glibc/sysdeps/riscv/dl-machine.h says:

```
    case R_RISCV_JUMP_SLOT:
    case __WORDSIZE == 64 ? R_RISCV_64 : R_RISCV_32:
      *addr_field = value;
      break;
```

On RISC-V, there is no SymbolRel GotRel difference as there is on other targets. (`R_X86_64_GLOB_DAT` is an unnecessary dynamic relocation; `R_X86_64_64` can be used instead) 


================
Comment at: ELF/Arch/RISCV.cpp:124-125
+void RISCV::writePltHeader(uint8_t *Buf) const {
+  if (Config->EFlags & EF_RISCV_RVE)
+    error("PLT is not supported for RVE");
+
----------------
jrtc27 wrote:
> ruiu wrote:
> > What is RVE?
> RV32E, as opposed to RV32I; it only has 16 GPRs available.
The issue is that RV32E doesn't have the register t3 so this PLT stub doesn't work.


================
Comment at: test/ELF/riscv-plt.s:2
+# REQUIRES: riscv
+# RUN: echo '.globl bar, weak; .type bar, at function; .type weak, at function; bar: weak:' > %t1.s
+
----------------
jrtc27 wrote:
> Can we put this in a file in Inputs rather than creating it dynamically?
I prefer inlined assembly if it is simple. This one-liner makes it easy for a reader.

I wanted to delete `.type bar, at function; .type weak, at function` but unfortunately neither binutils as nor llvm-mc marks a symbol STT_FUNC if there is a call instruction.


================
Comment at: test/ELF/riscv-plt.s:72
+  call foo
+  call bar
+  call bar at plt
----------------
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.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D63076