This is an archive of the discontinued LLVM Phabricator instance.

[ELF][MIPS] Fix calculation of GP relative relocations in case of relocatable output
ClosedPublic

Authored by atanasyan on Apr 23 2018, 9:53 AM.

Details

Summary

Some MIPS relocations depend on "gp" value. By default, this value has 0x7ff0 offset from a .got section. But relocatable files produced by a compiler or a linker might redefine this default value and we have to use it for a calculation of the relocation result. When we generate EXE or DSO it's trivial. Generating a relocatable output is more difficult case because the linker does calculate relocations in this case and cannot store individual "gp" values used by each input object file. As a workaround we add the "gp" value to the relocation addend.

This fixes https://llvm.org/pr31149

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Apr 23 2018, 9:53 AM

What bfd/gold do to solve this? I guess the same?

ELF/InputSection.cpp
393 ↗(On Diff #143585)

Does not seem that test case covers all of these relocations?

404 ↗(On Diff #143585)

I would probably make addend calculation a bit more explicit
to avoid additional variable, something like:

  int64_t Addend = getAddend<ELFT>(Rel);
  if (!RelTy::IsRela) {
    const uint8_t *BufLoc = Sec->Data.begin() + Rel.r_offset;
    Addend += Target->getImplicitAddend(BufLoc, Type);
  }

  if (Config->EMachine == EM_MIPS && ...)
    Addend += Sec->getFile<ELFT>()->MipsGp0;
...

What bfd/gold do to solve this? I guess the same?

Yes.

ELF/InputSection.cpp
393 ↗(On Diff #143585)

Exactly. Moreover the patch contains a bug related to non-tested functionality. I will add more tests in the updated patch.

404 ↗(On Diff #143585)

OK

atanasyan updated this revision to Diff 143754.Apr 24 2018, 8:44 AM
  • Add more tests
  • Fix handling non-zero GP0 in case of microMIPS code

It seems you can produce .o files you have added (mips-micro-gp0-non-zero.o, mips-n64-gp0-non-zero.o) with use of llvm-mc, right?
Can you do that in this patch? I think we generally want to avoid binaries in inputs. You might be able to test produced input objects to check they contain
non-zero gp0 if you think that makes sense.

(Would also probably be nice to do that for mips-gp0-non-zero.o in a followup).

test/ELF/mips-non-zero-gp0.s
26 ↗(On Diff #143754)

The source code is missing.

42 ↗(On Diff #143754)

Please move it to the first line of the test.

It seems you can produce .o files you have added (mips-micro-gp0-non-zero.o, mips-n64-gp0-non-zero.o) with use of llvm-mc, right?

LLD and llvm-mc both generate relocatable object files with zero GP0 value. Non-zero value appears in EXE / DSO only. That's why I have to generate the input object files using GNU as and ld.

It seems you can produce .o files you have added (mips-micro-gp0-non-zero.o, mips-n64-gp0-non-zero.o) with use of llvm-mc, right?

LLD and llvm-mc both generate relocatable object files with zero GP0 value. Non-zero value appears in EXE / DSO only. That's why I have to generate the input object files using GNU as and ld.

I see. Please add this information to the test case then (including GNU as version you use and invocation line for each object).

atanasyan updated this revision to Diff 143921.Apr 25 2018, 6:55 AM
  • Fix comments in the test file.
  • Add command line and version numbers of GNU tools used to produce input test files.
grimar accepted this revision.Apr 25 2018, 7:20 AM

LGTM with a nit.

test/ELF/mips-non-zero-gp0.s
10 ↗(On Diff #143921)

I would not use test.o as there is no such file in your test case.

I suggest to explicitly list all 3 invocations here: for mips-gp0-non-zero.o, for mips-n64-gp0-non-zero.o and for mips-micro-gp0-non-zero.o. With that it is trivial to reproduce and people unfamiliar with mips like me do not need to guess where to use -mips32/elf32btsmip and where -mips64/elf64btsmip if they would want to rebuild the inputs.

This revision is now accepted and ready to land.Apr 25 2018, 7:20 AM
atanasyan added a subscriber: ruiu.Apr 25 2018, 8:44 AM

Thanks for review.

This revision was automatically updated to reflect the committed changes.