Page MenuHomePhabricator

[PowerPC] Add support for R_PPC64_GOT_TPREL_PCREL34 used in TLS Initial Exec
ClosedPublic

Authored by stefanp on Aug 31 2020, 12:51 PM.

Details

Summary

Add Thread Local Storage Initial Exec support to LLD.

This patch adds the computation for the relocations as well as the relaxation from Initial Exec to Local Exec.

Initial Exec:

pld r9, x@got@tprel@pcrel
add r9, r9, x@tls@pcrel

or

pld r9, x@got@tprel@pcrel
lbzx r10, r9, x@tls@pcrel

Note that @tls@pcrel is actually encoded as R_PPC64_TLS with a one byte displacement.

For the above examples relaxing Intitial Exec to Local Exec:

paddi r9, r9, x@tprel
nop

or

paddi r9, r13, x@tprel
lbz r10, 0(r9)

Diff Detail

Event Timeline

stefanp created this revision.Aug 31 2020, 12:51 PM
stefanp requested review of this revision.Aug 31 2020, 12:51 PM
stefanp edited the summary of this revision. (Show Details)
NeHuang added inline comments.Sep 1 2020, 12:47 PM
lld/ELF/Arch/PPC64.cpp
867

I am not sure if the relocation we have here will be either 4 byte aligned or offset by one byte. Seems all the code here is based on the relocation is offset by one byte. Do we need to add an error if the offset is not offset by 1 byte?

1274

Can we combine these 3 relocation cases with R_PPC64_PCREL34?

lld/test/ELF/Inputs/ppc64-tls-vardef.s
1 ↗(On Diff #289009)

We can either use llvm-mc --defsym AUX=1 or extract to merge the variable definitions in ppc64-tls-pcrel-ie.s

MaskRay added inline comments.Sep 1 2020, 2:16 PM
lld/ELF/Arch/PPC64.cpp
844

In some recent code we add const whenever appropriate

847

Remove this trivial variable. Just inline its use

853

The variable can be removed.

lld/test/ELF/ppc64-tls-pcrel-ie.s
67

You can move instruction dump immediate above their related .section

Labels need to end with :

stefanp updated this revision to Diff 289784.Sep 3 2020, 12:21 PM

Removed a couple of variables that could be inlined.
Added an error in case the offset from the 4 byte alignment is 2 or 3.
Moved checks in the test case.

stefanp retitled this revision from [PowerPC] Add support for TLS Initial Exec to [PowerPC] Add support for R_PPC64_GOT_TPREL_PCREL34 used in TLS Initial Exec.Sep 3 2020, 12:22 PM
nemanjai accepted this revision.Sep 10 2020, 5:02 AM

Please mention the relevant sections of the ABI in the description (i.e. where it mentions the 1-byte offset). Other than that and the inline nits, LGTM. Please give @MaskRay some time to have another look before committing.

lld/ELF/Arch/PPC64.cpp
864

s/form/from

866

We have three calls to read32(loc - 1). I think that is a valid reason to have a temporary var.

870

Elaborate slightly. Why can it be turned into a nop? Presumably because the address computed by the pld points to the actual symbol now. Also, I imagine can should really be must.

This revision is now accepted and ready to land.Sep 10 2020, 5:02 AM
MaskRay requested changes to this revision.Sep 10 2020, 2:00 PM

Seems that this require a dependent patch to land first. I can only get around to it after llvm-mc can assemble the special form R_PPC64_TLS. Requested changes so that I will not forget it.

lld/ELF/Arch/PPC64.cpp
845

pld is used only once and can be inlined

852

locAstInt % 4 == 0

862

locAsInt % 4 == 1

871
876

Consider errorOrWarn whenever possible.

881

How is this conveyed in the ABI document (I don't have a copy)?

Is it: r_offset has to be 0 or 1 modulo 4?

Note that bufLoc is buffer address plus r_offset. bufLoc can be aligned while r_offset isn't.

lld/test/ELF/ppc64-tls-pcrel-ie.s
12

If a linked shared object is used to link another file, the shared object needs to specify --soname=t or another fixed string. Otherwise the DT_SONAME field of the other file is dependent on the full path of %t2.so, which can vary on different test machines.

This revision now requires changes to proceed.Sep 10 2020, 2:00 PM
MaskRay added inline comments.Sep 10 2020, 2:00 PM
lld/test/ELF/ppc64-tls-pcrel-ie.s
11

Consider split-file

stefanp updated this revision to Diff 291307.Sep 11 2020, 12:05 PM

Fixed formatting and comments in a few places.
Added --soname to the test file.

lld/ELF/Arch/PPC64.cpp
867

You are correct. I should add an error here for the cases where the offset is 2 or 3 bytes offset.

881

From the ABI:
Section 3.7.3.3. Initial Exec TLS Model
Here is a paragraph from that section:

Note that both the x@tls and x@tls@pcrel assembly forms are annotated with R_PPC64_TLS
relocations. To distinguish between the two, the second of these has a field value displaced by one
byte from the beginning of the instruction.

So the idea is that R_PPC64_TLS is either aligned with the beginning of the instruction or it is off by 1 byte. On PowerPC all of the instructions are either 4 or 8 bytes so all instructions are at least 4 byte aligned. Therefore, if this relocation is 4 byte aligned it will match the beginning of an instruction.

1274

We can.
However, I don't think that this patch is the right place to do that. This is really just unrelated code cleanup. I would prefer to do it after in an NFC patch.

lld/test/ELF/Inputs/ppc64-tls-vardef.s
1 ↗(On Diff #289009)

I was actually planning to use this in another test in a later patch that requires a similar setup. As a result I think I prefer to use a separate file for now.

lld/test/ELF/ppc64-tls-pcrel-ie.s
11

Sorry, I'm not sure what you mean.
Do you want me to split the SHARE and STATIC parts of this test into two tests?

stefanp updated this revision to Diff 292864.Sep 18 2020, 12:08 PM

Updated the patch to use the NOP constant.

@MaskRay
FYI: All of the dependent patches have landed so you should now be able to just apply this patch on top of trunk.

MaskRay added inline comments.Sep 18 2020, 9:25 PM
lld/ELF/Arch/PPC64.cpp
883

errorOrWarn

lld/test/ELF/Inputs/ppc64-tls-vardef.s
3 ↗(On Diff #292864)

You can delete .size and move this snippet to the main file with RUN: split-file

lld/test/ELF/ppc64-tls-pcrel-ie.s
24

You can omit for lld as the whole directory is testing lld.

25

Suggest: IE and LE instead of SHARED and STATIC.

It is normally incorrect to use IE for -shared. It is for very narrow cases where dlopen is not considered and the TLS size is small.

93

Please remove unneeded instructions like add and clrldi across the file

stefanp updated this revision to Diff 293177.Sep 21 2020, 7:59 AM

Updated error to errorOrWarn.
Merged the two test files.
Removed extra assembly from test case.

lld/ELF/Arch/PPC64.cpp
883

Sorry...
I keep missing these.

MaskRay added inline comments.Sep 21 2020, 9:54 AM
lld/test/ELF/ppc64-tls-pcrel-ie.s
10

Since you are using split-file, you can make the linker script a fragment, e.g. linkerscript/noload.s

17

.got content should be tested as well. See riscv-tls-ie.s

stefanp updated this revision to Diff 293229.Sep 21 2020, 12:15 PM

Added the linker script to the test file.
Added a check for the GOT.

MaskRay accepted this revision.Sep 21 2020, 12:24 PM

Looks great!

This revision is now accepted and ready to land.Sep 21 2020, 12:24 PM