This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary
  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 wants to know an actual position of the symbol. So compiler sets STO_MIPS_MICROMIPS flag in the st_other field.

In MIPS<ELFT>::relocateOne() method we have a symbol's value only and cannot access any symbol's attributes. To pass type of the symbol (regular/microMIPS) to that routine as well as other places where we write a symbol value as-is (.dynamic section, Elf_Ehdr::e_entry field etc) we set when necessary a less-significant bit in the getSymVA function.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Nov 16 2017, 1:22 PM
ruiu added inline comments.Nov 21 2017, 12:28 AM
ELF/Arch/Mips.cpp
459 ↗(On Diff #123236)

Can you write default at end?

482 ↗(On Diff #123236)

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

ruiu added inline comments.Nov 21 2017, 12:28 AM
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
459 ↗(On Diff #123236)

Can you write default at end?

Will do.

482 ↗(On Diff #123236)

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
491 ↗(On Diff #128847)

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

ELF/InputSection.cpp
686–689 ↗(On Diff #128847)

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

723 ↗(On Diff #128847)

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

ELF/SyntheticSections.cpp
1025 ↗(On Diff #128847)

Ditto

atanasyan added inline comments.Jan 10 2018, 9:10 AM
ELF/Arch/Mips.cpp
491 ↗(On Diff #128847)

OK

ELF/InputSection.cpp
686–689 ↗(On Diff #128847)

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
1025 ↗(On Diff #128847)

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
458 ↗(On Diff #132118)

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

685 ↗(On Diff #132118)

nit: add {}

ELF/SyntheticSections.cpp
1024 ↗(On Diff #132118)

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(); }});
1027–1029 ↗(On Diff #132118)

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
469–474 ↗(On Diff #134166)

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.May 8 2018, 7:07 AM

Rebased against the trunk.

Ping?

@ruiu could I use a post-commit review option in that case?

Perhaps someone else can review?

MaskRay added inline comments.Nov 25 2018, 4:48 PM
ELF/Arch/AArch64.cpp
46 ↗(On Diff #145686)

I like this change.

With const Symbol *S, we can improve error reporting of relocation overflow of checkUInt(...)

ruiu added inline comments.Nov 26 2018, 9:42 AM
ELF/Arch/AArch64.cpp
46 ↗(On Diff #145686)

Adding const Symbol * to relocateOne might be fine, but I don't like to add a default argument to the parameter. How many locations do we call relocateOne? Can we just always pass a symbol to the function?

atanasyan updated this revision to Diff 175352.Nov 26 2018, 3:12 PM
  • Rebase against the trunk
  • Remove default nullptr value from the relocateOne method
atanasyan updated this revision to Diff 175353.Nov 26 2018, 3:14 PM
  • Really remove default nullptr argument value from the relocateOne method
ruiu added a comment.Nov 26 2018, 4:36 PM

Adding a new parameter to relocateOne doesn't look beautiful, but that makes me think whether you are editing the right place of the code. relocateOne is supposed to apply a single relocation based on a value passed to the function. No complex logic should be in the function. Passing one more parameter so that you can do more complex computation in the function may not be a good idea.

Adding a new parameter to relocateOne doesn't look beautiful, but that makes me think whether you are editing the right place of the code. relocateOne is supposed to apply a single relocation based on a value passed to the function. No complex logic should be in the function. Passing one more parameter so that you can do more complex computation in the function may not be a good idea.

That's interesting - you suggested to pass a Symbol to relocateOne on Feb 16.
https://reviews.llvm.org/D40147#inline-379274

I will try to implement an alternative solution. Maybe by updating one of initial patches.

ruiu added a comment.Nov 27 2018, 9:11 AM

Adding a new parameter to relocateOne doesn't look beautiful, but that makes me think whether you are editing the right place of the code. relocateOne is supposed to apply a single relocation based on a value passed to the function. No complex logic should be in the function. Passing one more parameter so that you can do more complex computation in the function may not be a good idea.

That's interesting - you suggested to pass a Symbol to relocateOne on Feb 16.
https://reviews.llvm.org/D40147#inline-379274

I will try to implement an alternative solution. Maybe by updating one of initial patches.

Yeah, sorry, but until code is actually written, I don't know whether it is going to look good or not, so I can suggest, but my suggestion is not a promise or anything.

atanasyan updated this revision to Diff 177778.Dec 11 2018, 2:03 PM
atanasyan edited the summary of this revision. (Show Details)
  • Remove new argument from relocateOne methods
  • Introduce new helper function setMicroMipsBit and call it before the relocateOne

Gentle ping...

Gentle ping...

Any chance to get review this year or at least before 8.0 release?

Gentle ping...

atanasyan edited reviewers, added: grimar; removed: espindola.Jan 10 2019, 2:02 AM

Gentle ping...

I am pretty sure we can land this because having a patch on a review for about a year is always a terrible thing. My comments are inline.

ELF/InputSection.cpp
862 ↗(On Diff #177778)

I wonder if this piece of code can be in MIPS::relocateOne ?

964 ↗(On Diff #177778)

Same?

ELF/SyntheticSections.cpp
1256 ↗(On Diff #177778)

The comment does not answer why we do that..

ELF/Writer.cpp
2296 ↗(On Diff #177778)

Should it be in a getVA()?

test/ELF/mips-micro-bad-cross-calls.s
8 ↗(On Diff #177778)

Please move the REQUIRES on the first line.

I am pretty sure we can land this because having a patch on a review for about a year is always a terrible thing. My comments are inline.

Thanks for review.

We cannot move call of setMicroMipsBit to the MIPS::relocateOne because the relocateOne does not have a reference to the "target" symbol. One possible solution is to extend the getVA() and getPltVA() routines and add a less-significant bit there when necessary. That eliminates almost all manipulations with the bit in other places and makes calls of the setMicroMipsBit routine unnecessary. It's interesting that this approach was implemented in the first version of this patch https://reviews.llvm.org/D40147?id=123236.

I will try to restore that version of the patch.

atanasyan updated this revision to Diff 185709.Feb 6 2019, 11:59 PM
atanasyan edited the summary of this revision. (Show Details)
  • Move the most part of manipulations with the less-significant bit to the getSymVA and Symbol::getPltVA() methods.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 11:59 PM
grimar added a comment.Feb 8 2019, 1:27 AM

I am not a specialist in MIPS, but the code itself in this diff looks much better to me.

lld/ELF/Arch/Mips.cpp
471 ↗(On Diff #185709)

I would rewrite this in a shorter way:

return Type == R_MIPS_26 || Type == R_MIPS_PC26_S2 ... ;
483 ↗(On Diff #185709)

The same.

523 ↗(On Diff #185709)

Is it ok to ignore these, or should it trigger an error instead maybe?

grimar accepted this revision.Feb 8 2019, 1:32 AM

LGTM, but please wait for Rui's opinion too.

lld/ELF/Arch/Mips.cpp
523 ↗(On Diff #185709)

Oh, please ignore this comment. So the error below will be triggered.

This revision is now accepted and ready to land.Feb 8 2019, 1:32 AM
atanasyan updated this revision to Diff 185938.Feb 8 2019, 3:43 AM
  • Use if instead of switch

LGTM, but please wait for Rui's opinion too.

Thanks for review.

@ruiu : Do you have any comments?

grimar accepted this revision.Feb 14 2019, 3:39 AM

LGTM. This patch is more or less isolated, so I think you can land it if there will be no objections/comments in a reasonable amount of time, let's say until EOW.

This revision was automatically updated to reflect the committed changes.