[PATCH] D78438: [ELF] Delete "TLS attribute mismatch" diagnostics

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D78438#1992533 <https://reviews.llvm.org/D78438#1992533>, @psmith wrote:

> In D78438#1992307 <https://reviews.llvm.org/D78438#1992307>, @MaskRay wrote:
>
> > > For example if we added the check in scanRelocs the symbol combination would have completed so if there was a non-TLS symbol reference (via bitcode or assembly) that was replaced by a TLS definition we would accept that. However if the definition were non-TLS we would.
> >
> > (1)The example I intend to add to `lld/test/ELF/tls-mismatch.s` (below) and (2) https://bugs.llvm.org/show_bug.cgi?id=45600 suggests that testing STT_TLS is not suitable.
> >
> >   # RUN: echo '.globl tls1' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
> >   # RUN: ld.lld %t2.o %t.o -o /dev/null
> >
> >
> > We can add the diagnostic back when (2) is implemented (likely via a new RelExpr member) (My experience is that this diagnostic has not detected anything.)
>
>
> There is at least one that I know of that was caught by a link time error in LLVM D43860 <https://reviews.llvm.org/D43860> the linker was BFD
>
>   /usr/bin/aarch64-linux-gnu-ld: /tmp/test2-67df6a.o(.debug_info+0x37): R_AARCH64_ABS64 used with TLS symbol x
>
>
> At a glance it looks like that transformation of using an STT_SECTION symbol is not legal in ELF. I can see that an STT_SECTION symbol for a section with SHF_TLS could be considered equivalent to a local STT_TLS symbol, and LLD could be modified to handle it that way; but that isn't what the spec says and other linkers could have special case handling of TLS symbols for STT_TLS symbols and even if they don't error they may not produce correct output. Apologies if I've missed something in my haste.
>
>   STT_TLS
>   The symbol specifies a Thread-Local Storage entity. When defined, it gives the assigned offset for the symbol, not the actual address. Symbols of type STT_TLS can be referenced by only special thread-local storage relocations and thread-local storage relocations can only reference symbols with type STT_TLS. Implementation need not support thread-local storage.
>


A few parties agreed to remove `and thread-local storage relocations can only reference symbols with type STT_TLS` (https://groups.google.com/g/generic-abi/c/dJ4_Y78aQ2M/m/WKggp9fKCgAJ)

> What I was thinking of was something when processing relocations
>  if (reloc is TLS)
> 
>   check symbol is TLS and error/warn if not
> 
> else
> 
>   check symbol is not TLS and error/warn if not
>    
> 
> By that point we need to know if the symbol is TLS (reference from bitcode should be replaced by TLS definition). That should support the use case in PR45598. Looking at PR36049 I don't think that assigning a TLS variable to an absolute value in a script or defsym makes sense and I don't think BFD or gold are particularly helpful in providing guidance. Assigning a TLS variable to another tls2=tls1 can make sense and Gold and BFD do the right thing and make tls2 STT_TLS. I think we could fix that.

Incorporating the amendment to the ELF specification, scanRelocs can do:

  if (reloc is not TLS)
    error if symbol is STT_TLS

This can improve the diagnostics because scanRelocs knows where the symbol is used (https://bugs.llvm.org/show_bug.cgi?id=36049 "Mismatched TLS error could be better").

>>> Btw, do you know what is the behavior of modern bfd/gold and/or their plans?
>> 
>> GNU ld does not check TLS mismatching. gold has a `symbol '%s' used as both __thread and non-__thread` error but its use case is a bit different (despite the similarity of code).
> 
> https://bugs.llvm.org/show_bug.cgi?id=21077 suggests that BFD at least used to for AArch64
> 
>   /usr/bin/aarch64-linux-gnu-ld: /tmp/test2-67df6a.o(.debug_info+0x37): R_AARCH64_ABS64 used with TLS symbol x
> 
> 
> A grep for "used with TLS" brings up aarch64, arm, m68k, xtensa, and PPC backends with this error.

Thanks for checking. I just noticed that the x86 backend does not have the diagnostic. The backend differences have been consistently causing problems.

https://sourceware.org/pipermail/binutils/2020-March/000358.html "Current BFD linker delegates most, if not all, of relocation to each backend. There are many similar codes in backends.  I am sharing as much codes between i386 and x86-64 backends as I can."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78438