[PATCH] D112063: [lld][ELF] Add first bits to support relocation relaxations for AArch64

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/AArch64.cpp:578
+  // always appear in pairs.
+  for (size_t i = 0, size = relocs.size(); i + 1 < size; ++i) {
+    if (relocs[i].type == R_AARCH64_LD64_GOT_LO12_NC) {
----------------
`i + 1 < size` has a bug that the last relocation is not checked.

If `i` is moved outside the loop one single `safeToRelaxAdrpLdr = i == size;` can be used.

That said, the same check has been repeated in `tryRelaxAdrpLdr`. Maybe `AArch64Relaxer` is not needed.


================
Comment at: lld/ELF/Arch/AArch64.cpp:583
+    }
+    if (relocs[i].type != R_AARCH64_ADR_GOT_PAGE)
+      continue;
----------------
Inverting the condition into `==` may be clearer.


================
Comment at: lld/ELF/Arch/AArch64.cpp:595
+
+static bool withinRange(uint64_t lhs, uint64_t rhs, uint64_t range) {
+  uint64_t distance = lhs > rhs ? lhs - rhs : rhs - lhs;
----------------
`inBranchRange` names the parameters src and dst.

Inline the function (only called once) into the caller.


================
Comment at: lld/ELF/Arch/AArch64.cpp:629
+  // Skip undefined, preemptible and STT_GNU_IFUNC symbols.
+  if (sym.isUndefined() || sym.isPreemptible || sym.isGnuIFunc())
+    return false;
----------------
See `tryRelaxPPC64TocIndirection`. 

`isUndefined` may be insufficient. Use `!isDefined`

Combined the condition with the previous one. I find it sometimes easier to read by not interleaving comments.


================
Comment at: lld/ELF/Arch/AArch64.cpp:640
+  // Check if the second instruction is LDR.
+  if ((ldrInstr & 0x3B000000) != 0x39000000)
+    return false;
----------------
Combined the condition with the previous one.

Prefer lower-case hexadecimals. I know PPC64 may be inconsistent.


================
Comment at: lld/ELF/Arch/AArch64.cpp:672
+  write32le(buf + adrpSymRel.offset,
+            0x90000000 | adrpDestReg /* adrp x_<dest_reg> */);
+  write32le(buf + addRel.offset,
----------------
Place comment before the statement or on the right hand side as `//`


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:4
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/global.s -o %t-global.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %t/hidden.s -o %t-hidden.o
----------------
`-triple=aarch64`


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:14
+## Case 1. Symbol 'x' is preemptible, no relaxations should be applied.
+# RUN: ld.lld -shared %t-global.o -o %t-preemptible.so
+# RUN: llvm-objdump --no-show-raw-insn -d %t-preemptible.so \
----------------
Too many files may harm readability. Use one file to test symbols with different properties (STB_GLOBAL default visibility,  STB_GLOBAL hidden visibility, STB_LOCAL, ifunc).


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:81
+.word 10
+.space 1048640, 0
+.text
----------------
This uses unreliable dependency on LLD's layout algorithm. Second, this creates very large input and output files with zeros. 

Prefer a linker script a la ppc64-long-branch-pi.s ppc64-pcrel-long-branch.s


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:98
+main:
+        adrp    x0, :got:x
+        ldr     x0, [x0, #:got_lo12:x]
----------------
Perhaps just use 2 column indentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063