Page MenuHomePhabricator

[lld-macho] Add ARM64 target arch
Needs ReviewPublic

Authored by gkm on Sep 30 2020, 7:01 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

This is an initial base commit for ARM64 target arch support. I don't represent that it complete or bug-free, but wish to put it out for review now that some basic things like branch target & load/store address relocs are working.

I can add more tests to this base commit, or add them in follow-up commits.

It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

Diff Detail

Event Timeline

gkm created this revision.Sep 30 2020, 7:01 PM
gkm requested review of this revision.Sep 30 2020, 7:01 PM
gkm updated this revision to Diff 295458.Sep 30 2020, 8:17 PM
  • remove some left-over X86_64 code
gkm updated this revision to Diff 295735.Oct 1 2020, 10:37 PM
  • add stubs support
gkm updated this revision to Diff 295865.Oct 2 2020, 10:31 AM
  • cleanups
gkm updated this revision to Diff 295940.Oct 2 2020, 5:17 PM
  • correct some reloc length validity sets in arm64RelocAttrs[]
int3 added a subscriber: int3.Oct 2 2020, 6:55 PM
int3 added inline comments.
lld/MachO/Arch/ARM64.cpp
49

What's the difference between None and false here?

(Also, clang-tidy naming issues seem legit)

80–91

can we factor this out and reuse it across both architectures?

94–108

some comments about the different arm64 instruction formats would be helpful

lld/MachO/Arch/X86_64.cpp
131

what does 'base encoding' mean?

smeenai added a subscriber: smeenai.Oct 2 2020, 7:13 PM

It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

I would use arm64, since we're targeting Apple platforms.

Still working through the target code, but an initial round of comments.

lld/MachO/Arch/ARM64.cpp
55

Representing the valid lengths as a bitfield is a neat idea :)

111

Would be helpful to elaborate on what "base encoding" is.

112

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.

115

Would be helpful to comment the reason for this assertion (I imagine it's because an instruction VA should always be 4-byte aligned).

lld/MachO/Arch/X86_64.cpp
130–132

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

131

Is this comment applicable to X86_64?

lld/MachO/Driver.cpp
415–419

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.

lld/MachO/InputSection.cpp
56–57

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.

lld/test/lit.cfg.py
70 ↗(On Diff #295940)

I think the llvm-config output will always say AArch64 and not ARM64. Is this line needed?

It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

I'd go with AArch64 to match LLVM's naming convention with the backend, if you want to enable ARM64 support when compiling the monorepo, you have to add AArch64 to the cmake variable LLVM_TARGETS_TO_BUILD, so I say be consistent with that.

gkm marked 12 inline comments as done.Oct 2 2020, 11:54 PM
gkm added inline comments.
lld/MachO/Arch/ARM64.cpp
49

llvm::Optional<bool> is tri-state: true, false, and None meaning any / don't care / not applicable.

80–91

Yes. When the dust settles, I will look for this and other opportunities.

94–108

Indeed. When I find the document that catalogues and names the encoding formats I will reference those.

112

I renamed: s/encoding/base/; s/fixupRelocFormat/encodeRelocFormat/

lld/MachO/Arch/X86_64.cpp
130–132

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.

131

No, I suspect not. I will try assert(r.addend == 0) in X86_64::prepareSymbolRelocation() to be certain.

lld/MachO/Driver.cpp
415–419

Do you know offhand what is the recommended API for comparing the target arch with an arch name string?

lld/test/lit.cfg.py
70 ↗(On Diff #295940)

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.

tschuett added subscribers: psmith, tschuett.EditedOct 3 2020, 1:51 AM

@psmith might be able to help.

The encodings of adrp, add, stp, ldr, ... are described here:
https://developer.arm.com/documentation/ddi0487/latest/

I was referring to the fixup calls.

gkm marked 8 inline comments as done.Oct 3 2020, 1:31 PM

The encodings of adrp, add, stp, ldr, ... are described here:
https://developer.arm.com/documentation/ddi0487/latest/

😆 Yes, I have that 8248(!) page doc. Chapter C4: A64 Instruction Set Encoding has all the details. Unlike PowerPC, it fails to neatly classifies & names the main encodings. Even so, this ought to suffice.

tschuett added a comment.EditedOct 3 2020, 2:04 PM

Sorry for being slow.

For every adrp instruction you call fixupPage21.

inline uint64_t fixupPage21(uint64_t encoding, uint64_t va) {
  return (encoding | bitField(va, 12, 2, 29) | bitField(va, 14, 19, 5));
}

According to the ARMARM. the PC relative address is encoded into two sections. One starting at offset 5 and the width is 19. The second one starts at offset 29 and the width is 2. That matches the 2 and 29 and the 19 and 5 in fixupPage 21. I don't know yet how the explain the 12 and 14.

is encoded as "immhi:immlo" times 4096. This could explain the 12 and 14 for division.

tschuett added a comment.EditedOct 3 2020, 2:21 PM

only for the b instruction, you use

inline uint64_t fixupBranch26(uint64_t encoding, uint64_t va) {
  return (encoding | bitField(va, 2, 26, 0));
}

The label is encoded in a slot starting at 0 and the width is 26, which matches the 0 and 26. The label is encoded as imm26 times 4, which could explain the 2. The 2 in bitField divides by 4.

gkm added inline comments.Oct 4 2020, 12:50 AM
lld/MachO/InputSection.cpp
56–57

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.

int3 added inline comments.Oct 4 2020, 8:36 PM
lld/MachO/Arch/ARM64.cpp
49

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...

lld/MachO/Arch/X86_64.cpp
130–132

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.

lld/MachO/InputSection.cpp
56–57

my preference would be to keep it here but also to keep Val over VA.

int3 added inline comments.Oct 5 2020, 12:29 PM
lld/MachO/Arch/ARM64.cpp
94

nit: AFAIK inline doesn't have a major impact on the compiler's inlining decisions and isn't necessary within cpp files. OTOH we should mark this static.

gkm marked 4 inline comments as done.Oct 5 2020, 2:58 PM
gkm added inline comments.
lld/MachO/Arch/ARM64.cpp
49

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.

lld/MachO/InputSection.cpp
56–57

"value" is even better than "val", to further distinguish it visually from "va". 🚲

gkm updated this revision to Diff 296306.Oct 5 2020, 2:59 PM
gkm marked 2 inline comments as done.
  • rebase & amend according to review feedback
int3 added a comment.Oct 5 2020, 3:19 PM

I can add more tests to this base commit, or add them in follow-up commits.

I'm fine with either. @smeenai, what's the typical approach here?

lld/MachO/Arch/ARM64.cpp
128–131

ultra nit: I think there should be spaces around em dashes, i.e. --

also s/relococatable/relocatable/

lld/MachO/InputSection.cpp
56–57

while we're 🚲🏠ing... I think we should keep the referentVA name in lines 38-51 above and create a new value variable above line 54. That gives a clearer indication that we are handling a VA until we subtract the PC. Moreover, I think referentValue / referentVal is actually a bit of a misnomer, despite its parallels to referentVA -- "address of the referent" makes sense, but subtracting the PC doesn't give us the "value of the referent". relocValue or just value seems more accurate.

int3 added inline comments.Oct 5 2020, 4:08 PM
lld/test/MachO/relocations-x86_64.s
1

nit: lld-elf's tests use x86-64 instead of x86_64 in their filenames

gkm marked 2 inline comments as done.Oct 5 2020, 4:10 PM

@smeenai and I agreed that this diff should contain a complete set of tests.

lld/MachO/Arch/ARM64.cpp
128–131

The real problem is that there are 4(!) em dashes in one tortuous run-on sentence. I reworded.

gkm updated this revision to Diff 296324.Oct 5 2020, 4:15 PM
gkm marked an inline comment as done.
  • amend according to review feedback
gkm updated this revision to Diff 296327.Oct 5 2020, 4:22 PM
  • move encoding bitfield comments outside their functions
gkm updated this revision to Diff 302286.Mon, Nov 2, 7:57 AM
  • Factor skeletal paired-reloc support into a separate prerequisite diff
  • ARM64RelocAttrs: drop type, add name, drop Optional<> from tlv
  • s/getImplicitAddend/getAddend/ because it handles all forms of addend: implicit, explicit, paired.
  • Add arm64 arch to test inputs MacOSX.sdk/usr/lib/lib*.tbd
int3 added inline comments.Mon, Nov 9, 9:47 AM
lld/MachO/Arch/ARM64.cpp
95–108

can we factor out the validation code into a separate function?