Page MenuHomePhabricator

[ELF][PPC64] Detect missing R_PPC64_TLSGD/R_PPC64_TLSLD and disable TLS relaxation
ClosedPublic

Authored by MaskRay on Dec 9 2020, 11:55 AM.

Details

Summary

Alternative to D91611.

The TLS General Dynamic/Local Dynamic code sequences need to mark
__tls_get_addr with R_PPC64_TLSGD or R_PPC64_TLSLD, e.g.

addis r3, r2, x@got@tlsgd@ha # R_PPC64_GOT_TLSGD16_HA
addi r3, r3, x@got@tlsgd@l   # R_PPC64_GOT_TLSGD16_LO
bl __tls_get_addr(x@tlsgd)   # R_PPC64_TLSGD followed by R_PPC64_REL24
nop

However, there are two deviations form the above:

  1. direct call to __tls_get_addr. This is essential to implement ld.so in glibc/musl/FreeBSD.
bl __tls_get_addr
nop

This is only used in a -shared link, and thus not subject to the GD/LD to IE/LE
relaxation issue below.

  1. Missing R_PPC64_TLSGD/R_PPC64_TLSGD for compiler generated TLS references

According to Stefan Pintille, "In the early days of the transition from the
ELFv1 ABI that is used for big endian PowerPC Linux distributions to the ELFv2
ABI that is used for little endian PowerPC Linux distributions, there was some
ambiguity in the specification of the relocations for TLS. The GNU linker has
implemented support for correct handling of calls to __tls_get_addr with a
missing relocation. Unfortunately, we didn't notice that the IBM XL compiler
did not handle TLS according to the updated ABI until we tried linking XL
compiled libraries with LLD."

In short, LLD needs to work around the old IBM XL compiler issue.
Otherwise, if the object file is linked in -no-pie or -pie mode,
the result will be incorrect because the 4 instructions are partially
rewritten (the latter 2 are not changed).

Work around the compiler bug by disable General Dynamic/Local Dynamic to
Initial Exec/Local Exec relaxation. Note, we also disable Initial Exec
to Local Exec relaxation for implementation simplicity, though technically it can be kept.

ppc64-tls-missing-gdld.s demonstrates the updated behavior.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > lld.ELF::ppc64-options.s
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /mnt/disks/ssd0/agent/llvm-project/lld/test/ELF/ppc64-options.s -o /mnt/disks/ssd0/agent/llvm-project/build/tools/lld/test/ELF/Output/ppc64-options.s.tmp
80 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll
150 msx64 windows > lld.ELF::ppc64-options.s
Script: -- : 'RUN: at line 3'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llvm-mc.exe -filetype=obj -triple=x86_64-unknown-linux C:\ws\w16n2-1\llvm-project\premerge-checks\lld\test\ELF\ppc64-options.s -o C:\ws\w16n2-1\llvm-project\premerge-checks\build\tools\lld\test\ELF\Output\ppc64-options.s.tmp

Event Timeline

MaskRay created this revision.Dec 9 2020, 11:55 AM
MaskRay requested review of this revision.Dec 9 2020, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 11:55 AM
MaskRay updated this revision to Diff 310618.Dec 9 2020, 12:41 PM

Improve tests

MaskRay added inline comments.Dec 9 2020, 4:46 PM
lld/test/ELF/ppc64-options.s
1

I'll move it to target-specific-options.s

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

lld/ELF/Relocations.cpp
1566

I'm not sure what the code after the switch is for.

1568

The issue is that we don't actually check for calls to __tls_get_addr.
We just check that if there exists some R_PPC64_GOT_TLS... we also have at least one R_PPC64_TLSLD/R_PPC64_TLSGD. My concern with this is that we can have a place where the relocations are specified correctly and another place where they are not and this is not going to be detected.

MaskRay updated this revision to Diff 311056.Dec 10 2020, 4:17 PM
MaskRay marked an inline comment as done.
MaskRay retitled this revision from [ELF][PPC64] Add --fix-ppc64-tls-reloc to detect and fix missing R_PPC64_TLSGD/R_PPC64_TLSLD to [ELF][PPC64] Detect missing R_PPC64_TLSGD/R_PPC64_TLSLD and disable TLS relaxation.
MaskRay edited the summary of this revision. (Show Details)

Delete the option

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

lld/ELF/Relocations.cpp
1566

Thanks for the catch. It is unneeded.

MaskRay updated this revision to Diff 311059.Dec 10 2020, 4:23 PM

Delete unused variable

MaskRay updated this revision to Diff 311061.Dec 10 2020, 4:25 PM

Improve test

Harbormaster completed remote builds in B81933: Diff 311059.

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

I don't want to implement a complete fix of this. I am completely happy with just detecting the issue and disabling the relaxation if we find it.
However, if we have a scenario like this one:

Right:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr(x@tlsgd)
  nop
  blr

Wrong:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr
  nop
  blr

the function Wrong won't be caught by the code that you posted. We basically have two functions, one was compiled correctly and the other was not.

In the code that you have what if you just look for R_PPC64_REL24 and check those relocations for __tls_get_addr? I know that this adds more overhead but it helps with the backwards compatibility and I feel that is important. Also, if it adds too much overhead we can keep the option and that way we only do this extra check if the user asks for it.

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

I don't want to implement a complete fix of this. I am completely happy with just detecting the issue and disabling the relaxation if we find it.
However, if we have a scenario like this one:

Right:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr(x@tlsgd)
  nop
  blr

Wrong:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr
  nop
  blr

the function Wrong won't be caught by the code that you posted. We basically have two functions, one was compiled correctly and the other was not.

In the code that you have what if you just look for R_PPC64_REL24 and check those relocations for __tls_get_addr? I know that this adds more overhead but it helps with the backwards compatibility and I feel that is important. Also, if it adds too much overhead we can keep the option and that way we only do this extra check if the user asks for it.

On second thought. Don't worry about this. :)
The chance that the same object file will have a combination of good and bad code gen is so low that it is probably not worth the trouble to try to find it.

I think this patch is on the right track!
Thank you for your help MaskRay!

Missing R_PPC64_TLSGD/R_PPC64_TLSGD for one function and correct for another in the same translation unit - does this only happen with relocatable links? I'd hope we simply consider this too niche to fix and don't add more logic on this...

Missing R_PPC64_TLSGD/R_PPC64_TLSGD for one function and correct for another in the same translation unit - does this only happen with relocatable links? I'd hope we simply consider this too niche to fix and don't add more logic on this...

I agree this is too niche. We don't need to add anything for it.

stefanp accepted this revision.Dec 17 2020, 3:46 AM

Tested this on some actual legacy compiled code and it does what we want.
Only one minor question. Otherwise, LGTM.

Thank you MaskRay!

lld/test/ELF/ppc64-tls-missing-gdld.s
3

I see that the BE test was removed. Given that this test is getting bigger I think this is fine.

98

Is this to get rid of the warning for _start ?

This revision is now accepted and ready to land.Dec 17 2020, 3:46 AM
MaskRay marked an inline comment as done.Dec 17 2020, 9:04 AM
MaskRay added inline comments.
lld/test/ELF/ppc64-tls-missing-gdld.s
98

Yes. Otherwise the test needs to write -e 0 or specify another entry point.

Few comments/suggestions.

lld/ELF/Relocations.cpp
1541

You probably don't need to execute the body when sec.file->ppc64DisableTLSRelax is already true?
My concern is that scanning over all relocations twice can be slow.

1554

I'd probably exit the loop early when we have a "marker".
I guess it might look nicer if you move this code to a helper function like checkPPC64Relaxations.

if (config->emachine == EM_PPC64)
  checkPPC64Relaxations(...)
1570
lld/test/ELF/ppc64-tls-missing-gdld.s
19

It reads like the relaxation was disabled for a particular place.
I.e the message contains the file name and the section name (what is good), but I think the message could mention that the relaxation was disabled for a whole object.

MaskRay updated this revision to Diff 312820.Dec 18 2020, 9:00 AM
MaskRay marked 6 inline comments as done.

comments

MaskRay updated this revision to Diff 312822.Dec 18 2020, 9:03 AM

Improve diagnostic

lld/test/ELF/ppc64-tls-missing-gdld.s
19

I'll drop the section name. The diagnostic can mention R_PPC64_GOT_TLS* then the user should know readelf -r should be used to invesigate the issue.

grimar accepted this revision.Dec 21 2020, 12:42 AM

LGTM