Repository navigation
[WASM W1] Define supported profiles and ABI foundation - #2578
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: WASM target profiles + storage lowering
Reviewed the full diff (base 52ae044...head 5a43f6a) across code quality, performance, security, and documentation. This is a solid, well-tested change — the logical-value vs. physical-storage abstraction is coherent, the //go:uintptrescapes GC-root derivation correctly declines to chase integer conversions, and target/profile validation is gated by a closed allow-list. Findings below are inline; a few non-inline notes follow.
Non-inline notes
- Perf (hot compiler paths, all targets): the new storage helpers add uncached work to per-instruction type queries.
GoWordSize()(ssa/storage.go:147) recomputeseffectiveGOARCH()+ string compares on every call (unlike the cachedPointerSize());childStorageType(ssa/storage.go:258) routes everyElem/Index/FieldthroughhasNativeTypeLayout, which allocates apkg.Namestring and takes an RWMutex RLock per call; andwithNativeStorage(ssa/storage.go:171) clones + inserts into an unbounded map keyed by fresh pointer identity, so it never dedupes and the map grows for the whole compilation. Since these fire on native (non-WASM) builds too, consider a cached "wide/native storage active" boolean onaProgram(set once inNewProgram) and an early return before the layout lookups. - Stale references (outside this diff):
internal/build/outputs.go:325andoutputs_test.go:43still list"wasip2"/"wasm-unknown"indefaultAppExt, but this PR deletes those target JSONs and the proposal states they're removed. Dead entries now — worth a follow-up cleanup. - Info (runtime): the wasm32-j32 GC now scans at a 4-byte stride (
scanword_wasm32.go), doubling scan-loop iterations vs. the 8-byte word on that profile. Appears required for correctness (C/host pointers at 4-byte offsets) — flagging as a conscious trade-off, not a defect.
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 |
|
Addressed the non-inline review notes in 5943d5e as well: GoWordSize is computed once in NewProgram; native-storage propagation now returns immediately on ordinary native/J64 targets; canonical types cache and reuse one native view instead of allocating a fresh clone on each query; and a regression test checks both the W1 cache and the native fast path. Removed the explicit wasip2/wasm-unknown output entries and their stale test rows. The 4-byte J32 GC scan stride remains intentional for physical host/C pointers. |
|
@fennoai Please re-review the latest head |
Re-review completePosted a non-blocking PR review of head Requested focus areas — both verified correct:
Prior findings confirmed addressed: instantiation rejection propagation (+ Two non-blocking items raised inline: a latent wasm32-only GC-liveness gap in SSA-level tests need LLVM headers unavailable in this sandbox; those rely on CI per the PR description. |
There was a problem hiding this comment.
Re-review of 5943d5eb0 — WASM W1 profiles and ABI foundation
I re-reviewed the fixed range 52ae044d...5943d5eb0 with the two requested focuses, and ran the four review passes (quality, performance, security, docs).
Requested focus areas — both verified correct:
- CIF lifetime / container layout (
newSignatureStorage).signatureStorage.cifis the first struct field, so&storage.cifroots the whole allocation;args(typed*Typeroots) andslots(the physical 4-byteuint32array libffi retains afterPrepCif) stay reachable for the CIF's lifetime. The wasm32 4-byte slot width matches the read side inindex_wasm32.go(base + i*4, read as*uint32), and J64/memory64 correctly falls through to the default 8-byte path via build tags. The!llgo-taggedTestNewSignatureStorageOwnsArgumentArraypasses. ✅ - Native hot-path fast return (
ssa/stmt_builder.goReturn). The!isNativeStoragebranch reconstructsaggregateValue(rawType(tret), …), andrawType(*types.Tuple)lowers through the sametoLLVMTuple(out)used to declare InGo function return types — so the reconstructed aggregate matches the declared type. The native branch readsFunc.ll.ReturnType()element types directly, matchingtoLLVMNativeTypes. All storage helpers short-circuit onGoWordSize() <= PointerSize()(native targets), andisNativeStorageis only ever populated whenusesWideGoStorage(), so native codegen never enters the wide-storage reconstruction. ✅
Prior findings — confirmed addressed:
- Explicit instantiation rejection now propagates (
validWasmSelection, unsupported-profile/provider errors), covered byTestWasmProfileValidationErrors;wasip2.json/wasm-unknown.jsonremoved. DefaultBuildTagsdropped its unusedgoarch, targetparams; thesizesclosure's unused params are now_, _; native-closure return lowering fixed (toLLVMFuncBackground(entrySig, bg)instead of forced InGo).internal/crosscompileandinternal/targetssuites pass locally (40.6s / 2.6s);git diff --checkclean; no newt.Skip, probes, or debug prints.
(SSA-level tests require LLVM headers not present in this sandbox; relying on CI for those, per the PR description.)
One latent lifetime finding (wasm32-only) and one minor perf nit are inline below. Neither blocks the two focus areas, which are sound.
|
@fennoai Please re-review the latest head |
Re-review complete — follow-up fixes verifiedPosted a non-blocking PR review of head
One non-blocking note: the ffi ownership tests are Verdict: LGTM. |
There was a problem hiding this comment.
Re-review of fcafb8546 — follow-up fixes verified
Both prior findings are correctly resolved, and the lifetime audit was extended to the CIF return type as described. Ran all four review passes on the incremental diff (5943d5eb0..fcafb8546); no blocking issues.
Requested verification points — both confirmed:
- Aggregate/signature first-field ownership containers.
StructOf/ArrayOf/newComplexTypenow route through the newnewAggregateType, which wraps element roots inaggregateTypeStorage{typ Type; elements []*Type; slots []uint32}(wasm32) /{typ Type; elements []*Type}(default).typis the first field, so returning&storage.typroots the whole allocation, keeping the typedelementsroots (and physicalslots) GC-reachable for theType's lifetime — closing the wasm32 use-after-free gap where dynamically-built nested types (e.g. inmfinal_ffi.go) could be collected.signatureStoragenow also retainsret *Type, co-rooted via&storage.cif, closing the analogous return-type lifetime gap. No unrootedType.Elementsproducer remains (only the permanently-rootedtypeEmpty). ✅ - Native alignment fast return.
requireStorageAlignmentnow early-returns on!usesWideGoStorage(). On native targets the skipped work was always a no-op (storageType(t) == t.ll, sowant == haveandSetAlignmentwas never called), so this is a pure overhead removal — three per-alloca calls including a TargetData query — with no behavior change, and the wasm path is unaffected. ✅
New coverage confirmed: TestNewAggregateTypeOwnsElementArray + extended TestNewSignatureStorageOwnsTypes (now asserts ret retention) pass locally; TestWasm32StorageIntegerConversions, TestWasm32PhysicalPointerIndexBounds, and TestSyntheticFieldOutOfRangePanics added for J32 native arrays / signed width / physical indexes / synthetic aggregate fields. git diff --check clean; no new t.Skip/probes.
One non-blocking note: the ffi ownership tests are //go:build !llgo, so they exercise the default container rather than the wasm32 slot-encoding variant (typePointerArrayFromSlots and the uint32 narrowing). The rooting structure is identical across both builds, so the design is validated — but the wasm32-specific encoding remains CI-unverified on the default target. A wasm-tagged test would close that gap if/when a harness is available.
LGTM. SSA-level tests require LLVM headers unavailable in this sandbox and rely on CI per the PR description.
fcafb85 to
db71cc5
Compare
|
@fennoai Please re-review W1 after its conflict-free rebase onto current main 4564a01. The 16-commit range moved from fcafb85 to db71cc5, and git range-diff reports all 16 commits exactly equivalent. Please focus on interactions with the newly merged C ABI aggregate-return change (#2570) and panic/Goexit fixes (#2567), plus confirmation that prior review findings remain resolved. No content changes were made during rebase. |
Rebase re-review completePosted a non-blocking PR review of head
Verdict: LGTM. SSA-level codegen tests rely on CI (no LLVM headers in this sandbox). |
There was a problem hiding this comment.
Re-review of db71cc582 — post-rebase onto main 4564a01aa
Rebase verified clean and the two newly-merged PRs interact safely with W1. No new findings.
Rebase equivalence confirmed. git range-diff 52ae044d..fcafb8546 4564a01aa..db71cc582 reports all 16 W1 commits = (exactly equivalent) — no content changes during rebase, matching the description.
Prior findings remain resolved (verified at db71cc582):
- Aggregate/signature first-field ownership containers (
newAggregateType/signatureStoragewith retainedret) are intact;TestNewAggregateTypeOwnsElementArrayandTestNewSignatureStorageOwnsTypespass. requireStorageAlignmentnative fast return (!usesWideGoStorage()) is intact.internal/crosscompile+internal/targetssuites pass locally.
Interaction with #2570 (C ABI aggregate-return) — safe. W1 does not touch internal/cabi/ at all. #2570's reworked return-slot logic is gated to AttrWidthType/AttrWidthType2, which the wasm classifier (TypeInfoWasm) never emits — wasm aggregate returns take the AttrPointer/sret path, so that code is inert on wasm. Independently, W1's wide-pointer storage {ptr, iN} can never appear in a C-ABI aggregate return: native-storage types are excluded from wide-pointer wrapping (needsWidePointerStorage returns false when isNativeStorage), and nativeStorageLLVMType has no pointer case, so C-background pointer fields stay plain ptr — identical to the pre-W1 layout cabi expects. No path lets W1 layout corrupt the cabi return-slot bitcast/load.
Interaction with #2567 (panic line / Goexit) — safe. W1 does not touch caller.go, unwind_llgo.go, or z_rt.go. In the shared files the two changes occupy disjoint regions (W1: storage/return/phi lowering; #2567: PanicSite/recordPanicSite/nil-check-return), cleanly merged. The one plausible coupling — W1's AddIncoming terminator-move vs. #2567's new formal CreateRet(undef) nil-check failure block — is decoupled by design: AssertNilDeref always retargets b.blk.last to the success block blks[1] before returning, and AddIncoming reads only that success-block tail; it never inspects the failure block's terminator kind. Moreover NeedsFramePointer() is false on wasm (funcinfo.go), so #2567's new CreateRet path and preserveNilCheckCondition are inert on the W1 profiles, and that path bypasses Builder.Return() regardless.
Verdict: LGTM — rebase is content-preserving and integrates cleanly with #2570 and #2567. SSA-level codegen tests require LLVM headers unavailable in this sandbox and rely on CI per the PR description.
db71cc5 to
6b73504
Compare
|
The fork-only macOS comparison completed successfully on both exact W1 head and its base, without sampling, retries, or reduced test pressure; details are now in the validation section. The original upstream signal termination has not been reproduced or fixed. The independently reproduced loss of signal diagnostics in llgo test is isolated in #2600, with a failing-before/passing-after subprocess regression and no runtime changes. W1 has not been changed or pushed for this investigation. |
6b73504 to
cb7214a
Compare
2 participants
Tracks #2152.
R1-R3 are merged. This PR implements W1: the supported hosted-WebAssembly profiles and ABI foundation. Later W2 work owns host integration, reflection, and standard-library completeness; W3 owns broad test/** and GOROOT acceptance.
Scope
The raw JavaScript entry in W1 is still Emscripten-backed. A Go-compatible JavaScript host boundary and portable WASI reflection are later work. W64, WASI Preview 2/WIT, threads, WasmGC, EH, JSPI/stack switching, multi-worker scheduling, and parallel goroutines are outside W1.
Validation
Current head
cb7214ae45fais rebased onto main2db247e43848. The original 16 feature commits remain patch-equivalent; one integration fix updates the newly mergedllgo envcommand to reportWasmProfileandWasmProviderinstead of the removedConfig.WasmABI. The stale field caused the shared compiler-build failure in the previous stacked W2-A CI round. Regression tests cover all named profiles, both target aliases, and a non-WASM target; the modified target-field output function has 100% statement coverage. LLVM 22 compiler construction, completecmd/...tests, and target resolver tests pass locally. Patch coverage is now 96.47% against a 94.38% target. Remaining full CI and the size/performance audit are still required; a passing benchmark job alone is not approval of a size increase.The earlier native macOS
test/gosignal termination on6b7350457dc8remains unproven: paired fork runs passed on both W1 and its exact base without retries or sampling. This rebase fixes the separate deterministicllgo envbuild error, not that intermittent runtime termination. Earlier feature validation and benchmark results below are historical rather than current-head evidence.Post-rebase local checks pass for internal/targets, the complete internal/crosscompile suite, focused W1 SSA/compiler regressions, git diff --check, and a clean worktree. Review-fix checks additionally cover CIF return/argument ownership, aggregate ffi type ownership, instantiation failure propagation, native-storage caching/target gating, build tags/output selection, and real J32, J64, and W32 builds. J32/Emscripten and W32/WASI lifecycle fixtures both pass after the final wasm32 ownership fix.
The equivalent pre-rebase feature range passed focused fork CI:
This upstream PR refreshes CI against the current main rather than treating the pre-rebase runs as final-head evidence.
Size and build audit
Current-head paired benchmark, comparing main
2db247e43848with W1cb7214ae45faon the same runner (WASM module bytes):The goal remains no
cprintfgrowth where possible and only small justifiedprintlngrowth. The Memory32 figures above do not yet meet that goal. Diagnostics show growth distributed across Go64 runtime operations and checked Go64/C32 boundaries, rather than a bulk set of WASI reflection bridges; this identifies optimization targets, not proof that every added byte is unavoidable. W2-B separately reduces caller-wrapper overhead, and the final complete-stack comparison remains required.Earlier W1 same-run benchmark output sizes (base → W1):
The explicit Memory32 increases are the measured cost of switching Go storage from the former 32-bit model to the specified 64-bit Go model; J64 remains effectively flat. Raw js/wasm and wasip1/wasm aliases have larger small-program deltas because main routed them through incomplete compatibility runtimes, while W1 makes them aliases of the complete J32/W32 implementations; those are not like-for-like regressions. The final ffi ownership review fix leaves J32/W32 cprintf and println byte-for-byte unchanged from the prior head; its largest additional change is 754 bytes in J64 fmtprintf (about 0.02%).
Diff audit
The range consists of proposal/profile metadata, target and cache selection, Go64-on-Memory32 lowering, runtime/GC storage adaptation, executable acceptance, and benchmark/coverage support needed to verify them. It contains no diagnostic probes, temporary dependency overrides, or new t.Skip. The audit removed dead provider state and redundant helpers, restored alias regressions, and avoided an unnecessary itab traversal that changed native output.