[PATCH] D70406: Ignore R_MIPS_JALR relocations against non-function symbols

[Alexander Richardson via Phabricator via llvm-commits]

arichardson marked 2 inline comments as done.
arichardson added inline comments.


================
Comment at: lld/ELF/Arch/Mips.cpp:90
+    // relocation and emit a warning instead.
+    if (!s.isFunc()) {
+      warn(getErrorLocation(loc) +
----------------
atanasyan wrote:
> 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())`?
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?


================
Comment at: lld/test/ELF/mips-r-jalr-non-functions.s:1
+# REQUIRES: mips
+## Check that we ignore R_MIPS_JALR relocations agains non-function symbols.
----------------
atanasyan wrote:
> 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, at object
>   .section  .tbss,"awT", at nobits
> tls_obj:
>   .word 0
> 
>   .type  reg_obj, at object
>   .data
> reg_obj:
>   .word 0
> 
> # CHECK:      test:
> # CHECK-NEXT:   jr $25
> # CHECK-NEXT:   ...
> # CHECK-NEXT:   jr $25
> ```
Thanks, that looks good. Will update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70406