Ignore R_MIPS_JALR relocations against non-function symbols

[arichardson]

Summary

Older versions of clang would erroneously emit this relocation not only
against functions (loaded from the GOT) but also against data symbols
(e.g. a table of function pointers). LLD was then changing this into a
branch-and-link instruction, causing the program to jump to the data
symbol at run time. I discovered this problem when attempting to boot
MIPS64 FreeBSD after updating the to the latest upstream master.

[atanasyan]

lld/ELF/Arch/Mips.cpp

90

This change breaks mips-jalr.s test case. Now we have to explicitly provide .type @function directive. What do you think about this condition if (s.isObject() || s.isTls())?

lld/test/ELF/mips-r-jalr-non-functions.s

1 ↗

(On Diff #229876)

I'd like to suggest more compact and less verbose test case check both TLS and regular "object":

...
# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t.o
# RUN: ld.lld -shared %t.o -o %t.so 2>&1 | FileCheck %s -check-prefix WARNING-MESSAGE
# RUN: llvm-objdump -d %t.so | FileCheck %s

test:
  .reloc .Ltmp1, R_MIPS_JALR, tls_obj
.Ltmp1:
  jr  $t9
  nop
# WARNING-MESSAGE: warning: found R_MIPS_JALR relocation against non-function symbol tls_obj. This is invalid and most likely a compiler bug.

  .reloc .Ltmp2, R_MIPS_JALR, reg_obj
.Ltmp2:
  jr  $t9
  nop
# WARNING-MESSAGE: warning: found R_MIPS_JALR relocation against non-function symbol reg_obj. This is invalid and most likely a compiler bug.

  .type  tls_obj,@object
  .section  .tbss,"awT",@nobits
tls_obj:
  .word 0

  .type  reg_obj,@object
  .data
reg_obj:
  .word 0

# CHECK:      test:
# CHECK-NEXT:   jr $25
# CHECK-NEXT:   ...
# CHECK-NEXT:   jr $25

[arichardson]

lld/ELF/Arch/Mips.cpp

90

I think if (!s.isFunc() && s.type != STT_NOTYPE) might be better since that catches all other non-function types.

However, this is purely a missed optimization when not being applied, but an error when applied erroneously, so I'd lean towards only allowing function symbols.
Do you think that other than the testcase in LLD, there exists any code that would use R_MIPS_JALR against a STT_NOTYPE symbol that could not be marked with .type @function? I feel like in that case the programmer might as well write the bal explicitly in the assembly?

What do you think?

lld/test/ELF/mips-r-jalr-non-functions.s

1 ↗

(On Diff #229876)

Thanks, that looks good. Will update.

[atanasyan]

lld/ELF/Arch/Mips.cpp

90

if (!s.isFunc() && s.type != STT_NOTYPE) looks better.

I do not know are there any "real" assembler code use R_MIPS_JALR relocations and symbols without .type directive. But it's possible.

jalr can be replaced by bal right in a .s file if a distance to the target is known. But it's not so if the target and the relocation are in different object files.

[arichardson]

• Use !s.isFunc() && s.type != STT_NOTYPE
• Update test case

[atanasyan]

LGTM. Thanks

lld/ELF/Arch/Mips.cpp

 RelExpr MIPS<ELFT>::getRelExpr(RelType type, const Symbol &s,
                                const uint8_t *loc) const {
   // See comment in the calculateMipsRelChain.
   if (ELFT::Is64Bits || config->mipsN32Abi)
     type &= 0xff;
 
   switch (type) {
   case R_MIPS_JALR:
+    // Older versions of clang would erroneously emit this relocation not only
+    // against functions (loaded from the GOT) but also against data symbols
+    // (e.g. a table of function pointers). When we encounter this, ignore the
+    // relocation and emit a warning instead.
+    if (!s.isFunc() && s.type != STT_NOTYPE) {
+      warn(getErrorLocation(loc) +
+           "found R_MIPS_JALR relocation against non-function symbol " +
+           toString(s) + ". This is invalid and most likely a compiler bug.");
+      return R_NONE;
+    }
+
     // If the target symbol is not preemptible and is not microMIPS,
     // it might be possible to replace jalr/jr instruction by bal/b.
     // It depends on the target symbol's offset.
     if (!s.isPreemptible && !(s.getVA() & 0x1))
       return R_PC;
     return R_NONE;
   case R_MICROMIPS_JALR:
     return R_NONE;

lld/test/ELF/mips-jalr-non-functions.s

+# REQUIRES: mips
+## Check that we ignore R_MIPS_JALR relocations agains non-function symbols.
+## Older versions of clang were erroneously generating them for function pointers
+## loaded from any table (not just the GOT) as well as against TLS function
+## pointers (when using the local-dynamic model), so we need to ignore these
+## relocations to avoid generating binaries that crash when executed.
+
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t.o
+# RUN: ld.lld -shared %t.o -o %t.so 2>&1 | FileCheck %s -check-prefix WARNING-MESSAGE
+# RUN: llvm-objdump --no-show-raw-insn --no-leading-addr -d %t.so | FileCheck %s
+
+.set  noreorder
+test:
+  .reloc .Ltmp1, R_MIPS_JALR, tls_obj
+.Ltmp1:
+  jr  $t9
+  nop
+# WARNING-MESSAGE: warning: found R_MIPS_JALR relocation against non-function symbol tls_obj. This is invalid and most likely a compiler bug.
+
+  .reloc .Ltmp2, R_MIPS_JALR, reg_obj
+.Ltmp2:
+  jr  $t9
+  nop
+# WARNING-MESSAGE: warning: found R_MIPS_JALR relocation against non-function symbol reg_obj. This is invalid and most likely a compiler bug.
+
+  .reloc .Ltmp3, R_MIPS_JALR, untyped
+.Ltmp3:
+  jr  $t9
+  nop
+
+## However, we do perform the optimization for untyped symbols:
+untyped:
+  nop
+
+  .type  tls_obj,@object
+  .section  .tbss,"awT",@nobits
+tls_obj:
+  .word 0
+
+  .type  reg_obj,@object
+  .data
+reg_obj:
+  .word 0
+
+# CHECK-LABEL: Disassembly of section .text:
+# CHECK-EMPTY:
+# CHECK-NEXT: test:
+# CHECK-NEXT: jr  $25
+# CHECK-NEXT: nop
+# CHECK-NEXT: jr  $25
+# CHECK-NEXT: nop
+# CHECK-NEXT: b 8 <untyped>
+# CHECK-NEXT: nop