[lld][WebAssembly] Follow relocations of TLS-base accessors during GC - #206831
Conversation
In cooperative-threading mode the synthetic init functions (e.g.
`__wasm_init_tls` / `__wasm_init_memory`) call `__wasm_get_tls_base` and
`__wasm_set_tls_base` via raw `call` instructions that carry no
relocations. To keep those accessors in the output the linker marks them
live with `Symbol::markLive()`, which sets the live flag but does not push
the defining chunk onto the mark queue, so the mark phase never follows
the accessors' own relocations.
`__wasm_get_tls_base` happened to survive because real code references it,
but `__wasm_set_tls_base` is reachable only through the relocation-less
synthetic call. As a result, with `--gc-sections` (the default), the
function it calls -- the cooperative-threading `context.set` builtin for
TLS-base slot 1 -- was garbage collected and the call mis-resolved,
producing invalid output ("values remaining on stack at end of block").
Enqueue the chunks of `__wasm_{get,set}_tls_base` in `MarkLive::run()` so
their relocations are followed, mirroring how ctor functions reached via
the relocation-less `__wasm_call_ctors` are handled.
Without this fix the new test drops the `set_helper` import and rewrites
the `__wasm_set_tls_base` body to call an unrelated function.
|
Hello @ricochet 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-wasm Author: Bailey Hayes (ricochet) Changes[lld][WebAssembly] Follow relocations of TLS-base accessors during GCWith In cooperative-threading mode ( Root cause
This is the same situation already handled for constructors reached through the relocation-less FixIn for (Symbol *sym : {static_cast<Symbol *>(ctx.sym.getTLSBase),
static_cast<Symbol *>(ctx.sym.setTLSBase)})
if (sym)
if (InputChunk *c = sym->getChunk())
enqueue(c);The symbols are only set in the libcall-thread-context configuration and are null otherwise, so the loop is a no-op for all other builds. TestingAdds ninja check-lld-wasmFull diff: https://github.com/llvm/llvm-project/pull/206831.diff 2 Files Affected:
diff --git a/lld/test/wasm/cooperative-threading-gc.s b/lld/test/wasm/cooperative-threading-gc.s
new file mode 100644
index 0000000000000..2af2460939aad
--- /dev/null
+++ b/lld/test/wasm/cooperative-threading-gc.s
@@ -0,0 +1,57 @@
+# Verify that --gc-sections preserves functions reachable only through the
+# thread-context accessors __wasm_{get,set}_tls_base. These accessors are
+# invoked by synthetic init functions (e.g. __wasm_init_tls) via `call`
+# instructions that carry no relocations, so the linker marks the accessors
+# live explicitly. Their own relocations must still be followed during GC,
+# otherwise functions they call (here modeling the cooperative-threading
+# context.set builtin) are incorrectly collected and the call is mis-resolved.
+
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: wasm-ld --cooperative-threading --gc-sections -o %t.wasm %t.o
+# RUN: obj2yaml %t.wasm | FileCheck %s
+
+ .functype set_helper (i32) -> ()
+ .import_module set_helper, "env"
+ .import_name set_helper, "set_helper"
+
+.globl __wasm_get_tls_base
+__wasm_get_tls_base:
+ .functype __wasm_get_tls_base () -> (i32)
+ i32.const 0
+ end_function
+
+# Reachable only via the synthetic __wasm_init_tls. Its call to `set_helper`
+# must keep `set_helper` live.
+.globl __wasm_set_tls_base
+__wasm_set_tls_base:
+ .functype __wasm_set_tls_base (i32) -> ()
+ local.get 0
+ call set_helper
+ end_function
+
+.globl _start
+_start:
+ .functype _start () -> (i32)
+ call __wasm_get_tls_base
+ i32.const tls1@TLSREL
+ i32.add
+ i32.load 0
+ end_function
+
+.section .tdata.tls1,"",@
+.globl tls1
+tls1:
+ .int32 1
+ .size tls1, 4
+
+.section .custom_section.target_features,"",@
+ .int8 2
+ .int8 43
+ .int8 11
+ .ascii "bulk-memory"
+ .int8 43
+ .int8 7
+ .ascii "atomics"
+
+# The imported helper called by __wasm_set_tls_base must survive GC.
+# CHECK: Field: set_helper
diff --git a/lld/wasm/MarkLive.cpp b/lld/wasm/MarkLive.cpp
index 2b2cf19f14b30..3364be006ca24 100644
--- a/lld/wasm/MarkLive.cpp
+++ b/lld/wasm/MarkLive.cpp
@@ -126,6 +126,20 @@ void MarkLive::run() {
enqueueRetainedSegments(obj);
}
+ // `__wasm_{get,set}_tls_base` are called from synthetic init functions (e.g.
+ // `__wasm_init_tls`, `__wasm_init_memory`) via raw `call` instructions that
+ // carry no relocations, so the mark phase below cannot discover the functions
+ // they in turn call (e.g. the cooperative-threading
+ // `context.get`/`context.set` builtins). They are already marked live, but
+ // their defining chunks were never enqueued; enqueue them here so their
+ // relocations are followed. This mirrors the handling of ctor functions
+ // reached via `__wasm_call_ctors`.
+ for (Symbol *sym : {static_cast<Symbol *>(ctx.sym.getTLSBase),
+ static_cast<Symbol *>(ctx.sym.setTLSBase)})
+ if (sym)
+ if (InputChunk *c = sym->getChunk())
+ enqueue(c);
+
mark();
// If we have any non-discarded init functions, mark `__wasm_call_ctors` as
|
Confirming that I have read these policies. |
|
It seems rather odd to have to special case these in MarkLive.cpp. The idea behind calling |
|
It seems like maybe there is an ordering problem here:
So by the time that I'm not sure what the best solution to this problem is.. |
|
Hmm.. actually maybe that analysis was wrong. These symbol are actually marked as live in |
|
@sbc100 I dug a bit into this today as I was perplexed how this bug isn't affecting wasi-libc tests with wasip3. I believe the reason for this is because we've got a separate assembly file with live symbols which refer to @ricochet I might be missing some testing you're doing, however. Do you have a higher-level test case that fail without this PR in wasm-ld? I can try to play around with it and see if there's a workaround that may not require a backport, or otherwise inform that we'll ideally want this backported |
|
My higher-level use-case where I ran into this, now works with wasi-sdk-34-rc.1. I think you've essentially fixed it there, so no I don't think we need to backport. |
|
I tried and failed (so far) to come up with more elegant solution. This certainly does seems like a bug. The whole way we use However, if this is an issue/bug that you guys need fixes for the upcoming release maybe this small patch is the right wayt to go in the short term. Also, since this is clearly a very small/targeting bug fix I think it would be good can candidate for backporting at any stage. |
|
If you're ok with that it'd be appreciated yeah. I'd also be ok taking a look in the near future as well to see I can help improve the internal liveness of these symbols |
|
@ricochet Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…llvm#206831) With `--gc-sections` (the default), `wasm-ld` garbage-collects functions that are only reachable through `__wasm_get_tls_base` / `__wasm_set_tls_base` in the cooperative-threading (libcall thread-context) configuration. This produces a linked module that is invalid or behaves incorrectly: the relocation inside `__wasm_set_tls_base` is left dangling / mis-resolved, so callers trap at runtime (e.g. `validation error: ... values remaining on stack at end of block`, or a call to an unrelated function). In cooperative-threading mode (`--cooperative-threading`, added in llvm#200855), per-task thread context is accessed through libcalls rather than wasm globals. `wasm-ld` synthesizes `__wasm_init_tls` / `__wasm_init_memory`, which invoke `__wasm_get_tls_base` and `__wasm_set_tls_base` via **raw `call` instructions that carry no relocations**. To keep those accessors in the output, the linker marks them live with `Symbol::markLive()`. ## Error and Repro ```bash Error: failed to compile: wasm[0]::function[16]::__wasm_set_tls_base Caused by: 0: WebAssembly translation error 1: Invalid input WebAssembly code at offset 778: type mismatch: values remaining on stack at end of block ``` import was dropped and __wasm_set_tls_base was rewritten from ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_set_1) ;; correct ``` to ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_get_0) ;; wrong: get_0 is () -> i32 ``` Minimal linker-only repro (no runtime needed) is: ```bash wasm-ld --cooperative-threading tls.o libc.a # context-set-1 dropped, set_tls_base calls get_0 wasm-ld --cooperative-threading --no-gc-sections tls.o libc.a # correct ``` ## Root cause `Symbol::markLive()` sets the live flag (and the chunk's live bit) but does not push the defining chunk onto the mark queue. The mark phase only follows relocations of chunks that were enqueued via `MarkLive::enqueue()`. As a result the accessors' own relocations are never traversed. This is the same situation already handled for constructors reached through the relocation-less `__wasm_call_ctors`, which `MarkLive::run()` enqueues explicitly. ## Fix In `MarkLive::run()`, enqueue the defining chunks of `__wasm_get_tls_base` and `__wasm_set_tls_base` before `mark()`, so their relocations are followed. ```cpp for (Symbol *sym : {static_cast<Symbol *>(ctx.sym.getTLSBase), static_cast<Symbol *>(ctx.sym.setTLSBase)}) if (sym) if (InputChunk *c = sym->getChunk()) enqueue(c); ``` The symbols are only set in the libcall-thread-context configuration and are null otherwise, so the loop is a no-op for all other builds. ## Testing Adds `lld/test/wasm/cooperative-threading-gc.s`, which links with `--cooperative-threading --gc-sections` and checks (via `obj2yaml`) that the import called by `__wasm_set_tls_base` survives GC. Without this change the test drops the import and rewrites `__wasm_set_tls_base` to call an unrelated function; with it the import is retained and the call is correct. ```sh ninja check-lld-wasm ``` ## Note Human-in-the-loop with claude opus 4.8. I iterated with this patch and got to a working component running with a wasi-sdk fork.
…llvm#206831) With `--gc-sections` (the default), `wasm-ld` garbage-collects functions that are only reachable through `__wasm_get_tls_base` / `__wasm_set_tls_base` in the cooperative-threading (libcall thread-context) configuration. This produces a linked module that is invalid or behaves incorrectly: the relocation inside `__wasm_set_tls_base` is left dangling / mis-resolved, so callers trap at runtime (e.g. `validation error: ... values remaining on stack at end of block`, or a call to an unrelated function). In cooperative-threading mode (`--cooperative-threading`, added in llvm#200855), per-task thread context is accessed through libcalls rather than wasm globals. `wasm-ld` synthesizes `__wasm_init_tls` / `__wasm_init_memory`, which invoke `__wasm_get_tls_base` and `__wasm_set_tls_base` via **raw `call` instructions that carry no relocations**. To keep those accessors in the output, the linker marks them live with `Symbol::markLive()`. ## Error and Repro ```bash Error: failed to compile: wasm[0]::function[16]::__wasm_set_tls_base Caused by: 0: WebAssembly translation error 1: Invalid input WebAssembly code at offset 778: type mismatch: values remaining on stack at end of block ``` import was dropped and __wasm_set_tls_base was rewritten from ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_set_1) ;; correct ``` to ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_get_0) ;; wrong: get_0 is () -> i32 ``` Minimal linker-only repro (no runtime needed) is: ```bash wasm-ld --cooperative-threading tls.o libc.a # context-set-1 dropped, set_tls_base calls get_0 wasm-ld --cooperative-threading --no-gc-sections tls.o libc.a # correct ``` ## Root cause `Symbol::markLive()` sets the live flag (and the chunk's live bit) but does not push the defining chunk onto the mark queue. The mark phase only follows relocations of chunks that were enqueued via `MarkLive::enqueue()`. As a result the accessors' own relocations are never traversed. This is the same situation already handled for constructors reached through the relocation-less `__wasm_call_ctors`, which `MarkLive::run()` enqueues explicitly. ## Fix In `MarkLive::run()`, enqueue the defining chunks of `__wasm_get_tls_base` and `__wasm_set_tls_base` before `mark()`, so their relocations are followed. ```cpp for (Symbol *sym : {static_cast<Symbol *>(ctx.sym.getTLSBase), static_cast<Symbol *>(ctx.sym.setTLSBase)}) if (sym) if (InputChunk *c = sym->getChunk()) enqueue(c); ``` The symbols are only set in the libcall-thread-context configuration and are null otherwise, so the loop is a no-op for all other builds. ## Testing Adds `lld/test/wasm/cooperative-threading-gc.s`, which links with `--cooperative-threading --gc-sections` and checks (via `obj2yaml`) that the import called by `__wasm_set_tls_base` survives GC. Without this change the test drops the import and rewrites `__wasm_set_tls_base` to call an unrelated function; with it the import is retained and the call is correct. ```sh ninja check-lld-wasm ``` ## Note Human-in-the-loop with claude opus 4.8. I iterated with this patch and got to a working component running with a wasi-sdk fork.
…#206831) With `--gc-sections` (the default), `wasm-ld` garbage-collects functions that are only reachable through `__wasm_get_tls_base` / `__wasm_set_tls_base` in the cooperative-threading (libcall thread-context) configuration. This produces a linked module that is invalid or behaves incorrectly: the relocation inside `__wasm_set_tls_base` is left dangling / mis-resolved, so callers trap at runtime (e.g. `validation error: ... values remaining on stack at end of block`, or a call to an unrelated function). In cooperative-threading mode (`--cooperative-threading`, added in #200855), per-task thread context is accessed through libcalls rather than wasm globals. `wasm-ld` synthesizes `__wasm_init_tls` / `__wasm_init_memory`, which invoke `__wasm_get_tls_base` and `__wasm_set_tls_base` via **raw `call` instructions that carry no relocations**. To keep those accessors in the output, the linker marks them live with `Symbol::markLive()`. ## Error and Repro ```bash Error: failed to compile: wasm[0]::function[16]::__wasm_set_tls_base Caused by: 0: WebAssembly translation error 1: Invalid input WebAssembly code at offset 778: type mismatch: values remaining on stack at end of block ``` import was dropped and __wasm_set_tls_base was rewritten from ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_set_1) ;; correct ``` to ```wat (func $__wasm_set_tls_base (param i32) local.get 0 call $__wasm_component_model_builtin_context_get_0) ;; wrong: get_0 is () -> i32 ``` Minimal linker-only repro (no runtime needed) is: ```bash wasm-ld --cooperative-threading tls.o libc.a # context-set-1 dropped, set_tls_base calls get_0 wasm-ld --cooperative-threading --no-gc-sections tls.o libc.a # correct ``` ## Root cause `Symbol::markLive()` sets the live flag (and the chunk's live bit) but does not push the defining chunk onto the mark queue. The mark phase only follows relocations of chunks that were enqueued via `MarkLive::enqueue()`. As a result the accessors' own relocations are never traversed. This is the same situation already handled for constructors reached through the relocation-less `__wasm_call_ctors`, which `MarkLive::run()` enqueues explicitly. ## Fix In `MarkLive::run()`, enqueue the defining chunks of `__wasm_get_tls_base` and `__wasm_set_tls_base` before `mark()`, so their relocations are followed. ```cpp for (Symbol *sym : {static_cast<Symbol *>(ctx.sym.getTLSBase), static_cast<Symbol *>(ctx.sym.setTLSBase)}) if (sym) if (InputChunk *c = sym->getChunk()) enqueue(c); ``` The symbols are only set in the libcall-thread-context configuration and are null otherwise, so the loop is a no-op for all other builds. ## Testing Adds `lld/test/wasm/cooperative-threading-gc.s`, which links with `--cooperative-threading --gc-sections` and checks (via `obj2yaml`) that the import called by `__wasm_set_tls_base` survives GC. Without this change the test drops the import and rewrites `__wasm_set_tls_base` to call an unrelated function; with it the import is retained and the call is correct. ```sh ninja check-lld-wasm ``` ## Note Human-in-the-loop with claude opus 4.8. I iterated with this patch and got to a working component running with a wasi-sdk fork.
With
--gc-sections(the default),wasm-ldgarbage-collects functions that are only reachable through__wasm_get_tls_base/__wasm_set_tls_basein the cooperative-threading (libcall thread-context) configuration. This produces a linked module that is invalid or behaves incorrectly: the relocation inside__wasm_set_tls_baseis left dangling / mis-resolved, so callers trap at runtime (e.g.validation error: ... values remaining on stack at end of block, or a call to an unrelated function).In cooperative-threading mode (
--cooperative-threading, added in #200855), per-task thread context is accessed through libcalls rather than wasm globals.wasm-ldsynthesizes__wasm_init_tls/__wasm_init_memory, which invoke__wasm_get_tls_baseand__wasm_set_tls_basevia rawcallinstructions that carry no relocations. To keep those accessors in the output, the linker marks them live withSymbol::markLive().Error and Repro
Error: failed to compile: wasm[0]::function[16]::__wasm_set_tls_base Caused by: 0: WebAssembly translation error 1: Invalid input WebAssembly code at offset 778: type mismatch: values remaining on stack at end of blockimport was dropped and __wasm_set_tls_base was rewritten from
to
Minimal linker-only repro (no runtime needed) is:
Root cause
Symbol::markLive()sets the live flag (and the chunk's live bit) but does not push the defining chunk onto the mark queue. The mark phase only follows relocations of chunks that were enqueued viaMarkLive::enqueue(). As a result the accessors' own relocations are never traversed.This is the same situation already handled for constructors reached through the relocation-less
__wasm_call_ctors, whichMarkLive::run()enqueues explicitly.Fix
In
MarkLive::run(), enqueue the defining chunks of__wasm_get_tls_baseand__wasm_set_tls_basebeforemark(), so their relocations are followed.The symbols are only set in the libcall-thread-context configuration and are null otherwise, so the loop is a no-op for all other builds.
Testing
Adds
lld/test/wasm/cooperative-threading-gc.s, which links with--cooperative-threading --gc-sectionsand checks (viaobj2yaml) that the import called by__wasm_set_tls_basesurvives GC. Without this change the test drops the import and rewrites__wasm_set_tls_baseto call an unrelated function; with it the import is retained and the call is correct.Note
Human-in-the-loop with claude opus 4.8. I iterated with this patch and got to a working component running with a wasi-sdk fork.