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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/AArch64ErrataFix.cpp:352
+  uint64_t ISAddr = IS->getParent()->Addr + IS->OutSecOff;
+  const uint8_t *Buf = IS->Data.begin();
+
----------------
grimar wrote:
> 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.
> 
> 
> 
Good suggestion, thanks.


================
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;
----------------
grimar wrote:
> I think you missing curly brackets here to conform code style:
> 
> ```
> if (is843419ErratumSequence(Instr1, Instr2, Instr3)) {
> ...
> } else if
> ```
I've made the change. Just for my curiosity do you have a reference? I couldn't find anything in https://llvm.org/docs/CodingStandards.html . I guess it could be local lld convention.


================
Comment at: ELF/AArch64ErrataFix.cpp:471
+// does not attempt to fix it.
+void lld::elf::createA53Errata843419Fixes(
+    ArrayRef<OutputSection *> OutputSections) {
----------------
grimar wrote:
> Do you need argument ? It looks you could iterate over global `OutputSections` array.
> (if suggestion below does not work for some reason).
Yes I can use the global. I seem to remember that OutputSections used to be a member of the Writer class, I guess this must have changed since I first wrote the patch.


================
Comment at: ELF/AArch64ErrataFix.cpp:476
+
+  for (OutputSection *OS : OutputSections) {
+    if (!(OS->Flags & SHF_ALLOC) || !(OS->Flags & SHF_EXECINSTR))
----------------
grimar wrote:
> Seems you can just iterate global `InputSections` vector here ?
Maybe, however I think it is worth keeping it the way it is for the moment for a couple of reasons:
- In the follow up patch that adds the patching we need to pass in the InputSectionDescription to insert the synthetic sections containing patches into.
- I'm more confident that I'm not finding patches in InputSections that aren't in the image.   


================
Comment at: ELF/Config.h:119
   bool ExportDynamic;
+  bool FatalWarnings;
+  bool FixCortexA53Errata843419;
----------------
grimar wrote:
> Do you need 'FatalWarnings' ? Looks unused.
I've removed it, it looks like a mistake made when rebasing.


================
Comment at: ELF/SectionPatcher.cpp:412-426
+  // Collect mapping symbols for every executable InputSection.
+  for (InputFile *File : ObjectFiles) {
+    auto *F = cast<ObjFile<ELF64LE>>(File);
+    for (Symbol *B : F->getLocalSymbols()) {
+      auto *Def = dyn_cast<Defined>(B);
+      if (!Def)
+        continue;
----------------
ruiu wrote:
> Can you assume that symbol tables are sorted?
Unfortunately not. The original Arm proprietary toolchain used to guarantee that mapping symbols were sorted before all other symbols with the first element a special mapping symbol giving the number of mapping symbols. This was considered to much of a target specific requirement for the other toolchains so the requirement didn't go into the ABI. 


https://reviews.llvm.org/D36742