[lld][RISCV] Fixup PC-relative relocations to undefined weak symbols.

[bsdjhb]

Summary

Load an explicit NULL address for non-PIC linkage when possible.
Report an error for conditional branches that target undefined weak
symbols.

[bsdjhb]

This is needed to link a FreeBSD/riscv64 kernel which uses undefined weak symbols in the uart(4) driver.

lld/ELF/InputSection.cpp

999

These cases do not handle compressed jump and compressed branch relocations. I tried to use 'c.j' to in the new test case but the assembler turned it into a 4-byte 'j' instruction instead (even when forced to riscv32 and using '-mattr=+c'. Right now those relocations would at least exit with an error, but if I can fix the test case to exercise them I'm happy to fix it.

1019

This can result in duplicate warnings from getRISCVPCRelHi20(). One option @jrtc27 suggested was that perhaps we could lift the logic to compute the targetVA from hiRel out of getRelocTargetVA() to here so that we only call getRISCVPCRelHi20() once. Another option might be to make the nested call in getRelocTargetVA() for 'hiRel' to instead return a targetVA of 0 by changing the R_PC for undefined weak symbols in RISC-V to return 0 instead of dest - p. We kind of do this now for EM_PPC it seems by setting 'dest' to 'p' so that it effectively returns 0.

[MaskRay]

lld/ELF/InputSection.cpp

982

Adding the code here is not correct. The most appropriate place is getRelocTargetVA. However, I don't recommend adding any logic at all because weak undefined symbols do not have defined semantics. This means multiple things:

1. their behaviors may be different in different toolchains
2. their behavior may change with various configuration (-no-pie, -pie, -shared) even in the same toolchain

The binutils behaviors may sometime not easy to mimic in lld because their internals are so different.

I did add the following two lines for EM_PPC, but that was because I had no choice but to support that glibc use case.

else if (config->emachine == EM_PPC)
  dest = p;

For anything newly developed, we should try our best to avoid weak undefined symbols.

Can you share with me some instructions to build FreeBSD RISC-V?

lld/test/ELF/riscv-undefined-weak-branch.s

2

Delete -none-linux as this should be generic. Prefer # and ## for comments. See precedents in this directory.

lld/test/ELF/riscv-undefined-weak.s

31–33

If you can use CHECK-NEXT for all lines, you can delete addresses. They are not significant for the test.

[jrtc27]

lld/ELF/InputSection.cpp

982

*Calling* weak undefined symbols does not necessarily have well-defined semantics, depending on the architecture, but referencing them absolutely does (namely that the result of the code sequence is NULL) and is required for many systems, not just FreeBSD. The issue here is that, with -mcmodel=medany (LLVM's medium) in position-dependent code generation, the addressing mode is PC-relative, which breaks if linked at an address beyond 2 GiB as the 32-bit immediate pair is too small to negate PC, but the whole point of medany is to allow linking code at any address, including above the 2 GiB mark (otherwise you'd just use medlow).

[jrtc27]

lld/ELF/InputSection.cpp

982

The alternative is to generate code using the GOT for extern weak symbols, as is done on AArch64 (which also otherwise uses PC-relative addressing), but that would require changes for both GCC and LLVM.

[jrtc27]

lld/ELF/InputSection.cpp

982

Moreover, even if calling undefined weak functions is not well-defined in the ABI, it still needs to link without error to *something*, otherwise you can't write the perfectly-legitimate common construct that is:

extern void foo(void) __attribute__((weak));
if (&foo)
  foo();

The call can be turned into whatever you like at link time if the symbol is undefined, because it should be guarded by an if (and it's undefined behaviour otherwise, I would assume), but the linker needs to do something that's not failing with an error.

[MaskRay]

lld/ELF/InputSection.cpp

982

No. http://www.sco.com/developers/gabi/latest/ch4.symtab.html

The behavior of weak symbols in areas not specified by this document is implementation defined. Weak symbols are intended primarily for use in system software. Applications using weak symbols are unreliable since changes in the runtime environment might cause the execution to fail.

You may argue that we are targeting system software, but I still want to say that the GNU ld behavior may vary according to (1) instruction types, or (2) executable vs shared objects. lld's treatment is still different from GNU ld and gold in some cases on other architectures.

extern void foo(void) __attribute__((weak));
if (&foo)
  foo();

I hope we can make some efforts to get rid of such constructs in FreeBSD first, before trying a linker workaround.

I haven't checked the logic below clearly. It appears to be more of a hack.

hiRel && hiRel->type == R_RISCV_PCREL_HI20
            && hiRel->sym->isUndefWeak() && hiRel->addend == 0

982

The alternative is to generate code using the GOT for extern weak symbols, as is done on AArch64 (which also otherwise uses PC-relative addressing), but that would require changes for both GCC and LLVM.

Fixing GCC/llvm sounds a good idea to me. I can check how to fix it in llvm. Apparently PPC64 also decided to use GOT when they migrated from PPC32. (I am now just very concerned that some binutils/gcc decisions were made before people think hard about the implications... from the binutils bugs I reported, I at least can confirm that many bugs were copied from elf32-arm.c and elf*-mips.c ...)

[bsdjhb]

lld/ELF/InputSection.cpp

982

We can't really remove all weak symbols from FreeBSD's kernel. FreeBSD's kernel uses linker sets extensively to build link-time generated arrays of pointers to objects. If a linker set is empty, the start and stop symbols for that linker set are weak-undefined symbols that FreeBSD requires to resolve to NULL. Removing linker sets from FreeBSD and/or mandating that all linker sets are non-empty are not feasible.

I've opened an issue on the RISC-V psabi doc to track the ABI question:

https://github.com/riscv/riscv-elf-psabi-doc/issues/126

[bsdjhb]

lld/ELF/InputSection.cpp

982

(Note that originally I had assumed the only weak symbols were in the uart(4) driver, but after testing a patch to remove those, I ran into an empty linker set and noticed that those also use weak symbols.)

[MaskRay]

Created D72046. As per my interpretation at https://github.com/riscv/riscv-elf-psabi-doc/issues/126#issuecomment-568160758 , we have to assume

Compilers should not emit PC-relative relocations in -fpic/-fpie mode.

GCC/clang emit either absolute or GOT relocations, so the assumption is true. (If not in edge cases, we have to fix the compilers.) I don't think errors on branch relocations are useful. So, D72046 just does what I did to PPC32 (the patch changes p to p+a for PPC32 as well. It is a minor issue anyway).

[jrtc27]

In D71100#1800266, @MaskRay wrote:

Created D72046. As per my interpretation at https://github.com/riscv/riscv-elf-psabi-doc/issues/126#issuecomment-568160758 , we have to assume

Compilers should not emit PC-relative relocations in -fpic/-fpie mode.

GCC/clang emit either absolute or GOT relocations, so the assumption is true. (If not in edge cases, we have to fix the compilers.) I don't think errors on branch relocations are useful. So, D72046 just does what I did to PPC32 (the patch changes p to p+a for PPC32 as well. It is a minor issue anyway).

The problem is and always has been -mcmodel=medany. Neither GCC nor LLVM use the GOT in this case, as it’s not -fpic/-fpie. This is the case we care about for the kernel. Would I have made GCC do this? No. But it does, and we have to deal with that in a sensible way. Making it an error and changing GCC and LLVM is one such way. But that hasn’t happened.

[MaskRay]

Request changes as the patch place is incorrect. As mentioned, the most appropriate place is getRelocTargetVA.

We need some guidance from psABI as well (https://github.com/riscv/riscv-elf-psabi-doc/issues/126).

I think error for branch relocation types is an incorrect behavior: https://maskray.me/blog/2021-04-25-weak-symbol

[MaskRay]

Sent D103001

lld/ELF/InputSection.cpp

case R_PPC64_CALL:
         if (bufLoc + 8 > bufEnd || read32(bufLoc + 4) != 0x60000000) {
           error(getErrorLocation(bufLoc) + "call lacks nop, can't restore toc");
           break;
         }
         write32(bufLoc + 4, 0xe8410018); // ld %r2, 24(%r1)
       }
       target->relocateOne(bufLoc, type, targetVA);
       break;
+    case R_PC:
+      if (config->emachine == EM_RISCV && rel.sym->isUndefWeak() &&
+          !config->isPic) {
+        // Patch addresses of undefined weak symbols to be NULL.
+        if (rel.addend != 0)
+          error(getErrorLocation(bufLoc) +
+                "undefined weak symbol relocation with non-zero addend");
+        uint32_t insn = read32le(bufLoc);
+        switch (type) {
+        case R_RISCV_CALL:
+          // Clear immediate in following jalr.
+          write32le(bufLoc + 4, read32le(bufLoc + 4) & 0x000fffff);
+          LLVM_FALLTHROUGH;
+        case R_RISCV_PCREL_HI20:
+          // Rewrite auipc rd, %pcrel_hi(sym) to lui rd, 0.
+          write32le(bufLoc, (insn & 0x00000f80) | 0x00000037);
+          break;
+        case R_RISCV_JAL:
+          write32le(bufLoc, insn & 0x00000fff);
+          break;
+        case R_RISCV_BRANCH:
+          error(getErrorLocation(bufLoc) +
+                "conditional branch to undefined weak symbol '" +
+                rel.sym->getName() + "'");
+          break;
+        default:
+          error(getErrorLocation(bufLoc) +
+                "unknown RISC-V relocation " + toString(type) +
+                " for undefined weak symbol");
+          break;
+        }
+        break;
+      }
+      target->relocateOne(bufLoc, type, targetVA);
+      break;
+    case R_RISCV_PC_INDIRECT:
+      if (!config->isPic) {
+        const Relocation *hiRel = getRISCVPCRelHi20(rel.sym, rel.addend);
+        if (hiRel && hiRel->type == R_RISCV_PCREL_HI20
+            && hiRel->sym->isUndefWeak() && hiRel->addend == 0) {
+          // Patch addresses of undefined weak symbols to be NULL.
+          targetVA = 0;
+        }
+      }
+      target->relocateOne(bufLoc, type, targetVA);
+      break;
     default:
       target->relocateOne(bufLoc, type, targetVA);
       break;
     }
   }
 }
 
 // For each function-defining prologue, find any calls to __morestack,

lld/test/ELF/riscv-undefined-weak-branch.s

+// REQUIRES: riscv
+// RUN: llvm-mc -filetype=obj -triple=riscv64-none-linux %s -o %t.o
+// RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s
+
+// Check that we don't permit conditional branches to undefined weak
+// symbols in non-PIC code.
+  
+ .weak target
+
+ .text
+ .global _start
+_start:
+ beq a0, a1, target
+
+// CHECK: error: {{.*}}: conditional branch to undefined weak symbol 'target'

lld/test/ELF/riscv-undefined-weak.s

+// REQUIRES: riscv
+// RUN: llvm-mc -filetype=obj -triple=riscv64-none-linux %s -o %t.o
+// RUN: ld.lld %t.o -o %t
+// RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck %s
+
+// Check that undefined weak symbols are resolved to NULL in non-PIC
+// RISC-V code.
+  
+ .weak target
+
+ .text
+ .global _start
+_start:
+// R_RISCV_PCREL_HI20 + R_RISCV_LO12_I
+ la a0, target
+// R_RISCV_PCREL_HI20 + R_RISCV_LO12_I
+ ld a1, target
+// R_RISCV_PCREL_HI20 + R_RISCV_LO12_S
+ sd a2, target, t0
+// R_RISCV_JAL
+ j target
+// R_RISCV_JAL
+ jal target
+// R_RISCV_CALL
+ call target
+// R_RISCV_CALL
+ tail target
+
+// CHECK: Disassembly of section .text:
+// CHECK-EMPTY:
+// CHECK:         11120:        lui a0, 0
+// CHECK-NEXT:    11124:        mv  a0, a0
+// CHECK:         11128:        lui a1, 0
+// CHECK-NEXT:    1112c:        ld  a1, 0(a1)
+// CHECK:         11130:        lui t0, 0
+// CHECK-NEXT:    11134:        sd  a2, 0(t0)
+// CHECK-NEXT:    11138:        j 0
+// CHECK-NEXT:    1113c:        jal 0
+// CHECK-NEXT:    11140:        lui ra, 0
+// CHECK-NEXT:    11144:        jalr  ra
+// CHECK-NEXT:    11148:        lui t1, 0
+// CHECK-NEXT:    1114c:        jr  t1