Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lld][WebAssembly] Fix TLS-relative relocations when linking without shared memory #116136

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 14, 2024

TLS-relative relocations always need to be relative the TLS section since they get added to __tls_base at runtime.

Without this change the tls base address was effectively being added to the final value twice in this case.

This only effects code the is built with -pthread but linked without shared memory (i.e. without threads).

Fixes: emscripten-core/emscripten#22880

@llvmbot
Copy link

llvmbot commented Nov 14, 2024

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-wasm

Author: Sam Clegg (sbc100)

Changes

TLS-relative relocation always need to be relative the TLS section since they get added to __tls_base at runtime.

Without this change the tls base address was effectively being added to the final value twice in this case.

This only effects code the is built with -pthread but linked without shared memory (i.e. without threads).

Fixes: emscripten-core/emscripten#22880


Full diff: https://github.com/llvm/llvm-project/pull/116136.diff

5 Files Affected:

  • (modified) lld/test/wasm/tls-non-shared-memory.s (+9)
  • (modified) lld/wasm/Relocations.cpp (+6-6)
  • (modified) lld/wasm/Symbols.cpp (+4-5)
  • (modified) lld/wasm/Symbols.h (+3-1)
  • (modified) lld/wasm/SyntheticSections.cpp (+4-1)
diff --git a/lld/test/wasm/tls-non-shared-memory.s b/lld/test/wasm/tls-non-shared-memory.s
index 1754fd6254bb80..04fbb62228a7e2 100644
--- a/lld/test/wasm/tls-non-shared-memory.s
+++ b/lld/test/wasm/tls-non-shared-memory.s
@@ -44,6 +44,7 @@ tls1:
 
 # RUN: wasm-ld --no-gc-sections --no-entry -o %t.wasm %t.o
 # RUN: obj2yaml %t.wasm | FileCheck %s
+# RUN: llvm-objdump --disassemble-symbols=get_tls1 --no-show-raw-insn --no-leading-addr %t.wasm | FileCheck %s --check-prefixes DIS
 
 # RUN: wasm-ld --experimental-pic -shared -o %t.so %t.o
 # RUN: obj2yaml %t.so | FileCheck %s --check-prefixes=SHARED,PIC
@@ -97,6 +98,14 @@ tls1:
 # CHECK-NEXT:        Content:         2A000000
 # CHECK-NEXT:  - Type:            CUSTOM
 
+# The constant value here which we add to `__tls_base` should not be absolute
+# but relative to `__tls_base`, in this case zero rather than 1024.
+# DIS:      <get_tls1>:
+# DIS-EMPTY:
+# DIS-NEXT:  global.get 1
+# DIS-NEXT:  i32.const 0
+# DIS-NEXT:  i32.add
+# DIS-NEXT:  end
 
 # In PIC mode we expect TLS data and non-TLS data to be merged into
 # a single segment which is initialized via the  __memory_base import
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index 45ad32701616a1..2776ede8be9034 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -122,16 +122,16 @@ void scanRelocations(InputChunk *chunk) {
               " cannot be used against an undefined symbol `" + toString(*sym) +
               "`");
       }
+      if (!sym->isTLS()) {
+        error(toString(file) + ": relocation " +
+              relocTypeToString(reloc.Type) +
+              " cannot be used against non-TLS symbol `" + toString(*sym) +
+              "`");
+      }
       // In single-threaded builds TLS is lowered away and TLS data can be
       // merged with normal data and allowing TLS relocation in non-TLS
       // segments.
       if (config->sharedMemory) {
-        if (!sym->isTLS()) {
-          error(toString(file) + ": relocation " +
-                relocTypeToString(reloc.Type) +
-                " cannot be used against non-TLS symbol `" + toString(*sym) +
-                "`");
-        }
         if (auto *D = dyn_cast<DefinedData>(sym)) {
           if (!D->segment->outputSeg->isTLS()) {
             error(toString(file) + ": relocation " +
diff --git a/lld/wasm/Symbols.cpp b/lld/wasm/Symbols.cpp
index b2bbd11c53ef23..8df850ce23032f 100644
--- a/lld/wasm/Symbols.cpp
+++ b/lld/wasm/Symbols.cpp
@@ -310,12 +310,11 @@ uint32_t DefinedFunction::getExportedFunctionIndex() const {
   return function->getFunctionIndex();
 }
 
-uint64_t DefinedData::getVA() const {
+uint64_t DefinedData::getVA(bool absolute) const {
   LLVM_DEBUG(dbgs() << "getVA: " << getName() << "\n");
-  // In the shared memory case, TLS symbols are relative to the start of the TLS
-  // output segment (__tls_base).  When building without shared memory, TLS
-  // symbols absolute, just like non-TLS.
-  if (isTLS() && config->sharedMemory)
+  // TLS symbols are relative to the start of the TLS output segment
+  // (__tls_base).
+  if (isTLS() && !absolute)
     return getOutputSegmentOffset();
   if (segment)
     return segment->getVA(value);
diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h
index 5ce3ecbc4ab194..310842b89098f9 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -315,7 +315,9 @@ class DefinedData : public DataSymbol {
   static bool classof(const Symbol *s) { return s->kind() == DefinedDataKind; }
 
   // Returns the output virtual address of a defined data symbol.
-  uint64_t getVA() const;
+  // For TLS symbols, by default (unless absolute us set), this returns an
+  // address relative the `__tls_base`.
+  uint64_t getVA(bool absolute = false) const;
   void setVA(uint64_t va);
 
   // Returns the offset of a defined data symbol within its OutputSegment.
diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp
index a3bc90cfe759ca..1e8abe9bc7b246 100644
--- a/lld/wasm/SyntheticSections.cpp
+++ b/lld/wasm/SyntheticSections.cpp
@@ -514,7 +514,10 @@ void GlobalSection::writeBody() {
     } else {
       WasmInitExpr initExpr;
       if (auto *d = dyn_cast<DefinedData>(sym))
-        initExpr = intConst(d->getVA(), is64);
+        // In the sharedMemory case `__wasm_apply_global_tls_relocs` is used
+        // to set the final value of this globel, but in the non-shared case
+        // we know the absolute value at link time.
+        initExpr = intConst(d->getVA(/*absolute=*/!config->sharedMemory), is64);
       else if (auto *f = dyn_cast<FunctionSymbol>(sym))
         initExpr = intConst(f->isStub ? 0 : f->getTableIndex(), is64);
       else {

…shared memory

TLS-relative relocation always need to be relative the TLS section since
they get added to `__tls_base` at runtime.

Without this change the tls base address was effectively being added
to the final value twice in this case.

This only effects code the is built with `-pthread` but linked without
shared memory (i.e. without threads).

Fixes: emscripten-core/emscripten#22880
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff adfa6b762dc53bc53377785d824264a3311e829d de8fdac5f4c68dfee8f075d564b05a044858b648 --extensions cpp,h -- lld/wasm/Relocations.cpp lld/wasm/Symbols.cpp lld/wasm/Symbols.h lld/wasm/SyntheticSections.cpp
View the diff from clang-format here.
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index 2776ede8be..16afe5dbc2 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -123,8 +123,7 @@ void scanRelocations(InputChunk *chunk) {
               "`");
       }
       if (!sym->isTLS()) {
-        error(toString(file) + ": relocation " +
-              relocTypeToString(reloc.Type) +
+        error(toString(file) + ": relocation " + relocTypeToString(reloc.Type) +
               " cannot be used against non-TLS symbol `" + toString(*sym) +
               "`");
       }

relocTypeToString(reloc.Type) +
" cannot be used against non-TLS symbol `" + toString(*sym) +
"`");
}
// In single-threaded builds TLS is lowered away and TLS data can be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this comment move up too?

@@ -514,7 +514,10 @@ void GlobalSection::writeBody() {
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the extended-const case also need to be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emscripten crashes when using thread_local variables wihtout -pthread and -flto enabled
3 participants