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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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;
----------------
peter.smith wrote:
> 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.
Hmm, I also did not find anything, that is probably local convention, I think we always write in that style (use brackets even for single line if else branch uses them), not sure where it came from initially :)


================
Comment at: ELF/AArch64ErrataFix.cpp:476
+
+  for (OutputSection *OS : OutputSections) {
+    if (!(OS->Flags & SHF_ALLOC) || !(OS->Flags & SHF_EXECINSTR))
----------------
peter.smith wrote:
> 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.   
I am fine with it.
(regarding the second part I believe all Live InputSections anyways should be in the image, otherwise it would be bug, also it may make sence to rename 'createA53Errata843419Fixes' to 'reportA53Errata843419Fixes' for this iteration as it is exactly what it do now as far I understand).


================
Comment at: ELF/AArch64ErrataFix.h:20
+
+// 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 meant this probably should be at the top of the .cpp file, in its header part,
just like we have in ICF.cpp, MapFile.cpp, ScriptLexer.cpp and others.


https://reviews.llvm.org/D36742