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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay created this revision.
MaskRay added reviewers: grimar, ikudrin, psmith, ruiu, yshui.
Herald added subscribers: llvm-commits, arichardson, emaste.
Herald added a reviewer: espindola.
Herald added a project: LLVM.
MaskRay edited the summary of this revision.

D13550 <https://reviews.llvm.org/D13550> added the diagnostic to address/work around a crash.
The rule was refined by D19836 <https://reviews.llvm.org/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".

The fix is simple: just drop the diagnostics. The internals of lld have
changed a lot since D13550 <https://reviews.llvm.org/D13550> (2015) and we can no longer reproduce the
original crash.

See `test/ELF/tls-mismatch.s` for behavior differences. We will fail to diagnose
a likely runtime bug (non-TLS relocation referencing a TLS symbol).
Another argument in favor of dropping the diagnostic is that we don't rely on
MCELFStreamer::fixSymbolsInTLSFixups changing the referenced symbol to STT_TLS


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78438

Files:
  lld/ELF/Symbols.h
  lld/test/ELF/tls-mismatch.s


Index: lld/test/ELF/tls-mismatch.s
===================================================================
--- lld/test/ELF/tls-mismatch.s
+++ lld/test/ELF/tls-mismatch.s
@@ -4,14 +4,23 @@
 # RUN: ld.lld %t1.o %t.o -o /dev/null
 
 # RUN: echo '.globl tls1' | 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
+# RUN: ld.lld %t2.o %t.o -o /dev/null
 
+## Likely runtime bug: a non-TLS relocation references a TLS symbol.
 # 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
 
-# RUN: not ld.lld --defsym tls1=42 %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: ld.lld --defsym tls1=42 %t.o -o - | llvm-readelf -s - | FileCheck --check-prefix=ABS %s
+# ABS:    Value         Size Type   Bind   Vis     Ndx Name
+# ABS: 000000000000002a    0 NOTYPE GLOBAL DEFAULT ABS tls1
+# ABS: 0000000000000004    0 TLS    GLOBAL DEFAULT   2 tls2
 
-# RUN: not ld.lld --defsym tls2=tls1 %t.o -o /dev/null 2>&1 | FileCheck %s
+## PR36049 this does not work.
+# RUN: ld.lld --defsym tls1=tls2 %t.o -o %t.same
+# RUN: llvm-readelf -s %t.same | FileCheck --check-prefix=SAME %s
+# SAME:    Value         Size Type   Bind   Vis     Ndx Name
+# SAME: 0000000000000000    0 TLS    GLOBAL DEFAULT   2 tls1
+# SAME: 000000000020116a    0 NOTYPE GLOBAL DEFAULT   2 tls2
 
 # CHECK: error: TLS attribute mismatch: tls1
 
Index: lld/ELF/Symbols.h
===================================================================
--- lld/ELF/Symbols.h
+++ lld/ELF/Symbols.h
@@ -515,18 +515,6 @@
 // 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))
-    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());
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78438.258563.patch
Type: text/x-patch
Size: 2568 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200418/389de651/attachment.bin>