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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:982
       break;
+    case R_PC:
+      if (config->emachine == EM_RISCV && rel.sym->isUndefWeak() &&
----------------
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?


================
Comment at: lld/test/ELF/riscv-undefined-weak-branch.s:2
+// 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
----------------
Delete `-none-linux` as this should be generic. Prefer `#` and `##` for comments. See precedents in this directory.


================
Comment at: lld/test/ELF/riscv-undefined-weak.s:31-33
+// CHECK:         11120:      	lui	a0, 0
+// CHECK-NEXT:    11124:      	mv	a0, a0
+// CHECK:         11128:      	lui	a1, 0
----------------
If you can use CHECK-NEXT for all lines, you can delete addresses. They are not significant for the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71100