[ELF][MIPS] Don't change every dynamic relocation into R_MIPS_REL32

[arichardson]

Summary

I was debugging some strange crashes in a dynamically linked CHERI binary
and it turns out that LLD always emits R_MIPS_REL32/R_MIPS_64 dynamic
relocations (even for an undefined symbol). The crash I was getting
happened because the program loaded a global function pointer referencing
an undefined symbol. However, because the wrong relocation was being
emitted RTLD only added the relocbase to the function pointer. Therefore
the program crashed when jumping to the start of the mapped ELF file
since that does not contain valid instructions.

[arichardson]

Added the correct version of the test

[ruiu]

ELF/Arch/Mips.cpp

191–192

Is this correct? getDynRel is supposed to return a dynamic relocation type for a given static relocation type. Currently it returns a platform-specific relocation type for the dynamic relocation, which seems correct to me.

[arichardson]

ELF/Arch/Mips.cpp

191–192

I am not sure if the TLS relocations need any change but for preemptible relocations in the data section we want to emit an absolute relocation that RTLD can fill in and not one that just adds the base address to the addend.

I looked at both FreeBSD rtld and glib rtld and both only add the current objects base address for R_MIPS_REL32/R_MIPS_64/R_MIPS_NONE (Target->RelativeRel) which will break any pointers to undefined symbols in the data section.

[ruiu]

ELF/Arch/Mips.cpp

191–192

Sorry I think I didn't understand this function well. I believe this change is correct. You can remove this function entirely because it is a virtual function, and the base class' function does the same thing as this function does.

[arichardson]

ELF/Arch/Mips.cpp

191–192

Yes, I will do that if @atanasyan can confirm that there are no MIPS relocations that need to be converted to another one.

[atanasyan]

test/ELF/mips-32.s

51

I have not had a time for deep investigation yet, but both bfd and gold linker emit two R_MIPS_REL32 relocations for that test case. It looks like either a bug in GNU tools or problem in this fix.

[arichardson]

Turns out we can't emit R_MIPS_64 absolute relocations that don't need the GOT since neither glibc nor FreeBSD handle a R_MIPS_64 dynamic relocations.

I wonder if we should emit a R_MIPS_GLOB_DAT for unresolved symbols since that will always look in the global part of the GOT.

test/ELF/mips-32.s

51

It appears that while this fixes my issue it is not the actually correct fix. Emitting R_MIPS_REL32 is the correct thing because of the MIPS ABI:

R_MIPS_REL32 relocation depends on its r_symndx value. If the relocation r_symndx is less than DT_MIPS_GOTSYM, the value of EA is the symbol st_value plus displacement. Otherwise, the value of EA is the value in the GOT entry corresponding to the relocation entry r_symndx. The correspondence between the GOT and the dynamic symbol table is described in the "Global Offset Table" section in Chapter 5.

For some reason the r_symndx in my case ended up being less than DT_MIPS_GOTSYM even though it was referencing a preemptible symbol. Therefore it would only add the load address instead of the resolved value from the GOT. I will try to reduce my testcase (unfortunately it's libc.so, so it could take a while) to find out why I ended up getting the wrong value.

However, I don't really understand why we need to add a relocation in the GOT just to write an absolute data symbol. I believe emitting a R_MIPS_64 relocation should result in the same runtime value being stored but we don't need the additional GOT slot.

[atanasyan]

In D43952#1025301, @arichardson wrote:

However, I don't really understand why we need to add a relocation in the GOT just to write an absolute data symbol. I believe emitting a R_MIPS_64 relocation should result in the same runtime value being stored but we don't need the additional GOT slot.

I think it's betters to ask this question on GNU binutils / libc mail lists.

[arichardson]

This is wrong

ELF/Arch/Mips.cpp

RelExpr MIPS<ELFT>::getRelExpr(RelType Type, const Symbol &S,
   }
 }
 
 template <class ELFT> bool MIPS<ELFT>::isPicRel(RelType Type) const {
   return Type == R_MIPS_32 || Type == R_MIPS_64;
 }
 
 template <class ELFT> RelType MIPS<ELFT>::getDynRel(RelType Type) const {
-  return RelativeRel;
+  // TODO: are there any that need to change?
+  return Type;
 }
 
 template <class ELFT>
 void MIPS<ELFT>::writeGotPlt(uint8_t *Buf, const Symbol &) const {
   uint64_t VA = InX::Plt->getVA();
   if (isMicroMips())
     VA |= 1;
   write32<ELFT::TargetEndianness>(Buf, VA);

test/ELF/mips-32.s

 
 # SYM: SYMBOL TABLE:
 # SYM: 00020000 l       .data           00000004 v1
 # SYM: 00020004 g       .data           00000008 v2
 
 # REL:      Relocations [
 # REL-NEXT:   Section (7) .rel.dyn {
 # REL-NEXT:     0x20008 R_MIPS_REL32 - 0x0
-# REL-NEXT:     0x20004 R_MIPS_REL32 v2 0x0
+# REL-NEXT:     0x20004 R_MIPS_32 v2 0x0
 # REL-NEXT:   }
 # REL-NEXT: ]
 
 # REL:   DynamicSection [
 # REL:     Tag        Type                 Name/Value
 # REL:     0x00000012 RELSZ                16 (bytes)
 # REL:     0x00000013 RELENT               8 (bytes)
 # REL-NOT: 0x6FFFFFFA RELCOUNT

test/ELF/mips-64.s

 
 # SYM: SYMBOL TABLE:
 # SYM: 00020000 l       .data           00000004 v1
 # SYM: 00020008 g       .data           00000008 v2
 
 # CHECK:      Relocations [
 # CHECK-NEXT:   Section (7) .rel.dyn {
 # CHECK-NEXT:     0x20010 R_MIPS_REL32/R_MIPS_64/R_MIPS_NONE - 0x0
-# CHECK-NEXT:     0x20008 R_MIPS_REL32/R_MIPS_64/R_MIPS_NONE v2 0x0
+# CHECK-NEXT:     0x20008 R_MIPS_64/R_MIPS_NONE/R_MIPS_NONE v2 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
 # CHECK: DynamicSection [
 # CHECK:   Tag        Type     Name/Value
 # CHECK:   0x0000000000000012 RELSZ    32 (bytes)
 # CHECK:   0x0000000000000013 RELENT   16 (bytes)
 

test/ELF/mips-data-relocation-undef.s

+# REQUIRES: mips
+# RUN: llvm-mc -triple=mips64-unknown-freebsd -filetype=obj %s -o %t.o
+# RUN: ld.lld -shared %t.o -o %t.so
+# RUN: llvm-readobj -expand-relocs -r -t %t.so
+
+.global foo
+
+.data
+.global bar
+bar:
+  .quad foo
+.size bar, 8
+
+

test/ELF/rel-addend-with-rela-input.s

 # RELA-TO-REL:    DataEncoding: BigEndian
 # RELA-TO-REL:  Section {
 # RELA-TO-REL:       Name: .data
 # RELA-TO-REL:       SectionData (
 # RELA-TO-REL-NEXT:    0000: 00000000 00005544 ABCDEF00 12345678 |......UD.....4Vx|
 #                                        ^--- Addend for relocation in .rel.dyn
 # RELA-TO-REL:     Relocations [
 # RELA-TO-REL-NEXT:  Section ({{.+}}) .rel.dyn {
-# RELA-TO-REL-NEXT:     0x10000 R_MIPS_REL32/R_MIPS_64/R_MIPS_NONE foo 0x0
+# RELA-TO-REL-NEXT:     0x10000 R_MIPS_64/R_MIPS_NONE/R_MIPS_NONE foo 0x0
 # RELA-TO-REL-NEXT:  }
 # RELA-TO-REL-NEXT: ]
 
 .extern foo
 
 .data
 .quad foo + 0x5544
 .quad 0xabcdef0012345678