[PATCH] D91611: [PowerPC][LLD] Detecting and fixing missing TLS relocation on __tls_get_addr

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D91611#2417611 <https://reviews.llvm.org/D91611#2417611>, @stefanp wrote:

> I have done some testing with `gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0` and `GNU ld (GNU Binutils for Ubuntu) 2.30`.
>
> It appears that `ld.bfd` by default fixes up the relocation if it is missing and then does the relaxation correctly. I don't need to specify any options for this to be done.
> As a simple example:
>
>   Test1:
>   .LCF0:
>     addis 2,12,.TOC.-.LCF0 at ha
>     addi 2,2,.TOC.-.LCF0 at l
>     .localentry     Test1,.-Test1
>     addis 3,2,x at got@tlsgd at ha
>     addi 3,3,x at got@tlsgd at l
>     bl __tls_get_addr
>     nop
>
> The `bl __tls_get_addr` is missing the relocation `ld.bfd` still relaxes it correctly.
> However, with a more complex example that I wrote by hand:
>
>   Test1:
>   .LCF0:
>     addis 2,12,.TOC.-.LCF0 at ha
>     addi 2,2,.TOC.-.LCF0 at l
>     .localentry     Test1,.-Test1
>     addis 3,2,x at got@tlsgd at ha
>     addi 3,3,x at got@tlsgd at l
>     addis 3,2,x at got@tlsgd at ha
>     addi 3,3,x at got@tlsgd at l
>     bl __tls_get_addr
>     nop
>
> in the above case `ld.bfd` does not recognize the pattern for the TLS and so it turns off TLS relaxations completely.
> The reason I brought up these examples is because I feel that detecting the difference between the two cases in lld would be unreasonably complicated an probably not worth it. As a result I feel that it may not be feasible to do the same thing that `ld.bfd` is doing.

Thanks. I did not know that GNU ld already has the functionality. That does justify the motivation behind the patch.

> Here is my new proposal:
> We can add an option to `lld`. Unfortunately this option will not exist in `ld.bfd` because they wouldn't need it.

Thanks. This makes sense.

> The option is off by default.
> If the option is off then `lld` will behave exactly the same way that it always has. No error detection for missing relocations.

Sounds good, if we don't implement a fullblown heuristic (which can be overkill and become maintenance burden) to reduce false positives.

> If the option is on then `lld` will check all calls to `__get_tls_addr` and if it finds a missing relocation it will turn off TLS relaxations. It won't try to add the missing relocation and it won't throw an error.
> This design will not cause problems to default behaviour (and it shouldn't add overhead to the default link either) but it will also allow linking with code that was generated with the legacy compiler if the user specifies the option.

Oh, I missed the case that the current patch implemented TLS relaxation. This is indeed unnecessary if the idea is just to make legacy code work, not to make them also efficient.
If not implementing TLS relaxation can make the code simpler, then go for it!

Another thing: `R_PPC64_REL24_NOTOC` is new. Does it really need the workaround?

> How does this new idea sound?
> If everyone agrees on this idea (or a variant of this idea) I will rework the patch like this.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91611