[PATCH] D36742: [LLD][ELF][AArch64] Implement scanner for Cortex-A53 Erratum 843419

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments/suggestions below.



================
Comment at: ELF/AArch64ErrataFix.cpp:32
+
+// This file implements Section Patching for the purpose of working around
+// errata in CPUs. The general principle is that an erratum sequence of one or
----------------
I think this description should be in the header of this file, see ICF.cpp for example.


================
Comment at: ELF/AArch64ErrataFix.cpp:288
+// the instruction has writeback.
+static bool LoadStoreWritesToReg(uint32_t Instr, uint32_t Reg) {
+  return (isV8NonStructureLoad(Instr) && getRt(Instr) == Reg) ||
----------------
Function names should start from lowercase.
I would suggest to change name to something strarting from "is*" or "should*" or alike,
which looks more appropriate naming for helper returning bool.


================
Comment at: ELF/AArch64ErrataFix.cpp:352
+  uint64_t ISAddr = IS->getParent()->Addr + IS->OutSecOff;
+  const uint8_t *Buf = IS->Data.begin();
+
----------------
I would move this below declaration of `PatchOff` and declare as something like:
```
uint32_t *InstBuf = reinterpret_cast<uint32_t *>(Buf + Off);
```
That way you can get rid of multiple reinterpret_casts.





================
Comment at: ELF/AArch64ErrataFix.cpp:370
+  uint32_t Instr3 = *reinterpret_cast<const uint32_t *>(Buf + Off + 8);
+  if (is843419ErratumSequence(Instr1, Instr2, Instr3))
+    PatchOff = Off + 8;
----------------
I think you missing curly brackets here to conform code style:

```
if (is843419ErratumSequence(Instr1, Instr2, Instr3)) {
...
} else if
```


================
Comment at: ELF/AArch64ErrataFix.cpp:410
+        continue;
+      if (auto *Sec = dyn_cast<InputSection>(Def->Section)) {
+        if (Sec->Flags & SHF_EXECINSTR)
----------------
You don't need brackets:

```
      if (auto *Sec = dyn_cast<InputSection>(Def->Section))
        if (Sec->Flags & SHF_EXECINSTR)
          SectionMap[Sec].push_back(Def);
```


================
Comment at: ELF/AArch64ErrataFix.cpp:422
+    std::vector<const Defined *> &MapSyms = KV.second;
+    if (MapSyms.size() > 1) {
+      std::stable_sort(MapSyms.begin(), MapSyms.end(),
----------------
May be early continue ?
```
if (MapSyms.size() <= 1)
  continue;
```


================
Comment at: ELF/AArch64ErrataFix.cpp:471
+// does not attempt to fix it.
+void lld::elf::createA53Errata843419Fixes(
+    ArrayRef<OutputSection *> OutputSections) {
----------------
Do you need argument ? It looks you could iterate over global `OutputSections` array.
(if suggestion below does not work for some reason).


================
Comment at: ELF/AArch64ErrataFix.cpp:476
+
+  for (OutputSection *OS : OutputSections) {
+    if (!(OS->Flags & SHF_ALLOC) || !(OS->Flags & SHF_EXECINSTR))
----------------
Seems you can just iterate global `InputSections` vector here ?


================
Comment at: ELF/Config.h:119
   bool ExportDynamic;
+  bool FatalWarnings;
+  bool FixCortexA53Errata843419;
----------------
Do you need 'FatalWarnings' ? Looks unused.


https://reviews.llvm.org/D36742