[ELF][AArch64] Apply some NFC cleanups to AArch64ErrataFix.cpp

[grimar]

ELF/AArch64ErrataFix.cpp

438 ↗

(On Diff #219220)

It is not a NFC change.

This code now accepts $xfoo, what wasn't before.

[MaskRay]

ELF/AArch64ErrataFix.cpp

438 ↗

(On Diff #219220)

This is the question I asked in https://reviews.llvm.org/D67284#inline-604486

I'll probably remove NFC from the title. I actually mean this change doesn't intend to make any observable behavior changes, i.e. llvm only generates "$a" and "$t", not "$a." or "$xfoo".

[grimar]

ELF/AArch64ErrataFix.cpp

438 ↗

(On Diff #219220)

I see. I do not know the answer here, but all other changes LGTM.

[ruiu]

LGTM

[peter.smith]

ELF/AArch64ErrataFix.cpp

438 ↗

(On Diff #219220)

Strictly speaking $xfoo is not a mapping symbol. There is a very small chance of interpreting a user defined symbol as a mapping symbol. Probably unlikely in practice although I'd prefer to stick to the spec if it isn't too onerous to do so.

Once init() has been passed and we've added only $x and $x. symbols to the map, we can then use startswith("$x") as all our symbols are in the form $x, $x.n, $d $d.n

Looking at the spec below, one approach to simplify would be to use just the char 'x', 'd' or some int like 0 or 1 in the map as all we care about is whether the range of code is AArch64 or Data.

https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation

• A short form that uses a dollar character and a single letter denoting the class. This form can be used when an object producer creates mapping symbols automatically. Its use minimizes string table size.
• A longer form in which the short form is extended with a period and then any sequence of characters that are legal for a symbol. This form can be used when assembler files have to be annotated manually and the assembler does not support multiple definitions of symbols.

[peter.smith]

I'd prefer that we retained the strict semantics of mapping symbols, although there are other possible simplifications that could be made. Everything else looks fine.

[MaskRay]

Restore return b->getName() == "$x" || b->getName().startswith("$x.");

This change is strictly NFC now.

lld/trunk/ELF/AArch64ErrataFix.cpp

 #include <algorithm>
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::support;
 using namespace llvm::support::endian;
 
-using namespace lld;
-using namespace lld::elf;
+namespace lld {
+namespace elf {
 
 // Helper functions to identify instructions and conditions needed to trigger
 // the Cortex-A53-843419 erratum.
 
 // ADRP
 // | 1 | immlo (2) | 1 | 0 0 0 0 | immhi (19) | Rd (5) |
 static bool isADRP(uint32_t instr) {
   return (instr & 0x9f000000) == 0x90000000;
static uint64_t scanCortexA53Errata843419(InputSection *isec, uint64_t &off,
   }
   if (((isecAddr + off) & 0xfff) == 0xff8)
     off += 4;
   else
     off += 0xffc;
   return patchOff;
 }
 
-class lld::elf::Patch843419Section : public SyntheticSection {
+class Patch843419Section : public SyntheticSection {
 public:
   Patch843419Section(InputSection *p, uint64_t off);
 
   void writeTo(uint8_t *buf) override;
 
   size_t getSize() const override { return 8; }
 
   uint64_t getLDSTAddr() const;
 
   // The Section we are patching.
   const InputSection *patchee;
   // The offset of the instruction in the Patchee section we are patching.
   uint64_t patcheeOffset;
   // A label for the start of the Patch that we can use as a relocation target.
   Symbol *patchSym;
 };
 
-lld::elf::Patch843419Section::Patch843419Section(InputSection *p, uint64_t off)
+Patch843419Section::Patch843419Section(InputSection *p, uint64_t off)
     : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 4,
                        ".text.patch"),
       patchee(p), patcheeOffset(off) {
   this->parent = p->getParent();
   patchSym = addSyntheticLocal(
       saver.save("__CortexA53843419_" + utohexstr(getLDSTAddr())), STT_FUNC, 0,
       getSize(), *this);
   addSyntheticLocal(saver.save("$x"), STT_NOTYPE, 0, 0, *this);
 }
 
-uint64_t lld::elf::Patch843419Section::getLDSTAddr() const {
+uint64_t Patch843419Section::getLDSTAddr() const {
   return patchee->getVA(patcheeOffset);
 }
 
-void lld::elf::Patch843419Section::writeTo(uint8_t *buf) {
+void Patch843419Section::writeTo(uint8_t *buf) {
   // Copy the instruction that we will be replacing with a branch in the
   // Patchee Section.
   write32le(buf, read32le(patchee->data().begin() + patcheeOffset));
 
   // 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());
for (InputFile *file : objectFiles) {
     }
   }
   // For each InputSection make sure the mapping symbols are in sorted in
   // ascending order and free from consecutive runs of mapping symbols with
   // the same type. For example we must remove the redundant $d.1 from $x.0
   // $d.0 $d.1 $x.1.
   for (auto &kv : sectionMap) {
     std::vector<const Defined *> &mapSyms = kv.second;
-    if (mapSyms.size() <= 1)
-      continue;
     llvm::stable_sort(mapSyms, [](const Defined *a, const Defined *b) {
       return a->value < b->value;
     });
     mapSyms.erase(
         std::unique(mapSyms.begin(), mapSyms.end(),
                     [=](const Defined *a, const Defined *b) {
                       return (isCodeMapSymbol(a) && isCodeMapSymbol(b)) ||
                              (isDataMapSymbol(a) && isDataMapSymbol(b));
if (isecLimit > patchUpperBound) {
       patchUpperBound = prevIsecLimit + target->getThunkSectionSpacing();
     }
     prevIsecLimit = isecLimit;
   }
   for (; patchIt != patchEnd; ++patchIt) {
     (*patchIt)->outSecOff = isecLimit;
   }
 
-  // merge all patch sections. We use the outSecOff assigned above to
+  // Merge all patch sections. We use the outSecOff assigned above to
   // determine the insertion point. This is ok as we only merge into an
   // InputSectionDescription once per pass, and at the end of the pass
   // assignAddresses() will recalculate all the outSecOff values.
   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;

 // PreConditions:
 // The Output and Input Sections have had their final addresses assigned.
 //
 // PostConditions:
 // Returns true if at least one patch was added. The addresses of the
 // Ouptut and Input Sections may have been changed.
 // Returns false if no patches were required and no changes were made.
 bool AArch64Err843419Patcher::createFixes() {
-  if (initialized == false)
+  if (!initialized)
     init();
 
   bool addressesChanged = false;
   for (OutputSection *os : outputSections) {
     if (!(os->flags & SHF_ALLOC) || !(os->flags & SHF_EXECINSTR))
       continue;
     for (BaseCommand *bc : os->sectionCommands)
       if (auto *isd = dyn_cast<InputSectionDescription>(bc)) {
         std::vector<Patch843419Section *> patches =
             patchInputSectionDescription(*isd);
         if (!patches.empty()) {
           insertPatches(*isd, patches);
           addressesChanged = true;
         }
       }
   }
   return addressesChanged;
 }
+} // namespace elf
+} // namespace lld