[PATCH] D88629: [lld-macho] Add ARM64 target arch

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> I can add more tests to this base commit, or add them in follow-up commits.

I'm fine with either. @smeenai, what's the typical approach here?



================
Comment at: lld/MachO/Arch/ARM64.cpp:127-130
+// For instruction relocations (load/store & address arithmetic),
+// r.addend contains the base encoding--opcode and non-relococatable
+// operand fields pre-populated--with the relocatable operand
+// field--BRANCH26, PAGE21, PAGEOFF12--zero-filled.
----------------
ultra nit: I think there should be spaces around em dashes, i.e. ` -- `

also s/relococatable/relocatable/


================
Comment at: lld/MachO/InputSection.cpp:55
     if (r.pcrel)
-      referentVal -= getVA() + r.offset;
-    target->relocateOne(buf + r.offset, r, referentVal);
+      referentVA -= getVA() + r.offset;
+    target->relocateOne(buf + r.offset, r, referentVA);
----------------
gkm wrote:
> int3 wrote:
> > gkm wrote:
> > > smeenai wrote:
> > > > Same comment here about VA vs. val. I think I'd prefer to just shift the pcrel handling entirely into `relocateOne`; yes, it's architecture-agnostic, so there's a *tiny* bit of duplication per-architecture, but then all the logic of going from a referent VA to an actual relocation value gets shifted into `relocateOne`, which is nicer IMO.
> > > `pcrel` handling requires the value of `InputSection::getVA()`, accessible here via `this`. Moving it into `relocateOne` means passing another argument. I think it's better to keep it here.
> > my preference would be to keep it here but also to keep `Val` over `VA`.
> "value" is even better than "val", to further distinguish it visually from "va".  🚲
while we're 🚲🏠ing... I think we should keep the `referentVA` name in lines 38-51 above and create a new `value` variable above line `54`. That gives a clearer indication that we are handling a VA until we subtract the PC. Moreover, I think `referentValue` / `referentVal` is actually a bit of a misnomer, despite its parallels to `referentVA` -- "address of the referent" makes sense, but subtracting the PC doesn't give us the "value of the referent". `relocValue` or just `value` seems more accurate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629