[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:48
+  RelocationInfoType type;
+  llvm::Optional<bool> TLV;
+  bool PCrel;
----------------
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...


================
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
----------------
gkm wrote:
> 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.
I think what @smeenai meant is, even though this parameter doesn't include the addend, it still isn't the VA when we are handling pcrel relocations.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88629