ci(verify-baseline): parse //@ JSC flags, skip v128 wasm fixtures on x64, re-enable --jit-stress on WebKit changes - #34877
Conversation
…x64, re-enable --jit-stress on WebKit changes
hasWebKitChanges() was still checking for cmake/targets/SetupWebKit.cmake, a
file removed when the build moved off cmake, so verify-baseline --jit-stress
has not run since. Pointing it at scripts/build/deps/webkit.ts surfaces two
issues in the --jit-stress path:
* Fixtures were spawned with the bare process env, so //@ runDefault /
runFTLNoCJIT / runDefaultWasm directives (jitPolicyScale, useJSPI,
useDollarVM, createPreHeaders, ...) and the bunEnv test-harness knobs
(BUN_DEBUG_QUIET_LOGS, BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING, etc.) were
not applied. jsc-stress.test.ts already did both; factor parseJSCFlags
out to test/js/bun/jsc-stress/jsc-flags.ts and use it from both places.
* bbq-osr-with-exceptions.js and omg-tail-call-clobber-scratch-register.js
declare v128 (0x7B) types. On Nehalem (no AVX), JSC's
recomputeDependentOptions() force-disables useWasmSIMD, which makes v128
an invalid type and the module fails to parse with
"WebAssembly.Module doesn't parse ... can't get ... Type". Skip those
two on x64: a real Nehalem CPU never runs the wasm-SIMD JIT path anyway,
so there is nothing to verify. aarch64 keeps running them.
jsc-flags.test.ts runs every wasm fixture under BUN_JSC_useWasmSIMD=false so
the skip list drifts into a test failure if a new fixture starts (or an
existing one stops) requiring wasm SIMD.
|
Updated 11:20 PM PT - Jul 20th, 2026
❌ @robobun, your commit 326d498 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34877That installs a local version of the PR into your bun-34877 --bun |
|
Status: ready for review. CI build 76646 is green on everything this diff touches ( Remaining red on 76646 is unrelated to this diff:
Reproduced locally by forcing End-to-end
No |
WalkthroughChangesThe pull request centralizes JSC fixture flag parsing, adds wasm SIMD classification and tests, applies per-fixture environments during baseline verification, skips unsupported fixtures, and refines WebKit-triggered CI JIT-stress gating. Baseline verification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — CI-tooling only, no src/ changes.
What was reviewed:
parseJSCFlagsextraction tojsc-flags.tsis byte-identical to the removed copy injsc-stress.test.ts; verified the four unit-test assertions match the actual//@directives in the referenced fixtures.hasWebKitChanges()now checks paths that exist (scripts/build/deps/webkit.ts,vendor/WebKit/); confirmed both are real repo paths referenced elsewhere.verify-baseline.ts's newenvplumbing also affects the Phase 1 SIMD test run — the added bunEnv-style knobs (BUN_DEBUG_QUIET_LOGS,NO_COLOR, etc.) are harmless there.- New
jsc-flags.test.tsfollows harness conventions (concurrent, drains all pipes,bunEnvspread) and keeps thewasmSIMDFixturesskip list self-syncing.
Extended reasoning...
Overview
This PR fixes the verify-baseline --jit-stress CI phase, which has been silently disabled since the cmake→build.ts migration removed SetupWebKit.cmake. It touches five files, none in src/:
.buildkite/ci.mjs: one-line fix tohasWebKitChanges()— replace the staleSetupWebKit.cmakecheck withscripts/build/deps/webkit.ts/vendor/WebKit/.test/js/bun/jsc-stress/jsc-flags.ts: new file —parseJSCFlagsextracted verbatim fromjsc-stress.test.ts, plus awasmSIMDFixturesset naming the two v128-using fixtures.test/js/bun/jsc-stress/jsc-stress.test.ts: pure deletion of the localparseJSCFlagsin favor of the shared import. No behavioral change.scripts/verify-baseline.ts: import the shared helper; pass a bunEnv-equivalent base env plus the parsed//@flags to each fixture spawn; skip the two v128 wasm fixtures on x64 (Nehalem emulation force-disablesuseWasmSIMD, so the modules can never parse there).test/js/bun/jsc-stress/jsc-flags.test.ts: new — unit tests forparseJSCFlagsand a concurrent describe that runs every wasm fixture underBUN_JSC_useWasmSIMD=falseto assertwasmSIMDFixtureslists exactly the ones that fail to parse without SIMD.
Security risks
None. This is CI/test tooling. No user input parsing, no auth/crypto, no network. The changedFiles predicate in ci.mjs only gates whether an extra CI phase runs; a false positive just means more testing.
Level of scrutiny
Low-to-medium. No shipped-binary code changes. The riskiest effect is re-enabling a CI phase that hasn't run in a while, but that phase only fires on PRs touching WebKit paths, and the two known-failing fixtures under Nehalem are now skipped with a documented rationale (JSC's recomputeDependentOptions() disables wasm SIMD on x86_64 without AVX, so v128 modules can't parse — nothing to verify there).
Other factors
- The
parseJSCFlagsmove is a textbook "one implementation, in the right place" refactor per REVIEW.md — both consumers now share the same parser. - I checked the fixture headers on disk against the four
parseJSCFlagsunit-test expectations; all match (including the multi-directive case inomg-tail-call-clobber-scratch-register.jswhere a non-matching//@ skip if ...line correctly falls through tocontinue). - The new test file follows harness conventions:
describe.concurrent,{...bunEnv, ...}spread,Promise.allon stdout/stderr/exited, and the sameisDebug ? 180_000 : undefinedtimeout pattern asjsc-stress.test.ts. It adds ~22 concurrent subprocess spawns per CI run, which is acceptable given the debug-timeout headroom. - The self-syncing test ("every listed fixture exists" + per-fixture SIMD-off run) means the skip list can't drift silently — a nice touch that addresses the "never silently weaken a test" rule.
- The
fixtureBaseEnvinverify-baseline.tsnow applies to allrunTestcalls (including the Phase 1 SIMD test), not just the jit-stress fixtures. The added vars are the standard test-harness knobs and are benign forbun test. - No CODEOWNERS entries cover the touched paths.
…g fixture names in test Intel SDE's Pin instrumentation costs ~45s per process (build 76600: dfg-ai-fold-bigint.js 46.0s, dfg-call-class-constructor.js 44.9s, ...), so 80+ serial fixtures would take roughly an hour against a 30-minute step timeout. qemu-Nehalem on Linux verifies the same x64-no-AVX JIT output at ~1s/fixture, so Windows keeps only the static scan and the SIMD test. Also replace the per-fixture expect(bool).toBe(true) existence check with a single .toEqual([]) so a stale wasmSIMDFixtures entry prints its name in the failure diff.
There was a problem hiding this comment.
LGTM — well-reasoned CI-tooling fix with a guard test that keeps the skip list honest. The inline nit about vendor/WebKit/ is cosmetic; the live webkit.ts clause is what actually fires on WebKit bumps.
What was reviewed:
parseJSCFlagsextraction is byte-for-byte identical to what was injsc-stress.test.ts; verified the parser handles multi-line//@headers (e.g.skip if ...beforerunDefault(...)) correctly against the actual fixture files.wasmSIMDFixturesskip is gated on!isAarch64, matchingneedsBaselineVerification(Linux aarch64 still runs the v128 fixtures under Cortex-A53).- Windows
--jit-stressopt-out is a coverage reduction, but the justification (SDE ~45s/fixture vs 30min step timeout, same x64-no-AVX codegen already covered by qemu-Nehalem on Linux) holds. - Checked that the new
jsc-flags.test.tsunder debug/ASAN was examined for theASAN_OPTIONS/ wasm-fault-handler concern — ruled out.
Extended reasoning...
Overview
This PR touches only CI tooling and test-harness code — no src/ edits. It (1) re-points hasWebKitChanges() in .buildkite/ci.mjs from a since-deleted cmake file to scripts/build/deps/webkit.ts, so verify-baseline --jit-stress actually runs again on WebKit bumps; (2) extracts parseJSCFlags from jsc-stress.test.ts into a shared test/js/bun/jsc-stress/jsc-flags.ts and wires it into scripts/verify-baseline.ts so fixtures get their //@ directive flags plus a bunEnv-equivalent base env; (3) skips two v128-typed wasm fixtures on x64 baseline emulation (JSC force-disables useWasmSIMD without AVX, so they can't parse); (4) disables --jit-stress on the Windows SDE lane because Pin startup makes it ~1h vs a 30min timeout; and (5) adds jsc-flags.test.ts which unit-tests the parser and, more usefully, runs every wasm fixture under BUN_JSC_useWasmSIMD=false to assert wasmSIMDFixtures is exactly the set that fails to parse — so a new v128 fixture breaks this test before it breaks the verify-baseline lane.
Security risks
None. This is CI pipeline configuration and a test-harness helper. No untrusted input parsing, no auth/crypto/permissions, no changes to shipped code.
Level of scrutiny
Moderate — CI tooling that gates what runs on WebKit-bump PRs. The riskiest part is re-enabling a lane that hasn't run since the cmake removal, but the PR was written because that lane was exercised on build 76600 and the two failure modes it found are exactly what this PR fixes. The Windows --jit-stress opt-out reduces coverage, but the rationale (identical x64-no-AVX JIT output already verified under qemu-Nehalem at ~1s/fixture) is sound and documented inline.
Other factors
parseJSCFlagsis a verbatim move; I diffed the removed block against the new file and cross-checked the parser against the real//@headers in the fixture directory (multi-line headers like//@ skip if ...followed by//@ runDefault(...)are handled — thestartsWith("//@")guard keeps iterating and only therun*regex extracts flags).- The
!isAarch64gate on the skip matches the emulation matrix: only Linux x64-baseline and Windows x64-baseline emulate Nehalem; Linux aarch64 emulates Cortex-A53 where NEON is always present, so the v128 fixtures still run there. - Verified
scripts/build/deps/webkit.tsexists andvendor/is gitignored (.gitignore:156), confirming the inline nit that thevendor/WebKit/clause is dead but harmless. - The bug-hunting system also examined whether
jsc-flags.test.tsneeds theASAN_OPTIONS=allow_user_segv_handler=1override thatjsc-stress.test.tsapplies for debug-ASAN wasm fixtures, and ruled it out. - The one reported finding is a nit (dead
||clause) that doesn't affect behavior; the live clause covers WebKit version bumps.
vendor/ is gitignored (.gitignore:156); changedFiles comes from GitHub's PR-files API which only returns tracked paths, so the clause can never match. WebKit version bumps land in scripts/build/deps/webkit.ts, which the remaining clause covers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.buildkite/ci.mjs:
- Around line 763-765: Update the Windows SDE comment in the CI configuration to
remove the changeable timing and fixture-count measurements. Preserve only the
durable rationale that Windows SDE is too slow for the serial JIT-stress suite,
while noting that Linux qemu-Nehalem covers the same x64-no-AVX JIT output and
Windows retains the static scan and SIMD test.
- Around line 766-767: Update the wantJitStress calculation in the CI target
setup to exclude Android targets that use --skip-emulation, so WebKit changes
alone do not enable the no-op --jit-stress flag or the longer timeout. Apply the
same condition to the corresponding logic near the later Android target
handling, while preserving JIT stress for non-Android and Android targets
without --skip-emulation.
- Around line 749-750: Update the changedFiles check in the WebKit dependency
detection logic to compare each entry exactly with the root-relative path
"scripts/build/deps/webkit.ts" rather than using substring matching. Preserve
the existing some-based behavior so the suite is enabled only when that exact
file changes.
In `@scripts/verify-baseline.ts`:
- Around line 101-107: Update the fixtureBaseEnv definition to explicitly remove
the inherited JSC_useJIT environment variable, matching bunEnv’s handling and
the test harness behavior. Preserve the remaining inherited environment
variables and fixture-specific settings unchanged.
In `@test/js/bun/jsc-stress/jsc-flags.ts`:
- Around line 6-14: Condense the comments to three lines or fewer without
changing behavior: shorten the API documentation and SIMD rationale in
test/js/bun/jsc-stress/jsc-flags.ts (lines 6-14 and 45-52), the file-level test
and skip-list rationales in test/js/bun/jsc-stress/jsc-flags.test.ts (lines 1-5
and 46-50), and the Nehalem skip rationale in scripts/verify-baseline.ts (lines
272-275).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90af2833-5ffa-430f-aa4f-f915b019ac66
📒 Files selected for processing (5)
.buildkite/ci.mjsscripts/verify-baseline.tstest/js/bun/jsc-stress/jsc-flags.test.tstest/js/bun/jsc-stress/jsc-flags.tstest/js/bun/jsc-stress/jsc-stress.test.ts
…ation, strip JSC_useJIT, tighten comments
There was a problem hiding this comment.
LGTM — CI-tooling only, no src/ changes; all prior feedback (dead vendor/WebKit/ clause, exact-path match, JSC_useJIT strip, Android --skip-emulation gating, comment length) is addressed and resolved.
What was reviewed:
hasWebKitChangesnow points atscripts/build/deps/webkit.ts(verified to exist) with exact match;wantJitStresscorrectly excludes Windows SDE and Android--skip-emulationlanes, and the 30-min timeout keys off it.parseJSCFlagsis a byte-for-byte move fromjsc-stress.test.ts; spot-checked its output against the four fixture headers used in the new unit tests.verify-baseline.ts'sfixtureBaseEnvmirrorsbunEnv(including theJSC_useJITstrip), and the v128 skip is guarded byjsc-flags.test.tsso the list can't drift.- Ruled out: the missing
ASAN_OPTIONSoverride injsc-flags.test.ts—bunEnvalready sets it for the CI ASAN binary.
Extended reasoning...
Overview
This PR fixes the verify-baseline --jit-stress CI lane, which has been silently disabled since the cmake→build.ts migration removed SetupWebKit.cmake. Five files touched, none under src/:
.buildkite/ci.mjs: repointshasWebKitChangestoscripts/build/deps/webkit.ts(exact match), gates--jit-stressoff Windows SDE (Pin startup cost) and Android (--skip-emulation), and keys the 30-min timeout on the actualwantJitStressdecision instead of the raw predicate.scripts/verify-baseline.ts: spawns fixtures with a bunEnv-equivalent base env plus each fixture's parsed//@JSC flags; skips the two v128-typed wasm fixtures on x64 (JSC force-disablesuseWasmSIMDwithout AVX, so the modules cannot parse under Nehalem).test/js/bun/jsc-stress/jsc-flags.ts: extractedparseJSCFlags(moved verbatim) plus thewasmSIMDFixturesset.test/js/bun/jsc-stress/jsc-flags.test.ts: unit tests forparseJSCFlagsand a guard that runs every wasm fixture underBUN_JSC_useWasmSIMD=falseto provewasmSIMDFixtureslists exactly the ones that fail without SIMD.test/js/bun/jsc-stress/jsc-stress.test.ts: now importsparseJSCFlagsinstead of defining it inline (no behavioral change).
Security risks
None. This is CI pipeline configuration and test infrastructure. No auth, crypto, permissions, or user-facing surface. The only external effect is which CI steps run on WebKit-bump PRs.
Level of scrutiny
Medium-low. .buildkite/ci.mjs is load-bearing for the whole pipeline, but the edit is confined to getVerifyBaselineStep and its helper — the rest of the pipeline is untouched. The verify-baseline lane it re-enables only runs on PRs that touch scripts/build/deps/webkit.ts, and its failure mode is a red CI step, not a shipped-binary regression. verify-baseline.ts is a diagnostic script, not runtime code. The jsc-stress.test.ts change is a pure refactor (function extraction).
Other factors
- I verified
scripts/build/deps/webkit.tsexists on disk and that the four fixtures referenced in the newparseJSCFlagsunit tests have the exact//@headers the assertions expect (including the intervening//@ skip ifline inomg-tail-call-clobber-scratch-register.js, which the parser correctly steps past viacontinue). - The
wasmSIMDFixturesguard test is the right shape: it fails if a new v128 fixture is added without updating the list and if a listed fixture stops needing SIMD, so the skip list is self-maintaining. - My earlier review flagged a dead
vendor/WebKit/clause; that was dropped in 503fea4. CodeRabbit's follow-ups (exact-path match, Android gating,JSC_useJITstrip, comment length) were all applied in 326d498 and every thread is resolved. The one CodeRabbit suggestion the author declined (dropping the measured Pin timings from the comment) was reasonably pushed back on and withdrawn. - Finder agents raised whether
jsc-flags.test.tsneeds the sameASAN_OPTIONSfallback asjsc-stress.test.ts; verifiers refuted it —bunEnvalready setsASAN_OPTIONSwhen the binary name containsasan, which covers the CI release-ASAN lane. The extra override in the sibling file exists only for the localbun-debugcase.
1 participant
What
hasWebKitChanges()in.buildkite/ci.mjswas still checking forcmake/targets/SetupWebKit.cmake, a file removed when the build moved off cmake (#27973), soverify-baseline --jit-stresshas not run since. Re-pointing it atscripts/build/deps/webkit.tssurfaces two problems in the--jit-stressphase (seen on build 76600 while #34782 briefly re-enabled it):Root cause
bbq-osr-with-exceptions.jsandomg-tail-call-clobber-scratch-register.jsencodev128(0x7B) types in their wasm modules. Under Nehalem emulation (no AVX), JSC'srecomputeDependentOptions()does:which makes
v128an invalid wasm type and the module fails to parse. aarch64 is unaffected (NEON is always available), which is why only the Linux x64 lanes failed.Separately,
verify-baseline.tsspawned fixtures with the bare process env, so each fixture's//@ runDefault(...)/runFTLNoCJIT(...)/runDefaultWasm(...)directive (jitPolicyScale,useJSPI,useDollarVM,createPreHeaders, ...) was ignored and the bunEnv test-harness knobs (BUN_DEBUG_QUIET_LOGS,BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING) were not set, unlikejsc-stress.test.ts.Fix
parseJSCFlagstotest/js/bun/jsc-stress/jsc-flags.tsand import it from bothjsc-stress.test.tsandscripts/verify-baseline.ts.runTest()now spawns with a bunEnv-equivalent base env plus the parsed//@flags.hasWebKitChanges()atscripts/build/deps/webkit.tsso--jit-stressactually runs on WebKit bumps again.--jit-stresson the Windows SDE lane: Pin instrumentation costs ~45s per process (measured on build 76600), so 80+ serial fixtures would take roughly an hour against the 30-minute step timeout. qemu-Nehalem on Linux verifies the same x64-no-AVX JIT output at ~1s/fixture.Tests
test/js/bun/jsc-stress/jsc-flags.test.tsunit-testsparseJSCFlagsand, more importantly, runs every wasm fixture underBUN_JSC_useWasmSIMD=falseto assertwasmSIMDFixtureslists exactly the ones that fail to parse without SIMD. If a new fixture starts (or an existing one stops) requiring v128, this test fails before the verify-baseline lane does.This is a CI-tooling change (no
src/edits); the fail-before-fix gate that stashessrc/is not meaningful here.[stamp-90s] gate passed · iteration 0 · 5 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file