Page MenuHomePhabricator

[PPC64] Set the number of relocations processed for R_PPC64_TLS[GL]D to 2
ClosedPublic

Authored by MaskRay on Feb 4 2019, 2:08 AM.

Details

Summary

R_PPC64_TLSGD and R_PPC64_TLSLD are used as markers on TLS code sequences. For GD-to-IE and GD-to-LE relaxations, the next relocation R_PPC64_REL24 should be skipped to avoid "undefined symbol" (__tls_get_addr) error when linking statically.

R_PPC64_GOT_TLSGD16_HA
R_PPC64_GOT_TLSGD16_LO
R_PPC64_TLSGD R_TLSDESC_CALL
R_PPC64_REL24 __tls_get_addr

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Feb 4 2019, 2:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 185013.Feb 4 2019, 2:16 AM

drop an inadvertent change

MaskRay added a comment.EditedFeb 4 2019, 2:18 AM

Not directly related to the issue but I want to know whether it is allowed to have other relocation types between R_PPC64_GOT_TLSGD16_HA and R_PPC64_GOT_TLSGD16_LO. Below is one example:

1748:       00 00 62 3c     addis   r3,r2,0
                    1748: R_PPC64_GOT_TLSGD16_HA    foo
174c:       80 00 9f f8     std     r4,128(r31)
1750:       00 00 88 38     addi    r4,r8,0
                    1750: R_PPC64_TOC16_LO  .rodata.str1.1+0x364
1754:       00 00 63 38     addi    r3,r3,0
                    1754: R_PPC64_GOT_TLSGD16_LO    foo
1758:       78 00 9f f8     std     r4,120(r31)
175c:       01 00 00 48     bl      175c <foo+0x1dc>
                    175c: R_PPC64_TLSGD     foo
                    175c: R_PPC64_REL24     __tls_get_addr

If this is allowed. Is there any restriction on the TLS code sequence? e.g. Is it allowed to omit R_PPC64_GOT_TLSGD16_HA or swap the order of some sequences?

If this is allowed. Is there any restriction on the TLS code sequence? e.g. Is it allowed to omit R_PPC64_GOT_TLSGD16_HA?

In general other relocations are allowed to show up in-between the TLS relocations. You can omit the R_PPC64_GOT_TLSGD16_HA relocation and use a R_PPC64_GOT_TLSGD16 instead of the _LO. Clang and xlc don't do that but gcc will with small code model (clang not doing it is likely a bug, xlc doesn't support small code model for V2 abi).

or swap the order of some sequences?

I hadn't considered this before. I don't belive there is an abi guarantee that the HA/HI comes before the low so something like:

addi 3, 2, i@got@tlsgd@l
addis 3, 3, i@got@tlsgd@ha
bl __tls_get_addr(i@tlsgd)
nop

Is potentially valid, while the tls optimizations we produce in lld are likely invlaid :(. I'll have to look into this a bit deeper.

sfertile accepted this revision.Feb 4 2019, 9:22 AM

LGTM other then 1 minor comment.

ELF/Arch/PPC64.cpp
246 ↗(On Diff #185011)

I think we should describe it a bit more in depth as its not clear why we skip 2 unless you are familiar with PPC64 tls already. maybe along the lines of:

The calls to __tls_get_addr are marked with 2 relocations on the call instructions. The first is a TLS hint to tie the call to its tls related arguments,  the second relocation is the R_PPC64_REL24 used to speicfy the target of the call. When we relax the tls sequnce we no longer need the call and should skip both relocations to not create a false dependence on __tls_get_addr being defined.
This revision is now accepted and ready to land.Feb 4 2019, 9:22 AM
MaskRay updated this revision to Diff 185212.Feb 4 2019, 6:24 PM

Update comment

MaskRay updated this revision to Diff 185213.Feb 4 2019, 6:26 PM
MaskRay marked an inline comment as done.

Fix a typo

MaskRay marked an inline comment as done and an inline comment as not done.Feb 4 2019, 6:27 PM
MaskRay added inline comments.
ELF/Arch/PPC64.cpp
246 ↗(On Diff #185011)

Added a comment with your suggestion in mind. Does it look good?

sfertile added inline comments.Feb 5 2019, 8:49 AM
ELF/Arch/PPC64.cpp
246 ↗(On Diff #185011)

Yeah, that's perfect.

MaskRay updated this revision to Diff 185459.Feb 5 2019, 5:49 PM
MaskRay marked an inline comment as not done.

Rebase

ruiu accepted this revision.Feb 5 2019, 5:52 PM

LGTM

This revision was automatically updated to reflect the committed changes.