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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

> It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

I would use arm64, since we're targeting Apple platforms.

Still working through the target code, but an initial round of comments.



================
Comment at: lld/MachO/Arch/ARM64.cpp:54
+static ARM64RelocAttrs arm64RelocAttrs[] = {
+    {ARM64_RELOC_UNSIGNED, llvm::None, false, 8 | 4},
+    {ARM64_RELOC_SUBTRACTOR, llvm::None, false, 8 | 4},
----------------
Representing the valid lengths as a bitfield is a neat idea :)


================
Comment at: lld/MachO/Arch/ARM64.cpp:110
+void ARM64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t va) const {
+  // For instruction fixups, r.addend contains the base encoding
+  uint64_t fixup = 0;
----------------
Would be helpful to elaborate on what "base encoding" is.


================
Comment at: lld/MachO/Arch/ARM64.cpp:111
+  // For instruction fixups, r.addend contains the base encoding
+  uint64_t fixup = 0;
+  switch (r.type) {
----------------
What is the term "fixup" supposed to refer to? It's a little confusing because ld64 appears to use the term "fixup" instead of "relocation", so I don't know if this is a reference to that or something else.


================
Comment at: lld/MachO/Arch/ARM64.cpp:114
+  case ARM64_RELOC_BRANCH26:
+    assert((va & 0x3) == 0);
+    fixup = fixupBranch26(r.addend, va);
----------------
Would be helpful to comment the reason for this assertion (I imagine it's because an instruction VA should always be 4-byte aligned).


================
Comment at: lld/MachO/Arch/X86_64.cpp:124
 
-void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t val) const {
+void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t va) const {
+  // For instruction fixups, r.addend contains the base encoding
----------------
I think `val` makes more sense than `va` here, since it's the calculated value for the relocation that you're writing out, which isn't necessary the target's VA (e.g. for PC-relative relocations).


================
Comment at: lld/MachO/Arch/X86_64.cpp:125
+void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t va) const {
+  // For instruction fixups, r.addend contains the base encoding
+  va += r.addend;
----------------
int3 wrote:
> what does 'base encoding' mean?
Is this comment applicable to X86_64?


================
Comment at: lld/MachO/Driver.cpp:420
       // TODO: Update when we extend support for other archs
-      if (arch != "x86_64")
+      if (arch != "x86_64" && arch != "arm64")
         continue;
----------------
This should be a comparison against the arch that we're targeting, not a comparison against all the archs we support, since the intent is to filter out lines that aren't for the current target arch.


================
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);
----------------
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.


================
Comment at: lld/test/lit.cfg.py:70
      ('--targets-built', {'AArch64': 'aarch64',
+                          'ARM64': 'arm64',
                           'AMDGPU': 'amdgpu',
----------------
I think the `llvm-config` output will always say `AArch64` and not `ARM64`. Is this line needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629