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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

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

> I am still questioning the motivation behind this change. If there are indeed ABI issues from older compilers,
> can they add a wrapper around the linker if the check is deemed useful?

I don't really see it as an improvement in usability to have to post-process object files, potentially introducing another possible point of failure vs. providing an option to the linker.

> (In addition, binutils does not have --add-missing-tls-reloc.)

I think that a departure from option compatibility is reasonable here if we can provide an option that will:

- Allow us to handle the missing relocation when the option is specified
- Not introduce any performance overhead when the option is not specified (i.e. the default behaviour)

Note that I am not saying this option achieves both of those goals.
It would appear to me that we really have 3 situations here that are kind of at odds with each other:

1. Typical compiler-introduced calls to `__tls_get_addr` with the necessary relocations for the symbol
2. The weird usage in `ld.so` with a missing relocation that does not require a relocation as there is no symbol
3. A non-ABI compliant compiler-introduced call with a value associated with a symbol in the parameter register but a missing relocation

The current state is that the linker does not produce errors on any of those but produces incorrect code for 3. Adding this patch introduces performance overhead by default and handles 3. by either emitting an error or fixing up the relocation to avoid producing incorrect code. It has the further issue that it assumes the immediately preceding relocation was for setting up the parameter to `__tls_get_addr` which may not be the case as @sfertile pointed out.
So can we do something like this:

- Leave default behaviour as it is now
- Provide an option to search for and fix up missing relocations and error out if it cannot

Case 2. above would not work with this option, but that really doesn't seem like a problem as builds that do that will never specify the option. Furthermore, we will continue to emit incorrect code for case 3. above by default, but that is also not a problem as the documentation can suggest using the option whenever some objects are produced by older versions of the XL compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91611