This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Fix PLT entries generation in case of linking regular and microMIPS code
ClosedPublic

Authored by atanasyan on Sep 12 2017, 8:15 AM.

Details

Summary

Currently LLD calls the isMicroMips routine to determine type of PLT entries needs to be generated: regular or microMIPS. This routine checks ELF header flags in the FirstObj to retrieve type of linked object files. So if the first file does not contain microMIPS code, LLD will generate PLT entries with regular (non-microMIPS) code only.

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.

It's rather difficult to implement this ideal solution. But we can assume that if there is an input object file with microMIPS code, the most part of the code is microMIPS too. So we need to deduce type of PLT entries based on finally calculated ELF header flags and do not check only the first input object file.

This change implements this.

  • The getMipsEFlags function caches result of ELF flags calculation. Now it's cheap to call it multiple times.
  • The isMicroMips and isMipsR6 routines call getMipsEFlags to get and check calculated ELF flags.
  • A side effect - these functions become non-template.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan created this revision.Sep 12 2017, 8:15 AM
ruiu added inline comments.Sep 15 2017, 4:48 PM
ELF/Arch/Mips.cpp
219–220 ↗(On Diff #114846)

I'd remove the temporary variable.

return (getMipsEFlags() & EF_MIPS_ARCH_ASE) == EF_MIPS_MICROMIPS;
ELF/Arch/MipsArchTree.cpp
284 ↗(On Diff #114846)

We do use global variables, but I'd like to reduce usage of it to make it more friendly with embedding, so I wouldn't add a new global variable just for caching. If you do need this, I'd probably add a new member to Config. I don't like to add yet another MIPS-specific member to Config, but that is perhaps still better than adding a new global variable.

Address review comments:

  • Removed a temporary variable in the isMicroMips function.
  • Moved MipsEFlags global variable to the Config. Calculation of its value requires iterating over all input object files so doing that each time when we call getMipsEFlags() is expensive.
ruiu added inline comments.Sep 19 2017, 4:49 PM
ELF/Arch/MipsArchTree.cpp
296 ↗(On Diff #115531)

I'd avoid dispatching based on Config->EKind. Can you initialize Config->EKind after all object files are ready?

atanasyan added inline comments.Sep 19 2017, 9:59 PM
ELF/Arch/MipsArchTree.cpp
296 ↗(On Diff #115531)

I'd avoid dispatching based on Config->EKind. Can you initialize Config->EKind after all object files are ready?

Do you mean that we need an explicit guarantee that the calcMipsEFlags is called after the Config->EKind initialization? When I code this function I based on an implicit guarantee like in some other existing LLD routines.

ruiu added inline comments.Sep 27 2017, 9:25 PM
ELF/Arch/MipsArchTree.cpp
296 ↗(On Diff #115531)

Do you mean that we need an explicit guarantee that the calcMipsEFlags is called after the Config->EKind initialization?

Yes, what I meant. Then you can remove this dispatch function, no?

  • Removed getMipsEFlags.
  • Call calcMipsEFlags from the LinkerDriver::link to initialize the Configuration::MipsEFlags field.
  • Replaced call of the getMipsEFlags by access to the Configuration::MipsEFlags in all other places.
ruiu accepted this revision.Oct 1 2017, 7:42 PM

LGTM

I think this looks much nicer than before. Thank you for making the change.

ELF/Config.h
215 ↗(On Diff #116924)

cache

This revision is now accepted and ready to land.Oct 1 2017, 7:42 PM
This revision was automatically updated to reflect the committed changes.