This is an archive of the discontinued LLVM Phabricator instance.

[ELF] MIPS paired R_MIPS_HI16/LO16 relocations support
ClosedPublic

Authored by atanasyan on Dec 1 2015, 8:02 AM.

Details

Summary

Some MIPS relocations including R_MIPS_HI16/R_MIPS_LO16 use combined addends. Such addend is calculated using addends of both paired relocations. Each R_MIPS_HI16 relocation is paired with the next R_MIPS_LO16 relocation. ABI requires to compute such combined addend in case of REL relocation record format only.

For details see p. 4-17 at ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf

This patch implements lookup of the next paired relocation suing new InputSectionBase::findPairedRelocLocation method. The primary disadvantage of this approach is that we put MIPS specific logic into the common code. The next disadvantage is that we lookup R_MIPS_LO16 for each R_MIPS_HI16 relocation, while in fact multiple R_MIPS_HI16 might be paired with the single R_MIPS_LO16. From the other side this way allows us to keep MipsTargetInfo class stateless and implement later relocation handling in parallel.

This patch does not support R_MIPS_HI16/R_MIPS_LO16 relocations against _gp_disp symbol. In that case the relocations use a special formula for the calculation. That will be implemented later.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan updated this revision to Diff 41515.Dec 1 2015, 8:02 AM
atanasyan retitled this revision from to [ELF] MIPS paired R_MIPS_HI16/LO16 relocations support.
atanasyan updated this object.
atanasyan added reviewers: ruiu, rafael.
atanasyan set the repository for this revision to rL LLVM.
atanasyan added a project: lld.
atanasyan added a subscriber: llvm-commits.
ruiu edited edge metadata.Dec 1 2015, 8:58 AM

Overall looks good but a few nits.

ELF/InputSection.cpp
96

Do you have to template isRela? IIRC, it is always Elf_Rel on MIPS.

106–118

I'd write this as it's shorter.

if (Type == R_MIPS_HI16)
  Type = R_MIPS_LO16;
else if (Type == R_MIPS_PCHI16)
  Type = R_MIPS_PCLO16;
else if (Type == R_MICROMIPS_HI16)
  Type = R_MICROMIPS_LO16;
else
  return nullptr;
ELF/Target.h
62

Remove "= nullptr" since the argument is always passed.

atanasyan marked an inline comment as done.Dec 1 2015, 1:05 PM
atanasyan added inline comments.
ELF/InputSection.cpp
96

Yes, isRela is always false for MIPS O32 ABI but I have to template isRela to be able to put the findPairedRelocLocation call into the relocate function. Otherwise compiler complains about conversion NextRelocs from RelIteratorRange<true> to RelIteratorRange<false>.

ELF/Target.h
62

The relocateTlsOptimize is also called from routines like relocateTlsIeToLe, writePltZeroEntry etc. Do you think it is better to pass the last argument as nullptr explicitly in these cases too?

ruiu added inline comments.Dec 1 2015, 1:09 PM
ELF/InputSection.cpp
96

Got it.

98

Can you rename findMipsPairedReloc? I'd like to include "Mips" in this function name.

ELF/Target.h
62

I don't know which is better, but I want to make it consistent. Could you remove nullptr from "relocateOne(..., nullptr)" in this patch?

atanasyan updated this revision to Diff 41557.Dec 1 2015, 1:20 PM
atanasyan edited edge metadata.
  • Rebased
  • s/findPairedRelocLocation/findMipsPairedReloc/
  • Do not pass nullptr to the findMipsPairedReloc explicitly
  • Replace switch by series of if/else if/else operators
ruiu accepted this revision.Dec 1 2015, 1:24 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 1 2015, 1:24 PM
This revision was automatically updated to reflect the committed changes.