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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 12 inline comments as done.
gkm added inline comments.


================
Comment at: lld/MachO/Arch/ARM64.cpp:48
+  RelocationInfoType type;
+  llvm::Optional<bool> TLV;
+  bool PCrel;
----------------
int3 wrote:
> What's the difference between `None` and `false` here?
> 
> (Also, clang-tidy naming issues seem legit)
`llvm::Optional<bool>` is tri-state: `true`, `false`, and `None` meaning any / don't care / not applicable.


================
Comment at: lld/MachO/Arch/ARM64.cpp:79-90
+  switch (rel.r_length) {
+  case 0:
+    return *loc;
+  case 1:
+    return read16le(loc);
+  case 2:
+    return read32le(loc);
----------------
int3 wrote:
> can we factor this out and reuse it across both architectures?
Yes. When the dust settles, I will look for this and other opportunities.


================
Comment at: lld/MachO/Arch/ARM64.cpp:93-107
+inline uint64_t bitField(uint64_t value, int right, int width, int left) {
+  return ((value >> right) & ((1 << width) - 1)) << left;
+}
+
+inline uint64_t fixupBranch26(uint64_t encoding, uint64_t va) {
+  return (encoding | bitField(va, 2, 26, 0));
+}
----------------
int3 wrote:
> some comments about the different arm64 instruction formats would be helpful
Indeed. When I find the document that catalogues and names the encoding formats I will reference those.


================
Comment at: lld/MachO/Arch/ARM64.cpp:111
+  // For instruction fixups, r.addend contains the base encoding
+  uint64_t fixup = 0;
+  switch (r.type) {
----------------
smeenai wrote:
> 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.
I renamed: s/`encoding`/`base`/; s/`fixup`RelocFormat/`encode`RelocFormat/


================
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
----------------
smeenai wrote:
> 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).
This was intentional: `va` = Virtual Address. The caller `InputSection::writeTo()` used to combine the `va` and `r.addend` to yield the `val`, but I moved that inside the `TargetInfo::relocateOne()` members.

For clarity, I prefer `VMA` over `VA` for these names, but there are many instances, e.g., `getVA()`, so I'll only do that with consent, and in a separate diff.


================
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;
----------------
smeenai wrote:
> int3 wrote:
> > what does 'base encoding' mean?
> Is this comment applicable to X86_64?
No, I suspect not. I will try `assert(r.addend == 0)` in `X86_64::prepareSymbolRelocation()` to be certain.


================
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;
----------------
smeenai wrote:
> 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.
Do you know offhand what is the recommended API for comparing the target arch with an arch name string?


================
Comment at: lld/test/lit.cfg.py:70
      ('--targets-built', {'AArch64': 'aarch64',
+                          'ARM64': 'arm64',
                           'AMDGPU': 'amdgpu',
----------------
smeenai wrote:
> I think the `llvm-config` output will always say `AArch64` and not `ARM64`. Is this line needed?
It's a temporary expediency so that test case `REQUIRES: arm64` filter works. I'll iron this out when the AArch64 vs. ARM64 choice is resolved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629