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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/ARM64.cpp:119-122
+// For instruction relocations (load/store & address arithmetic),
+// r.addend contains the base encoding, which are opcode and
+// non-relococatable operand fields pre-populated. The the relocatable
+// operand field (BRANCH26, PAGE21, PAGEOFF12) are zero-filled.
----------------
I think there are some typos in this paragraph...


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


================
Comment at: lld/MachO/Driver.cpp:129
 static TargetInfo *createTargetInfo(opt::InputArgList &args) {
-  StringRef arch = args.getLastArgValue(OPT_arch, "x86_64");
-  config->arch = MachO::getArchitectureFromName(
-      args.getLastArgValue(OPT_arch, arch));
-  switch (config->arch) {
-  case MachO::AK_x86_64:
-  case MachO::AK_x86_64h:
+  // TODO: should unspecified arch be an error rather than defaulting?
+  StringRef archName = args.getLastArgValue(OPT_arch, "x86_64");
----------------
ld64 seems to make unspecified arch an error when LTO is being used. I'm not sure why though. Feels like we should be able to infer the arch from our input files regardless


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


================
Comment at: lld/MachO/InputSection.h:36
   uint64_t addend;
-  llvm::PointerUnion<Symbol *, InputSection *> referent;
+  llvm::PointerUnion<Defined *, Symbol *, InputSection *> referent;
 };
----------------
what's the motivation behind this change?


================
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
----------------
so we can actually bind to `__dso_handle`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629