Skip to content

Add JIT-tier regression tests for int32-boundary parseInt/Map/switch values; fix local WebKit LTO flags - #31679

Open
robobun wants to merge 1 commit into
mainfrom
farm/f7c95afc/fix-parseint-lto-jsc
Open

Add JIT-tier regression tests for int32-boundary parseInt/Map/switch values; fix local WebKit LTO flags#31679
robobun wants to merge 1 commit into
mainfrom
farm/f7c95afc/fix-parseint-lto-jsc

Conversation

@robobun

@robobun robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

What this PR is now

Regression tests for the int32-boundary JIT miscompiles, plus a local-build LTO flag fix. The JSC source fixes landed in oven-sh/WebKit#244 and reached main via the WebKit upgrade in #31724 — this PR was rebased accordingly (it previously pinned preview artifacts of the equivalent oven-sh/WebKit#245, now superseded).

The bug being guarded

On official Linux x64 release builds (--lto=on), values at or above 2^31 came back as sign-wrapped int32s once the call site tiered up to the DFG:

function f(s) { return parseInt(s, 16) }
// after ~10k hot calls on 1.4 canary (≤ e75a55dab):
f("80000000") === -2147483648   // expected 2147483648; wrong for the life of the process

Same failure mode for Map keys (#31080: 2**31, even ±Infinity-2147483648) and switch-immediate dispatch. Fresh processes replayed clean — lower tiers box through hardened paths — making this miserable to track down downstream.

Root cause: JSC had out-of-range doubleint casts (undefined behavior) in round-trip checks deciding int32 boxing/dispatch (parseIntResult, slow_path_switch_imm, and friends). The Linux release pipeline ships JSC as LTO bitcode whose final optimization runs in rust-lld — rustc 1.97's LLVM 22, newer than the clang 21 that produced the bitcode — and LLVM 22's InstCombine legally folds (double)(int)x == x into a bare integrality test, deleting the overflow guard. Verified by disassembly (vroundsd+unguarded vcvttsd2si in the shipped artifact vs. the intact check from lld-21 on identical bitcode). x86-64 only (aarch64's fcvtzs round-trip survives the fold), LTO only, JIT-tier only — exactly the reported symptom matrix.

Changes

  • test/js/bun/jsc/parseint-jit-int32-overflow.test.ts — two concurrent tests spawning bun with a small jitPolicyScale, hammering parseInt (hex/decimal/negative/Infinity-overflow), Map key normalization (keys ≥ 2^31, ±Infinity, has() probes), and a dense op_switch_imm jump table with out-of-range scrutinees. They fail in ~130 ms on the last broken canary and exercise the real failure mode on CI's Linux x64 release (LTO) lanes; debug builds pass by construction.
  • scripts/build/deps/webkit.ts — forward the artifact builders' LTO flags (-flto=full -fwhole-program-vtables -fforce-emit-vtables, per the oven-sh/WebKit Dockerfile, C and C++ alike) to local Linux/FreeBSD WebKit builds. With plain -flto=thin, --webkit=local --lto=on fails to link with inconsistent LTO Unit splitting (bun-profile links with -fwhole-program-vtables). This is how the fix was validated end-to-end before prebuilt artifacts existed.

Verification

binary result
official canary e75a55dab (pre-#31724) parseInt repro fails @ ~22k iters; regression test fails in ~130 ms
--lto=on @ current main pin (6d586e29, includes oven-sh/WebKit#244) parseInt repro clean through 2M iters; both tests pass
--lto=on @ patched local WebKit (pre-artifact validation) clean through 10M iters incl. forced FTL; guard visibly restored in disassembly
bun bd (debug/ASAN) both tests pass (5.4 s)

Fixes #31080

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR updates the WebKit build dependency to a newer autobuild tag and refines LTO configuration on Unix non-darwin platforms, then adds two regression tests that verify JSC JIT behavior does not corrupt parseInt results or Map/switch dispatch outcomes for values at or above the int32 boundary (2^31).

Changes

WebKit dependency and JIT regression coverage

Layer / File(s) Summary
WebKit version and LTO configuration
scripts/build/deps/webkit.ts
WEBKIT_VERSION is pinned to a newer autobuild-preview-pr-245-* tag. LTO flags are conditionalized: full LTO with whole-program and force-emit vtables on Unix non-darwin; thin LTO otherwise.
JIT regression tests for int32 overflow
test/js/bun/jsc/parseint-jit-int32-overflow.test.ts
Two concurrent tests spawn subprocess JSC execution to verify parseInt and Map/switch dispatch do not truncate or wrap double values at/above 2^31 after JIT tier-up with low policy scale.

Possibly related PRs

  • oven-sh/bun#31169: Also updates WEBKIT_VERSION in scripts/build/deps/webkit.ts as part of WebKit dependency wiring.

Suggested reviewers

  • Jarred-Sumner
  • dylan-conway
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately describes the two primary changes: adding JIT regression tests for int32-boundary issues and fixing WebKit LTO flags.
Description check ✅ Passed The description includes both required sections with comprehensive detail: 'What does this PR do?' covers the regression tests and LTO flag fix; 'How did you verify your code works?' provides extensive verification results across multiple binary versions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the claude label Jun 2, 2026
@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:04 PM PT - Jun 2nd, 2026

@robobun, your commit cd67c9e has 1 failures in Build #60027 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31679

That installs a local version of the PR into your bun-31679 executable, so you can run:

bun-31679 --bun

@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Status: root-caused and fixed.

  • Reproduced: parseInt("80000000", 16) === -2147483648 after DFG tier-up on official linux-x64 + x64-baseline canary artifacts (e75a55dab); 10M iterations clean on non-LTO builds of the same source.
  • Root cause: out-of-range static_cast<int>(double) UB in JSC's DFG parseIntResult(); LLVM 22 (rust-lld LTO backend) folds the overflow guard away. Confirmed by disassembly and by replaying the exact pipeline (clang-21 bitcode → rust-lld) on the isolated pattern: lld-21 keeps the guard, LLVM 22 deletes it.
  • Fixes: source fix in JSTests: stress tests for the int32-boundary double->int UB fixed in #244 WebKit#245 (validated end-to-end with --webkit=local --lto=on); this PR links the non-LTO JSC prebuilt until a pin includes it, with a self-obsoleting workaround-registry entry.
  • Test: fails in ~130 ms on the broken canary, passes on fixed LTO builds, passes in 1.3 s on debug. Note: fix is scripts/build/**, so the stash-based debug gate can't exercise it — fail-before/pass-after was demonstrated on release LTO binaries (table in the PR body).

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Number map keys are truncated to int32 in canary #31080 - Reports numbers >= 2^31 truncated to -2147483648 in Map keys on canary (Linux x86_64 only), which is the same int32 sign-wrapping caused by the LTO miscompilation this PR fixes

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #31080

🤖 Generated with Claude Code

@robobun
robobun force-pushed the farm/f7c95afc/fix-parseint-lto-jsc branch from f1eb621 to 1445d1a Compare June 2, 2026 01:32
Comment thread scripts/build/deps/webkit.ts Outdated
Comment thread scripts/build/deps/webkit.ts
@robobun robobun changed the title Fix parseInt returning sign-wrapped int32 for results >= 2^31 after JIT warmup on Linux LTO builds Fix parseInt returning sign-wrapped int32 for results >= 2^31 after JIT warmup on Linux x64 LTO builds Jun 2, 2026
@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Status update: reworked after CI feedback. The non-LTO-JSC swap broke whole-program devirtualization (native-archive vtables are invisible to the LTO summary) — jsc-stress segfaults + a GC-finalizer miss on aarch64 and several x64 shards. Reverted it; the PR now pins WEBKIT_VERSION to autobuild-preview-pr-245-88395aed, the preview artifacts of the actual JSC source fix (oven-sh/WebKit#245), keeping the exact build layout CI already ships. Locally validated on linux-x64 --lto=on: parseInt repro clean through 10M iterations with forced FTL, regression test passes (still fails ~130 ms on current canary), and bundler_defer + jsc-stress — the suites that caught the bad approach — pass. Re-pin to the merged sha once oven-sh/WebKit#245 lands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 scripts/build/deps/webkit.ts:70-80 — Following up on the comment above: the "update the test" option there is unsafe — only "gate the suffix removal to cfg.linux" is acceptable. Dropping -lto unconditionally also moves the darwinCross release link (an actual shipping config — ltoDefault at config.ts:761 enables LTO for cross-built macOS CI lanes) onto native-object JSC, while that link still passes -Wl,-mllvm,-whole-program-visibility (flags.ts:864) under -flto=thin -fwhole-program-vtables -fforce-emit-vtables -fno-split-lto-unit. The safety comment immediately above that flag (flags.ts:855-861) says the assertion holds because "every derived class is visible in this link" — true only when JSC is ThinLTO bitcode in the index. With JSC's vtables now in native Mach-O archives, index-based WPD sees an incomplete hierarchy for JSC/WTF base typeids that bun's bindings derive from, which is the configuration LLVM documents as unsafe for -whole-program-visibility. The rust-lld/LLVM-22 fold is ELF-only and the PR body says macOS is clean, so there's zero benefit to dropping -lto on darwin and a real silent-miscompile risk.

    Extended reasoning...

    What the bug is

    This builds on the existing inline comment at this location (windows-cross test failure). That comment offers two fixes — update the test or gate the removal to cfg.linux. This finding shows the first option is unsafe and only the second is acceptable, because the unconditional -lto removal also breaks an invariant the darwin LTO link is documented to depend on.

    prebuiltSuffix() now drops the -lto suffix for all platforms. On darwin that means the LTO build downloads bun-webkit-macos-{amd64,arm64}.tar.gz — native Mach-O object archives — instead of the -lto ThinLTO-bitcode prebuilt. But the darwin LTO link is explicitly architected around WebKit being ThinLTO bitcode:

    • flags.ts:472-474: "The WebKit macos -lto prebuilts and rustc's -Clinker-plugin-lto bitcode are both ThinLTO-summaried, so this makes the whole link one uniform ThinLTO graph with cross-module importing across C++/Rust/JSC boundaries."
    • flags.ts:534-535: "0 is also the default … for the WebKit macos/windows -lto prebuilts, so this is the configuration that can't drift."
    • flags.ts:855-861 (the comment above -Wl,-mllvm,-whole-program-visibility): "the linker's assertion that every derived class is visible in this link — without the visibility upgrade WPD only fires for classes explicitly annotated [[clang::lto_visibility]], i.e. never. A static executable that only dlopens C-ABI addons (NAPI) satisfies the whole-program assumption."

    That last assumption is what this PR violates. With JSC/WTF/bmalloc as native objects, their C++ class hierarchies are no longer visible in the ThinLTO index — but the link still asserts to LLVM that they are.

    Why this is a shipping config, not a dev corner case

    config.ts:761: ltoDefault = release && (linux || darwinCross) && ci && …, and config.ts:696: darwinCross = darwin && host.os !== "darwin". config.ts:759-760 notes "The -lto WebKit prebuilts only exist for the cross toolchain" — i.e., darwinCross was the consumer of bun-webkit-macos-*-lto.tar.gz. Official macOS release binaries are cross-built on Linux CI with lto: true, so this is the path that ships to users.

    The specific code path

    On a darwinCross release build (darwin && lto):

    • Compile (flags.ts:476-477, 517-518, 540-541): bun's C++ compiles with -flto=thin -fforce-emit-vtables -fwhole-program-vtables -fno-split-lto-unit -fvisibility=hidden. So every TU emits !type metadata, llvm.type.test calls, and available_externally vtables for JSC/WTF base classes visible via headers, and the type hierarchy goes into per-module ThinLTO summaries (index-based WPD).
    • Link (flags.ts:864-866, 869-871): -Wl,-mllvm,-whole-program-visibility plus -flto=thin -fwhole-program-vtables -fforce-emit-vtables. The -mllvm form sets the raw cl::opt directly (flags.ts:861-862: "ld64.lld has no named option for this; -mllvm reaches the underlying cl::opt directly") — so it's not clear ld64.lld even has lld-ELF's VisibleToRegularObjSymbols safety net, and no --lto-validate-all-vtables-have-type-infos equivalent is passed.
    • WebKit (this PR): now native .a archives with no !type metadata and no ThinLTO summaries. JSC's own subclasses of its polymorphic bases are invisible to the index.

    Step-by-step proof

    1. CI builds the official macOS arm64 release: darwin = true, darwinCross = true, release = true, ci = trueltoDefault = truecfg.lto = true, cfg.webkit = "prebuilt".
    2. prebuiltSuffix() (after this PR): not linux → skip musl/android; not baseline → skip; not debug → skip; -lto branch is gone; not asan → skip. Returns "". prebuiltUrl()bun-webkit-macos-arm64.tar.gz (native objects, not bitcode).
    3. bun's src/bun.js/bindings/** derives many classes from JSC/WTF polymorphic bases (e.g. JSC::WeakHandleOwner, JSC::Watchpoint, WTF::FunctionDispatcher, JSC::JSDestructibleObject, Inspector agents). Compiled -flto=thin -fwhole-program-vtables -fforce-emit-vtables, those TUs emit !type metadata for the JSC base typeids and llvm.type.test at virtual call sites; -fforce-emit-vtables adds available_externally vtables for JSC types whose definitions are visible in headers.
    4. ld64.lld receives -mllvm -whole-program-visibility. Index-based WPD upgrades vcall_visibility for those JSC base typeids to LinkageUnit and consults the ThinLTO summaries for the set of vtables carrying each typeid.
    5. JSC's own subclasses of those same bases live in the native .a archives — no summaries, no !type metadata. WPD's "set of all implementations" for a shared base typeid is therefore bun's bindings' subclasses only. If that set has a single implementation for some slot, WPD devirtualizes to it — and a JSC-side instance flowing to that call site at runtime jumps to the wrong function.
    6. This is exactly the configuration LLVM documents as unsafe for --lto-whole-program-visibility: native objects in the link defining classes related to those in the bitcode.

    Why existing code doesn't prevent it

    The codebase's only guard is the design assumption documented in the flags.ts comments — that the macOS -lto WebKit prebuilt is always selected so the ThinLTO graph is uniform. Nothing checks it mechanically. ld64.lld has no named --lto-whole-program-visibility (flags.ts:861-862), so whatever native-object vtable-symbol guards lld-ELF has on its named option don't obviously apply. The link will succeed silently with whatever WPD decided.

    Impact

    Silent-miscompilation risk in shipped macOS release binaries — wrong-target devirtualized calls that only manifest at runtime on specific code paths, with no link-time diagnostic. Unlike the windows-cross issue above (loud CI test failure), nothing will flag this. And unlike Linux, there is no benefit to taking the change on darwin: per the PR body, "macOS/Windows are clean" and the rust-lld/LLVM-22 fold is reached only via the rust-lld-for-crosslang-lto swap, which is ELF-only.

    How to fix

    Gate the suffix removal to Linux — the same fix the comment above already needs:

    else if (cfg.lto && !cfg.linux) s += "-lto";
    // -lto disabled on Linux only: … (rust-lld is ELF-only; darwin/windows
    // keep the ThinLTO bitcode prebuilt so the -whole-program-visibility
    // invariant in flags.ts holds)

    and narrow the workaround entry's applies to cfg.linux && cfg.lto && cfg.webkit === "prebuilt". Do not take the "update the test" option from the comment above — that would leave shipping macOS binaries linking with a violated WPD whole-program assumption.

  • 🟡 scripts/build/workarounds.ts:215 — Nit: expectedToBeFixed keys off cfg.webkitVersion (which reflects the per-invocation --webkit-version=<hash> CLI override) rather than the WEBKIT_VERSION source constant. A dev testing an unrelated preview via e.g. configure --webkit-version=autobuild-preview-... --lto=on will hit a hard "workaround obsolete — delete this entry" BuildError, which is wrong guidance for a one-off override. Comparing the imported WEBKIT_VERSION constant instead (expectedToBeFixed: () => WEBKIT_VERSION !== "963f8758...") trips exactly when the pin is bumped — the stated intent — and ignores transient CLI overrides.

    Extended reasoning...

    What the bug is

    The new webkit-lto-parseint-fold registry entry uses expectedToBeFixed: cfg => cfg.webkitVersion !== "963f8758c29e965471c191668d5776a1a1b014b6". cfg.webkitVersion is the resolved runtime value — config.ts:1022 sets it to partial.webkitVersion ?? WEBKIT_VERSION, and config.ts:1018-1019 documents it as "Override via --webkit-version=<hash> etc. to test a branch before bumping the pinned default". So this predicate is keyed to a per-invocation CLI flag, not to committed source state.

    The PR's stated intent — both the inline comment in prebuiltSuffix() ("workarounds.ts … trips on the next pin bump") and the PR body ("the next WEBKIT_VERSION bump fails configure") — is to trip when the source pin changes. The cleanup text itself says "Verify the new WEBKIT_VERSION includes…", referencing the source constant. Comparing the imported WEBKIT_VERSION constant matches that intent exactly; comparing cfg.webkitVersion does not.

    Step-by-step proof

    1. A developer wants to test an unrelated WebKit preview build before bumping the pin (the workflow config.ts:1018 explicitly documents). They run configure --webkit-version=autobuild-preview-abc123 --lto=on.
    2. resolveConfig() sets cfg.webkitVersion = "autobuild-preview-abc123", cfg.lto = true, cfg.webkit = "prebuilt" (the default per config.ts:1129).
    3. configure() calls checkWorkarounds(cfg) unconditionally at configure.ts:275 — there is no skip flag or env-var escape hatch.
    4. applies(cfg)cfg.lto && cfg.webkit === "prebuilt"true.
    5. expectedToBeFixed(cfg)"autobuild-preview-abc123" !== "963f8758…"true.
    6. checkWorkarounds() throws BuildError: Workaround 'webkit-lto-parseint-fold' is obsolete — upstream fix is available, with a hint telling the dev to restore the -lto branch in prebuiltSuffix() and delete the registry entry. Configure aborts.

    That guidance is wrong for a one-off override: the source pin hasn't changed, the workaround in prebuiltSuffix() is still active and correct (it doesn't consult cfg.webkitVersion at all), and the dev's only options are to dirty workarounds.ts locally or drop --lto.

    Edge case making it clearer: --webkit-version=autobuild-963f8758c29e965471c191668d5776a1a1b014b6 — the same broken pin in prefixed form, which both prebuiltUrl() (version.startsWith("autobuild-") ? version : …) and prebuiltDestDir() accept and normalize to the identical artifact — also trips the check, since the predicate is an exact string compare. Same broken bitcode, falsely claimed obsolete.

    Why this differs from the rest of the registry

    Every other expectedToBeFixed in this file keys off committed/detected state — cfg.clangVersion, cfg.rustLlvmVersion, the contents of Cargo.lock, the SDK's libcompiler_rt.tbd — none key off a transient CLI override. The file's design doc says false positives are acceptable because "bump the threshold… takes 30 seconds", but that escape hatch presumes a committable threshold edit; a one-off --webkit-version test has no threshold to bump.

    Impact

    Low — dev-only configure-time friction on a niche flag combo (--webkit-version override + LTO + prebuilt), loud error rather than a miscompile, easy local workaround. Hence nit. But it's a real correctness gap in code this PR adds, the fix is one line, and it matches the author's own stated intent more precisely than the current code does.

    Fix

    import { WEBKIT_VERSION } from "./deps/webkit.ts";
    // …
    expectedToBeFixed: () => WEBKIT_VERSION !== "963f8758c29e965471c191668d5776a1a1b014b6",

    This trips exactly when someone edits the source pin (the intended trigger) and ignores transient --webkit-version overrides.

Comment thread scripts/build/deps/webkit.ts Outdated
Comment thread test/js/bun/jsc/parseint-jit-int32-overflow.test.ts Outdated
@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the previous run (build 59800) was green on all 280 completed jobs — every Linux/Windows/FreeBSD build+test lane and the darwin build lanes passed, including the new regression tests. The only red was one darwin-14 x64 test shard whose failures were agent-environment issues unrelated to this diff (PostgresError: role "bun_sql_test" does not exist — unprovisioned postgres on the runner — and the autobahn docker service failing to start), plus macOS agent-queue expiries that Buildkite auto-retried. Re-rolled once for a clean run.

Comment thread test/js/bun/jsc/parseint-jit-int32-overflow.test.ts Outdated
Comment thread test/js/bun/jsc/parseint-jit-int32-overflow.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@scripts/build/deps/webkit.ts`:
- Around line 271-273: The vtable LTO flags (-fwhole-program-vtables,
-fforce-emit-vtables) are being added to the shared optFlags and therefore end
up in C builds (CMAKE_C_FLAGS); change the logic so those flags are only
appended to the C++ flags: stop pushing them into optFlags for the cfg.lto &&
cfg.unix && !cfg.darwin branch and instead append them only to the C++-specific
flag string (e.g., after optFlagStr is created, add them to cxxOptFlagStr or
maintain a cxxOnly array and concatenate it into cxxOptFlagStr). Update
references to optFlags, optFlagStr, and cxxOptFlagStr accordingly so
CMAKE_C_FLAGS no longer contains the vtable flags.
🪄 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: 758ced0f-8cfb-4acb-8711-7f52c296d1bb

📥 Commits

Reviewing files that changed from the base of the PR and between 561eb8f and 57e166b.

📒 Files selected for processing (2)
  • scripts/build/deps/webkit.ts
  • test/js/bun/jsc/parseint-jit-int32-overflow.test.ts

Comment thread scripts/build/deps/webkit.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found in the current revision — all prior nits have been addressed — but this bumps the WebKit/JSC pin to an unmerged preview artifact and changes release-build LTO flags, so a human should sign off on shipping a canary from this pin (vs. waiting for oven-sh/WebKit#245 to merge and re-pinning to the merged sha).

Extended reasoning...

Overview

This PR touches two files: scripts/build/deps/webkit.ts (the WebKit/JavaScriptCore dependency pin and local-build LTO flag selection) and a new regression test test/js/bun/jsc/parseint-jit-int32-overflow.test.ts. The substantive change is bumping WEBKIT_VERSION from a merged 40-hex sha (963f8758…) to a preview-PR release tag (autobuild-preview-pr-245-29422029) that contains a UB fix for parseIntResult() and slow_path_switch_imm, plus switching local Linux/FreeBSD WebKit builds from -flto=thin to -flto=full -fwhole-program-vtables -fforce-emit-vtables to match the prebuilt artifact builders and the bun link line.

Security risks

None identified. No auth, crypto, permissions, or untrusted-input handling is touched. The change is a dependency pin bump and compiler-flag adjustment; the new test only spawns bunExe() subprocesses with fixed inline scripts.

Level of scrutiny

High. WEBKIT_VERSION selects the JavaScriptCore engine that ships in every release artifact — it is among the most production-critical knobs in the repo. The pin is also explicitly temporary (the inline comment and PR body both say "Re-pin to the merged sha once #245 lands"), and it points at an unmerged preview build. Whether to merge now (and ship a canary from a preview pin, with the known process.versions.webkit / sync-webkit-source.ts quirks acknowledged in the resolved thread) or wait for the upstream merge is a release-management judgment call a maintainer should make. The earlier revision of this PR (non-LTO JSC swap) caused jsc-stress segfaults and GC-finalizer misses on aarch64 before being reverted, which underscores that JSC pin/build-config changes carry real runtime risk and deserve human eyes on the final CI run.

Other factors

All five of my prior inline comments have been addressed and resolved (Windows -lto suffix → reverted with the approach change; FreeBSD LTO gate → mirrors flags.ts in e1fbc09; preview-tag consumers → acknowledged as temporary; explicit timeout → dropped; test.concurrent → applied; dense op_switch_imm case set → fixed in 57e166b). The bug-hunting pass on the current revision found nothing. The PR body's verification table is thorough and CI build 59800 was reported green on 280 lanes with only environment-related flakes on one darwin shard; build 59887 against the current head was in progress at review time. The diff itself is small and well-commented — my deferral is about the nature of the change (core engine pin to an unmerged preview), not about correctness concerns in the code.

@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Status — ready for maintainer review/merge.

The diff is green everywhere it runs: two consecutive full CI builds (59800, 59887) passed 280 jobs each — every Linux/Windows/FreeBSD build+test lane, darwin builds, and the new regression tests. The single red lane in both builds is the same darwin-14 x64 test shard failing on agent-environment issues only, identically both times and unrelated to this diff:

  • test/js/sql/sql-prepare-false.test.tsPostgresError: role "bun_sql_test" does not exist (postgres not provisioned on that macOS runner)
  • test/js/web/websocket/autobahn.test.ts — the autobahn docker-compose service fails to start on that runner

Remaining darwin shards from the latest build are queued behind macOS agent capacity but have passed in both builds wherever they ran.

Merge notes:

Full investigation, root cause (LLVM 22 LTO folding UB in JSC's parseIntResult/slow_path_switch_imm), and the fail-before/pass-after matrix are in the PR description.

@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Addendum: a second darwin-14 x64 shard completed with the identical agent-environment signature (role "bun_sql_test" does not exist across all sql test files incl. test/regression/issue/21311.test.ts, plus docker/network-dependent websocket tests) — all on the same runner pool, all unrelated to this diff. The new regression tests pass on the darwin x64 release binary (2 pass, 79 ms), extending the validation matrix to macOS. 282 jobs green on this build; the red is confined to that agent pool's provisioning.

…bKit LTO flags

parseInt results, Map keys, and switch-immediate scrutinees at or above
2^31 came back as sign-wrapped int32s on official Linux x64 release
builds once the call site tiered up to the DFG: JSC had out-of-range
double->int casts (undefined behavior) in the round-trip checks that
decide int32 boxing/dispatch, and the LLVM 22 LTO backend (rust-lld,
via the cross-language LTO pipeline) folds those checks into bare
integrality tests. parseInt("80000000", 16) === -2147483648 after
~10k hot calls, persisting for the life of the process; Map keys
(issue #31080) and switch dispatch had the same failure mode. Fresh
processes replayed clean because lower tiers box through the hardened
paths.

The source fixes landed in oven-sh/WebKit#244 and reached main with the
WebKit upgrade in #31724. This adds the regression coverage: tests that
hammer all three paths past JIT tier-up (they fail in ~130 ms on the
last broken canary and run on CI's Linux x64 release lanes, which build
with LTO — debug builds cannot exhibit the fold).

Also forward the artifact builders' LTO flags (-flto=full
-fwhole-program-vtables -fforce-emit-vtables) to local Linux/FreeBSD
WebKit builds: with plain -flto=thin, --webkit=local --lto=on fails to
link with 'inconsistent LTO Unit splitting' because bun-profile links
with -fwhole-program-vtables. This is how the fix was validated
end-to-end before prebuilt artifacts existed.

Fixes #31080
@robobun
robobun force-pushed the farm/f7c95afc/fix-parseint-lto-jsc branch from 57e166b to cd67c9e Compare June 2, 2026 22:01
@robobun robobun changed the title Fix parseInt returning sign-wrapped int32 for results >= 2^31 after JIT warmup on Linux x64 LTO builds Add JIT-tier regression tests for int32-boundary parseInt/Map/switch values; fix local WebKit LTO flags Jun 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues with the final diff — all prior review feedback is addressed and CI is green. Worth noting for whoever picks this up: after rebasing on #31724 (which landed the merged WebKit sha), the diff no longer touches WEBKIT_VERSION; what remains is just the local-WebKit LTO flag fix (dev-only, --webkit=local --lto=on) plus the regression tests, so the review surface is smaller than the description suggests.

Extended reasoning...

Overview

The final diff touches two files:

  • scripts/build/deps/webkit.ts (+11/−1): replaces -flto=thin with -flto=full -fwhole-program-vtables -fforce-emit-vtables for local WebKit builds when cfg.lto && cfg.unix && !cfg.darwin. This codepath only runs when cfg.webkit !== "prebuilt" — i.e., the --webkit=local dev workflow — and fixes a hard "inconsistent LTO Unit splitting" link error. It does not affect CI/release builds, which use prebuilt WebKit.
  • test/js/bun/jsc/parseint-jit-int32-overflow.test.ts (new, 105 lines): two test.concurrent regression tests spawning Bun subprocesses with BUN_JSC_jitPolicyScale=0.001 to verify parseInt, Map key normalization, and op_switch_imm dispatch don't sign-wrap doubles ≥ 2³¹ after JIT tier-up.

Notably, the WEBKIT_VERSION pin to autobuild-preview-pr-245-* described in the PR body and earlier review threads is no longer in the diff#31724 landed the merged WebKit sha (6d586e29…) first, so the actual JSC source fix shipped there and this PR was rebased on top.

Security risks

None. No auth, crypto, permissions, network, or untrusted-input handling. The build-script change only affects compiler flags for a developer-local build mode; the test file only spawns the bun binary under test with inline scripts.

Level of scrutiny

Moderate, warranting a maintainer sign-off rather than bot approval. Although the final diff is small and the webkit.ts change is dev-only, it adjusts LTO/vtable compilation flags for JavaScriptCore. This PR's own history demonstrates how subtle JSC link-configuration changes can be (an earlier revision swapping to non-LTO JSC archives caused jsc-stress segfaults and GC-finalizer misses on aarch64). The author also explicitly requested maintainer review.

Other factors

  • All six of my prior inline comments (one blocking, five nits) have been addressed and resolved; the coderabbit C-vs-C++ flag concern was discussed and intentionally kept as-is to mirror the upstream Dockerfile.
  • CI is green across 282 jobs on the latest build; the only red is a known macOS runner provisioning issue (unprovisioned postgres role / autobahn docker) unrelated to this diff.
  • No CODEOWNERS coverage for either file.
  • The regression tests follow test/CLAUDE.md conventions (concurrent, no explicit timeouts, subprocess via bunExe()/bunEnv).

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Triage note: the JSC fix reached main in #31724 and main still has no test for parseInt results, Map keys or switch scrutinees at or above 2^31 after tier-up, so the test file is still worth landing (both tests pass against current main). The scripts/build/deps/webkit.ts hunk should be dropped though: since #34782 Linux links with ThinLTO plus -fno-split-lto-unit, and the WebKit prebuilts are built the same way, so switching local WebKit builds to -flto=full no longer matches anything. The header comment in the test should also point at oven-sh/WebKit#244 rather than #245.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Number map keys are truncated to int32 in canary

1 participant