This is an archive of the discontinued LLVM Phabricator instance.

[ELF][MIPS] Support GOT entries for non-preemptible symbols with different addends
ClosedPublic

Authored by atanasyan on Jun 13 2016, 9:24 AM.

Details

Summary

There are two motivations for this patch. The first one is a preparation for support MIPS TLS relocations. It might sound like a joke but for GOT entries related to TLS relocations MIPS ABI uses almost regular approach with creation of dynamic relocations for each GOT enty etc. But we need to separate these 'regular' TLS related entries from MIPS specific local/global parts of GOT. ABI declare simple solution - all TLS related entries allocated at the end of GOT after local/global parts. The second motivation it to support GOT relocations for non-preemptible symbols with addends. If we have more than one GOT relocations against symbol S with different addends we need to create GOT entries for each unique Symbol/Addend pairs.

So we store all MIPS GOT entries in separate containers. For non-preemptible symbols we have to maintain two data structures. The first one is MipsLocal vector. Each entry corresponds to the GOT entry from the 'local' part of the GOT contains the symbol's address plus addend. The second one is MipsLocalMap. It is a map from Symbol/Addend pair to the GOT index.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan updated this revision to Diff 60531.Jun 13 2016, 9:24 AM
atanasyan retitled this revision from to [ELF][MIPS] Support GOT entries for non-preemptible symbols with different addends.
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.Jun 13 2016, 2:49 PM

I think I don't fully digest the MIPS ABI yet, so I guess this review is superficial. I'll take time to read it again. Overall it looks good though.

ELF/OutputSections.cpp
95

Define GotSection::addMipsEntry and move MIPS-related code there, so that this function is as short as

if (Config->EMachine == EM_MIPS) {
  addMipsEntry(Sym, Addend, Expr);
  return;
Sym.GotIndex = Entries.size();
Entries.push_back(&Sym);
95–96

Can this be

MipsLocalMap.insert({{&Sym, Addend}, MipsLocalMap.size()})

?

189

If the control cannot reach here only because of a bug, use llvm_unreachable.

ELF/OutputSections.h
136

Add a newline before the comment.

156

Add newlines before comments.

ELF/Symbols.h
99

Move this after IsLocal so that SymbolKind is aligned on 8 byte boundary.

atanasyan updated this revision to Diff 60654.Jun 14 2016, 12:20 AM
atanasyan edited edge metadata.
  • Moved MIPS specific code to the GotSection::addMipsEntry
  • Simplified insertion into the MipsLocalMap
  • Fixed comments formatting
  • Fixed SymbolKind alignment
rafael edited edge metadata.Jun 14 2016, 6:21 AM
rafael added a subscriber: rafael.

So, there something I don't understand about the mips abi. In other
ABIs, a got relocation of symbol foo with addend X means
"got_entry(foo) + X". For example, R_X86_64_GOTPCREL normally has and
addend of -4 since it is commonly used with pc relative addressing
that uses the address of the next instruction.

What exactly is the rule for mips? Is a R_MIPS_SOMETHING_GOT with an
addend of X supposed to mean "got_entry(foo + X)"? Is that always the
case? Only for some relocations? Only when foo is local?

Cheers,
Rafael

What exactly is the rule for mips? Is a R_MIPS_SOMETHING_GOT with an
addend of X supposed to mean "got_entry(foo + X)"? Is that always the
case? Only for some relocations? Only when foo is local?

In general all MIPS GOT related relocations against (S+A) expect that corresponding GOT entry contain a real address of S + A. For example, here is a quote from R_MIPS_GOT_DISP relocation result description:

The offset into the global offset table at which the address of the relocation entry symbol, adjusted by the addend, resides during execution.

In fact compilers/assemblers generate GOT related relocations for non-preemptible symbols because in that case they know what happens at the S+A address. I do not know why does compiler require allocation of additional GOT entry instead of adding addend to the result of GOT entry reading. Maybe the idea is to escape additional instructions.

On my test base I have never seen a GOT relocation against preemptible symbol with addend, but I am going to add fatal call to catch such case.

In general all MIPS GOT related relocations against (S+A) expect that corresponding GOT entry contain a real address of S + A. For example, here is a quote from R_MIPS_GOT_DISP relocation result description:

The offset into the global offset table at which the address of the relocation entry symbol, adjusted by the addend, resides during execution.

In fact compilers/assemblers generate GOT related relocations for non-preemptible symbols because in that case they know what happens at the S+A address. I do not know why does compiler require allocation of additional GOT entry instead of adding addend to the result of GOT entry reading. Maybe the idea is to escape additional instructions.

It sounds like we should have another expression. Say
R_GOT_ADDEND_OFF. With that it would be easier to write code that

  • Creates a got entry for S+A instead of just S. If the addend is 0,

it uses the existing GotIndex/GotPltIndex. Otherwise we need a map. I
would suggest starting with just a (sym, addend). We can try
optimizing it afterwards.

  • When handling the expression, we would have

case R_GOT_ADDEND_OFFSET:

return Body.getGotOffset<ELFT>(A);

And the implementation of getGotOffset uses the hash table in the Got
section if the addend is not 0.

This should allow handling both preemptable and non preemptable
symbols uniformly, no?

It also looks like this code would be cleaner if we used
section+offsets instead of symbols is the Relocation vector. I will
give that a try.

On my test base I have never seen a GOT relocation against preemptible symbol with addend, but I am going to add fatal call to catch such case.

What does gold/bfd do in that case?

Cheers,
Rafael

It sounds like we should have another expression. Say
R_GOT_ADDEND_OFF. With that it would be easier to write code that

  • Creates a got entry for S+A instead of just S. If the addend is 0,

it uses the existing GotIndex/GotPltIndex. Otherwise we need a map. I
would suggest starting with just a (sym, addend). We can try
optimizing it afterwards.

  • When handling the expression, we would have

case R_GOT_ADDEND_OFFSET:

return Body.getGotOffset<ELFT>(A);

And the implementation of getGotOffset uses the hash table in the Got
section if the addend is not 0.

So you suggest to hide the fact that it is MIPS specific stuff. Ok, I will rename related functions and expressions. Now I use a similar approach for the R_MIPS_GOT_LOCAL expression so it will be rather easy.

This should allow handling both preemptable and non preemptable
symbols uniformly, no?

I'm not sure. We still need to write GOT entries for preemptable and non preemptable symbols to the different parts of the GOT. Probably we can use a single expression type in both cases but check symbol type in more than one place.

It also looks like this code would be cleaner if we used
section+offsets instead of symbols is the Relocation vector. I will
give that a try.

What will be a section for preemptable symbol comes from DSO?

On my test base I have never seen a GOT relocation against preemptible symbol with addend, but I am going to add fatal call to catch such case.

What does gold/bfd do in that case?

BFD and Gold handle (Symbol,Addend) pairs for any type of symbols. So they are ready to handle GOT relocations against preemptable symbols and non-zero addends. So your approach covers this case too.

I will try to re-write the patch and send update for review.

atanasyan updated this revision to Diff 60947.Jun 15 2016, 11:08 PM
atanasyan edited edge metadata.

I was not quite right when said that BFD/Gold linkers are able to handle GOT relocations against preemptable symbols and non-zero addends. The accept such relocations because sometimes we need to create a GOT entry just because we need to create a dynamic relocation and the dynamic relocation handles addend (see rL266921). But anyway each preemptable symbol referenced by GOT relocations got single GOT entry.

In this patch I try to handle GOT relocation for preemptable and non-preemptable symbols uniformly.

  • There are only two MIPS GOT specific relocation expressions R_MIPS_GOT_LOCAL_PAGE and R_MIPS_GOT_OFF.
  • All GOT entries created for R_MIPS_GOT_OFF expressions stored in MipsLocal and MipsGlobal vectors.
  • We do not need to save addends for preemptable symbol but using the same type for MipsLocal and MipsGlobal elements simplify the code.
  • All GOT offsets calculated by GotSection<ELFT>::getMipsGotOffset method. I did not put this code to the SymbolBody::getGotOffset to escape exposing GotSection implementation details.
ruiu added inline comments.Jun 16 2016, 10:42 AM
ELF/OutputSections.cpp
192–193
assert(It != MipsGotMap.end());
ELF/OutputSections.h
164

Instead of std::pair, can you define a struct, so that instead of first/second, we can use meaningful names?

168

Add newlines before comments.

atanasyan marked 2 inline comments as done.Jun 16 2016, 1:10 PM
atanasyan added inline comments.
ELF/OutputSections.h
164

Instead of std::pair, can you define a struct

In that case I have to define some additional stuff like getEmptyKey, isEqual, ... to be able to use this struct in the DenseMap. For my taste it makes the code overcomplicated.

ruiu added a comment.Jun 16 2016, 1:14 PM

Fair point.

atanasyan updated this revision to Diff 61066.Jun 16 2016, 10:22 PM
  • Replace llvm_unreachable by assert.
  • Fix comment formatting.
  • Use lambda for adding entries to the MipsLocal and MipsGlobal containers. For my taste now GotSection<ELFT>::addMipsEntry looks a bit more clear.
ruiu accepted this revision.Jun 17 2016, 6:31 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 17 2016, 6:31 AM
This revision was automatically updated to reflect the committed changes.

Thanks for review.