This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PowerPC] Add support for R_PPC64_GOT_TLSGD_PCREL34 used in TLS General Dynamic
ClosedPublic

Authored by stefanp on Sep 8 2020, 11:50 AM.

Details

Summary

Add Thread Local Storage support for the 34 bit relocation R_PPC64_GOT_TLSGD_PCREL34 used in General Dynamic.

The compiler will produce code that looks like:

pla r3, x@got@tlsgd@pcrel            R_PPC64_GOT_TLSGD_PCREL34
bl __tls_get_addr@notoc(x@tlsgd)     R_PPC64_TLSGD
                                     R_PPC64_REL24_NOTOC

LLD should be able to correctly compute the relocation for R_PPC64_GOT_TLSGD_PCREL34 as well as do the following two relaxations where possible:
General Dynamic to Local Exec:

paddi r3, r13, x@tprel
nop

and General Dynamic to Initial Exec:

pld r3, x@got@tprel@pcrel
add r3, r3, r13

Note:
This patch adds support for the PC Relative (no TOC) version of General Dynamic on top of the existing support for the TOC version of General Dynamic.
The ABI does not provide any way to tell by looking only at the relocation R_PPC64_TLSGD when it is being used in a TOC instruction sequence or and when it is being used in a no TOC sequence. The TOC sequence should always be 4 byte aligned. This patch adds one to the offset of the relocation when it is being used in a no TOC sequence. In this way LLD can tell by looking at the alignment of the offset of R_PPC64_TLSGD whether or not it is being used as part of a TOC or no TOC sequence.

Diff Detail

Event Timeline

stefanp created this revision.Sep 8 2020, 11:50 AM
stefanp requested review of this revision.Sep 8 2020, 11:50 AM

Are the mentions of R_PPC64_GOT_TLSGD34 in the description typos of some sort? In the code, there are only mentions of R_PPC64_GOT_TLSGD_PCREL34.

If I'm not mistaken, this patch is dependent on another one. If so, please add the dependency in the review.

lld/ELF/Arch/PPC64.cpp
1368

Maybe just return expr? Or we can simply omit this and change the below condition to
if (type != R_PPC64_GOT_TLSGD_PCREL34 && expr == R_RELAX_TLS_GD_TO_IE)

lld/ELF/Relocations.cpp
1363

I think you should mention here that we look at the next relocation to see if it is a NOTOC relocation.

Does this depend on D83404 or D86608?

stefanp edited the summary of this revision. (Show Details)Sep 11 2020, 2:00 PM
stefanp updated this revision to Diff 291337.EditedSep 11 2020, 2:36 PM

I've updated the description to only use the correct version of the relocation: R_PPC64_GOT_TLSGD_PCREL34.

I'm sorry that I didn't add the patches that this depends on. I've added all three patches now as parents to this patch.

I updated a comment and simplified one of the if conditions.

MaskRay added inline comments.Sep 11 2020, 11:05 PM
lld/ELF/Relocations.cpp
1350

I am not sure we want to add error checking for every relocation usage. We should add them considering the possibility of such errors (can a compiler/tool/assembly generate such erroneous relocation usage?)

stefanp added inline comments.Sep 14 2020, 6:12 AM
lld/ELF/Relocations.cpp
1350

I do not believe that the compiler can generate either of these situations from C source code.

I can generate the bad alignment in assembly (by adding .space 1 for example) but it is not likely that someone will do that.

In general there are a couple of reasons why I add code like this.
First, it is to prevent future development work from creating hidden bugs. Maybe a little like
https://llvm.org/docs/CodingStandards.html#assert-liberally
from a philosophical perspective. I know that these are not asserts but I think the argument remains valid...
Second, it is my way of documenting assumptions in the code.

I understand your point of view. You don't want want half the code to be error checking so if you really want me to remove these two errors I can. I just wanted to give you my perspective on this. Let me know what you think!

stefanp updated this revision to Diff 293596.Sep 22 2020, 5:18 PM

Rebased this patch to Top of Trunk.
Fixed all of the error to errorOrWarn.
Made use of NOP.

This patch should now no longer have any dependencies.

stefanp updated this revision to Diff 293597.Sep 22 2020, 5:20 PM

Fixed spacing that I missed.

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

nit: can we add the comment to elaborate Relax from ... to ... ?

749

ditto

759

nit: clang-format needed.

1410

nit: can we add more comments as existing cases like // Relax from ...... to .....?

1413

return?

1415

ditto, add the comment Relax from ... to ...

lld/ELF/Relocations.cpp
1354

Will the return be executed after the errorOrWarn?

1360

ditto

lld/test/ELF/ppc64-tls-pcrel-gd.s
68

Are we missing the relocation R_PPC64_TPREL34 here for GDTOLE?

144

alignment issue or phabricator display issue?

182

ditto

sfertile added inline comments.Sep 24 2020, 10:07 AM
lld/ELF/Arch/PPC64.cpp
1276

It looks like these 2 cases and R_PPC64_GOT_PCREL34 et al share the same implementation, we should combine them.

lld/ELF/Relocations.cpp
1350

We dereference 'i' so we need the check that i is still valid. The validation of the offset is less critical and could be omitted or converted to an assert.

stefanp updated this revision to Diff 294345.Sep 25 2020, 9:13 AM

Addressed a number of nits.
Added an NFC change before this patch and rebased this patch.

lld/ELF/Arch/PPC64.cpp
1276

Initially I wanted to do this after all of the LLD patches were in. However, you are now the second person to ask for this so I'm going do it now.
I'm going to do this in an NFC patch and then rebase this patch on top.

lld/ELF/Relocations.cpp
1350

In that case I'll keep the check on i but I'll remove the offset check.

1354

Yes it could be executed. The function errorOrWarn will sometimes exit and it will sometimes return back here.

1360

Same as above. We may execute the return.

lld/test/ELF/ppc64-tls-pcrel-gd.s
68

That relocation should not be in the final linked object file. The linker can compute the value of it and just modify the immediate in the instruction.

NeHuang accepted this revision.Sep 28 2020, 12:43 PM

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

This revision is now accepted and ready to land.Sep 28 2020, 12:43 PM
MaskRay requested changes to this revision.Sep 28 2020, 1:20 PM
MaskRay added inline comments.
lld/ELF/Arch/PPC64.cpp
731

The comment on this line is redundant.

745

Delete parens around %. ditto below

754

The comment is redundant

1427

Delete parens.

1432

User lower-case hexidecimal literals.

lld/ELF/Relocations.cpp
1364

config->isMips64EL -> /*isMips64EL=*/false

1365

The coding standard prefers ++offset

lld/test/ELF/ppc64-tls-pcrel-gd.s
185

Delete the trailing unrelated instructions

This revision now requires changes to proceed.Sep 28 2020, 1:20 PM
stefanp updated this revision to Diff 294997.Sep 29 2020, 8:05 AM

Updated a number of formatting issues.
Updated the test case to remove instructions that are not needed.

MaskRay added inline comments.Sep 29 2020, 9:39 AM
lld/test/ELF/ppc64-tls-pcrel-gd.s
169

This function does not add coverage. Consider deleting it

stefanp updated this revision to Diff 295327.EditedSep 30 2020, 9:35 AM

Removed the last function from the test assembly file.

MaskRay added inline comments.Sep 30 2020, 10:25 AM
lld/test/ELF/ppc64-tls-pcrel-gd.s
109

It seems that GDAddr vs GDVal does not add test coverage as well.

I think the test can be simply:

paddi 3, 0, x@got@tlsgd@pcrel, 1
bl __tls_get_addr@notoc(x@tlsgd)
paddi 4, 0, y@got@tlsgd@pcrel, 1
bl __tls_get_addr@notoc(y@tlsgd)
blr
stefanp updated this revision to Diff 295356.Sep 30 2020, 11:05 AM

Further reduced the test case.

sfertile added inline comments.Sep 30 2020, 1:14 PM
lld/ELF/Arch/PPC64.cpp
1276

Thanks.

lld/ELF/Relocations.cpp
1358

The first line does a good job explaining what we are doing, the following then somewhat repeats it. I think we can pare this comment down without losing much:

// Offset the 4-byte aligned R_PPC64_TLSGD by one byte in the NOTOC case, so we can discern it later from the toc-case.
MaskRay accepted this revision.Sep 30 2020, 1:21 PM

LGTM. Worth checking with @sfertile in case he has more comments

This revision is now accepted and ready to land.Sep 30 2020, 1:21 PM
stefanp updated this revision to Diff 295526.Oct 1 2020, 4:18 AM

Updated the comment.

sfertile accepted this revision.Oct 1 2020, 6:53 AM

LGTM also.

This revision was landed with ongoing or failed builds.Oct 1 2020, 11:01 AM
This revision was automatically updated to reflect the committed changes.
stefanp reopened this revision.Oct 1 2020, 12:07 PM

Reopening this to see why the test failed.

This revision is now accepted and ready to land.Oct 1 2020, 12:07 PM

Looks like .rela.dyn is located in different places depending on the machine.
From two different bots:

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10128 contains 2 entries:

or

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10148 contains 2 entries:

Looks like .rela.dyn is located in different places depending on the machine.
From two different bots:

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10128 contains 2 entries:

or

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10148 contains 2 entries:

I mentioned this several times previously. If you use a shared object for another link, please set -soname=t or another fixed string.

Ok, thank you MaskRay.
Sorry, in my head I didn't put the error together with what you had said previously.