[LLD][RISCV] Error on PCREL_LO referencing other Section

[lenary]

The RISC-V psABI states that "The R_RISCV_PCREL_LO12_I or R_RISCV_PCREL_LO12_S relocations contain a label pointing to an instruction in the same section with an R_RISCV_PCREL_HI20 relocation entry that points to the target symbol."

In this case, GNU ld errors, but LLD does not -- I think because it is doing the right thing, certainly in the testcase provided.

Nonetheless, I think an error is good here to bring LLD in line with what GNU ld is doing in showing that the object they are using is not following the psABI as written.

Fixes #107304

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Sam Elliott (lenary)

Changes

The RISC-V psABI states that "The R_RISCV_PCREL_LO12_I or R_RISCV_PCREL_LO12_S relocations contain a label pointing to an instruction in the same section with an R_RISCV_PCREL_HI20 relocation entry that points to the target symbol."

In this case, GNU ld errors, but LLD does not -- I think because it is doing the right thing, certainly in the testcase provided.

Nonetheless, it would be good for LLD to at least warn the user, so they know the objects they are using are not following the psABI as written.

Fixes #107304

---

Full diff: https://github.com/llvm/llvm-project/pull/107558.diff

2 Files Affected:

• (modified) lld/ELF/Arch/RISCV.cpp (+9)
• (added) lld/test/ELF/riscv-pcrel-hilo-error-sections.s (+35)

diff --git a/lld/ELF/Arch/RISCV.cpp b/lld/ELF/Arch/RISCV.cpp
index 2435864ce5a7f0..80a7909c42f2e5 100644
--- a/lld/ELF/Arch/RISCV.cpp
+++ b/lld/ELF/Arch/RISCV.cpp
@@ -671,6 +671,15 @@ void RISCV::relocateAlloc(InputSectionBase &sec, uint8_t *buf) const {
       errorOrWarn(sec.getLocation(rel.offset) +
                   ": R_RISCV_SET_ULEB128 not paired with R_RISCV_SUB_SET128");
       return;
+    case R_RISCV_PC_INDIRECT:
+      if (Defined *def = dyn_cast<Defined>(rel.sym);
+          def->section && def->section != &sec) {
+        errorOrWarn(sec.getLocation(rel.offset) +
+              ": R_RISCV_PCREL_LO12 relocation points to a symbol '" +
+              rel.sym->getName() + "' in a different section '" +
+              def->section->name + "'");
+      }
+      break;
     default:
       break;
     }
diff --git a/lld/test/ELF/riscv-pcrel-hilo-error-sections.s b/lld/test/ELF/riscv-pcrel-hilo-error-sections.s
new file mode 100644
index 00000000000000..deffbd91b0ff61
--- /dev/null
+++ b/lld/test/ELF/riscv-pcrel-hilo-error-sections.s
@@ -0,0 +1,35 @@
+# REQUIRES: riscv
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 %s -o %t.o
+# RUN: not ld.lld %t.o 2>&1 | FileCheck %s
+
+# CHECK: error: {{.*}}:(.text.sec_one+0x0): R_RISCV_PCREL_LO12 relocation points to a symbol '.Lpcrel_hi0' in a different section '.text.sec_two'
+# CHECK: error: {{.*}}:(.text.sec_one+0x4): R_RISCV_PCREL_LO12 relocation points to a symbol '.Lpcrel_hi1' in a different section '.text.sec_two'
+# CHECK-NOT: R_RISCV_PCREL_LO12 relocation points to a symbol '.Lpcrel_hi2'
+
+## This test is checking that we warn the user when the relocations in their
+## object don't follow the RISC-V psABI. In particular, the psABI requires
+## that PCREL_LO12 relocations are in the same section as the pcrel_hi
+## instruction they point to.
+
+  .section .text.sec_one
+  .globl sec_one
+sec_one:
+  addi a0, a0, %pcrel_lo(.Lpcrel_hi0)
+  sw a0, %pcrel_lo(.Lpcrel_hi1)(a1)
+
+  .section .text.sec_two
+sec_two:
+.Lpcrel_hi0:
+  auipc a0, %pcrel_hi(a)
+.Lpcrel_hi1:
+  auipc a1, %pcrel_hi(a)
+
+.Lpcrel_hi2:
+  auipc a2, %pcrel_hi(a)
+  addi a2, a2, %pcrel_lo(.Lpcrel_hi2)
+
+  .data
+  .global a
+a:
+  .word 50

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[lenary]

Some notes on how I fixed this:

• The function which does the other validation of PCREL_LO relocations is getRISCVPCRelHi20 - but it, and where it is called from (InputSectionBase::getRelocTargetVA) do not have access to the input section, for comparison with the section of the symbol referenced by the relocation. Changing this seems quite an invasive change (likely needing to make getRelocTargetVA non-static, changing quite a few call sites)

• I've been fairly indecisive about whether this should be an errorOrWarn, or a warn. Guidance about which is preferred would be helpful.

[MaskRay]

(I am on vacation and is likely slow to respond.)

[lenary]

ping

[lenary]

Thanks for the refactor in 2b5cb1bf628fc54473355e0675f629d9332089df - I'll change the approach and implement this in getRISCVPCRelHi20 - updated PR coming imminently.

[lenary]

I also took the opportunity to improve the other errors/warnings emitted by the same function to point to exactly where the issue is.