Repository navigation
Conversation
|
@fennoai Please review the intended W2-B range |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review
Comprehensive review of the WASM (W2) runtime lowering PR across code quality, performance, security, and documentation. Overall the change is high quality: the wide-Go-word storage abstraction is coherent, load/store/atomic/struct-init paths are consistent, the JS/host byte-copy boundary is bounds-checked and type-validated, integer narrowing on the wasm32 ABI is guarded by runtime/compile-time checks, and the documentation (doc/wasm-proposal.md, benchmark/stdlib READMEs) matches the implementation. No security or documentation issues found.
The findings below are one performance concern in the per-GC-cycle finalizer scan (inline) plus a few maintainability/consistency notes.
Additional notes (no reliable inline location)
ssa/reflect_bridge.go(wasmReflectSlot/wasmReflectFrame) — GEP index constants useprog.Int().ll(i64 on the wide-storage profiles) as the index type, while dynamic pointer indices elsewhere are deliberately narrowed viaphysicalPointerIndexto avoid leaving i64 addresses on memory32 loads at -O0. These indices are compile-time constants so the backend folds them, but a comment (or using a physical-width constant) would keep the invariant uniform.internal/abi/large.go(LowerWasmAggregateCopies) — The fixed-pointfor {}loop relies on an implicit termination argument (each pass strictly reduces remaining large aggregate loads as projections descend nested types). A brief comment asserting monotonic progress would make the termination explicit.cl/gcroot.go— The//go:uintptrescapesspecial case (forcingGCRootCount = 1inprepareGCRootsand storing the raw uintptr viaSetGCRootinpublishGCRoot, bypassingGCRootPointers) couples two functions implicitly. A cross-referencing comment or shared helper would reduce drift risk.
Reviewing PR #2580 (W2-B range)
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
W2-B range review (7d63156a1fa5..a82726740fba, 84 files, +3867/-166)
I reviewed the intended W2-B range across all four dimensions (code quality, performance, security, documentation), focusing on the areas you called out. Overall this is careful, well-tested work — I found no blocking correctness or security defects, and the reported size/build claims match the implementation. Two small items below (one worth confirming, one comment nit), plus a few non-blocking observations.
Focus-area verification (all confirmed sound):
- Logical-goroutine ownership —
releaseGoroutineLocalBlocksclears onlyblock.cacheSlot = niland never writes through*cacheSlot, so it can't corrupt the shared process-global GLS key (a real hazard thatreleaseLocalBlocksavoids). GLS context is released on the scheduler after the G is_Gdeadin bothproc_wasm.goandproc_wasip1.go— the correct point. - Finalizer/GC — the new registry-driven scan drops the Phase-3 walk from O(heap) to O(registered callbacks);
preserveFinalizableObjectsvisits each object once per cycle and defers cleanups-after-finalizers to a later collection.NumGCexposure is consistent acrossgc.go/gc_tinygo.go/runtime_gc_nonmoving.go. - Panic/caller fidelity & Memory32 bounds — traceback and
Caller/Callersiterate the logical shadow stack (store.stack), bounded by the live instrumented frame count;PopCallerLocationFramevalidatesmarkagainstlen(store.stack)before slicing. No OOB. - wasm32 physical-address overflow —
checkUnsafeBuiltinBounds(ssa/expr.go) computesremaining = maxAddr - addrin physical-width space withmaxAddrclamped to the pointer width; the unsigned subtraction can't underflow and the byte-size multiply can't wrap. Sound for J32-over-wasm32. - Aggregate-copy root timing / SjLj locality — the 4 KiB
MinWasmAggregateCopySizethreshold is applied consistently inabi/large.go,cl/gcroot.go(root safepoint accounting), and validated by the4095→false / 4096→trueboundary tests. TheLowerWasmAggregateCopiesfixpoint terminates (each pass exposes only strictly-nested loads).localizeWasmStackAddressesonly rewrites constant-index GEPs rooted at allocas and preserves volatility/ordering/alignment. - Fixed Fiber budgets — 128 KiB Memory32 / 256 KiB Memory64 match
stack_default*.go, with the Asyncify buffer tied to the same budget. - Std-lib accounting — the six profiles agree across
dev/wasmstdlib/README.md,main.go, and.github/workflows/wasm-stdlib.yml; all six acceptance witnesses exist in theirtest/stdpackages.
Non-blocking observations (no change required):
cl/gcroot.go(isGCSafepoint): every aggregate load ≥ 4 KiB on Wasm is treated as a GC safepoint, but the single adjacent load/store pair lowers to amemmovewith noAllocU(no new safepoint), so those loads get root frames the lowering never needs. Safe (extra roots, not missing), minor shadow-frame overhead.internal/abi/large.go(allocResult): reuses an existingruntimeAllocUdecl but builds the call against a locally-derivedintTypewidth. Safe becausegoWordSizeis fixed per module; an assertion orfn.GlobalValueType()reuse would make the coupling explicit.dev/wasmstdlib/main.go:not-runpackages carry two different reason strings ("outside this acceptance slice; not validated"vs. the preflight"source selection not completed; not validated") for the same status — cosmetic.panic_traceback_wasm.go:printWasmPanicTracebackreadsgetg().goidafter guarding onlystore != nil; no realistic nil-getg()path on the fatal-panic path, noted for completeness.
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
|
@fennoai Please re-review the incremental range |
Re-reviewing PR #2580 (incremental)
|
There was a problem hiding this comment.
Re-review: incremental range a82726740fba..311a615707b9 (7 files)
All prior findings are resolved. I read the full incremental diff and traced the semantics of each change; the fixes are correct and backed by targeted tests. No new issues found. I could not run the tests locally (this sandbox has no LLVM C headers, so the cgo-bound packages don't build); the review below is from source inspection, and the PR reports the focused lifecycle tests passing on J32/J64/W32 CI.
[P2] Wasm C-export wrapper aggregate lowering — resolved.
buildMainLink now calls the new lowerMainCExportModule, which for GOARCH == "wasm" runs lowerWasmAggregateCopies, applySizeOptimizationAttributes, the LLVM optimization pipeline, and localizeWasmStackAddresses — matching compilePackageModule. The opt pipeline is nicely factored into a shared optimizeLLVMModule used by both paths, and errors now propagate instead of being dropped. TestLowerMainCExportModuleWasmCopies builds an 8 KiB-array export wrapper and asserts the load/store [8192 x i8] are replaced by @llvm.mem*, i.e. the copy pass actually ran on the wrapper module.
[P3] Stale finalizer comment — resolved, and the loop is now genuinely linear.
The rewrite replaces the O(D·N) nested scan (and its record = finalizers restart) with a single-pass unlink over finalizers, deleting candidateForObject/earlierFinalizerForObject/hasCandidateFinalizer/finalizerObjectBlocked/queueCallbacksForObject. I checked the behavior is preserved:
- Dropping
earlierFinalizerForObjectin the dependency scan is safe —startMarkoperates on the object block and is idempotent under theblockStateHeadgate, so starting from every candidate finalizer of an object equals starting once. noteFinalizerReference→markFinalizerObjectBlockednow gates onrecord.candidate; sinceblockedis only ever read on candidate records, this is equivalent to the old outercandidateForObject != nilguard.- The two-phase "unlink all eligible, then
startMark" split correctly resurrects only this pass's newly-queued records —readyFinalizersis a LIFO stack and therecord != readyBeforebound walks exactly the records pushed in this pass.candidateimpliesfinalizerActive(set at the top), so the oldstate == finalizerActivefilter is preserved.
TestCandidateTraversalScalesLinearly (1,024 records, instrumented registryReads) makes the linear-scaling claim deterministic rather than timeout-based — a good approach.
Note items — addressed. The uintptr-root single-slot reservation now has paired invariant comments in cl/gcroot.go (prepareGCRoots ↔ publishGCRoot), the LowerWasmAggregateCopies fixed-point termination is documented as monotonic in internal/abi/large.go, and the reflect constant-GEP folding is cross-referenced in ssa/reflect_bridge.go (wasmReflectSlot/wasmReflectFrame).
LGTM.
311a615 to
a50fd7c
Compare
|
@fennoai Please re-review the rebased intended W2-B range |
Re-reviewing PR #2580 (rebased W2-B)
|
There was a problem hiding this comment.
Re-review: rebased W2-B range 8a7f17a8a227..a50fd7c6aee5
Verified via git range-diff against the range I reviewed and approved last round. 25 of 26 commits are exact range-diff matches (=); the only changed commit is 7827393e2 fix(wasm): preserve single-worker Go semantics, and its sole textual delta is the panic-site/nil-guard reconciliation described in the request. No new issues found. (As before, I couldn't execute tests locally — this sandbox lacks LLVM C headers so the cgo-bound packages don't build; the reconciled commit is small and verified by reading. The PR reports its W2-B-specific and upstream native panic regression tests passing.)
Reconciled commit — correct. In the field-store path (cl/compile.go:2270-2284) the change computes wasmNilGuard := p.needsWasmNilGuard(va) once, then:
- WASM (
wasmNilGuardtrue): records the panic site once, emits the base check + explicitAssertNilDeref; the native branch is skipped by!wasmNilGuard. - Native (
wasmNilGuardalways false, sinceneedsWasmNilGuardis gated onGOARCH == "wasm"): records the panic site iff!isKnownNonNilAddr && !isWrapNilCheckCall, preserving main's hardware-fault panic-site behavior; no explicit guard.
The two recordPanicSite branches are mutually exclusive, so the naive union's double-record can't occur. I checked all four combinations: on WASM with a provably-non-nil address, needsWasmNilGuard is false and the native branch's !isKnownNonNilAddr && !isWrapNilCheckCall is false, so nothing is recorded — correct, since such a store can't fault. Logic is sound across the matrix.
Prior findings remain resolved. git diff 311a615707b9 a50fd7c6aee5 over runtime/.../tinygogc/finalizer.go, internal/build/build.go, internal/abi/large.go, cl/gcroot.go, and ssa/reflect_bridge.go is empty — these are byte-for-byte identical to the versions confirmed resolved last round:
- Finalizer ready traversal is the single-pass linear form (the O(D·N) helpers
queueCallbacksForObject/candidateForObject/hasCandidateFinalizer/earlierFinalizerForObject/finalizerObjectBlockedare gone). lowerMainCExportModulestill runslowerWasmAggregateCopies+ opt pipeline +localizeWasmStackAddresseson wasm export wrappers.- Medium-aggregate root timing and the documentation/invariant comments are unchanged.
LGTM. The rebase is clean and the single reconciliation is correct.
|
@fennoai Please re-review the incremental range |
Re-reviewing PR #2580 (incremental)
|
There was a problem hiding this comment.
Re-review: incremental range a50fd7c6aee5..dff40ac32f8b (1 commit, 10 files)
Reviewed the single commit dff40ac32 fix(wasm): preserve panic caller identity in full. Both regression fixes are correct and well-covered. No new issues found. (As in prior rounds I couldn't execute tests locally — no LLVM C headers in this sandbox; verified by reading. The PR reports the new scheduler coverage passing on J32/J64/W32.)
Prefix lifetime — correct. capturePanicCallerFrames freezes the prefix in place by recording panicDepth = len(store.stack) (no copy). PopCallerLocationFrame (caller.go:87-91) only raises mark up to panicDepth, so a repanic longjmp can't truncate below the frozen prefix. The prefix is released by clearPanicCallerSnapshot() (panicDepth = 0) on every recover-completion path — EndRecoverFrame, EndRecoverFrameAlias, and abortPanics (the latter also covers Goexit/teardown). Both PopCallerLocationFrame and activePanicCallerFrames/panicCallerSnapshotAvailable additionally guard panicDepth <= len(store.stack), so a stale depth exceeding a shortened stack is safely ignored rather than reading OOB.
Replacement / later-same-value — correct. Retention requires raw interface identity (_type + data, not Go equality, so uncomparable values still match) and p.recovered.frame == gp.recoverFrame != nil. Since p.recovered is armed only in Recover and zeroed in EndRecoverFrame*/abortPanics, a later panic with a coincidentally-identical value after the recovering activation has returned sees recovered.frame == nil → treated as new → fresh panicDepth. A different value during an active repanic window replaces the prefix. Both behaviors match Go's same-value repanic semantics.
PC sequence wrap / lookup — correct. The GLS nextCallerPCBase (caller_gls.go) draws a process-unique value from an atomic counter, so goroutine-local synthetic PCs no longer collide in the process-wide FuncForPC cache — the root cause of the second regression. seq << 2 with the seq == 0 || seq > ^uintptr(0)>>2 guard fatals on exhaustion instead of wrapping into a colliding PC (correct on 32-bit wasm uintptr). Because sequences are now sparse per store, syntheticFrameForPC correctly switches from the old dense seq-1 index to a binary search over PC &^ callerPCMask; the slice stays sorted since assignment is monotonic even under goroutine interleaving. capturePC/captureFrameAt consistently use the local frame.captured-1 index while encoding the global base from s.synthetic[idx].PC.
Bare-metal isolation — correct. caller_global.go provides a non-atomic single-store nextCallerPCBase (len+1), and panic_snapshot_default.go (//go:build !wasm) keeps capturePanicCallerFrames/clearPanicCallerSnapshot as no-ops with panicCallerSnapshotAvailable reading the native panicPCs.n. Native same-value repanic detection stays in SavePanicCallerFrames, unchanged. On WASM, capturePanicCallerFrames runs first in Panic and authoritatively consumes p.recovered for the shadow-stack mechanism the WASM traceback actually uses; the native snapshot path (only linked with unwind_llgo) then sees a zeroed recovered but that path is unused for WASM tracebacks, so there's no conflict.
Coverage. testCallerCacheIsolation asserts FuncForPC(Caller(0)).Name() resolves per-goroutine across two goroutines (exercises the collision fix); repanicTracebackCaller drives a nested same-value repanic and asserts the traceback origin is the original main.repanicTracebackOrigin site (exercises the prefix fix). Both wired across the three profiles via LLGO_WASM_SCHEDULER_REPANIC_TRACEBACK.
The reported +0.65% raw / +0.63% gzip incremental size is consistent with what the diff adds (one atomic global, a binary-search lookup, and small snapshot bookkeeping; the prefix is frozen in place with no per-panic copy).
LGTM.
|
@fennoai Please review the final test-only incremental range |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Re-review: test-only incremental range dff40ac32f8b..748483792502 (1 commit, 2 files, +17/-2)
Reviewed 748483792 test(wasm): leave fatal subprocesses to the host. Clean, correct, minimal. No issues found — and this round the change is pure-Go test code with no LLVM dependency, so I was able to run the tests: both pass on the native target.
Build-tag boundary — correct. The two subprocess-driven entry points are renamed to unconstrained helpers testCallerPanicTraceback/testCallerRepanicTraceback in caller_runtime_test.go (no //go:build line, compiles on every target), and re-exported as TestCallerPanicTraceback/TestCallerRepanicTraceback from the new caller_panic_native_test.go gated //go:build !wasm. So on native the tests run exactly as before; on the WASM guest the TestXxx entry points simply don't exist — no t.Skip, no runtime-behavior change, matching the stated approach. Package name (gotest) matches and all imports (os, os/exec, …) remain used by the retained helpers.
Self-re-exec stays consistent. testCallerRepanicTraceback re-launches the test binary with -test.run=^TestCallerRepanicTraceback$ (caller_runtime_test.go:175) to drive its child panic modes. Since that whole path only executes on !wasm and the entry point exists there, the child filter resolves correctly in the same binary.
Native coverage intact. The helper bodies are unchanged (only the signatures were renamed), and go test ./test/go/ -run '^TestCallerPanicTraceback$|^TestCallerRepanicTraceback$' passes both. go vet ./test/go/ compiles the package with no new diagnostics on the two touched files (the pre-existing vet notes are in unrelated files). WASM traceback behavior remains exercised by the host-driven J32 Emscripten / J64 Memory64 / W32 WASI scheduler fixtures (the panic/repanic traceback fixtures added earlier in this stack), so gating the in-guest subprocess harnesses loses no WASM coverage — it removes an invalid pipe/ENOSYS call the Go-compatible WASM process contract can't satisfy.
LGTM. This completes my review of the stack.
1 participant
Tracks #2152. Stacked on #2579; review this PR's 28-commit W2-B range after W2-A.
This combines the single-worker runtime/standard-library completeness layer with the aggregate/SjLj lowering it requires for realistic WASM packages. It deliberately excludes the broad
test/**and GOROOT acceptance inventory, which remains W3.Scope
runtime.MemStats.NumGC, and schedule pending cleanup work after an explicit collection.test/stdpackage as unvalidated or source-excluded.Root cause and boundary
The runtime half closes observable single-worker correctness gaps found by host, GC, panic, and standard-library acceptance. The lowering half is required by that acceptance: W32
go/typespreviously produced a roughly 14.2 MBgo/types.(*Checker).builtinfunction after LLVM scalarized aggregate loads and SjLj extended stack-slot liveness, exceeding Wasmtime's function-body limit. The final lowering shares its 4 KiB threshold with frontend safepoint analysis, roots the source before allocation and the snapshot afterwards, and does not raise a host limit.The post-rebase W3 acceptance run also exposed two caller-state bugs in newly merged main tests. A recovered same-value panic could lose its original WASM shadow-stack prefix after nested longjmps, while goroutine-local synthetic PC sequences collided in the public runtime's process-wide
FuncForPCcache. The follow-up freezes the existing shadow-stack prefix without copying it, retains it only for the matching recover activation, and assigns process-unique synthetic PCs on hosted LLGo targets; bare-metal and host-tool builds keep the non-atomic single-store path.Current head is
748483792502, based on W2-A head8a7f17a8a227(28 commits, 92 files, +4,216/-272). Twenty-five original dedicated commits retain exact one-to-one range-diff equivalence; one commit reconciles main’s broader native panic-site recording with W2-B’s explicit WebAssembly nil guard, the caller follow-up addresses the runtime regressions found by stacked acceptance, and the final test-only follow-up keeps subprocess-based fatal checks out of WASM guests that correctly returnENOSYSforpipe.Validation
llgo testgate, Ubuntu instrumented coverage, and WASM benchmark. Codecov reported 100% coverage of modified coverable lines.llgo testgate, Ubuntu instrumented coverage, and WASM benchmark. Codecov again reported 100% coverage of modified coverable lines.go/typespasses; the pre-Asyncify module falls from about 36 MB to 12 MB and its largest function falls from about 14.2 MB to 400,916 bytes.clandinternal/buildtests, upstream native panic regression tests,ssa,internal/abi,internal/locality/layout,dev/wasmstdlib, relevant runtime packages, workflow actionlint, shell syntax, andgit diff --checkpass.cl/ssa/internal/buildtests, the complete caller/statement-line order on J32 Emscripten, and normal plus nested same-value-repanic scheduler fixtures on J32 Emscripten (wasm32 with Emscripten/JavaScript), J64 Emscripten Memory64 (wasm64 with Emscripten/JavaScript), and W32 WASI (wasm32 with WASI Preview 1).t.Skip, TODO/FIXME, diagnostic probes, temporary dependency changes, or undocumented package exclusions.Size and build-cost audit
The runtime layer adds about 4 KiB (3.0%-3.5%) to optimized tiny
cprintf/printlnWASM modules for source-aware panic/caller support and logical goroutine ownership; generated Emscripten glue is unchanged. Larger programs improve:fmtprintfshrinks 7.3%-9.1%,reflectcallshrinks 11.4%-13.6%, and the measured W32 reflection cold build falls from 32.03 s to 16.81 s.Against the previous W2-B head, the caller follow-up changes an unchanged J32 Emscripten
wasm-runtimemodule from 407,190 to 409,817 bytes (+2,627, +0.65%); gzip size changes from 154,947 to 155,919 bytes (+972, +0.63%), and generated JS changes by 4 bytes. It allocates no panic snapshot and adds no atomic dependency to bare-metal builds.The aggregate/SjLj layer adds zero bytes to tiny programs and makes J32/J64 large modules effectively unchanged or up to 100 bytes smaller. W32
fmtprintfshrinks 3.86%-3.97% and W32reflectcallshrinks 2.42%-2.62%; every paired cold-build sample improved by 0.50%-5.30%. The consolidated benchmark will refresh these measurements on the final stack.Review boundary
The intended review range is
8a7f17a8a227..748483792502. It covers single-worker scheduler/runtime semantics, tinygogc finalizer accounting, fixed-stack sizing established by acceptance, the focused standard-library slice, bounded aggregate/SjLj lowering, and the final panic/caller identity corrections. It does not claim the complete applicabletest/**or GOROOT corpus; W3 owns that final acceptance and classification.