This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF][AArch64] Handle R_AARCH64_PLT32 relocation
ClosedPublic

Authored by leonardchan on Jun 4 2020, 11:54 AM.

Details

Summary

This is the followup to D77647 which implements handling for the new R_AARCH64_PLT32 relocation type in lld. This relocation would benefit the PIC-friendly vtables feature described in D72959.

Diff Detail

Event Timeline

leonardchan created this revision.Jun 4 2020, 11:54 AM
leonardchan planned changes to this revision.Jun 4 2020, 11:54 AM

I'm not sure unconditionally changing .word foo - . to use R_AARCH64_PLT32 is the appropriate change. Note that x86 R_X86_64_PC32 doesn't do this.
.word foo@plt - . would probably make more sense. For one reason, R_AARCH64_PLT32 is not supported by binutils, unconditionally using it can break -fno-integrated-as.
There is also no need to special case on visiblity: (ElfSym.getVisibility() == llvm::ELF::STV_HIDDEN) ? R_CLS(PREL32) : R_CLS(PLT32);, you can entirely defer the handling to the linker

Additionally, the lld changes should probably be moved to a separate patch. It is not tightly coupled with MC. The tests are also inadequate - you'd need a range extension thunk test. I can do this part if you don't mind.

psmith added a subscriber: psmith.Jun 4 2020, 3:15 PM

I'm not sure unconditionally changing .word foo - . to use R_AARCH64_PLT32 is the appropriate change. Note that x86 R_X86_64_PC32 doesn't do this.
.word foo@plt - . would probably make more sense. For one reason, R_AARCH64_PLT32 is not supported by binutils, unconditionally using it can break -fno-integrated-as.
There is also no need to special case on visiblity: (ElfSym.getVisibility() == llvm::ELF::STV_HIDDEN) ? R_CLS(PREL32) : R_CLS(PLT32);, you can entirely defer the handling to the linker

Additionally, the lld changes should probably be moved to a separate patch. It is not tightly coupled with MC. The tests are also inadequate - you'd need a range extension thunk test. I can do this part if you don't mind.

I agree with MaskRay that we should not unconditionally change .word foo - . to use R_AARCH64_PLT32. I think the suggestion to use foo@plt - . is a good one as I think this is how the X86 version is used. I noticed your LLVM-DEV message on assertion failures. I've not got a suggestion at the moment it is late at night right now; I'd expect the assertion is there as we wouldn't expect @plt to be used without a relocation like R_AARCH64_PLT32.

The range thunks for R_AARCH64_PLT32 will need a large distance to the symbol destination as the range for the relocation is +/- 2^31, I suggest using a linker script to avoid creating a gigantic file. Will also be useful to test that R_AARCH64_PLT32 can generate a PLT entry.

lld/ELF/Arch/AArch64.cpp
263–264

This will need updating for R_AARCH64_PLT32, the range is larger as well, from memory it is a signed 32-bit value.

First implement the lld change with standalone tests covering all cases; that's one change.
Then do the assembly bits. As others have said, follow the x86 model where @plt is required to request this reloc type.
The assembler doesn't do its own logic based on the symbols to decide what reloc types to use. The assembly syntax has to request it directly.
When the compiler is producing a reference, it explicitly uses the symbol@plt syntax (or internal equivalent for integrated-as) when a PLT entry might be required.

leonardchan retitled this revision from [WIP][ELF][AArch64] Implement the logic for handling R_AARCH64_PLT32 to [lld][ELF][AArch64] Handle R_AARCH64_PLT32 relocation.
leonardchan edited the summary of this revision. (Show Details)
  • Removed the llvm changes to be moved to a separate patch.
  • Added a test for checking the relocation used with an undefined weak symbol. IIRC, the behavior of this is unspecified anyway.
  • Added test for checking overflow.

I also removed the changes relating to thunk emission since I'm not confident in how they should be used here. For example, if I have .word func@plt - . and the plt entry for this is more than 2^31 B away, should we have a thunk in the first place? If so, would this word be replaced with the offset to the thunk instead of the PLT, and then have the thunk return the actual offset?

Rebased and updated tests with assembly instead of yaml.

Looking good so far. Would be good to add a range extension thunk test case. For example:

 .section .text.1, "ax", %progbits
 .global _start
 .type _start, %function
_start: .word callee@PLT - .

 .section .text.2, "ax", %progbits
 .global callee
 .type callee, %function        
callee: ret

Using a linker script to get a > 4gb gap. The AT is there to stop a multi gigabyte file being written.

SECTIONS {
  .text.1 0x10000 : { *(.text.1) }
  .text.2 0x200000000 : AT(0x20000) { *(.text.2) }
}

The .word should be an offset to the range extension thunk instead of the target.

Looking good so far. Would be good to add a range extension thunk test case. For example:

 .section .text.1, "ax", %progbits
 .global _start
 .type _start, %function
_start: .word callee@PLT - .

 .section .text.2, "ax", %progbits
 .global callee
 .type callee, %function        
callee: ret

Using a linker script to get a > 4gb gap. The AT is there to stop a multi gigabyte file being written.

SECTIONS {
  .text.1 0x10000 : { *(.text.1) }
  .text.2 0x200000000 : AT(0x20000) { *(.text.2) }
}

The .word should be an offset to the range extension thunk instead of the target.

Thanks for the example. Added and rebased.

MaskRay added inline comments.Jun 22 2020, 5:08 PM
lld/ELF/Arch/AArch64.cpp
269

(type == R_AARCH64_PLT32) => type == R_AARCH64_PLT32

Drop two other unneeded parentheses.

lld/test/ELF/aarch64-plt32.s
15 ↗(On Diff #272548)

Place RUN and CHECK lines before code. (I know that some tests do not enforce this rule.)

lld/test/ELF/aarch64-range-thunk-extension-plt32.s
25
// CHECK-LABEL: <_start>:
// CHECK-NEXT:    10000:     04 00 00 00     .word   0x00000004

Omitting addresses on the label line avoids duplication.

29

ditto

lld/test/ELF/aarch64-undefined-weak-plt32.s
1 ↗(On Diff #272548)

As a separate test file this adds very little value. You can move the test to the main test file.

13 ↗(On Diff #272548)

Delete Disassembly of section .text: - not useful.

15 ↗(On Diff #272548)

Omitting addresses on the label line avoids duplication.

lld/test/ELF/relocation-plt32-aarch64.s
11 ↗(On Diff #272548)

NEXT

12 ↗(On Diff #272548)

Use /// for non-CHECK-non-RUN comments, or use # RUN and ## for comments.

MaskRay added inline comments.Jun 22 2020, 5:09 PM
lld/test/ELF/relocation-plt32-aarch64.s
1 ↗(On Diff #272548)

This file can be folded into aarch64-plt32.s (I would name it aarch64-reloc-plt32.s)

leonardchan marked 12 inline comments as done.
leonardchan added inline comments.
lld/test/ELF/aarch64-undefined-weak-plt32.s
1 ↗(On Diff #272548)

Looks like there was already aarch64-undefined-weak.s so I moved this to there.

Thanks for the update, I don't have any more comments. Happy when MaskRay is happy.

MaskRay accepted this revision.Jun 23 2020, 3:00 PM

LGTM.

lld/test/ELF/aarch64-range-thunk-extension-plt32.s
4

Nit: use '

" can appear in linker scripts and using ' can avoid extra escape in some cases.

25

Indent this line

This revision is now accepted and ready to land.Jun 23 2020, 3:00 PM
leonardchan marked 2 inline comments as done.
This revision was automatically updated to reflect the committed changes.