[lld][RISCV] Print error when encountering R_RISCV_ALIGN

[jrtc27]

Summary

Unlike R_RISCV_RELAX, which is a linker hint, R_RISCV_ALIGN requires the
support of the linker even when ignoring all R_RISCV_RELAX relocations.
This is because the compiler emits as many NOPs as may be required for
the requested alignment, more than may be required pre-relaxation, to
allow for the target becoming more unaligned after relaxing earlier
sequences. This means that the target is often not initially aligned in
the object files, and so the R_RISCV_ALIGN relocations cannot just be
ignored. Since we do not support linker relaxation, we must turn these
into errors.

[MaskRay]

lld/ELF/Arch/RISCV.cpp

240–247

I think R_HINT is not useful, so I created D71822.

245

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.

443

Full stop, though the examples above and below (they are inconsistent with other places) do not use it...

lld/test/ELF/riscv-reloc-align.s

4

-unknown-elf is implied, I think.

5

-o /dev/null

[jrtc27]

lld/ELF/Arch/RISCV.cpp

240–247

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.

245

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).

443

Even for phrases that aren't complete sentences?

lld/test/ELF/riscv-reloc-align.s

4

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.

[jrtc27]

@MaskRay Do you think we could get this landed (with or without D71822) before 10 branches tomorrow?

[jrtc27]

Rebased and addressed review comments.

[jrtc27]

In D71820#1819146, @jrtc27 wrote:

@MaskRay Do you think we could get this landed (with or without D71822) before 10 branches tomorrow?

I see you've already committed the RISC-V parts of that, so the dependency no longer exists.

[MaskRay]

I was waiting on D71822 . @grimar thought it is good, and I guess you would say so. So I went ahead and commit it. LG, since users can use --noinhibit-exec to downgrade the error to a warning.

[asb]

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

[grimar]

In D71820#1836028, @asb wrote:

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

FWIW, I think it is fine to backport this to 10.0

[MaskRay]

In D71820#1838015, @grimar wrote:

In D71820#1836028, @asb wrote:

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

It feels like getting this error reporting in place ASAP would be valuable for end users, and could reduce erroneous bug reports / wasted debugging time.

FWIW, I think it is fine to backport this to 10.0

+1

@hans

[hans]

Cherry-picked to 10.x in a3db4e05e36e836077e6e551feb6841c7e8023a0
Thanks!

lld/ELF/Arch/RISCV.cpp

RelExpr RISCV::getRelExpr(const RelType type, const Symbol &s,
   case R_RISCV_TLS_GOT_HI20:
     config->hasStaticTlsModel = true;
     return R_GOT_PC;
   case R_RISCV_TPREL_HI20:
   case R_RISCV_TPREL_LO12_I:
   case R_RISCV_TPREL_LO12_S:
     return R_TLS;
   case R_RISCV_RELAX:
-  case R_RISCV_ALIGN:
   case R_RISCV_TPREL_ADD:
     return R_NONE;
+  case R_RISCV_ALIGN:
+    // Not just a hint; always padded to the worst-case number of NOPs, so may
+    // not currently be aligned, and without linker relaxation support we can't
+    // delete NOPs to realign.
+    errorOrWarn(getErrorLocation(loc) + "relocation R_RISCV_ALIGN requires "
+                "unimplemented linker relaxation; recompile with -mno-relax");
+    return R_NONE;
   default:
     error(getErrorLocation(loc) + "unknown relocation (" + Twine(type) +
           ") against symbol " + toString(s));
     return R_NONE;
   }
 }
 
 // Extract bits V[Begin:End], where range is inclusive, and Begin must be < 63.
void RISCV::relocateOne(uint8_t *loc, const RelType type,
 
   case R_RISCV_TLS_DTPREL32:
     write32le(loc, val - dtpOffset);
     break;
   case R_RISCV_TLS_DTPREL64:
     write64le(loc, val - dtpOffset);
     break;
 
-  case R_RISCV_ALIGN:
   case R_RISCV_RELAX:
     return; // Ignored (for now)
 
   default:
     llvm_unreachable("unknown relocation");
   }
 }
 
 TargetInfo *getRISCVTargetInfo() {
   static RISCV target;
   return &target;
 }
 
 } // namespace elf
 } // namespace lld

lld/test/ELF/riscv-reloc-align.s

+# REQUIRES: riscv
+
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s -o %t.o
+# RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK: relocation R_RISCV_ALIGN requires unimplemented linker relaxation
+
+.global _start
+_start:
+    nop
+    .balign 8
+    nop