fix(wasi): size initial memory from static data and heap budget - #2541
Merged
cpunion merged 4 commits intoSep 22, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fix WASI initial heap
Solid, well-tested change. The switch from a fixed --initial-memory=67108864 cap for the single-worker WASI C profile to a --initial-heap=56623104 (54 MiB) default — appended only when the user has not already set --initial-memory/--initial-heap — is a sound approach: it stops static data from eating into the previous 64 MiB total while preserving the effective budget. The Link/LinkArguments/linkArguments split is clean and lets the default selection inspect the effective linker command (prefix + env/config flags + args) without executing it.
I verified the load-bearing details:
- Heap math is correct and consistent.
56623104= 54 MiB, and thewasip1/wasmlink path always sets-z stack-size=10485760(10 MiB) atinternal/crosscompile/crosscompile.go:686, so the comment's "64 MiB total minus 10 MiB stack" (54 MiB heap) holds for every path where the default heap is injected. - wasi_threads keeps its
--initial-memory=67108864via configLDFLAGS, sodefaultWASIHeapArgscorrectly detects it and does not add the heap default. Covered byTestWASIHeapReachesLinker(shared-memory-contract case) and the crosscompile tests. - Detection covers
-Wl,, comma-separated,-Xlinker, direct, CCFLAGS, and LDFLAGS forms, with good false-positive tests (data-initial-memory.o,-Wl,-Map,initial-heap.map).
One known limitation worth documenting (not blocking): the detection deliberately does not expand @response-file contents (asserted by the "response contents not expanded" test). If a user passes --initial-memory=<N> inside a response file, the default --initial-heap is still appended, which — per wasm-ld's initial-memory = stack + static + heap default — could make an explicit --initial-memory too small and fail the link. The code comment mentions response files are left to the driver; a one-line note that explicit memory/heap flags must be passed directly (not via @file) to be honored would help future users.
Minor inline notes below. No blocking issues.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
CI triage: the new Windows x64 MinGW LLGo failure is a dependency-download transport error, not a heap-policy regression. The Windows runtime, stdlib, FFI and network smoke tests completed; the earlier nil-pointer panic is explicitly the expected unrecovered-fault smoke case. The next step failed while downloading github.com/goplus/lib@v0.3.1 from proxy.golang.org: HTTP/2 stream INTERNAL_ERROR from the peer, before test/buildcache could compile. See job https://github.com/xgo-dev/llgo/actions/runs/34255224036/job/102159518324 . No source change or blanket retry is being added for this failure. An upstream maintainer can rerun the failed job; upstream Actions have not been modified. |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/fix-wasi-initial-heap-20260909
branch
from
September 11, 2026 05:01
865c7c5 to
93a0584
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
force-pushed
the
codex/fix-wasi-initial-heap-20260909
branch
2 times, most recently
from
September 14, 2026 05:32
c4d3700 to
a7997fc
Compare
Collaborator
Author
|
@fennoai Review again after rebasing onto the latest main. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Review (post-rebase)
Re-reviewed at head a7997fc on the fixed range 5c58743...a7997fc. The core WASI initial-memory fix is sound and the change is well-reasoned, with unusually thorough test coverage (nested response files, -Xlinker split options, CCFLAGS/LDFLAGS parsing, and the zero-heap boundary are all exercised).
Verified as correct (no action needed):
- The
56623104heap constant is exactly 54 MiB (64 − 10 MiB stack); with--initial-heapand no explicit--initial-memory, wasm-ld sizes initial memory asstack + static data + heap, matching the "64 MiB plus static data" comment. - The
gcWasmGrowMemoryreturn-value polarity is consistent across both call sites:_wrap/gc_wasm.creturns 1 on success / 0 on failure, so the bootstrap branch (!= 0→ success) andgcGrowMemory(== 0→ failure) both read it correctly. The grow-failure path is intentionally caught downstream byinitGC's "gc: invalid heap range" panic (asserted by the shell test). - Response-file contents are deliberately not parsed; any leading
@token cedes memory policy to the user, avoiding a nested-response-file trust-boundary problem. Overflow guards ingcGrowMemoryare sound.LinkArgumentsinspection performs no file I/O for WASI targets.
Findings below are minor quality/maintainability items — none block merge.
Additional findings
cl/compile_test.go:40: [P3] Unrelated LTO-plugin change bundled into a WASI-memory PR: This context-timeout + exit-code-1 assertion (paired with thereport_fatal_error→reportFatalUsageErrorswap inLLGOInterfaceMethodTypeIDPass.cpp) is good hardening, but it is unrelated to WASI initial-memory sizing. Bundling an independent LTO-plugin behavior change hurts reviewability and bisection. Consider splitting it out, or (as the PR description does note the dependency corrections) calling it out explicitly as a separate concern.
Collaborator
Author
|
@fennoai Review again after the two latest review-comment updates. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Review (head 63b2d5c)
Re-reviewed after the two review-comment updates. The delta since the last reviewed head (a7997fc) is exactly two cosmetic changes on the files I commented on — both directly address the prior findings:
- ✅
wasm_memory.go:30— Added the cross-reference// The 10 MiB value matches -z stack-size=10485760 in crosscompile.go., making the cross-package coupling of the 54 MiB heap default to the 10 MiB stack size discoverable. (Comment-only; the56623104= 54 MiB value is unchanged and still correct.) - ✅
test_wasm_single_worker.sh— Extracted a namedwasm_page_size=65536local and used it in the boundary check, replacing the bare magic number.
Both changes are documentation/refactor only, with no behavior change, so the core WASI initial-memory fix verified at the prior head still holds. No new issues.
Remaining (optional, non-blocking, unchanged from prior review): the LTO-plugin hardening in cl/compile_test.go + LLGOInterfaceMethodTypeIDPass.cpp is still bundled into this WASI-memory PR. Splitting it out (or calling it out explicitly) would improve reviewability/bisection, but it is a reasonable dependency correction to carry if that's intentional.
LGTM.
Collaborator
Author
|
Final validation on 63b2d5c: all 64 checks passed, no pending or failed checks, and Codecov confirms all modified coverable lines are covered. Both wasm-runtime jobs passed, as did Linux/macOS/Windows coverage, the full LLGo matrix, benchmarks, and release-artifact execution tests. LLGo workflow: https://github.com/xgo-dev/llgo/actions/runs/34810489181 . Release workflow: https://github.com/xgo-dev/llgo/actions/runs/34810489059 . The branch is based on main 5c58743; the requested stack-size cross-reference and named Wasm page-size constant are implemented, the latest fennoai review covers the current head, and all review threads are answered and resolved. No extra coverage-only changes or workflow relaxation were necessary. |
cpunion
force-pushed
the
codex/fix-wasi-initial-heap-20260909
branch
from
September 18, 2026 01:18
63b2d5c to
ea838b5
Compare
visualfc
approved these changes
Sep 22, 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.
Summary
Fix a main-branch WASI linker limit exposed by expanded WebAssembly acceptance: a legal program with 100 MiB of static data cannot link because the toolchain forces an exact 64 MiB initial memory size.
For single-worker WASI, reserve the existing 54 MiB heap budget after static data and the 10 MiB process stack with
--initial-heap, allowing wasm-ld to size initial memory from the actual layout. Keep the WASI pthreads backend's explicit 64 MiB shared-memory contract unchanged. This does not add threads support or change maximum memory or stack sizes.The final-link default is omitted when effective driver arguments already specify initial-memory, initial-heap, or a user-authored response file. Package/config/environment/linker-prefix options remain authoritative.
clang.Cmd.LinkArgumentsis a small extraction of the existing argument merger, so inspection and execution use the same flags; no unrelated FuncForPC relinking code is included.An explicitly empty initial heap is valid even when static data ends exactly at a Wasm page boundary. Initialize it by growing one page before GC metadata setup; normal heap growth then applies. If the host refuses growth, retain the explicit initialization failure. Baremetal is unchanged.
Validation and CI coverage
linkObjFiles → linker subprocessregressions verify that defaults actually reach the production command and explicit/shared-memory flags are not lost. They run in the existing main Go coverage workflow, with no new CI job or SDK dependency.--max-memory), requiring exit 2 and the precise initialization diagnostic. All three pass locally. These links reuse package caches; changing heap flags did not rebuild the internal runtime/TinyGC archives.fixedbugs/issue34395.gobuilds and runs through rawGOOS=wasip1 GOARCH=wasm(wasm32 with WASI Preview 1 host, using that historical revision's data model) with clang/LLD 22 (7.77s for the case). This is local focused evidence, not a claim that the new PR's CI has finished.Response-file policy
User-authored response files own the memory policy: do not inject the 54 MiB default alongside an opaque file that may specify its own exact initial memory or zero heap, including in nested files. Contents and quoting remain the external driver's responsibility. An object-only response file therefore starts with page-rounding slack (possibly zero) and grows memory when needed. LLGo's automatic response-file creation happens after inspection and does not opt out of the default.
The underlying failure exists on main and is not waived with a GOROOT exclusion or a larger timeout.
Rebase validation (2026-09-14)
Rebased the three WASI commits onto current main
5c5874359. Main now contains the descendant-probe corrections, so Git dropped both obsolete goroot dependency commits. The branch still carries the independently reviewed #2562 malformed-LTO-metadata fix as one dependency commit; its patch-id is identical to the rebased #2562 head and it can be dropped from this PR once #2562 lands on main.The
dev/test_wasm_single_worker.shconflict was resolved by preserving main’s TinyGC host test inside theruntime/allsuite before the target fixtures; the newgc-heapsuite remains limited to the dedicated empty-heap checks. Focused WASI heap/default/linker-argument tests, the completeinternal/clangandinternal/crosscompilepackages, and the nested TinyGC tests pass locally. Fresh CI is running on the rebased head.