Repository navigation
Conversation
|
@fennoai Please review the intended W3 range |
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 summary
Reviewed the substantive non-test changes across the SSA reflect-bridge / storage / GC-root passes, the cl uintptr-escapes & locality passes, internal/build WASM passes, and the reflect / syscall-js / tinygogc runtime, plus the new CI workflows, dev scripts, and C/C++ wrap code. Also reviewed the changed markdown against the code it describes.
This is a large, carefully written PR. The tricky encodings (bijective base-61 bridge IDs, uint64Hex/itoa, GC head-cache complemented indices, wide-pointer storage/atomic round-trips) all trace correctly, and the reviewers rejected several plausible-looking false positives on close inspection.
No blocking correctness, security, or performance issues were found.
- Security: New workflows (
wasm-acceptance.yml,wasm-stdlib.yml) correctly usepull_request(notpull_request_target) withcontents: readand no secrets. Downloads are pinned + SHA-256 verified. Shell/mjs scripts quote expansions and use argv-formexec.Command; JS↔WASM boundary copies are bounded and re-acquire memory views after growth. No injection or overread introduced. - Performance: Expensive compiler-pass work is memoized/bounded (
sync.Oncereachability, shape-keyed bridge cache, precomputed caller-tracking). No hot-path regressions. - Docs: Documented commands, flags, profile names, and build counts match the implementations. Removed targets (
wasip2.json,wasm-unknown.json) confirmed gone and consistent withdoc/wasm-proposal.md.
A few low-severity, advisory notes are inline below. None require changes to merge.
Additional findings
runtime/internal/lib/syscall/js/func.go:49: [P3] nextFuncID has no wraparound guard:nextFuncIDis incremented and used asfuncs[id] = fnwith no wraparound check or existing-key probe; after 2^32FuncOfregistrations the counter wraps and could silently alias a still-live id later resolved indispatchCallback. Practically unreachable and matches upstream Go's behavior, but a short comment (or guard) documenting the assumption would help.
Reviewing W3 range
|
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 |
There was a problem hiding this comment.
W3 acceptance review — 311a615707b9..04f0aac65bb8
Reviewed the intended 32-commit W3 range (96 files, +4,041/-471). Ran all four review lenses (quality, performance, security, docs). This range is test/tooling/CI/doc only — no production runtime or compiler changes, so it cannot affect release size or runtime behavior. Answers to your specific concerns:
Can full package discovery hide source exclusions or skips? No, by construction. discover/discoverFull walk the source tree rather than trusting go list, so native build tags can't silently drop a package (dev/wasmstdlib/main.go, full.go). sourceSelection hard-errors on any package outside the expected prefix, and every non-run status (source-excluded, unresolved, not-run, other-shard, incomplete) counts as a shard failure rather than a pass (full.go ~L361). Backed by TestFullAuditDoesNotHideSourceSelectionErrors, TestFullAuditClassifiesUnknownSelectionAndWitnessFailures, TestFullDiscoveryIncludesRootAndExcludedSource.
Are GOROOT xfail / not-applicable classifications narrow and auditable? Mostly yes — entries are literal paths with specific, per-entry reasons, and TestRepositoryExpectationsAreSeparated enforces reason prefixes, global-scoping of not-applicable, and cross-file de-duplication. One hardening gap (below).
Do fatal-child and artifact checks reject false positives? Yes. The witness validators (validateFullPanic/FinalizerInvalid/GoexitLifecycle/BuiltinPrint in hostcheck.go) all require an *exec.ExitError with code 1–2 and a witness string, explicitly rejecting exit-0, wrong-line, and reversed-ordering cases (TestFullFatalValidatorsRejectFalsePositives). The shell harness expect_failure similarly requires exit code 2 plus a witness; run_emscripten/run_wasi require both wasm-tools validate and a grep witness.
Are all four profiles executed without multiplying provider-independent GOROOT work? Confirmed. The full recursive GOROOT corpus runs once in the goroot job pinned to PROFILE: J32-GoJS across 4 shards (coverage mode). The four-profile matrix (goroot-smoke) is bounded to two sentinels (^(helloworld\.go|bom\.go)$, ci mode), and packages is a bounded 4×2 matrix. No accidental full-corpus fan-out. TestFullChildCommandProfiles/TestFullProfileCommandsKeepLLGoAndReferenceDistinct assert each profile maps to a distinct, correctly-bounded command (WASI→wasmtime, JS→emscripten-runner).
CI resource bounds: All jobs carry explicit timeout-minutes plus inner -test.timeout/GNU timeout wrappers; matrix totals 17 jobs with no unbounded expansion; concurrency + cancel-in-progress prevents pileups. Reviewed exclusions and extended timeouts are closed, per-package switches guarded by tests (TestFullSourceExclusionsAreProfileSpecific, TestFullCommandTimeoutIsTargeted, TestFullLongTimeoutIsTargeted).
Failure-path coverage: Broad — preparation failures, artifact-write failures, log-write, incremental- and final-report write errors are all tested (TestFullAuditReportsPreparationCommandFailures, TestFullAuditReportsArtifactWriteFailures, TestRunFullHostCheckRecordsFailuresAndWriteErrors).
Security: No pull_request_target; workflows use least-privilege permissions: contents: read; matrix values pass via env: and are referenced as quoted shell vars (no ${{ github.event.* }} injection into run:); harnesses use set -euo pipefail, mktemp -d + trap cleanup, quoted expansions, no eval, 127.0.0.1-bound servers, and a path-traversal guard in test_wasm_browser.mjs. Generated binary fixtures are only parsed (debug/elf|macho|pe|dwarf), never executed.
Findings (all low / hardening — none blocking)
[Body — pre-existing code, not in this diff] matchEntry has no catch-all guard. In test/goroot/runner_test.go, matchEntry returns true when casePattern == "", and matches via path.Match (glob), swallowing pattern errors (return err == nil && ok). Today every xfail.yaml/notapplicable.yaml entry is a literal path, so this is latent — but since this PR is the auditability layer, consider extending TestRepositoryExpectationsAreSeparated to assert every expectation entry has a non-empty case (and directive), and that no case: contains glob metacharacters. That closes the one path by which a future malformed entry could silently reclassify an unbounded set of GOROOT cases.
[Docs, low] dev/wasmstdlib/README.md:42 ("W32-WASI's six Binaryen/Asyncify links") and :13 (reviewed-exclusions prose) hard-code facts tied to the current 6-package W2 slice / exclusion switch; the reference-only test/llgoext exclusion (full.go) isn't listed. Accurate today but will silently drift if the slice/switch changes — worth a note or a test-derived count.
Two additional low-severity inline notes below. Nice work — the accounting discipline here is unusually thorough.
|
@fennoai Please re-review the updated intended W3 range 311a615..ff6ae46. The two follow-up commits address the prior review findings and the first upstream run: explicit artifact validation, literal/non-empty expectation guards, precedence clarity, audited native-only weak stress classification, an explicit real-page Chrome result protocol with process-group cleanup, and measured adaptive 25-job sharding. Please focus on false-positive resistance, browser timeout/cleanup behavior, shard completeness and resource bounds, coverage of the new failure paths, and confirmation that the range remains test/tooling/CI/documentation only. The previously noted nextFuncID uint32 wrap is unchanged because it matches upstream Go behavior, is practically unreachable, and is outside the W3 test-only boundary. |
Re-reviewing W3 range
|
There was a problem hiding this comment.
W3 re-review — 311a615707b9..ff6ae46a6 (2 follow-up commits)
Re-reviewed the delta since the prior head (04f0aac..ff6ae46a, 8 files). All three prior findings are addressed with dedicated regression tests, and the new browser/sharding work holds up. This range remains test/tooling/CI/doc only — no runtime/compiler production changes; dev/wasmstdlib is a dev-only tool, not a shipped package — so it cannot affect release size or runtime behavior.
Prior findings — all resolved:
- Precedence (
main.go:200): explicit parentheses added exactly as suggested. ✅ - Explicit artifact validation (
full.go): newvalidateFullHostArtifactrequires a non-empty regular file; the fatal host-checks now gate onhostArtifactReady, and a missing/empty artifact iserrors.Join-ed into the package failure rather than silently skipping the child. Covered byTestFullAuditRejectsMissingHostArtifactandTestFullHostArtifactMustBeNonEmptyRegularFile. ✅ - Literal/non-empty expectation guard (
runner_unit_test.go):checkSelectornow fails on an emptydirective/caseand on any glob metacharacter (*?[\) across xfail / flake / host-skip / timeout / not-applicable. This closes the exact catch-all/glob path I flagged as latent. ✅
Focus areas for this round:
-
False-positive resistance: Strengthened. Host-checks can no longer run against a phantom artifact, and the expectation guard prevents a malformed entry from silently reclassifying cases. The witness validators (exit 1–2 + witness string) are unchanged and still reject exit-0/wrong-line/reversed-order.
-
Browser timeout/cleanup: The
--dump-dom/--virtual-time-budgetscrape is replaced by an explicit/__resultbeacon that validatesstatus ∈ {success,failure}server-side, raced against processcloseand a 60s hard timeout, with an in-page 25s fallback and the outer 90s shellrun_with_timeout_limit.killBrowsersendsSIGKILLto the detached process group (falls back to the direct child /child.killon win32), and connections are torn down viacloseAllConnections?.()beforeserver.close. The dedicated--user-data-dir(.chrome-profile-<pid>) is created inside themktemp -dwork dir thattest_wasm_single_worker.shremoves viatrap ... EXIT, so no profile leak. -
Shard completeness & resource bounds: Partition is
i % shards == shardwith a hardshards<1 || shard<0 || shard>=shardsguard;other-shard/not-run/incompleteare never counted as passes. GOROOT goes 4→6 shards (J32-GoJS only — no cross-profile fan-out), and packages use 3 shards for JS providers / 5 for W32-WASI, with the WASI count expressed consistently in both the job name and theSHARDSenv. RSS/memory guards (-max-rss-mib 4096 -rss-warn-mib 1024 -min-memory-free-percent 15) and per-jobtimeout-minutesare retained. -
New failure-path coverage: The missing-artifact and non-regular/empty-file paths are both tested; the reused-build host-check tests were updated to actually materialize the artifact so they exercise the new gate.
-
Weak-stress classification:
test/_stress/runtime/weak/weak_stress_test.gois genuinely//go:build go1.24 && !baremetal && !nogc && !wasm, so the newnot-applicableexclusion is accurate, andTestFullSourceExclusionsAreProfileSpecific/TestFullAuditAcceptsReviewedSourceExclusionsnow assert it.
On the noted nextFuncID uint32 wrap: agreed and out of scope — it matches upstream Go, is practically unreachable, and lives outside this test-only boundary. Not something this PR should touch.
No new findings. The follow-up commits are a clean, well-tested response to the prior review. LGTM.
|
@fennoai Please review the latest follow-up commit 8da9d1d (delta ff6ae46..8da9d1d). The previous upstream head passed 24/25 WebAssembly jobs; W32-WASI (wasm32 with WASI Preview 1) shard 3 showed one empty-output GNU timeout 124 for test/std/net/rpc/jsonrpc during linking. The change gives exactly test/std/net/rpc and test/std/net/rpc/jsonrpc the existing 10-minute W32 allowance, with negative profile/path tests. It also changes GOROOT from six to seven shards: all 2,316 recorded case durations reconstructed the measured six-shard maximum as 3,725 seconds and predict the seven-shard maximum as 2,758 seconds, while eight would add another job for only modest additional headroom. Please check timeout scope, matrix/name/argument consistency, and whether seven is the minimal defensible runner bound. |
Reviewing follow-up
|
There was a problem hiding this comment.
W3 re-review — follow-up 8da9d1d51 (delta ff6ae46a6..8da9d1d51)
Reviewed the single follow-up commit (3 files: workflow + dev/wasmstdlib/full.go + its test). Confirms the fix for the one failing job (W32-WASI shard 3, test/std/net/rpc/jsonrpc empty-output timeout 124 during linking) and the 6→7 GOROOT reshard. Range stays test/tooling/CI only — no runtime/compiler production changes.
Timeout scope — correct and tightly bounded. fullCommandTimeout now grants the existing 10-minute allowance to exactly test/std/net/rpc and test/std/net/rpc/jsonrpc via an exact-string switch under p.Name == "W32-WASI". Good properties:
- It targets the GNU
timeoutwrap (full.go:119), which bounds compilation/linking + children — precisely where the empty-output124occurred — while the inner-timeout=60stest-runtime budget (fullTestTimeout) is left unchanged, which is right since jsonrpc isn't slow at runtime, only at reflected-bridge link time. - Exact match means it can't leak to siblings like
test/std/net/rpc/internal; the new negative cases assertrpc/internal→5m and both packages on the GoJS profile→5m.jsonrpcis a real discovered package (test/std/net/rpc/jsonrpc/jsonrpc_test.go). - The extension stays profile-scoped to W32-WASI only.
Matrix / name / argument consistency — clean. All three shard references agree at 7: job name .../7, matrix shard: [0,1,2,3,4,5,6] (7 entries), and -shard-total 7. No stale 6 remains (only the explanatory comment). timeout-minutes: 180, RSS/memory guards, and GOMAXPROCS are unchanged. The J32-GoJS-only full-corpus sharding is preserved (no cross-profile fan-out).
Is seven the minimal defensible bound? Reasonable, and data-backed rather than guessed: the six-shard upstream run showed a 62-min (~3,725s) worst-case step, and replaying all 2,316 recorded case durations predicts ~46min (2,758s) at seven, versus a full extra job at eight for only modest headroom. Since the hard timeout-minutes: 180 ceiling was never actually at risk, this is a wall-clock/runner-lease headroom optimization, not a correctness fix — and seven is a defensible minimum under that framing. (If future case-time drift is a concern, the reconstruction script/data behind the 2,758s prediction would be the thing to keep alongside the workflow, but that's not blocking.)
Boundary confirmed: workflow YAML + dev-only wasmstdlib acceptance tool + its unit test. No shipped code, so no release-size or runtime impact.
No findings. Clean, well-scoped, and tested. LGTM.
bb2cbfd to
7a4f5b2
Compare
|
@fennoai Please re-review W3 at |
Re-reviewing rebased W3 range
|
There was a problem hiding this comment.
W3 rebase re-review — 748483792502..7a4f5b22e762
git range-diff against the prior reviewed range confirms the described shape: commits #2–35 are exact matches (=), #1 changed, and #36 is new.
Commit #1 (validate portable host contracts) — verified benign. The only delta is dropping the native subprocess wrapper test/go/caller_panic_native_test.go from this commit (plus adding GOTRACEBACK=single to the child env). Confirmed that file already exists at the W3 range base (git cat-file -e 748483792502:test/go/caller_panic_native_test.go succeeds), i.e. it is now supplied by the stacked-below W2-B range — no loss of coverage, no duplicate definition.
Commit #36 (run repanic tracebacks from the host) — solid and false-positive-resistant. It launches six repanic variants (same, wrapper, indirect-wrapper, slice, different, later) from the acceptance host against the retained test/go artifact via the -llgo.caller-repanic-child=<mode> argv selector, for each profile. validateFullRepanic enforces three independent gates:
- Exit status — requires an
*exec.ExitErrorwith code 1–2; explicitly rejects success, timeout, and launch failure. - Exact source line — parses the marker (
REPANIC_ORIGIN_MARK/SLICE_REPANIC_ORIGIN_MARK/REPLACEMENT_PANIC_MARK/LATER_SAME_VALUE_PANIC_MARK) out ofcaller_runtime_test.goand requirescaller_runtime_test.go:<line>in the traceback, anchored with(?:\D|$)so:102can't be satisfied by:1020. - Function identity (and panic message for
different→panic: replacement panic).
Verified all four markers exist at unique panic-origin lines (102/109/121/153) and all six driver functions (callerRepanicOrigin, callerReplacementPanic, callerLaterSameValuePanic, callerWrappedRepanic, callerSliceRepanic, callerNestedRepanic) are present. This is a real host-executed contract, not a skip — consistent with WASM guests returning ENOSYS for pipe so the guest can't self-spawn.
Failure-path coverage is complete: TestFullFatalValidatorsRejectFalsePositives now exercises every repanic case for the positive path plus four negatives (successful child, wrong source line, missing traceback identity, missing source file/marker), and TestFullChildCommandProfiles asserts the repanic selector on every profile. Supports the 100%-statement-coverage claim.
Prior findings all still resolved at head: precedence parens, validateFullHostArtifact/hostArtifactReady, literal/non-empty expectation guard, weak-stress exclusion, net/rpc+jsonrpc 10-min WASI allowance, 7-shard GOROOT. ✅
Boundary: the W3 range (97 files) contains no production files — entirely test / dev/wasmstdlib tooling / CI / doc. No release-size or runtime impact.
Rebase verified, no new findings. LGTM.
|
@fennoai Please review the incremental CI-only fix |
Reviewing CI-only fix
|
There was a problem hiding this comment.
W3 CI-only fix review — 7a4f5b22e762..d626eaded
One commit, one file (.github/workflows/wasm-acceptance.yml, +10/-3). Provisions Chrome for the browser-acceptance job instead of assuming a preinstalled google-chrome. Confirms the fix for the Qiniu-runner failure. All three focus points check out:
Runner independence — yes. browser-actions/setup-chrome@v2 (with install-dependencies: true) installs Chrome regardless of what the hosted runner preloads, and its resolved steps.setup-chrome.outputs.chrome-path is threaded via BROWSER_BIN into both the verify step and dev/test_wasm_browser.sh. Nothing now depends on google-chrome being on PATH. Passing the explicit path also means the script uses that exact binary rather than its own PATH-candidate search.
Existing-pattern reuse — confirmed. This mirrors the repo's established usage in .github/workflows/llgo.yml:684 (same action, same id: setup-chrome, same install-dependencies: true, same chrome-path output). No new/novel mechanism introduced.
Missing-browser failure semantics — error, not skip. The Require the hosted browser step runs test -x "$BROWSER_BIN" then "$BROWSER_BIN" --version under the job's default set -e; an empty/invalid path fails test -x "" and stops the job. test_wasm_browser.sh independently runs set -euo pipefail and exit 1 when no browser resolves. So an absent browser remains a hard failure.
No redundant setup remains. Single Set up Chrome step; the previous command -v google-chrome check and the literal BROWSER_BIN: google-chrome are both removed. No leftover apt/manual Chrome install.
Minor (non-blocking, optional): browser-actions/setup-chrome@v2 is tag-pinned rather than SHA-pinned — consistent with the repo's existing convention (the llgo.yml reference is pinned the same way), so not a regression; SHA-pinning third-party actions is a general hardening option if the project ever wants it.
CI-only, no production/test-logic changes. LGTM.
|
@fennoai Please review the documentation-only delta |
Reviewing doc-only delta
|
There was a problem hiding this comment.
W3 doc-only review — d626eaded..cb588686
Two commits, both touching only doc/wasm-proposal.md (+76/-8). Adds a "Design overview" section (Go source layer, Memory ABI, Host ABI, Provider) plus a provider tree, and rewords the compatibility contract. Confirmed doc-only — no production or test changes in this delta.
English/Chinese consistency — good. The new Design overview / 设计总览 lists, the ASCII provider tree, and the assembly/runtime boundary paragraph all appear in both languages with matching technical content:
- Go source layer / Go 源码层: 64-bit word model, low-32-bit Memory32 addresses, prefer shared GOROOT sources.
- Memory ABI: J32/W32 → Memory32/
i32/ILP32; J64 → Memory64/i64/LP64. - Host ABI: JavaScript vs WASI, with
js && wasm/wasip1 && wasmhost files remaining distinct. - Provider: GoJS vs Emscripten share Go API semantics; "provider selection does not define another Go ABI" / "provider 选择不定义另一套 Go ABI".
- The provider tree is identical between the two renderings (Memory32 → GoJS / Emscripten / WASI Preview 1 providers; Memory64 → Emscripten provider).
J64 in the common Go data model — explicitly included in both. Line 14 (EN) / 129 (ZH): J64 is "extending the same Go data model to a wider address space" / "将同一 Go 数据模型扩展到更大的地址空间", and the compatibility contract's first bullet now reads "All supported profiles, including J64 …" / "包括 J64 … 在内的所有受支持 profile". The tree correctly shows J64/Emscripten as the sole Memory64 leaf, consistent with "J64 initially uses Emscripten" elsewhere in the doc.
Source-reuse wording does not imply gc-object or cross-target artifact compatibility — verified. The disclaimer is explicit and mirrored (line 41 EN / 156 ZH): "Source/API compatibility does not require identical generated function signatures or binary compatibility with gc compiler objects or artifacts for other targets." This is actually stronger than the text it replaces (the old "Binary compatibility with Go compiler object files is not required" bullet), since it now also excludes other-target artifacts and function-signature identity. The syscall/js bullet likewise keeps "source/API compatibility, not stock wasm_exec.js binary compatibility." No wording overstates reuse.
Doc reads cleanly and stays consistent with the profile/provider terminology used across the workflows and tooling. No findings. LGTM.
|
@fennoai Please review only the packaging follow-up The previous CI round is now completely finished: 91 checks passed, with only the Windows MSVC ARM64 release staging failure and three skipped downstream release tasks. All 26 WASM acceptance jobs, native tests, benchmarks, and Codecov passed. No push interrupted that round. The updated body includes the complete package/GOROOT accounting, current coverage, and size comparisons against both W2-B and main. The fix runs the existing ESP validation from the final staged path after moving the payload and cleaning up its extraction directory. Please check that all validation remains mandatory, failures still prevent archive creation, and no executable is moved/deleted by the temporary staging block after it has been run. There are no compiler/runtime changes or new skips. Windows CI on the new head must confirm the file-lock fix. |
Reviewing packaging fix
|
There was a problem hiding this comment.
Packaging follow-up review — cb588686..62d85a81
One commit, +3/-1 in .github/windows/build-release.ps1 (also independently proposed as #2588). It moves the Assert-ReleaseESPPayload call from before the Move-Item + temp-dir cleanup to after it, validating the final crosscompile/clang location. All four requested properties hold:
Validation remains mandatory and fatal. Assert-ReleaseESPPayload (.github/windows/release-lib.ps1:157) throws on every failure path — manifest arch/version mismatch, unexpected PE machine, a missing imported DLL, zero PE binaries — and executes all six tools (clang, clang++, ld.lld, llc, opt, llvm-config) via Invoke-ReleaseCapture, which itself throws on any non-zero exit. Nothing was downgraded to a warning or skipped; the check still runs unconditionally on every build.
Failures still prevent archive creation. The relocated call sits at top level after the try/finally but before release.json is written and before release_zip.py create. A throw aborts the script non-zero, so no .zip (and no .sha256) is produced when validation fails.
No executable is moved/deleted after being run — this is precisely the fix. Previously the tools were executed while still under the temporary $espParent, and the subsequent Move-Item relocated that just-executed directory, so the residual Windows execution file-lock produced the sharing-violation staging failure. Now Move-Item runs first, the (emptied) $espParent is removed in finally, and validation runs against the permanent staged path with only a LICENSE copy-in / metadata write / zip afterward — none of which move or delete the validated binaries.
No compiler/runtime changes or new skips. The delta touches only this one PowerShell script.
Scope note: $crosscompile is assigned inside the try block, but PowerShell function-scope (not block-scope) keeps it visible at the post-finally Assert line; if extraction throws earlier, the exception propagates before that line executes, so there's no undefined-root case.
One thing I can't verify from here: the actual Windows CI run confirming the file-lock is resolved — I can't read CI logs, so that remains the final gate, as you noted. The change is logically correct and correctly ordered. LGTM.
Keep winbatch.go and issue78081.go runnable, with js/wasm-only budgets grounded in observed passing and timed-out runs. Cover the timeout scope and absence of xfail classifications. Partition the unchanged full corpus into nine shards. Replaying 2,316 measured case timings lowers the estimated longest execution from 51 to 41 minutes without relaxing resource guards. This mitigates long-job exposure, but does not claim to repair runner disconnections.
|
@fennoai Please review only The preceding round is fully finished: 91 checks passed, six failed, and the actual release-publication job was normally skipped. All four Windows MSVC/MinGW × amd64/arm64 builds and their artifact tests passed, confirming the carried #2588 staging fix. No push interrupted the round. The two GOROOT case failures were execution timeouts: Four other failures were lost Please check the narrow timeout scope, retained runnable classifications/resource guards, consistent nine-shard matrix/name/command, and whether the diff contains anything unnecessary. All runner unit tests, actionlint, and diff checks pass locally. The preceding head's Codecov patch coverage is 98.52% (target 94.35%); all 52 WASM module/glue size measurements still exactly match final W2-B. The new head must pass CI and coverage; please do not treat the previous incomplete acceptance result as green. |
Reviewing
|
There was a problem hiding this comment.
W3 review — 62d85a81..10b96399
One commit, three files (wasm-acceptance.yml, runner_unit_test.go, xfail.yaml), +20/-12. Raises two GOROOT js/wasm timeout allowances and reshards GOROOT 7→9. Confirmed no compiler/runtime changes and no new skips. All requested points check out:
Timeout scope is narrow and does not leak. Both bumped entries in xfail.yaml (winbatch.go 2m→4m, fixedbugs/issue78081.go 3m→6m) carry platform: js/wasm + directive: run, and MatchTimeout (runner_test.go:2260) only returns an override when entry.matches(goVersion, platform, tc). The new regression loop in TestWasmObservedResourceExceptions explicitly asserts the allowance does not match on linux/amd64, darwin/arm64, windows-msvc/arm64, or wasip1/wasm — so no leakage to native/WASI. The reasons are now specific and auditable (115s/147s measured passing runs vs. the former 120s/180s limits).
Runnable classifications retained. Both cases are added to TestObservedPassesDoNotHaveXFailClassifications, which asserts they are NOT xfail/not-applicable, and neither appears in any xfail/not_applicable section — they live only under timeouts. So they remain mandatory passing cases with a wider allowance, not suppressed. Resource guards (-max-rss-mib 4096 -rss-warn-mib 1024 -min-memory-free-percent 15 -min-swap-free-mib 512) are unchanged.
Nine-shard matrix/name/command are consistent. Job name .../9, matrix shard: [0,1,2,3,4,5,6,7,8], and -shard-total 9 all agree; no stale 7 remains. timeout-minutes: 180 and GOMAXPROCS unchanged; still J32-GoJS-only (no cross-profile fan-out). The comment correctly frames this as measured headroom mitigation (51m→~41m est.), not a repair of the lost-runner disconnections.
Nothing unnecessary. The diff is exactly: the shard bump, the two timeout values (yaml + unit test), the two reason strings, two runnable assertions, and the anti-leak loop. No unrelated edits, no label substitution — consistent with your note that qiniu→e2b-* was left as-is.
The scoping/classification logic is correct. As you flagged, the previous acceptance round was incomplete, so CI + coverage on this new head remain the gating signals — I can't read CI logs from here, so I'm reviewing the change itself, which is sound. LGTM.
|
@fennoai Please incrementally review The complete preceding CI round finished before this push: 98 passing checks, one expected release skip, one failure. All 28 WASM jobs finished without runner loss; only GOROOT shard 5/9 failed, because A systematic near-budget audit of the full corpus also found Please check that both cases remain runnable, the guards reject js/wasm xfail/flaky/not-applicable classifications, existing native issue79186 90-second allowances are preserved, and WASI remains unaffected. No skip/flake or new timeout framework, compiler/runtime changes, reduced test contents, or global resource-limit changes were introduced. All GOROOT runner unit tests and diff checks pass. Previous-head coverage is 98.52% against 94.35%; all 52 WASM module/glue sizes are unchanged versus W2-B. New-head CI/coverage are still required. |
Reviewing
|
There was a problem hiding this comment.
W3 review — 10b96399b..14a2f5247
One commit, two GOROOT files (xfail.yaml, runner_unit_test.go), +32/-6. Adds js/wasm timeout allowances for two more measured near-budget cases. No compiler/runtime changes, no new skip/flake, no new framework. All requested properties verified:
Both cases remain runnable. fixedbugs/issue79186.go (run, 2m) and fixedbugs/issue5162.go (runoutput, 4m) are added to TestObservedPassesDoNotHaveXFailClassifications, and neither appears in any xfail/not_applicable section — they exist only under timeouts. issue5162.go is absent from notapplicable.yaml entirely. So they stay mandatory passing cases with a wider allowance.
Guards reject js/wasm xfail/flaky/not-applicable. TestWasmObservedResourceExceptions now additionally asserts, for all four resource cases, that MatchFlaky and notApplicable.Match return no match on js/wasm. Note issue79186.go does appear in the flakes: section — but only for native darwin/arm64 and linux/amd64, correctly not js/wasm, so there's no classification conflict.
Native issue79186 90s allowances preserved. The darwin/arm64, linux/amd64, windows/amd64, and windows/arm64 90s entries are all retained, and the test now asserts nativeTimeout = 90s on those platforms. I confirmed matchPlatform maps windows-msvc/arm64 → the windows/arm64 entry (prefix rule), so the test's native assertion is a real match, not an accidental pass. The new js/wasm 2m allowance is a separate entry and does not disturb them.
WASI unaffected. The test pins want = 0 for wasip1/wasm and asserts no timeout override matches there — so none of the four GoJS allowances leak to WASI (or to native for the WASI-only-0 cases).
Nothing unnecessary / no global changes. The diff is exactly: two timeouts: entries with specific measured reasons, two runnable assertions, and the expanded per-platform/flaky/not-applicable guard matrix. No new skip/flake mechanism, no reduced test content, no -max-rss/-min-memory or other global resource-limit edits.
Scoping and classification logic are correct. As you noted, this new head still needs its own green CI + coverage — I can't read CI logs from here, so I'm reviewing the change itself, and it's sound. LGTM.
1 participant
Tracks #2152. Stacked on #2580.
This adds complete host-driven compatibility acceptance for the supported single-worker WebAssembly targets. Tests that require subprocesses, native binary inspection, or host services now have explicit portable contracts; fatal WASM checks run from the host against retained artifacts, including exact panic/repanic traceback locations.
Scope
syscall/jscallback in real headless Chrome. Provision the browser explicitly and fail if the executable, artifact, or expected result is missing.doc/wasm-proposal.md: the Go source layer, Memory ABI, Host ABI, and provider are presented as a list and complete provider tree, with a concise assembly/runtime adaptation boundary. All supported profiles share the Go data model, including J64 (wasm64 with JavaScript).Review boundary
The original W3 range is
748483792502..cb588686a49b0: 39 commits, 97 files, +4,442/-498, based on the final W2-B head from #2580. The range consists of tests, acceptance tools, CI, fixtures, classifications, and proposal documentation. Compiler/runtime changes visible in the full PR-versus-main diff belong to the stacked dependencies. The latest follow-up,10b96399b..14a2f5247, changes only two GOROOT configuration/unit-test files (+32/-6): two measured case budgets and classification/platform regression assertions. The preceding acceptance-only follow-up adjusted two other case budgets and GOROOT sharding.The W3 range introduces no production implementation changes or new
t.Skipcalls. It cannot itself change generated release size or runtime performance; benchmarks and coverage remain required gates for the complete stack.The branch also carries the isolated main-branch packaging fix from #2588 as
62d85a811:.github/windows/build-release.ps1(+3/-1) validates ESP tools after relocation and temporary cleanup. All four Windows MSVC/MinGW × amd64/arm64 release builds and their four artifact tests passed on that commit.CI and validation
14a2f5247finished with 99 passing checks, zero failures, and one normally skipped release-publication job. All 28 WASM acceptance jobs passed, as did native tests, release builds/artifact checks, benchmarks, and coverage. The acceptance checkout was synthetic merge1d19cd9, combining this head with main6eaf7eb982. No push interrupted the preceding round; its sole remaining GOROOT timeout was fixed in the follow-up before this complete rerun.issue79186.goperforms 51.2 million RWMutex/map operations; prior passing JavaScript-host executions took 55.144 and 55.710 seconds before another run exceeded the default minute. Its narrowly scopedjs/wasmallowance is now two minutes, and the final CI run passed in 56.740 seconds. A full-corpus timing audit also identifiedfixedbugs/issue5162.go: building its 768 generated array-comparison functions previously took 168.753 seconds against the default 180-second build limit. Its existing case-budget mechanism now allows four minutes; the final build passed in 165.231 seconds. The previously handledissue78081.goalso passed, running in 146.921 seconds under its six-minute allowance.js/wasm, preserveissue79186.go's existing native 90-second allowances, and prevent overrides leaking to WASI. A compiler built from clean10b96399bpassed the unchangedissue79186.goon Node 24 locally in 22.802 seconds. All GOROOT runner unit tests pass; no compiler/runtime implementation, test contents, global limits, or memory guards changed.e2b-*runner-disconnection failures with no uploaded logs. Nine GOROOT shards reduced the next round's execution times to 32.47–43.47 minutes, down from about 51 minutes at seven shards. Both subsequent rounds completed all WASM jobs without runner loss, but that does not establish a permanent infrastructure fix. Missing results never count as passes; no unverified runner-label workaround was introduced.14a2f5247account for 189 applicable packages per LLGo path, 756 package executions, 9,599 passed top-level tests, and 56 explicit host checks, without duplicated or omitted package execution. Package classifications and exclusion reasons are unchanged. The increase of eight top-level executions versus the previous round comes from main reflect: index named function conversions and pointer caches #2565's two named-function cache publication tests passing on all four paths. All nine GOROOT reports account for 2,316 distinct selected cases: 2,178 pass, 17 expected failures, 117 not applicable, three host/resource skips, and one previously registered flaky failure. The only classification transition since the preceding round isissue79186.gofrom unexpected timeout to pass. Non-passing classifications are reported separately, not counted as passes.test/goartifact rather than requiring unsupported guestpipe/os.execoperations. Six repanic modes pass locally on all four LLGo paths and official Go js/wasm and wasip1/wasm references, with exit status, function identity, and exact source-line assertions.go test -cover ./dev/wasmstdlib ./test/goroot ./test/internal/binaryfixtureandgo test ./test ./test/gopass.dev/wasmstdlibhas 100.0% statement coverage and the binary-fixture package has 95.0%. The follow-up additionally passes every GOROOT runner unit test,actionlint, andgit diff --check; the full corpus is delegated to CI.14a2f5247passed with 98.52% patch coverage against a 94.35% target. All prior review threads are resolved, and the incremental review of14a2f5247received LGTM with no new findings. CI, review, coverage, and artifact audits are all complete for this head.14a2f5247. All 52 module/glue size measurements exactly match both the preceding W3 round and the final W2-B artifacts at748483792502: W3 adds no measured output-size overhead. This is not a claim of zero growth for the whole W1/W2 stack versus main; for example,cprintfgrows by 21,658 bytes on J32-Emscripten (wasm32 with Emscripten), 66,566 bytes on J32-GoJS (wasm32 with the Go-compatible JavaScript host), and 4,821 bytes on J64-Emscripten (wasm64 with Emscripten Memory64). Those differences already exist below W3. Single-sample build/startup timings are not sufficient to establish a runtime performance regression or its absence.Compilation/linking budgets remain bounded. Only
test/std/net/rpcandtest/std/net/rpc/jsonrpcreceive the measured 10-minute W32-WASI (wasm32 with WASI Preview 1) allowance; their guest test-runtime budget stays unchanged. Missing tools and missing test results are errors.