[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) {
----------------
alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > MaskRay wrote:
> > > `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.
> > that was intentional (potentially not checking the last relocation), but to be rigorous we can do that.
> as Wilco pointed out, for the extra safety we check if these relocations show up in pairs (R_AARCH64_ADR_GOT_PAGE and R_AARCH64_LD64_GOT_LO12_NC), this needs to happen in advance before any changes/writes happen.
> Rather than keeping the state in boolean variables / exposing these details / complexity to higher level code it seems to be better to encapsulate this logic.
If that is the case, some `tryRelaxAdrpLdr` checks should be turned into `assert` .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063