This is an archive of the discontinued LLVM Phabricator instance.

Ignore R_MIPS_JALR relocations against non-function symbols
ClosedPublic

Authored by arichardson on Nov 18 2019, 10:07 AM.

Details

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.

Event Timeline

arichardson created this revision.Nov 18 2019, 10:07 AM
atanasyan added inline comments.Nov 19 2019, 3:38 AM
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 marked 2 inline comments as done.Nov 19 2019, 3:56 AM
arichardson added inline comments.
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 added inline comments.Nov 19 2019, 5:48 AM
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.

  • Use !s.isFunc() && s.type != STT_NOTYPE
  • Update test case
atanasyan accepted this revision.Nov 20 2019, 3:59 AM

LGTM. Thanks

This revision is now accepted and ready to land.Nov 20 2019, 3:59 AM
This revision was automatically updated to reflect the committed changes.