[PATCH] D36749: [LLD][ELF][AArch64] Complete implementation of -fix-cortex-a53-843419

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SectionPatcher.cpp:72
+  write32le(Buf, read32le(Patchee->Data.begin() + PatcheeOffset));
+  // Return Address is next instruction after the load/store register
+  // above.
----------------
nit: add a blank line before comment


================
Comment at: ELF/SectionPatcher.cpp:96
+
+size_t Patch843419::size() const { return 8; }
+
----------------
I'd move this to the .h file.


================
Comment at: ELF/SectionPatcher.cpp:506
+                                 PatchSection *CurPS) {
+  if (CurPS == nullptr) {
+    uint64_t Off = 0;
----------------
Flip the condition and return early.


================
Comment at: ELF/SectionPatcher.cpp:577
+                // Find any existing relocations at the location we are patching
+                auto RelIt =
+                    std::find_if(IS->Relocations.begin(), IS->Relocations.end(),
----------------
I don't fully understand the new code, but it looks like it nests too deeply. Can you split the function?


================
Comment at: ELF/SectionPatcher.h:24
+
+// Class to describe an instance of a Patch.
+// A Patch is a sequence of instructions that are a substitute for one or more
----------------
Can you mention that this is ARM only?

The comment is somewhat abstract, but as we support only one feature (-fix-cortex-a53-843419) at the moment, I'd describe it directly. Say, an early version of of the processor has a bug that executes some instructions wrongly if they are near page boundaries. As a workaround, the linker replaces such instructions with branch instructions that jump to linker-generated thunks. Thunks are not at page boundaries. They executes the original instructions and then jump back to the original locations.


https://reviews.llvm.org/D36749