Page MenuHomePhabricator

[lld-macho] Add ARM64 target arch
ClosedPublic

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

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG87104faac433: [lld-macho] Add ARM64 target arch
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

There are a very large number of changes, so older changes are hidden. Show Older Changes
smeenai added inline comments.Oct 2 2020, 7:13 PM
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
83

Is this comment applicable to X86_64?

lld/MachO/InputSection.cpp
89

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.

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
82–83

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.

83

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

lld/MachO/Driver.cpp
455–456

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
89

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
82–83

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
89

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
89

"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
89

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.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.Nov 2 2020, 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.Nov 9 2020, 9:47 AM
lld/MachO/Arch/ARM64.cpp
96–109

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

gkm updated this revision to Diff 318138.Jan 21 2021, 2:44 AM
  • Update according to refactored reloc handling
int3 added inline comments.Jan 21 2021, 2:07 PM
lld/MachO/Arch/ARM64.cpp
120–123

I think there are some typos in this paragraph...

261–262

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

lld/MachO/Driver.cpp
129–136

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

648

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

lld/MachO/InputSection.h
35–36

what's the motivation behind this change?

lld/MachO/SyntheticSections.cpp
383

so we can actually bind to __dso_handle?

gkm marked 6 inline comments as done.Jan 27 2021, 5:54 PM
gkm added inline comments.
lld/MachO/Arch/ARM64.cpp
261–262

The comment lines are copied directly and unaltered from the ARM manual.

lld/MachO/InputSection.h
35–36

That's a leftover from an aborted experiment.

lld/MachO/SyntheticSections.cpp
383

Yes, we do for arm64

gkm updated this revision to Diff 319971.Jan 28 2021, 2:50 PM
gkm marked 3 inline comments as done.
  • revise according to review feedback
int3 accepted this revision.Jan 29 2021, 12:03 AM
int3 added inline comments.
lld/MachO/Arch/ARM64.cpp
261–262

ah fair enough. Not sure if I missed it, but it'd be good to leave a comment somewhere that C6.2.131 refers to the section in the ARM manual.

lld/MachO/Driver.cpp
648

could you leave a TODO here so we remember to test this behavior?

lld/MachO/SyntheticSections.cpp
383

would be good to add a test, or at least leave a TODO here before landing it. I want to make sure I didn't miss a similar case for x86.

lld/test/MachO/relocations.s
1

unaddressed nit :p

This revision is now accepted and ready to land.Jan 29 2021, 12:03 AM
int3 added a comment.Jan 29 2021, 12:03 AM

(looks like there are failing tests)

gkm updated this revision to Diff 322185.Feb 8 2021, 11:59 AM
  • rebase & remove incomplete+failing test (will add later)
gkm updated this revision to Diff 322218.Feb 8 2021, 2:27 PM
gkm marked 3 inline comments as done.
  • rebase
gkm updated this revision to Diff 322241.Feb 8 2021, 4:35 PM
  • Make RelocAttrBits::LLVM_BITMASK_LARGEST_ENUMERATOR adequate to escape assertions.
This revision was automatically updated to reflect the committed changes.
uabelho added inline comments.
lld/MachO/Arch/ARM64.cpp
74

gcc warns about "type < 0" always being false since type is unsigned.
Can it be removed?

Similarly with the assert above, I suppose type >= 0 is always true and it can be removed.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 11:28 PM
uabelho added inline comments.Feb 16 2021, 11:24 PM
lld/MachO/Arch/ARM64.cpp
74

I removed the always true/false conditions in
https://reviews.llvm.org/rGcaff023b7799
to silence compiler warnings.