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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

In D91611#2400509 <https://reviews.llvm.org/D91611#2400509>, @MaskRay wrote:

> I remember that I have questioned about the error checking when the patch was initially brought up. The introduction of a new linker option for an incorrect case where compiler will not generate such code makes me feel more uneasy about it. Surely the linker can detect such errors but does it really have to? With assembly the user can play all sorts of tricks which will not work, but is that the duty of the linker?

I agree that we cannot generate this sequence from C source using clang. However, the reason I am still trying to detect this problem is because we know that we have older versions of the XLC compiler that actually produced the incorrect setup with the missing relocation and we end up with bad code gen when linking in binaries with that missing relocation. These incorrectly compiled binaries do exist in real life and the users may have no idea that they have been compiled incorrectly.



================
Comment at: lld/ELF/Relocations.cpp:1316
+    const RelType prevType = prevRel.getType(config->isMips64EL);
+    // Check if this call to __tls_get_addr is part of a TLS sequence.
+    if (prevType == R_PPC64_GOT_TLSGD16_HA ||
----------------
sfertile wrote:
> I don't think this is something you can reliably determine. For example consider the code:
> 
> ```
>        # r30 is previously saved to the stack.
>        # setup r3 for the call to __tls_get_addr
>         addis 3, 2, i at got@tlsgd at ha
>         addi 3, 3, i at got@tlsgd at l
>        # Load unrelated variable into a non-volatile register so it will stay live across the call.
>         addis 30, 2, j at toc@ha
>         addi 30, j at toc@l(30)
>         # call __tls_get_addr. Relocation I - 2 is NOT one of the relocations used to setup the argument.
>         bl __tls_get_addr(i at tlsgd)
>         nop
> ```
I agree.
However, in the above situation we are no worse off than we are now. Right now we fail to detect all such situations. If we add this patch we will detect most of these situations but as you mentioned above we can miss a few.

If we want to get them all we have to also flag the direct call to `__tls_get_addr` but I'm trying to avoid that situation.

Another option is to search up until I find a relocation that is either 
1) `@tlsgd` which indicates an error.
OR 
2) Function call which indicates we should stop the search without an error.
3) Finished the search and nothing was found of the other cases in which case we stop without an error. 


================
Comment at: lld/test/ELF/ppc64-tls-add-missing-gdld.s:30
+# CHECK-NEXT:    blr
+GeneralDynamic:
+  addis 3, 2, x at got@tlsgd at ha
----------------
NeHuang wrote:
> do we need to add two test cases here to cover?
> - add missing tls relocation for `__tls_get_addr at notoc(x at tlsgd)`
> - add missing tls relocation for `__tls_get_addr at notoc(x at tlsld)`
Yes I can add the `@notoc` versions.
Good catch. I missed those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91611