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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Arch/ARM64.cpp:260-261
+  uint32_t instruction = read32le(loc);
+  // C6.2.131 LDR (immediate)
+  // LDR <Xt>, [<Xn|SP>{, #<pimm>}]
+  if ((instruction & 0xffc00000) != 0xf9400000)
----------------
gkm wrote:
> int3 wrote:
> > nit: can we have `ldr` in lowercase? since it's also in lowercase in e.g. `stubHelperEntryCode` above.
> > 
> > I also have a slight preference that the error messages have them in lowercase as well, but I guess it doesn't matter as much as long as things are consistent, and all-caps saves us from having to quote them for clarity
> The comment lines are copied directly and unaltered from the ARM manual.
ah fair enough. Not sure if I missed it, but it'd be good to leave a comment somewhere that `C6.2.131` refers to the section in the ARM manual.


================
Comment at: lld/MachO/Driver.cpp:639
+  assert(config->arch == AK_x86_64 || config->arch == AK_x86_64h ||
+         config->arch == AK_arm64 || config->arch == AK_arm64e);
 
----------------
int3 wrote:
> as the comment above states, we should always return `true` from this function when we have arm64. (ld64 will emit a warning if `-no_pie` is used on that arch.)
could you leave a TODO here so we remember to test this behavior?


================
Comment at: lld/MachO/SyntheticSections.cpp:383
       in.weakBinding->addEntry(sym, section, offset, addend);
-  } else if (isa<DSOHandle>(sym)) {
-    error("cannot bind to " + DSOHandle::name);
-  } else {
+  } else if (!isa<DSOHandle>(sym)) {
     // Undefined symbols are filtered out in scanRelocations(); we should never
----------------
gkm wrote:
> int3 wrote:
> > so we can actually bind to `__dso_handle`?
> Yes, we do for arm64
would be good to add a test, or at least leave a TODO here before landing it. I want to make sure I didn't miss a similar case for x86.


================
Comment at: lld/test/MachO/relocations-x86_84.s:1
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
----------------
int3 wrote:
> nit: lld-elf's tests use `x86-64` instead of `x86_64` in their filenames
unaddressed nit :p  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629