Page MenuHomePhabricator

[PowerPC][LLD] Support for PC Relative TLS for Local Dynamic
ClosedPublic

Authored by stefanp on Sep 11 2020, 3:55 AM.

Details

Summary

Add support to LLD for PC Relative Thread Local Storage for Local Dynamic.
This patch adds support for two relocations: R_PPC64_GOT_TLSLD_PCREL34 and
R_PPC64_DTPREL34.

The Local Dynamic code is:

pla r3, x@got@tlsld@pcrel        R_PPC64_GOT_TLSLD_PCREL34
bl __tls_get_addr@notoc(x@tlsld) R_PPC64_TLSLD
                                 R_PPC64_REL24_NOTOC
...
paddi r9, r3, x@dtprel           R_PPC64_DTPREL34

After relaxation to Local Exec:

paddi r3, r13, 0x1000
nop
...
paddi r9, r3, x@dtprel          R_PPC64_DTPREL34

Diff Detail

Event Timeline

stefanp created this revision.Sep 11 2020, 3:55 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
stefanp requested review of this revision.Sep 11 2020, 3:55 AM
MaskRay added inline comments.Sep 11 2020, 10:14 AM
lld/ELF/Arch/PPC64.cpp
819

locAsInt % 4 == 0

820

NOP

lld/test/ELF/ppc64-tls-pcrel-ld.s
53

misaligned.

Use 2 space indentation everywhere?

stefanp updated this revision to Diff 291569.Sep 14 2020, 7:28 AM

Fixed formatting in source file.
Fixed spacing in test file.

stefanp updated this revision to Diff 291571.Sep 14 2020, 7:34 AM

Replace error() with errorOrWarn().

MaskRay added inline comments.Sep 14 2020, 9:58 AM
lld/test/ELF/ppc64-tls-pcrel-ld.s
39

It is important to check .got

See riscv-tls-ld.s

MaskRay added inline comments.Sep 14 2020, 10:05 AM
lld/ELF/Arch/PPC64.cpp
797

Use the constant NOP

802

This can be errorOrWarn as well

We sometimes use error/fatal when we want to exit prematurely because some constraints makes it difficult not to do so. For recoverable errors, use errorOrWarn

stefanp updated this revision to Diff 292782.Sep 18 2020, 7:14 AM

Updated test case to check for .got.
Used warningOrErr.
Used NOP.

NeHuang added inline comments.Sep 24 2020, 9:56 AM
lld/ELF/Arch/PPC64.cpp
792

nit: please add the comment to elaborate Relax from ... to ... for PCRel LD to LE relaxation.

792

Do we need to add , 0x1000 in the comment?

797

nit: // nop

800

nit: // nop

lld/test/ELF/ppc64-tls-pcrel-ld.s
38

missing relocation for R_PPC64_TPREL34?

stefanp updated this revision to Diff 294389.Sep 25 2020, 11:58 AM

Rebased this patch on top of master and on top of the updated D87318.
This patch does depend on D87318.

Also added more comments and addressed a number of nits.

NeHuang added inline comments.Mon, Sep 28, 1:02 PM
lld/ELF/Arch/PPC64.cpp
801

nit: remove to

stefanp updated this revision to Diff 295846.Fri, Oct 2, 9:09 AM

Rebased to top of trunk.
Fixed a couple of comments.
Removed some lines that are not required form the test.

lld/ELF/Arch/PPC64.cpp
792

I think we should add the constant. The way that the relaxation works is that we will always be using that same constant in this situation so I figured it should be there. However, I will add the , 0 at the end so show that we are not using the PC Rel version of the instruction.

lld/test/ELF/ppc64-tls-pcrel-ld.s
38

I don't believe that this relocation would appear in the final output file. The linker can compute that value and just fill in the instruction.

All of the dependencies of this patch have now been committed.

stefanp edited the summary of this revision. (Show Details)Fri, Oct 2, 9:14 AM

LGTM but of course please wait to hear from @sfertile/@MaskRay before committing it.

NeHuang accepted this revision.Mon, Oct 5, 12:34 PM
This revision is now accepted and ready to land.Mon, Oct 5, 12:34 PM
sfertile added inline comments.Tue, Oct 13, 8:58 AM
lld/ELF/Arch/PPC64.cpp
1325

Do we not need to adjust the value being relocated by the dynamic thread pointer bias?

From the ISA:

Each DTV pointer points 0x8000 bytes past the start of each TLS block. (For implementation
reasons, the actual value stored in the DTV may point to the start of a TLS block. However, values
returned by accessor functions will be offset by 0x8000 bytes.)
stefanp added inline comments.Tue, Oct 13, 10:25 AM
lld/ELF/Arch/PPC64.cpp
1325

Yes, you are correct we are off by an offset of 0x8000.
Thank you for finding this!

stefanp updated this revision to Diff 297911.Tue, Oct 13, 10:53 AM

Fixed the issue where the 0x8000 offset was not being considered.

sfertile added inline comments.Tue, Oct 13, 1:56 PM
lld/ELF/Arch/PPC64.cpp
793

missing the 'r' on the registers.

794

Real minor nit: No need to duplicate the comment on this line.

798

Real minor nit: move the 'to' to the next line. It matches how you structured the comment on the toc case, and helps to delineate the original code from the optimized code.

1329

Would you consider having the R_PPC64_DTPREL34 case come first and adjust the value, then fall through to the common case?

1330

We have a constant for this already: dynamicThreadPointerOffset.

stefanp updated this revision to Diff 298346.Thu, Oct 15, 4:01 AM

Fixed a couple of comments.
Moved the R_PPC64_DTPREL34 case first and adjusted the dynamic thread offset
there before falling through to the common case.

sfertile added inline comments.Fri, Oct 16, 12:22 PM
lld/ELF/Arch/PPC64.cpp
1319

LLVM_FALLTHROUGH;

lld/test/ELF/ppc64-tls-pcrel-ld.s
3

Is using split file the preferred way to do this now?

11

missing --soname

36

Am I reading/interpreting this correctly?

The .got section is at address: 0x01004150, This is the first word of hex.
The first entry is the .TOC. symbol with a value of .got + 0x8000 = 0x000000000100c150. This is the second and third words of hex.
Then there are 2 words of zeros --> This is the ti_module field of the struct tls_index for local tls symbols for this module.

Then output ends. I would expect 2 more words of zeros for the ti_offset field, although maybe readobj is truncating the output, or we are missing a line?

Also shouldn't the relocation 0000000001004150 0000000000000044 R_PPC64_DTPMOD64 0
Be relocating address 0x1004158, since that is the address of the e ti_module field? Right now it seems to me like it would modify the toc-base value instead of filing in the second entry with the module tls id ...

stefanp updated this revision to Diff 299363.Tue, Oct 20, 7:55 AM

Fixed the LLVM_FALLTHROUGH
Added split-file and -soname to the test file.

Added a .TOC. symbol to the test file. This addition is a workaround for the
GOT offsets that are incorrect. This is a bug that was noticed in this test case
but has existed previous to this patch and should be fixed in a later patch.

sfertile accepted this revision.Thu, Oct 22, 1:48 PM
sfertile added inline comments.
lld/test/ELF/ppc64-tls-pcrel-ld.s
63

Minor nit: use '##' for comments.

I suggest tweaking the comment a bit:

Adding a reference to .TOC. since LLD doesn't gracefully handle the case where we define a .got section but have no references to
the toc base yet.
stefanp updated this revision to Diff 300258.Fri, Oct 23, 6:06 AM

Updated comment in test case.

This revision was landed with ongoing or failed builds.Fri, Oct 23, 6:24 AM
This revision was automatically updated to reflect the committed changes.