runtime/wasip1: discover current pthread stack top for GC - #2666
Merged
Merged
Conversation
cpunion
force-pushed
the
codex/wasi-threaded-stack-bounds
branch
from
September 24, 2026 06:19
619a781 to
c97d9e4
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Solid, well-scoped experimental WASI-threads enablement. The build-tag partitioning across the nanotime_*/time_*/sema_* files is exhaustive and non-overlapping, the per-thread GC stack-bound helper (gc_wasm.c) has rigorous overflow/NULL guards and traps cleanly on failure, and the -tags nogc gate with its actionable error message is a sensible guardrail until a threaded collector exists. The Realloc change correctly moves the metadata read inside the lock (an improvement) and fixes the zeroSizedAlloc sentinel path so realloc(malloc(0), n) no longer walks block metadata from the sentinel. The new iwasm cache-id keying and the WASI-threads probe test are good hardening. Findings below are minor/non-blocking.
Note on Realloc (gc_tinygo.go:388): the Memcpy/freeObject after unlock remains a copy-after-unlock window, but this PR does not regress it (the unlock actually moved later than before) and the inline comment correctly flags it as future work for the not-yet-existent threaded collector. No action needed now; worth a tracking note when concurrent collection lands.
Findings without inline locations
runtime/internal/lib/runtime/time_wait_unix_llgo.go:20: Misleading panic message on the WASI-threads path. This file now compiles forwasip1 && llgo.wasi_threads, wherec_timerCondInitroutes tollgo_timer_cond_init, which deliberately initializes the condition with the default clock (notCLOCK_MONOTONIC) because WAMR 2.4.5 workers can't readCLOCK_MONOTONIC—llgo_timer_cond_timedwaitusesCLOCK_REALTIMEthere. Calling it a "monotonic timer condition" is inaccurate for this build. Consider "failed to initialize timer condition variable" so the message stays correct across clock domains.
| // CLOCK_MONOTONIC there. Use one clock domain on every M so deadlines | ||
| // created on one thread can be consumed by the timer thread. | ||
| if ct.ClockGettime(ct.CLOCK_REALTIME, tv) != 0 { | ||
| return atomic.LoadInt64(&wasiThreadNanoLast) |
Contributor
There was a problem hiding this comment.
On the first-ever call, if ClockGettime(CLOCK_REALTIME) fails, wasiThreadNanoLast is still 0 and this returns 0 as a legitimate timestamp — any deadline computed against it would be wildly wrong. This mirrors the existing single-worker fallback (which also returns 0), so it's not a regression, but a CLOCK_REALTIME read failure on a live worker is unexpected enough that trapping (like the stack-bound helper does) may be preferable to silently returning epoch-relative garbage. Non-blocking.
| if atomic.CompareAndSwapInt64(&wasiThreadNanoLast, last, now) { | ||
| return now | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: this publishes to the shared wasiThreadNanoLast via CAS on every forward clock tick, making that global a cross-core cache-line hotspot under multi-thread load (nanotime is called from the scheduler, timers, and sema spin paths). The fast path (now <= last) is correctly a load-only, no-store return. If contention shows up in profiling, consider a single unconditional CAS-and-return-now when now > last (the loop's retry is only needed to correct an observed backward jump). Correct as-is; flagging for later.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
This was referenced Sep 24, 2026
cpunion
force-pushed
the
codex/wasi-threaded-stack-bounds
branch
from
September 26, 2026 14:39
4b03c7a to
c4a6fbe
Compare
cpunion
force-pushed
the
codex/wasi-threaded-stack-bounds
branch
from
September 27, 2026 15:06
c4a6fbe to
8db4345
Compare
Collaborator
Author
|
Thanks for the review. Commit |
visualfc
approved these changes
Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The linear-memory collector uses
__stack_highas the stack top on WASI. That symbol describes the main thread, so collecting from a WASI pthread would scan the wrong stack and could reclaim live objects. Resolve worker stack bounds through WASI libc; retain__stack_highfor the main thread, for whichpthread_getattr_npreturnsENOSYS. Trap if another stack-bound lookup fails.The WAMR
nogcprobe compiles and calls the collector helper on both the main and worker threads. It now requires pthread stack discovery on workers, so a worker cannot pass by falling back to the main-thread stack top. Thenogcgate remains until thread root registration, stop-the-world coordination, and allocator locking in #2669 are complete. This PR builds on merged #2653.Validation: focused WASI build tests and runtime compilation pass; the WAMR probe completed on macOS, although repeated runs with the local WAMR 2.4.5 binary were intermittent both before and after this probe change. The earlier Windows MinGW TLS timeout passed on rerun. A later run canceled the Linux Dev LTO demo after its one-hour limit while downloading the default PyTorch CUDA dependencies; the demo needs only CPU tensors, so Linux now installs PyTorch from the official CPU-only index. The CPU-only setup and Dev LTO demos passed on the next CI run. A separate README link check received a transient GitHub 503 while following a file URL written with
/tree/; it now links directly to/blob/, and local lychee checked all 96 README links successfully. On the latest head,Dev LTO GlobalDCE and Go Method Dropanddoc_verifyboth pass; the remaining matrix jobs are still running.