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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 4 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:
> gkm wrote:
> > 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.
> What I was trying to ask was: when is the distinction between `false` and `None` important? After looking a bit more closely at the code, I guess we want `None` for things like `ARM64_RELOC_UNSIGNED` which can refer to both TLV and non-TLV symbols. Anyway, would be good to have a comment about it...
non-`None` means perform the validity check against `sym->isTlv()`. I populated the table according to the checks performed in `lld/MachO/Arch/X86_64.cpp`. It occurs to me now that more relocation types can likely be non-`None`, e.g., `ARM64_RELOC_BRANCH26` should be `false`. As I get further with test cases and understand more, I might find others.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629