[PATCH] D71820: [lld][RISCV] Print error when encountering R_RISCV_ALIGN

Sun Dec 22 17:23:01 PST 2019

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

================
Comment at: lld/ELF/Arch/RISCV.cpp:231
   case R_RISCV_TPREL_ADD:
     return R_HINT;
+  case R_RISCV_ALIGN:
----------------
MaskRay wrote:
> I think `R_HINT` is not useful, so I created D71822.
It might be if LLD learns RISC-V's linker relaxations, although that could instead be a new `R_RELAX` (or RISC-V specific type). I can rebase if your patch is going to land first.

================
Comment at: lld/ELF/Arch/RISCV.cpp:236
+    // delete NOPs to realign.
+    error(getErrorLocation(loc) +
+          "relocation R_RISCV_ALIGN requires unimplemented linker relaxation"
----------------
MaskRay wrote:
> If you think there are needs (which I think so) to test lld on existing `R_RISCV_ALIGN` relocations,
> use `errorOrWarn` so that errors can be downgraded to warnings with --noinhibit-exec.
I guess it does no harm, given --noinhibit-exec is a debugging option rather than intended for users (for whom this should definitely be an error).

================
Comment at: lld/ELF/Arch/RISCV.cpp:439
   case R_RISCV_JUMP_SLOT:
+  // Should have already given an error in getRelExpr
+  case R_RISCV_ALIGN:
----------------
MaskRay wrote:
> Full stop, though the examples above and below (they are inconsistent with other places) do not use it...
Even for phrases that aren't complete sentences?

================
Comment at: lld/test/ELF/riscv-reloc-align.s:3
+
+# RUN: llvm-mc -filetype=obj -triple=riscv32-unknown-elf -mattr=+relax %s -o %t.o
+# RUN: not ld.lld %t.o -o %t 2>&1 | FileCheck %s
----------------
MaskRay wrote:
> `-unknown-elf` is implied, I think.
Yeah, not that it even matters which is used; the tests are inconsistent and I copied from one that happened to spell it in full. Will make shorter.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71820