[PATCH] D143673: [lld][RISCV] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/test/ELF/riscv-relax-hi20-lo12.s:6
+# RUN: llvm-mc -filetype=obj -triple=riscv32-unknown-elf -mattr=+c,+relax %s -o %t.rv32c.o
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=+c,+relax %s -o %t.rv64c.o
+
----------------
craig.topper wrote:
> MaskRay wrote:
> > We may need a test for `-pie`.
> -pie should use %pcrel relocations right?
We should have a meaningful diagnostic referencing a non-absolute symbol with an absolute relocation in -pie mode. It probably doesn't belong to this test.

In addition, it's valid to reference SHN_ABS symbols in `-pie` mode. We should test that SHN_ABS symbols are correctly handled. Whether it's handled doesn't matter since such cases are super rare.


================
Comment at: lld/test/ELF/riscv-relax-hi20-lo12.s:30
+_start:
+  lui a0, %hi(foo)
+  addi a0, a0, %lo(foo)
----------------
craig.topper wrote:
> MaskRay wrote:
> > I usually test at least two symbols so that I can check in-bounds and out-of-bounds in one file. It is useful to test the largest displacement which is still relaxable.
> > 
> > You may check how riscv-relax-call2.s and x86-64-tlsdesc-ld.s test stuff.
> > 
> > Also, like riscv-relax-align.s, we'd better test that symbol values after the relaxed location is still correct.
> Which part of riscv-relax-align.s tests that?
You can just define a symbol in the end of this file and test its value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143673