[PATCH] D92959: [ELF][PPC64] Detect missing R_PPC64_TLSGD/R_PPC64_TLSLD and disable TLS relaxation

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

In D92959#2449603 <https://reviews.llvm.org/D92959#2449603>, @stefanp wrote:

> In D92959#2447405 <https://reviews.llvm.org/D92959#2447405>, @MaskRay wrote:
>
>> In D92959#2447003 <https://reviews.llvm.org/D92959#2447003>, @stefanp wrote:
>>
>>> 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 at got@tlsgd at ha
>     addi 3, 3, x at got@tlsgd at l
>     bl __tls_get_addr(x at tlsgd)
>     nop
>     blr
>   
>   Wrong:
>     addis 3, 2, x at got@tlsgd at ha
>     addi 3, 3, x at got@tlsgd at 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92959/new/

https://reviews.llvm.org/D92959