[PATCH] D67284: [LLD][ELF][ARM] Implement --fix-cortex-a8 to fix erratum 657417

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Just a few nits from me.



================
Comment at: ELF/ARMErrataFix.cpp:176
+  // subtract to avoid double counting.
+  if (!this->relocations.empty()) {
+    this->relocateAlloc(buf - outSecOff, buf - outSecOff + getSize());
----------------
Doesn't seem you need `this->`?


================
Comment at: ELF/ARMErrataFix.cpp:188
+  uint64_t s = getThumbDestAddr(getBranchAddr(), instr);
+  uint64_t p = getVA(a);
+  target->relocateOne(buf, isARM ? R_ARM_JUMP24 : R_ARM_THM_JUMP24, s - p);
----------------
Perhaps just inline `a` here?


================
Comment at: ELF/ARMErrataFix.cpp:216
+    // We must extract the offset from the addend manually.
+    destAddr = getThumbDestAddr(sourceAddr, instr);
+
----------------
You need to wrap this line into curly bracers I think, because first branch has it.
(LLD coding style feature)


================
Comment at: ELF/ARMErrataFix.cpp:281
+             ": skipping cortex-a8 657417 erratum sequence, section " +
+             isec->name + " is too large to patch");
+    }
----------------
`{}` too.


================
Comment at: ELF/ARMErrataFix.cpp:300
+  // the mapping symbol name, $a for Arm code, $t for Thumb code, $d for data.
+  auto isArmMapSymbol = [](const Symbol *b) {
+    return b->getName() == "$a" || b->getName().startswith("$a.");
----------------
What is `b` stands for (here and below)? I'd expect to see `sym` or `s`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67284/new/

https://reviews.llvm.org/D67284