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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexander-shaposhnikov 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) {
----------------
MaskRay wrote:
> 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` .
yeah, the check that second relocation is R_AARCH64_LD64_GOT_LO12_NC can be turned into assert, left it as is since I find the symmetric form a tiny bit easier to read. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063