This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF][AArch64] Add support for R_AARCH64_LD64_GOTPAGE_LO15 relocation
ClosedPublic

Authored by zatrazz on Jan 13 2021, 9:35 AM.

Details

Summary

It is not used by LLVM, but GCC might generates it when compiling
with -fpie, as indicated by PR#40357 [1].

The testcase depends on https://reviews.llvm.org/D94611 to parse
gotpage_lo15 and https://reviews.llvm.org/D94809 to set the correct
placement of _GLOBAL_OFFSET_TABLE_ as expected by gcc.

Besides the regression check, I build a llvm (clang;clang-tools-extra;
compiler-rt;lld) with -fpie and -DLLVM_ENABLE_PIC=OFF with
gcc9 with DLLVM_ENABLE_LLD=ON to check if the
R_AARCH64_LD64_GOTPAGE_LO15 is handled correctly on large
projects.

The resulted build generates about 445619
R_AARCH64_LD64_GOTPAGE_LO15 relocations and the resulting
llvm shows no regression on check-all.

[1] https://bugs.llvm.org/show_bug.cgi?id=40357

Diff Detail

Event Timeline

zatrazz created this revision.Jan 13 2021, 9:35 AM
zatrazz requested review of this revision.Jan 13 2021, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 9:35 AM
psmith added a subscriber: psmith.Jan 14 2021, 8:10 AM

Thanks for working on this. It will be very useful for people using GCC yet using LLD as a linker on AArch64. Will try and take a look later today, if not then I should have some time tomorrow morning.

psmith added inline comments.Jan 14 2021, 9:31 AM
lld/ELF/InputSection.cpp
708

I think that this should be

return getAArch64Page(sym.getGotVA() + a) - getAArch64Page(in.got->getVA());

From https://github.com/ARM-software/abi-aa/blob/master/aaelf64/aaelf64.rst the expression is: G(GDAT(S+A))-Page(GOT)
Where GOT is "GOT is the address of the Global Offset Table"
Page(p) will be the same as Page(GOT) for many entries close to the start of the GOT.

IIUC the intent is to get an offset from the base of the GOT so that the same register can hold the base of the GOT for the whole function.

adrp x0, _GLOBAL_OFFSET_TABLE_
ldr x1, [x0, #:gotpage_lo15:foo]
ldr x2, [x0, #:gotpage_lo15:bar]
lld/test/ELF/aarch64-gotpage.s
6

This particular test won't catch the case where Page(GOT) != Page(P). Perhaps something like a linker script that puts the GOT at the last 8-bytes of a page (I think .got is 8 byte aligned on AArch64)

.got (0x10000 - 8) : { *.got }

This would put the first entry in the same page as Page(GOT) but the second entry should be in the second page. This should be visible in the ldr disassembly.

I will update the patch, since it is incomplete and requires https://reviews.llvm.org/D94809 .

lld/ELF/InputSection.cpp
708

Indeed it misses the addend in the account. I am not sure if G() should be page aligned as well, the AArch64 ABI is only explict about Page() operation, gold at least does not seem to align it. I think the correct formula is in fact:

return sym.getGotVA() + a - getAArch64Page(in.got->getVA());
zatrazz updated this revision to Diff 317019.Jan 15 2021, 10:33 AM
zatrazz edited the summary of this revision. (Show Details)

Patch updated from comments, main changes are:

  • Fixed getRelocTargetVA fomula.
  • Fixed some lint issue.
  • Add a got displacement on .got for the test.

Looks like we need to make sure that the relocation is a static link time constant so that it can be used in PIE and Shared Libs. Otherwise LGTM.

lld/ELF/Relocations.cpp
409

I think R_AARCH64_GOT_PAGE needs to be added here, otherwise we get a link error about text relocations when -pie or --shared.

lld/test/ELF/aarch64-gotpage.s
5

Will be worth adding a test case with pie or shared to make sure the relocation is allowed in those cases.

MaskRay accepted this revision.Jan 21 2021, 11:20 AM

LG, but please wait for @psmith.

lld/test/ELF/aarch64-gotpage.s
9

Nit: newer tests prefer ## for non-RUN-non-CHECK comments.

This revision is now accepted and ready to land.Jan 21 2021, 11:20 AM
MaskRay added inline comments.Jan 25 2021, 11:19 AM
lld/test/ELF/aarch64-gotpage.s
5

Please see aarch64-fpic-got.s. The idea is that we want to exercise the isStaticLinkTimeConstant path, i.e.

if global is defined in a shared object, this can still be linked in -shared mode.

Personally I don't think having another test making global defined in the executable is necessary.

You can change common symbols to regular data symbols since we've already have sufficient common tests and this test does not need to tie to common.

MaskRay requested changes to this revision.Jan 25 2021, 11:21 AM
This revision now requires changes to proceed.Jan 25 2021, 11:21 AM
zatrazz updated this revision to Diff 319088.Jan 25 2021, 12:27 PM

Updated patch based on previous comments. The R_AARCH64_GOT_PAGE is added on
the isStaticLinkTimeConstant and the test is changed to check for shared build as well.

MaskRay accepted this revision.Jan 25 2021, 12:31 PM
This revision is now accepted and ready to land.Jan 25 2021, 12:31 PM

Thanks for the updates. LGTM too.

zatrazz closed this revision.Jan 26 2021, 4:43 AM