Page MenuHomePhabricator

ELF: Add support for remaining R_AARCH64_MOVW* relocations.
ClosedPublic

Authored by pcc on Jul 12 2019, 6:40 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Jul 12 2019, 6:40 PM
MaskRay added inline comments.Jul 12 2019, 7:19 PM
lld/test/ELF/aarch64-relocs.s
242 ↗(On Diff #209662)

You can consider moving this chunk to a separate test and using llvm-objdump -d --print-imm-hex, then you don't have to explain -65537 = 0xfffffffffffeffff.

x86-64-reloc-*.s and ppc32-reloc-*.s are organized this way.

lld/ELF/Arch/AArch64.cpp
257 ↗(On Diff #209662)

Really trivial nit, we use lower case in the comment before the function but upper case in the body. Is it worth choosing one? The rest of the file seems to mostly use lower case although there are some uses of MOVZ and MOVK in relaxTlsIeToLE.

262 ↗(On Diff #209662)

To save a lookup in the Arm ARM, is it worth a quick comment along the lines:
// opcode field is bits 30, 29, with 10 = movz, 00 = movn and 11 = movk.

ruiu accepted this revision.Jul 16 2019, 2:49 AM

LGTM

lld/ELF/Arch/AArch64.cpp
260 ↗(On Diff #209662)

As a single-letter variable, l is perhaps not a good name because it looks like 1. Maybe loc?

This revision is now accepted and ready to land.Jul 16 2019, 2:49 AM
pcc marked 5 inline comments as done.Jul 18 2019, 10:10 AM
pcc added inline comments.
lld/test/ELF/aarch64-relocs.s
242 ↗(On Diff #209662)

I think I'll leave this as is. The switch to --print-imm-hex would probably be better done as a cleanup over this entire file since there are already a number of instances of the same pattern here.

This revision was automatically updated to reflect the committed changes.