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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

LGTM, just a few nits and a requested test.

> I will add a case to make sure the 0xffc is handled in a similar way to 0xffe and will add to the nopatch test case.



================
Comment at: ELF/ARMErrataFix.cpp:108
+static bool is32bitInstruction(uint16_t hw) {
+  return ((hw & 0xe000) == 0xe000 && (hw & 0x1800) != 0x0000);
+}
----------------
The outer pair of `()` is unnecessary I think.


================
Comment at: ELF/ARMErrataFix.cpp:234
+  uint32_t instr;
+  // branch relocation at off. Will be nullptr if no relocation exists.
+  Relocation *rel;
----------------
Capitalize


================
Comment at: ELF/ARMErrataFix.cpp:246
+  uint64_t isecAddr = isec->getVA(0);
+  // Advance Off so that (isecAddr + off) modulo 0x1000 is at least 0xffa. We
+  // need to check for a 32-bit instruction immediately before a 32-bit branch
----------------
is at least 0xffa -> is 0xffa?

I haven't verified, but `off = alignTo(isecAddr+off, 0x1000, 0xffa) - isecAddr;` probably works.


================
Comment at: ELF/ARMErrataFix.cpp:373
+  for (const InputSection *isec : isd.sections) {
+    isecLimit = isec->outSecOff + isec->getSize();
+    if (isecLimit > patchUpperBound) {
----------------
isecLimit can be defined here, i.e. remove the definition above.


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

https://reviews.llvm.org/D67284