This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Initial support of microMIPS code linking
ClosedPublic

Authored by atanasyan on Aug 31 2017, 7:36 AM.

Details

Summary

The patch implements initial support of microMIPS code linking:

  • Handle microMIPS specific relocations.
  • Emit both R1-R5 and R6 microMIPS PLT records.

For now linking mixed set of regular and microMIPS object files is not supported. Also the patch does not handle (setup and clear) the least-significant bit of an address which is utilized as the ISA mode bit and allows to make jump between regular and microMIPS code without any thunks.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Aug 31 2017, 7:36 AM
ruiu added inline comments.Aug 31 2017, 2:44 PM
ELF/Arch/Mips.cpp
179–180

So this expression is all R_MICROMIPS_* relocations except ones that have 1 or 2 byte opcodes instead of 4 bytes, right? It might be helpful to add a comment about that because it is not obvious that R_MICROMIPS_26_S1 is the first relocation type and R_MICROMIPS_PC19_S2 the last for R_MICROMIPS_* relocations.

190–192

Is this the same as swapping the first two bytes with the following two bytes? If so, is this different from unshuffle?

Can this be something like this?

uint16_t *A = (uint16_t *)Loc;
std::swap(A[0], A[1]);
201

BSIZE and SHIFT can be passed as function arguments instead of template arguments.

291

Do you need this?

488

nit: add {}

624–625

Hmm, it looks like you shuffle bytes and then restore the bytes at end of the function. Looks like it's a bit tricky. Is it possible to define a writeRelocation function for R_MICROMIPS_* and shuffle bytes in that function?

atanasyan updated this revision to Diff 113792.Sep 4 2017, 4:39 PM
  • Remove needShuffling function.
  • Remove separate shuffle and unshuffle functions and move the code into the writeMicroRelocation32 routine. Now we "unsuffle" data when read implicit addend and "unshuffle" and then "shuffle" data back when writing result of microMIPS relocation.
  • Fix shuffling of 16-bit words. Do it iif target binary is little-endian.
  • Keep memset calls in writePltHeader and writePlt methods. In short, size of actual PLT entry can be less than PltEntrySize and we need to fill unused space by "nops". When we initialize PltEntrySize and PltHeaderSize fields we do not know what kind of PLT header/entry we will need to create: regular of microMIPS. These entries have different sizes. Ideally, if a PLT entry is referenced by microMIPS code only this entry should contain microMIPS code, if a PLT entry is referenced by regular code this entry should contain regular code. In a "mixed" case the PLT entry can be either microMIPS or regular, but each "cross-mode-call" has additional cost. Now LLD cannot: a) maintain PLT records with different sizes; b) record type of references (regular / microMIPS) for a PLT entry. To simplify the patch I assume that usually the most or all code have the same type regular or microMIPS. I allocate space for the largest PLT header/entry, but at the time of PLT creation I write either regular or microMIPS entries only. In the last case PLT has gaps which needs to be filled by "nops".
ruiu added inline comments.Sep 5 2017, 3:11 PM
ELF/Arch/Mips.cpp
181–184

I don't believe this is correct as I seems to be violating the strict aliasing rule. I'd do

return (V << 16) | (V >> 16);
291

Is this needed?

atanasyan added inline comments.Sep 5 2017, 3:47 PM
ELF/Arch/Mips.cpp
181–184

OK

291

In short - yes, the memset call is required to overwrite trap instructions 0xefefefef. Here is a slightly modified quote from my previous reply:

Keep memset calls in writePltHeader and writePlt methods. In short, size of actual PLT entry can be less than PltEntrySize and we need to fill unused space by "nops". When we initialize PltEntrySize and PltHeaderSize fields we do not know what kind of PLT header/entry we will need to create: regular of microMIPS. These entries have different sizes. Ideally, if a PLT entry is referenced by microMIPS code only this entry should contain microMIPS code, if a PLT entry is referenced by regular code this entry should contain regular code. In a "mixed" case the PLT entry can be either microMIPS or regular, but each "cross-mode-call" has additional cost. Now LLD cannot: a) maintain PLT records with different sizes; b) record type of references (regular / microMIPS) for a PLT entry. To simplify the patch I assume that usually the most or all code have the same type regular or microMIPS. I allocate space for the largest PLT header/entry, but at the time of PLT creation I write either regular or microMIPS entries only. In the last case PLT has gaps filled by 0xefefefef which needs to be filled by "nops".

ruiu added inline comments.Sep 5 2017, 4:29 PM
ELF/Arch/Mips.cpp
291

Thanks. Can you add a comment saying that this is to overwrite trap instructions written by Writer::writeTrapInstr?

atanasyan updated this revision to Diff 113955.Sep 5 2017, 10:08 PM
  • Added comments to explain memset calls
  • Fixed violating the strict aliasing rule.
ruiu added inline comments.Sep 6 2017, 10:00 AM
ELF/Arch/Mips.cpp
239

Add a blank line here so that it is clear that the comment is saying only about the memset.

293

Ditto

488

?

519

Since the following case body is essentially one line, it is probably better to copy code instead of falling through.

atanasyan updated this revision to Diff 114043.Sep 6 2017, 12:15 PM

Address review comments: added blank lines, curly brackets, replaced LLVM_FALLTHROUGH by actual code.

ruiu accepted this revision.Sep 11 2017, 4:19 PM

LGTM

This revision is now accepted and ready to land.Sep 11 2017, 4:19 PM
This revision was automatically updated to reflect the committed changes.