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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the update. The code change looks good to me. Some nits about the tests are inlined.



================
Comment at: lld/ELF/Arch/AArch64.cpp:580
+  const size_t size = relocs.size();
+  for (; i < size; ++i) {
+    if (relocs[i].type == R_AARCH64_ADR_GOT_PAGE) {
----------------
`!=` in loop condition is more common in llvm. And potentially compiles better.


================
Comment at: lld/ELF/Arch/AArch64.cpp:590
+    } else {
+      continue;
+    }
----------------
Delete unused continue


================
Comment at: lld/ELF/InputSection.cpp:1020
+  AArch64Relaxer aarch64relaxer(relocations);
+  for (size_t i = 0, size = relocations.size(); i < size; ++i) {
+    const Relocation &rel = relocations[i];
----------------



================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got-symbols.s:7
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %t/global.s -o %t/global.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %t/hidden.s -o %t/hidden.o
----------------
Instead of having multiple object files, consider placing symbols with different properties to the same assembly file, and rename `x` to avoid conflicts. This can save many ld.lld and llvm-objdump invocations, decrease the number of lines without losing readability.

Tests which cause GOT optimization to fail can be placed separately.

Then move comments like `## Symbol 'x' is nonpreemptible, but --no-relax surpresses relaxations.` just before the associate CHECK lines.


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:16
+# RUN: llvm-objdump --no-show-raw-insn -d %t/load-to-x1.so \
+# RUN:  | FileCheck --check-prefix=RELAXED-X1 %s
+
----------------
More common style is two-column indentation.

(Super nit: In this directory and some binary utilities ` | \` in the previous line is more common)


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:97
+.global main
+main:
+  adrp    x1, :got:x
----------------
Unlike Mach-O, ELF uses `_start`. Missing `_start` leads to a warning in an executable link.

If you combine the tests, lots of boilerplate (duplicate `.rodata`, duplicate `main:`, etc) can be avoided.


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:166
+  ldr     x0, [x0, #:got_lo12:x]
+
----------------
delete blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063