[PATCH] D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419

[Rafael Avila de Espindola via llvm-commits]

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:
> -// ADRP (0xff8 or 0xffc)
> +// ADRP (0xff8 or 0xffc).
>  // 2.)
> -// - load or store single register or either integer or vector registers
> -// - STP or STNP of either vector or vector registers
> -// - Advanced SIMD ST1 store instruction
> -// Must not write Rn
> -// 3.) optional instruction, can't be a branch, must not write Rn, may read Rn
> +// - load or store single register or either integer or vector registers.
> +// - STP or STNP of either vector or vector registers.
> +// - Advanced SIMD ST1 store instruction.
> +// - Must not write Rn.
> +// 3.) optional instruction, can't be a branch, must not write Rn, may read Rn.
>  // 4.) A load or store instruction from the Load/Store register unsigned
> -// immediate class using Rn as the base register
> +// immediate class using Rn as the base register.
>  
>  // Each section contains a sequence of instructions that should be recognized
>  // as erratum 843419. The test cases cover the major variations such as:
> -// adrp starts at 0xfff8 or 0xfffc
> -// Variations in instruction class for instruction 2
> -// Optional instruction 3 present or not
> -// Load or store for instruction 4.
> +// - adrp starts at 0xfff8 or 0xfffc.
> +// - Variations in instruction class for instruction 2.
> +// - Optional instruction 3 present or not.
> +// - Load or store for instruction 4.

These comment updates are fine. Please commit them first.



> +#include <map>
> +#include <vector>
> +
>  namespace lld {
>  namespace elf {
>  
> +class Defined;
> +class InputSection;
> +class InputSectionDescription;
>  class OutputSection;
> -void reportA53Errata843419Fixes();
> +class Patch843419Section;
> +
> +class SectionPatcher {

Maybe call it AArch843419Patcher?

> +  // Apply any relocation transferred from the original PatcheeSection.
> +  // For a SyntheticSection Buf already has OutSecOff added, but relocateAlloc
> +  // also adds OutSecOff so we need to subtract to avoid double counting.
> +  this->relocateAlloc(Buf - OutSecOff, Buf - OutSecOff + getSize());

I wonder if we could read the already patched output buffer and avoid
this? Can we guarantee that the patch is always written after the patchee?


> +// Insert the PatchSections we have created back into the
> +// InputSectionDescription. As inserting patches alters the addresses of
> +// InputSections that follow them, we try and place the patches after all the
> +// executable sections, although we may need to insert them earlier if the
> +// InputSectionDescription is larger than the maximum branch range.
> +void SectionPatcher::insertPatches(InputSectionDescription &ISD,
> +                                   std::vector<Patch843419Section *> &Patches) {
> +  uint64_t ISLimit;
> +  uint64_t PrevISLimit = ISD.Sections.front()->OutSecOff;
> +  uint64_t PatchUpperBound = PrevISLimit + Target->ThunkSectionSpacing;
> +
> +  // Set the OutSecOff of patches to the place where we want to insert them.
> +  // We use a similar strategy to Thunk placement. Place patches roughly
> +  // every multiple of maximum branch range.
> +  auto PatchIt = Patches.begin();
> +  auto PatchEnd = Patches.end();
> +  for (const InputSection *IS : ISD.Sections) {
> +    ISLimit = IS->OutSecOff + IS->getSize();
> +    if (ISLimit > PatchUpperBound) {
> +      while (PatchIt != PatchEnd) {
> +        if ((*PatchIt)->getLDSTAddr() >= PrevISLimit)
> +          break;
> +        (*PatchIt)->OutSecOff = PrevISLimit;
> +        ++PatchIt;
> +      }
> +      PatchUpperBound = PrevISLimit + Target->ThunkSectionSpacing;
>      }
> -    if (DataSym == MapSyms.end())
> -      break;
> -    CodeSym = std::next(DataSym);
> +    PrevISLimit = ISLimit;
> +  }
> +  for (; PatchIt != PatchEnd; ++PatchIt) {
> +    (*PatchIt)->OutSecOff = ISLimit;
>    }
> -}
>  
> -// Scan all the executable code in an AArch64 link to detect the Cortex-A53
> -// erratum 843419.
> -// FIXME: The current implementation only scans for the erratum sequence, it
> -// does not attempt to fix it.
> -void lld::elf::reportA53Errata843419Fixes() {
> -  std::map<InputSection *, std::vector<const Defined *>> SectionMap =
> -      makeAArch64SectionMap();
> +  // merge all patch sections
> +  std::vector<InputSection *> Tmp;
> +  Tmp.reserve(ISD.Sections.size() + Patches.size());
> +  auto MergeCmp = [](const InputSection *A, const InputSection *B) {
> +    if (A->OutSecOff < B->OutSecOff)
> +      return true;
> +    if (A->OutSecOff == B->OutSecOff && isa<Patch843419Section>(A) &&
> +        !isa<Patch843419Section>(B))
> +      return true;
> +    return false;
> +  };
> +  std::merge(ISD.Sections.begin(), ISD.Sections.end(), Patches.begin(),
> +             Patches.end(), std::back_inserter(Tmp), MergeCmp);

After this the patch sections still have a OutSecOff of the limit of
where they can be placed, no? Is that OK?


> +void SectionPatcher::patchInputSectionDescription(
> +    InputSectionDescription &ISD, std::vector<Patch843419Section *> &Patches) {

return the std::vector.

Cheers,
Rafael