[ELF] Fix "TLS attribute mismatch" false positives for STT_NOTYPE undefined symbols

[MaskRay]

Summary

D13550 added the diagnostic to address/work around a crash.
The rule was refined by D19836 (test/ELF/tls-archive.s) to exclude Lazy symbols.

https://bugs.llvm.org/show_bug.cgi?id=45598 reported another case where the current logic has a false positive:

Bitcode does not record undefined module-level inline assembly symbols
(IRSymtab.cpp:Builder::addSymbol). Such an undefined symbol does not
have the FB_tls bit and lld will not consider it STT_TLS. When the symbol is
later replaced by a STT_TLS Defined, lld will error "TLS attribute mismatch".

This patch fixes this false positive by allowing a STT_NOTYPE undefined
symbol to be replaced by a STT_TLS.

Considered alternative:

Moving the diagnostics to scanRelocs() can improve the diagnostics (PR36049)
but that requires a fair amount of refactoring. We will need more
RelExpr members. It requires more thoughts whether it is worthwhile.

See test/ELF/tls-mismatch.s for behavior differences. We will fail to
diagnose a likely runtime bug (STT_NOTYPE non-TLS relocation referencing
a TLS definition). This is probably acceptable because compiler
generated code sets symbol types properly.

[MaskRay]

Improve test

[psmith]

I agree that the diagnostic where it currently lives is not ideal. I'm not comfortable with removing it completely as over the years I've seen several compiler bugs detected before release by this type of error; TLS variables are almost always accessed via C-code so we don't tend to see mistakes unless there is a bug. Is there another place that we can put this warning and get most of the benefit? 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.

[grimar]

I'd also try to improve it somehow rather than dropping it. (Because of a possible runtime error concern). Perhaps introducing a new option to disable this diagnostic would make sense?
Btw, do you know what is the behavior of modern bfd/gold and/or their plans?

[MaskRay]

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

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

[psmith]

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

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.

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.

[MaskRay]

In D78438#1992533, @psmith wrote:

In 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 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 defer and thread-local storage relocations can only reference symbols with type STT_TLS to psABIs (https://groups.google.com/g/generic-abi search "TLS relocations referencing section symbols").

Rethinking about this. We can allow STT_NOTYPE, STT_SECTION and STT_TLS symbol to be overridden by a STT_TLS definition.

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 TLS)
  error if symbol is neither of STT_TLS, STT_SECTION, STT_NOTYPE
else
  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").

Due to the way we handle TLS relocations: not everything is handled in handleTlsRelocation, more precisely, R_TLS/R_NEG_TLS/R_DTPREL/some needsGot relocations/R_TLSLD_PC / some R_ARM_TLS_LDO32 are not handled.

Moving the diagnostic to scanRelocs() requires some refactoring.

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

[psmith]

Thanks for the update. If the gABI is going in that direction (let's hope Cary can get the new version in a place where it can be updated fairly soon) then I'm happy about that part. I think that if we are able to keep the error where it makes sense such as in scanRelocs then I'm happy to remove it from where it is currently, which I agree is not ideally placed to avoid false positives.

I think the link to the gABI is https://groups.google.com/forum/#!topic/generic-abi/dJ4_Y78aQ2M I think the m/WKggp9fKCgAJ part is specific to your login.

[MaskRay]

In D78438#1992809, @psmith wrote:

Thanks for the update. If the gABI is going in that direction (let's hope Cary can get the new version in a place where it can be updated fairly soon) then I'm happy about that part. I think that if we are able to keep the error where it makes sense such as in scanRelocs then I'm happy to remove it from where it is currently, which I agree is not ideally placed to avoid false positives.

I think the link to the gABI is https://groups.google.com/forum/#!topic/generic-abi/dJ4_Y78aQ2M I think the m/WKggp9fKCgAJ part is specific to your login.

Sorry, I just edited my comment (after re-reading TLS relocations referencing section symbols). The new content is very different from when I made the comment initially.

If we are going to retain the "TLS mismatch" diagnostics in Symbols.h, we need to preclude STT_TLS, STT_SECTION (reasonable symbols types from regular object files), STT_NOTYPE (from bitcode module-level inline assembly).

If we are going to move the "TLS mismatch" diagnostics to scanRelocs. We need to refactor handleTlsRelocations (which requires a considerable amount of work), we may need to split some RelExpr members (R_GOT_PC -> non-TLS / TLS). We will hit the the limit of 64 RelExpr members. We can have location information for https://bugs.llvm.org/show_bug.cgi?id=36049

[MaskRay]

Edit description

[MaskRay]

Edit description

[MaskRay]

After more thoughts, I think we should retain the diagnostic but add an exception that STT_NOTYPE undefined symbol should be allowed to be replaced.

The existing wording is not ideal because TLS relocations should support some non-TLS symbols, e.g. STT_SECTION, and probably also STT_NOTYPE undefined.

Doing the diagnostic in scanRelocs may be overkill.

In GNU ld, bfd/elflink.c _bfd_elf_merge_symbol detects several TLS mismatch problems.

      if (tdef && ntdef)
	_bfd_error_handler
	  /* xgettext:c-format */
	  (_("%s: TLS definition in %pB section %pA "
	     "mismatches non-TLS definition in %pB section %pA"),
	   h->root.root.string, tbfd, tsec, ntbfd, ntsec);
      else if (!tdef && !ntdef)
	_bfd_error_handler
	  /* xgettext:c-format */
	  (_("%s: TLS reference in %pB "
	     "mismatches non-TLS reference in %pB"),
	   h->root.root.string, tbfd, ntbfd);
      else if (tdef)
	_bfd_error_handler
	  /* xgettext:c-format */
	  (_("%s: TLS definition in %pB section %pA "
	     "mismatches non-TLS reference in %pB"),
	   h->root.root.string, tbfd, tsec, ntbfd);
      else
	_bfd_error_handler
	  /* xgettext:c-format */
	  (_("%s: TLS reference in %pB "
	     "mismatches non-TLS definition in %pB section %pA"),
	   h->root.root.string, tbfd, ntbfd, ntsec);

If we are going to improve the diagnostics (PR36049), we should extract the code from Symbol::replace to Symbol::mergeProperties. tls-mismatch.s has a test case where we currently fail to flag an incorrect use case.

tls-in-archive.s and tls-weak-undef.s contain two other interesting use cases which we should not error. I have not unified them into tls-mismatch.s yet.

[MaskRay]

Fix a comment

[psmith]

Thanks for the update, I'm much more comfortable with this version.

[grimar]

Looks safer to me too.

lld/ELF/Symbols.h

 }
 
 // replace() replaces "this" object with a given symbol by memcpy'ing
 // it over to "this". This function is called as a result of name
 // resolution, e.g. to replace an undefind symbol with a defined symbol.
 void Symbol::replace(const Symbol &newSym) {
   using llvm::ELF::STT_TLS;
 
-  // Symbols representing thread-local variables must be referenced by
-  // TLS-aware relocations, and non-TLS symbols must be reference by
-  // non-TLS relocations, so there's a clear distinction between TLS
-  // and non-TLS symbols. It is an error if the same symbol is defined
-  // as a TLS symbol in one file and as a non-TLS symbol in other file.
-  if (symbolKind != PlaceholderKind && !isLazy() && !newSym.isLazy() &&
-      (type == STT_TLS) != (newSym.type == STT_TLS))
+  // st_value of STT_TLS represents the assigned offset, not the actual address
+  // which is used by STT_FUNC and STT_OBJECT. STT_TLS symbols can only be
+  // referenced by special TLS relocations. It is usually an error if a STT_TLS
+  // symbol is replaced by a non-STT_TLS symbol, vice versa. There are two
+  // exceptions: (a) a STT_NOTYPE lazy/undefined symbol can be replaced by a
+  // STT_TLS symbol, (b) a STT_TLS undefined symbol can be replaced by a
+  // STT_NOTYPE lazy symbol.
+  if (symbolKind != PlaceholderKind && !newSym.isLazy() &&
+      (type == STT_TLS) != (newSym.type == STT_TLS) &&
+      type != llvm::ELF::STT_NOTYPE)
     error("TLS attribute mismatch: " + toString(*this) + "\n>>> defined in " +
           toString(newSym.file) + "\n>>> defined in " + toString(file));
 
   Symbol old = *this;
   memcpy(this, &newSym, newSym.getSymbolSize());
 
   // old may be a placeholder. The referenced fields must be initialized in
   // SymbolTable::insert.

lld/test/ELF/tls-mismatch.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: echo 'movq tls1@GOTTPOFF(%rip), %rax' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
 # RUN: ld.lld %t1.o %t.o -o /dev/null
 # RUN: ld.lld %t.o %t1.o -o /dev/null
 # RUN: ld.lld --start-lib %t.o --end-lib %t1.o -o /dev/null
 # RUN: ld.lld %t1.o --start-lib %t.o --end-lib -o /dev/null
 
 ## The TLS definition mismatches a non-TLS reference.
 # RUN: echo '.type tls1,@object; movq tls1,%rax' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
 # RUN: not ld.lld %t2.o %t.o -o /dev/null 2>&1 | FileCheck %s
 ## We fail to flag the swapped case.
 # RUN: ld.lld %t.o %t2.o -o /dev/null
 
+## We fail to flag the STT_NOTYPE reference. This usually happens with hand-written
+## assembly because compiler-generated code properly sets symbol types.
 # RUN: echo 'movq tls1,%rax' | llvm-mc -filetype=obj -triple=x86_64 - -o %t3.o
-# RUN: not ld.lld %t3.o %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld %t3.o %t.o -o /dev/null
 
 ## Overriding a TLS definition with a non-TLS definition does not make sense.
 # RUN: not ld.lld --defsym tls1=42 %t.o -o /dev/null 2>&1 | FileCheck %s
 
 ## Part of PR36049: This should probably be allowed.
 # RUN: not ld.lld --defsym tls1=tls2 %t.o -o /dev/null 2>&1 | FileCheck %s
 
+## An undefined symbol in module-level inline assembly of a bitcode file
+## is considered STT_NOTYPE. We should not error.
 # RUN: echo 'target triple = "x86_64-pc-linux-gnu" \
 # RUN:   target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" \
 # RUN:   module asm "movq tls1@GOTTPOFF(%rip), %rax"' | llvm-as - -o %t.bc
 # RUN: ld.lld %t.o %t.bc -o /dev/null
-# RUN: not ld.lld %t.bc %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld %t.bc %t.o -o /dev/null
 
 # CHECK: error: TLS attribute mismatch: tls1
 
 .globl _start
 _start:
   addl $1, %fs:tls1@TPOFF
   addl $2, %fs:tls2@TPOFF
 
 .tbss
 .globl tls1, tls2
   .space 8
 tls1:
   .space 4
 tls2:
   .space 4