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

Mon Nov 22 19:21:25 PST 2021

MaskRay added a comment.

There are 16 unresolved comments (tip: click "Done" on a comment, then `arc diff 'HEAD^'` will resolve the comment when you upload a new revision), so I did not take another look.

I was also waiting for https://github.com/ARM-software/abi-aa/pull/106 .
For similar PowerPC/RISC-V/x86-64 ABI changes, we wait for the ABI to be updated before landing the lld/ELF, MC, compiler implementation.
abi-aa/pull/106 has been accepted and I assume will not have large directional changes, so I think the patch can proceed after various comments are resolved.

> (I'm happy to help refactor the existing code in LLD / as I mentioned above - would be great to be able to move iteratively)

For such a situation (dependent patches), reviewers typically want to read all patches, otherwise landing some partial commits may cause churn which will be changed by subsequent patches. We want to avoid the churn.

================
Comment at: lld/ELF/Arch/AArch64.cpp:572
+AArch64Relaxer::AArch64Relaxer(ArrayRef<Relocation> relocs) {
+  if (!config->relax || config->emachine != EM_AARCH64) {
+    safeToRelaxAdrpLdr = false;
----------------
The condition `config->emachine != EM_AARCH64` is unneeded. It may be needed with your unposted dependented patches. This is one reason I suggest that you post the dependent patch together.

================
Comment at: lld/ELF/Arch/AArch64.cpp:581
+  for (; i < size; ++i) {
+    if (relocs[i].type == R_AARCH64_LD64_GOT_LO12_NC) {
+      break;
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

I think the early return pattern harms readability here. 
```
if (relocs[i].type == R_AARCH64_ADR_GOT_PAGE)
  ...
else if (relocs[i].type == R_AARCH64_LD64_GOT_LO12_NC)
  ...
```
will be clearer.

================
Comment at: lld/ELF/Arch/AArch64.cpp:628
+  // Check if the first instruction is ADRP and the second instruction is LDR.
+  if ((adrpInstr & 0x9F000000) != 0x90000000 ||
+      (ldrInstr & 0x3B000000) != 0x39000000)
----------------
Prefer lower-case hexadecimals. I know PPC64 may be inconsistent.

================
Comment at: lld/ELF/Arch/AArch64.cpp:645
+      getAArch64Page(sym.getVA()) - getAArch64Page(secAddr + adrpRel.offset);
+  if (val != llvm::SignExtend64(val, 33))
+    return false;
----------------
The out-of-range condition is not tested. You can see `ppc64-long-branch.s` how I test the condition.

================
Comment at: lld/ELF/Arch/AArch64.cpp:648
+
+  Relocation adrpSymRel = {
+      R_AARCH64_PAGE_PC,          // expr
----------------
In other replaces we just use the concise form to initialization a `Relocation`

```
   Relocation adrpRel{R_AARCH64_PAGE_PC, R_AARCH64_ADR_PREL_PG_HI21,
                      adrpRel.offset, 0, &sym};
```
I assume that `adrpSymRel` can be named to `adrpRel`

================
Comment at: lld/ELF/InputSection.cpp:1031
+      secAddr += sec->outSecOff;
+    uint64_t addrLoc = secAddr + offset;
     RelExpr expr = rel.expr;
----------------
Why does `tryRelaxAdrpLdr` need `addrLoc`? I think you can just pass `addrLoc` to it.

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got-symbols.s:48
+{
+.rodata 0x100: { *(.rodata) }
+.text   0x200100: { *(.text) }
----------------
Indent.

It is uncommon to use such a small address (0x100). 0x1000 is more common.

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got-symbols.s:62
+  ldr     x0, [x0, #:got_lo12:x]
+
+#--- hidden.s
----------------
Non-zero addends and a lone `R_AARCH64_LD64_GOT_LO12_NC` are not tested.

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:9
+
+## Case 1. Symbol 'x' is nonpreemptible, the relaxation should be applied.
+## This test verifies the encoding when the register x1 is used.
----------------
Avoid `Case 1. `. New cases may be inserted between two existing one and the case number would just cause unneeded number adjustment.

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:20
+# RUN: ld.lld -shared %t-load-to-x1.o -T %t/linker.script --no-relax -o %t-load-to-x1.so
+# RUN: llvm-objdump --no-show-raw-insn -d %t-load-to-x1.so \
+# RUN: | FileCheck --check-prefix=NO-RELAX %s
----------------
Indent the continuation line. My preferred style (and jhenderson's) is
```
# RUN: llvm-objdump --no-show-raw-insn -d %t-load-to-x1.so | \
# RUN:   FileCheck --check-prefix=NO-RELAX %s
```

When split-file and %t are used, I'd place all temporary files (including executables) under %t, too. So that you can delete all temporary files at once.

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:27
+## Case 3. LDR and ADRP use different registers, no relaxations should be applied.
+# RUN: ld.lld %t-mismatched-src-reg.o -o %t-mismatched-src-reg.exe
+# RUN: llvm-objdump --no-show-raw-insn -d %t-mismatched-src-reg.exe \
----------------
For executables, just omit the suffix name. Avoid `.exe`

================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:51
+## far apart so that the adrp + ldr pair cannot be relaxed to adr + nop.
+#--- linker.script
+SECTIONS
----------------
You can use `.lds` or `.t` suffix for a linker script. `linker` doesn't carry more information than `a.lds` or `a.t`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063