[MIPS] Handle cross-mode (regular <-> microMIPS) jumps
Needs ReviewPublic

Authored by atanasyan on Nov 16 2017, 1:22 PM.

Details

Summary

[MIPS] Handle cross-mode (regular <-> microMIPS) jumps

The patch solves two tasks:

  1. MIPS ABI allows to mix regular and microMIPS code and perform cross-mode jumps. Linker needs to detect such cases and replace jump/branch instructions by their cross-mode equivalents.
  1. Other tools like dunamic linkers need to recognize cases when dynamic table entries, e_entry field of an ELF header etc point to microMIPS symbol. Linker should provide such information.

The first task is implemented in the MIPS<ELFT>::relocateOne() method. New routine fixupCrossModeJump detects ISA mode change, checks and replaces an instruction.

The main problem is how to recognize that relocation target is microMIPS symbol. For absolute and section symbols compiler or assembler set the less-significant bit of the symbol's value or sum of the symbol's value and addend. And this bit signals to linker about microMIPS code. For global symbols compiler cannot do the same trick because other tools like, for example, disassembler want to know an actual position of the symbol. So compiler sets STO_MIPS_MICROMIPS flag in the st_other field.

We set the less-significant bit for symbols with the STO_MIPS_MICROMIPS flag in the following places:

  • DynamicSection<ELFT>::addSym
  • Writer<ELFT>::getEntryAddr
  • MIPS<ELFT>::relocateOne()

To provide all necessary information for setup the bit this patch starts to pass a pointer to the relocated symbol to the relocateOne methods.

Diff Detail

Repository
rLLD LLVM Linker
atanasyan created this revision.Nov 16 2017, 1:22 PM
ruiu added inline comments.Nov 21 2017, 12:28 AM
ELF/Arch/Mips.cpp
471

Can you write default at end?

494

This needs a comment so that not only a person who understand the details of MIPS ABI can edit this function.

ELF/Symbols.cpp
107 ↗(On Diff #123236)

I do understand why you need this by reading comment, but doing this in this function doesn't feel right. This function returns an address of a symbol, and the tag whether the target is microMIPS or not is not logically part of its address. Can't you do this when you apply relocations?

atanasyan added inline comments.Nov 21 2017, 2:37 AM
ELF/Arch/Mips.cpp
471

Can you write default at end?

Will do.

494

This needs a comment so that not only a person who understand the details of MIPS ABI can edit this function.

OK

ELF/Symbols.cpp
107 ↗(On Diff #123236)

Can't you do this when you apply relocations?

I had such patch. In that case we need:

  • Add the less-significant bit in the InputSectionBase::relocateAlloc() or getRelocTargetVA before calling the relocateOne. We can't do that in the MIPS::relocateOne because we do not have any information about target there.
  • Add the less-significant bit in the Writer::getEntryAddr. The Elf_Ehdr::e_entry field holds just a value so we have to signal a loader that the entry symbol is microMIPS.
  • Add the less-significant bit in the DynamicSection::writeTo. By the same reason as above.

This way looks more correct, but introduces more changes in the code. Do you think it's acceptable?

atanasyan updated this revision to Diff 125961.Dec 7 2017, 7:55 AM
atanasyan edited the summary of this revision. (Show Details)

In this variant of the patch I do not modify the getSymVA function. Instead of that the less-significant bit is configured right before using in the following methods:

  • InputSection::relocateNonAlloc
  • InputSectionBase::relocateAlloc
  • DynamicSection<ELFT>::addSym
  • Writer<ELFT>::getEntryAddr

It's more correct solution but it adds more if (MIPS) statements in the code.

atanasyan updated this revision to Diff 128847.Jan 6 2018, 4:41 AM
  • Updated the patch against the trunk.
ruiu added inline comments.Jan 8 2018, 5:35 PM
ELF/Arch/Mips.cpp
494

I'd return a new Val instead of mutating a given value through a pointer.

ELF/InputSection.cpp
692–695

Is this really unavaoidable to add this MIPS-specific code to this generic function?

723

Ditto -- adding more code to this function makes this function hard to understand.

ELF/SyntheticSections.cpp
1040

Ditto

atanasyan added inline comments.Jan 10 2018, 9:10 AM
ELF/Arch/Mips.cpp
494

OK

ELF/InputSection.cpp
692–695

There are alternatives:

  • Setup the bit in the getSymVA or in the Symbol::getVA like I did in the first version of the patch.
  • Pass more information about the symbol to the relocateOne routine and setup the bit there.
  • Create and use R_MICROMIPS_xxx relocations expressions. In that case we need to add 5-7 new expressions.

I could try to create a patch with the third solution if you think it might make a sense.

ELF/SyntheticSections.cpp
1040

When dynamic linker reads "symbol" tags it needs to know a type of the symbol to use, for example, a correct instruction for jumping. It gets a symbol's value only so we have to setup the less-significant bit to pass the symbol's type to it. The first alternative solution is to setup the bit in every place where we call DynamicSection<ELFT>::addSym. The second alternative is to setup the bit in the getSymVA or in the Symbol::getVA.

atanasyan updated this revision to Diff 132118.Jan 31 2018, 2:27 AM

One more variant of the patch. The if Mips code were removed from InputSectionBase::relocate and InputSection::relocateNonAlloc methods. Now the less-significant bit is configured in the getRelocTargetVA routine. I could not remove Mips-ness from DynamicSection<ELFT>::addSym and Writer<ELFT>::getEntryAddr because if an e_entry field or a dynamic table entry contains microMIPS symbol value this value should have the less-significant bit to notify loader / dynamic-linker about the symbol's type.

ruiu added a comment.Feb 13 2018, 5:46 PM

Sorry for the belated response. A few minor comments.

ELF/InputSection.cpp
465

Please add comment so that people who don't know about MIPS can change this code and this file.

685

nit: add {}

ELF/SyntheticSections.cpp
1039–1047

I'd move this if out of this lambda like this.

if (Config->EMachine == EM_MIPS && ...)
  Entries.push_back({Tag, [=] { return Sym->getVA() | 1; }});
else
  Entries.push_back({Tag, [=] { return Sym->getVA(); }});
1042–1044

I don't think people who don't know MIPS can understand this comment. Why does the reader (who is the reader?) has to know if it is MicroMIPS?

  • Addressing review comments
ruiu added inline comments.Feb 15 2018, 8:50 PM
ELF/InputSection.cpp
465–470

Oh, thank you for writing this. I think I finally understand what it does correctly. So, you are using the least significant bit just to signal to the later pass that this is a MicroMIPS code, right? I was thinking that jump instructions jumping to MicroMIPS code must be marked with LSB 1, but it looks like you cleared the last bit, so the bit you set in this function is not used there. You set the least significant bit just to encode some information that is consumed in the later pass.

That makes me wonder if that's the best way of doing that. Apparently, the side effect of embedding some information to the address isn't negligible -- you needed to update a lot of places to set/unset the least significant bit. That doesn't seem like a good way to encode that kind of out-of-bound information. Address should be just an address.

What if you pass Symbol to relocateOne? Does this solve the issue? I'm not suggesting you do right now, but I'm wondering that's what you wanted to do.

atanasyan edited the summary of this revision. (Show Details)
  • Pass a pointer to relocatable symbol to the relocateOne method
  • Move handling of the less-significant bit from relocateNonAlloc and relocateAlloc to MIPS::relocateOne
nhaehnle removed a subscriber: nhaehnle.Feb 21 2018, 5:00 AM

Ping? This patch got the last comment more than a month ago.

Rebased against the trunk.

Ping?

This patch have not got any comments since Feb 16. Does it mean that the patch cab be committed without any additional approval?

Ping? I'm going to commit this patch in a couple of weeks if I do not receive any objections. We can always perform a post-commit review.

atanasyan updated this revision to Diff 145686.Tue, May 8, 7:07 AM

Rebased against the trunk.