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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SectionPatcher.cpp:1
+//===- SectionPatcher.cpp -------------------------------------------------===//
+//
----------------
This is pretty much AArch64-specific, so SectionPatcher is not a good name.


================
Comment at: ELF/SectionPatcher.cpp:60
+// reused outside the context of detecting the erratum sequence.
+struct A64 {
+
----------------
It looks like you can define member functions as non-member, file-scope functions.


================
Comment at: ELF/SectionPatcher.cpp:371-373
+  auto GetInstr = [&](uint64_t Instr) {
+    return *reinterpret_cast<const uint32_t *>(Buf + Off + (Instr - 1) * 4);
+  };
----------------
I feel it is easier to read without this helper.



================
Comment at: ELF/SectionPatcher.cpp:377
+  uint32_t Instr1 = GetInstr(1);
+  if (A64::isADRP(Instr1)) {
+    uint32_t Instr2 = GetInstr(2);
----------------
GetInst() is basically free, so you don't need to avoid calling it, do you? It is not clear to me why you had to check if Inst1 satisfies the condition. You'll always pass it to is843419ErratumSequence(), so you could check if it is ADRP in that function.


================
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;
----------------
Can you assume that symbol tables are sorted?


================
Comment at: ELF/SectionPatcher.cpp:462-497
+    for (BaseCommand *BC : OS->SectionCommands)
+      if (auto *ISD = dyn_cast<InputSectionDescription>(BC)) {
+        for (InputSection *IS : ISD->Sections) {
+          //  LLD doesn't use the erratum sequence in SyntheticSections.
+          if (isa<SyntheticSection>(IS))
+            continue;
+          // Use SectionMap to make sure we only scan code and not inline data.
----------------
Nesting seems too deep. Could you simplify?


================
Comment at: ELF/SectionPatcher.h:1
+//===- SectionPatcher.h -----------------------------------------*- C++ -*-===//
+//
----------------
Will we have something like this for non-ARM targets? If not, this class name and file name sound too generic. I'd name AArch64ErrataPatcher or something like that.


https://reviews.llvm.org/D36742