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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 24 13:44:03 PST 2020

MaskRay added a comment.

In D91611#2413841 <https://reviews.llvm.org/D91611#2413841>, @nemanjai wrote:

> 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.

Looks like the proposed option needs to be tri-state? (a) no diagnostic (the default behavior, this is needed to link glibc/musl/FreeBSD ld.so) (b) call `handleTlsRelocation` (this looks out-of-place to me and I would really hope we don't have this) (c) error.

If GNU ld is going to add a similar option, I hope we can be consistent. So can someone also make a request on the mailing list binutils at sourceware.org ?
I would hope `ppc64` is part of the option name to make it clear this is a ppc64 specific workaround.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list