Fail fast at startup when simdutf has no usable implementation for this CPU - #30642
Fail fast at startup when simdutf has no usable implementation for this CPU#30642robobun wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a runtime probe and early startup guard that aborts with a platform-specific SIMDUTF requirement message (exit 134) when no real SIMDUTF implementation is selected; includes a low-level abort routine, a defensive empty-slice fix, a Cargo dependency, and two regression tests. ChangesSIMDUTF CPU support validation and early failure
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/regression/issue/30613.test.ts`:
- Around line 42-43: Remove the brittle negative assertions that check stderr
for crash words and instead assert on the process termination status: delete the
two expect(...) calls referencing stderr and replace them with assertions that
validate the child/process finished cleanly using signalCode and/or exitCode
(e.g., expect(signalCode).toBeNull() or expect(exitCode).toBe(0)) so the test
verifies termination status rather than text content; update the test in
issue/30613.test.ts to reference the existing stderr, signalCode and exitCode
variables accordingly.
🪄 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: 730dd3f2-63b2-41b8-82dc-cb25184e786e
📒 Files selected for processing (5)
src/main.zigsrc/simdutf_sys/bun-simdutf.cppsrc/simdutf_sys/simdutf.zigsrc/string/immutable.zigtest/regression/issue/30613.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing all three points — the reorder before convertEnvToWTF8/initArgv and the raw-stderr/std.c.getenv path look right, and the Environment.baseline branch fixes the diagnostic wording. I'm not auto-approving only because this touches main() startup ordering on every platform; worth a human sign-off (and confirming the Windows shards are green on 6671584, since the CI comment above is still pinned to e03c408).
Extended reasoning...
Overview
This PR adds a fail-fast probe at the very top of main() that calls simdutf::validate_ascii("a", 1) to detect when simdutf has fallen back to its unsupported_implementation stub (which happens on CPUs below the build's compiled-in ISA floor — SSE4.2 for baseline, AVX2 for default). On detection it prints a build-variant-specific diagnostic to raw stderr and exits 134, replacing a ~16 s / ~4 GB death spiral ending in SIGSEGV. It also adds a defensive empty-slice short-circuit to firstNonASCII and a regression test that simulates the condition via SIMDUTF_FORCE_IMPLEMENTATION.
Files touched: src/main.zig (entry point), src/simdutf_sys/bun-simdutf.cpp + simdutf.zig (new probe), src/string/immutable.zig (one-line guard), test/regression/issue/30613.test.ts (new).
Security risks
None identified. The new code reads one environment variable (SIMDUTF_FORCE_IMPLEMENTATION) for diagnostic display only, writes a fixed-format message to stderr, and exits. No parsing of untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
High. src/main.zig:main() is the absolute entry point — every Bun invocation runs this code, and the probe is now the first non-trivial call after crash_handler.init(). Startup ordering here is platform-sensitive: my earlier review caught that the original placement ran after Windows convertEnvToWTF8/initArgv (which themselves use simdutf), causing a panic before the diagnostic could print. That was fixed in 6671584 by moving the probe earlier and switching to raw std.fs.File.stderr().writerStreaming(&.{}) + std.c.getenv (both established patterns — the crash handler uses the same writer construction). The fix looks correct, but changes to process-startup ordering across Windows/POSIX deserve a maintainer's eyes rather than bot approval.
Other factors
- All three of my prior inline comments (Windows ordering 🔴, baseline-vs-default diagnostic wording 🟡, negative stderr assertions 🟡) were addressed in b03a7eb / e03c408 / 6671584 and are marked resolved.
- The robobun CI status comment is still showing results for e03c408 (which had the expected Windows failures I'd flagged); 6671584 is the fix for those, so the Windows shards should be confirmed green on the latest commit before merge. The musl build failures listed there look like infra and are likely unrelated.
- The
firstNonASCIIempty-slice guard is a one-line, zero-risk hardening on a hot path. - Test coverage is reasonable: one case forces the stub and asserts the diagnostic + clean exit, one case asserts normal runs are unaffected.
- Exit code 134 (SIGABRT-like) is a minor design choice a maintainer may want to weigh in on vs. a plain non-zero exit.
There was a problem hiding this comment.
Thanks — all three of my earlier comments are addressed and the current revision looks correct to me, but since this adds a hard-exit probe to main() that runs before Output/env/argv init on every process start, I'd like a human to sanity-check the startup ordering rather than auto-approving.
Extended reasoning...
Overview
This PR adds an early-startup probe (simdutf__has_implementation() → validate_ascii("a", 1)) in src/main.zig that detects when simdutf has fallen back to its unsupported_implementation stub (host CPU below the build's SIMD floor) and exits with a clear diagnostic via a new bun_abort_missing_simd C helper, instead of the prior ~16 s / ~4 GB heap-walk segfault. It also adds a defensive empty-slice short-circuit to firstNonASCII, the Zig binding hasAnyImplementation(), and a regression test that forces the stub via SIMDUTF_FORCE_IMPLEMENTATION. Six files touched: src/main.zig, src/jsc/bindings/c-bindings.cpp, src/simdutf_sys/{bun-simdutf.cpp,simdutf.zig}, src/string/immutable.zig, and test/regression/issue/30613.test.ts.
Security risks
None identified. The new code reads one environment variable (SIMDUTF_FORCE_IMPLEMENTATION) for diagnostic display only and otherwise just calls fprintf(stderr) + exit(134) with compile-time-constant strings. No user-controlled data flows into format strings, allocations, or control flow.
Level of scrutiny
High. The probe is inserted into main() immediately after crash_handler.init() and before Windows convertEnvToWTF8/initArgv and Output.Source.Stdio.init(). It executes on every Bun invocation on every platform; a false negative from validate_ascii("a", 1) (or any future simdutf change that makes a real implementation return false for a 1-byte input) would brick every process with exit 134. The ordering relative to other early-startup steps is subtle — I already caught one ordering bug here (probe originally ran after Windows UTF-16→UTF-8 env conversion, which itself uses simdutf), and the fix routed the diagnostic through raw C-runtime stderr/getenv to avoid depending on uninitialized Output/converted env. That interaction is exactly the kind of thing a maintainer familiar with Bun's startup sequencing should eyeball.
Other factors
All prior review feedback (mine and CodeRabbit's) has been addressed in commits e03c408, b03a7eb, 6671584, and 92c1aa5: the brittle not.toContain("panic") assertions were removed, the diagnostic now branches on Environment.baseline to name AVX2 vs SSE4.2 correctly and point non-baseline users at the baseline download, and the probe was hoisted before the Windows env/argv conversion with output rerouted through fprintf(stderr). The regression test covers both the failure and the happy path. The change is well-reasoned and a strict improvement over the status quo; my deferral is purely about the blast radius of main(), not any specific concern with the current code.
CI statusRebuilt on current main at ad9b670 (single commit, 6 files, +121). The rebase conflicted with two upstream changes that obsoleted parts of the earlier revisions, so rather than carry dead code through 13 commits the branch was squashed and re-derived:
What remains is the startup check, Verification on this revision:
Rebased onto main at 0cf6ac1 (the commit's diff is byte-identical to ad9b670; only the base moved). Reason: build 98222 for the previous head finished with 177/177 real jobs passed and zero test failures, but ended
Build 98597 is now complete: 174 of 179 jobs passed; every failure is a shard of the single debian-13 x64-asan lane. All other lanes, including the restored darwin aarch64 lane, are green. Each failing test is also failing on other branches' concurrent builds, so this is a regression in the ASAN lane on main (which is PR-only, so main's own builds stay green while every PR's ASAN lane fails). All six are reported to main-break triage:
This diff adds one Ready for a maintainer to merge. |
|
Rebased onto main (post-Rust-rewrite) and ported:
The Gate verified: without the |
de746bc to
f079b91
Compare
082c451 to
1726642
Compare
robobun
changed the title
There was a problem hiding this comment.
All prior feedback is addressed and I didn't find anything new, but this touches the very first code in main() on every invocation plus platform-specific Rosetta 2 dispatch recovery that relies on simdutf's internal kernel ordering — worth a human pass before merge.
Extended reasoning...
Overview
This PR adds an early-startup probe in src/bun_bin/lib.rs::main() that detects simdutf's unsupported_implementation stub (selected when the host CPU lacks every ISA the binary was compiled for) and either (a) aborts with a clear diagnostic via a new bun_abort_missing_simd C function, or (b) on macOS x64 under Rosetta 2, forces simdutf onto its least-demanding compiled kernel and continues. Supporting changes span bun-simdutf.cpp (probe + recovery), c-bindings.cpp (abort routine + Rosetta-aware suppression of the pre-existing AVX warning), upgrade_command.rs (baseline-download hint constant), Rust/Zig FFI wrappers, a defensive empty-slice guard in firstNonASCII, and a regression test.
Security risks
None identified. The new code reads SIMDUTF_FORCE_IMPLEMENTATION and sysctl.proc_translated, prints static C-string diagnostics to stderr, and calls exit(134). No untrusted input is parsed; no auth/crypto/permissions surfaces are touched.
Level of scrutiny
High. The probe runs as the very first statement in main() on every Bun invocation across all platforms — before argv capture, before the crash handler, before Windows env conversion. The Rosetta recovery path mutates process-global simdutf dispatch state based on a non-obvious invariant ("the last entry in get_available_implementations() requires no more than the binary's compiled ISA") that depends on simdutf's internal CAN_ALWAYS_RUN_* pruning behavior. The recovery branch is only exercised on the x64-under-Rosetta CI lane, not by the env-var simulation in the regression test. These are exactly the kinds of cross-platform, hard-to-test-locally startup changes that benefit from a maintainer's eye.
Other factors
This PR has been through six rounds of inline review (Windows ordering, baseline-vs-default diagnostic wording, test assertion style, stale comment cross-reference, pre-existing musl URL bug, and the spurious AVX warning under Rosetta) — all resolved, the last one in the current HEAD commit 1c90755. CI was reported green on the diff with only unrelated infra flakes remaining. The implementation is well-commented and the PR description is thorough. My hesitation to auto-approve is purely about scope (process entry point + platform-specific runtime dispatch manipulation), not about any specific concern with the code as written.
97d49af to
d89628d
Compare
There was a problem hiding this comment.
All my prior feedback has been addressed and I have no further findings, but this touches the process entry point and adds platform-specific CPU-dispatch recovery that relies on simdutf's internal implementation ordering — worth a human sign-off.
Extended reasoning...
Overview
This PR adds a simdutf probe at the very top of main() (before argv/env capture, before the crash handler), a Rosetta 2 recovery path that force-selects a simdutf kernel via get_active_implementation() = ..., a new [[noreturn]] C entry point (bun_abort_missing_simd), a sysctl-gated early-return in bun_warn_avx_missing, and a baseline-URL hint constant in upgrade_command.rs. It spans Rust, C++, and reference-only Zig across 11 files, plus a 3-case regression test.
Security risks
None identified. The only new untrusted input is the SIMDUTF_FORCE_IMPLEMENTATION env var, which is read via CRT getenv and printed back through fprintf("%s") — no format-string or injection surface. The sysctl read is fixed-name with a stack-local int.
Level of scrutiny
High. The probe runs unconditionally on every process start, before any other initialization, and a false negative would make Bun refuse to launch on valid hardware. The Rosetta recovery depends on two simdutf-internal invariants (the available-implementations list is ordered most→least demanding, and CAN_ALWAYS_RUN_* pruning leaves the last entry no stricter than the TU's own -march); both are well-argued in the comments and re-probed defensively, but they're assumptions about a vendored dependency's internals that a maintainer should ratify. The change also reorders main() so the probe precedes init_argv / the crash handler — intentional and explained, but entry-point ordering is exactly the kind of thing that benefits from a second pair of eyes.
Other factors
I've reviewed this PR across five revisions; every issue I raised (Windows ordering, baseline-vs-AVX2 wording, stale Zig cross-reference, AVX warning under Rosetta, pre-existing musl URL) has been addressed or explicitly deferred. The bug-hunting system found nothing this run. CI is green on the new test across all lanes; the remaining failures are documented as repo-wide and unrelated. The change is well-reasoned and well-tested — I'm deferring purely on scope (process entry point + cross-language FFI + platform-specific dispatch), not on any open concern.
d89628d to
8b56842
Compare
|
Is there a chance that this could be merged soon? |
8b56842 to
a8b03a0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
a8b03a0 to
4de8c29
Compare
robobun
changed the title
4de8c29 to
ad9b670
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped startup diagnostic that's been through several review rounds; all prior threads are resolved.
What was reviewed:
- Probe placement in
main():init_argvonly stores raw pointers (no simdutf), and the check runs before the crash handler / Windows env conversion, both of which do reach simdutf. - False-positive risk on supported CPUs:
validate::ascii(b"a")can only return false via the unsupported stub; the second regression test guards this on every CI lane. bun_abort_missing_simduses CRTfprintf/getenvonly, so it works beforeOutputand the WTF-8 env block are initialized (including on Windows).- Test follows harness conventions (concurrent pipe drain,
bunEnvspread,skipIf(isArm64)with a stated reason, stderr asserted before exit code).
Extended reasoning...
Overview
Adds a one-byte simdutf probe at the top of main() (src/bun_bin/lib.rs) that aborts with a clear diagnostic when simdutf's runtime dispatcher has installed its unsupported_implementation stub — the case hit on pre-SSE4.2 x64 CPUs (or QEMU's default TCG vCPU) where the -march=nehalem build has compiled out the scalar fallback. The abort path is a new #[cold] Rust helper calling a new C++ bun_abort_missing_simd in c-bindings.cpp that writes to CRT stderr and exits 134. has_any_implementation() is a 3-line wrapper over the existing validate::ascii FFI in src/simdutf_sys/simdutf.rs. Cargo.toml/Cargo.lock add the bun_simdutf_sys dep to bun_bin. A two-test regression file reproduces the stub via SIMDUTF_FORCE_IMPLEMENTATION and confirms the happy path is unaffected.
Security risks
None. No user input is parsed; the only external read is getenv("SIMDUTF_FORCE_IMPLEMENTATION"), printed via %s (not a format string). No auth, crypto, or permission surfaces touched.
Level of scrutiny
Moderate. The change sits in main(), so a false positive would break every invocation — but the addition is a single validate_ascii(b"a") call whose only false path is the stub itself, and the second regression test (runs normally when a supported simdutf implementation is available) directly covers that on every platform lane. init_argv (called before the probe) was verified to only store atomic pointers with no simdutf dependency. The C++ side deliberately uses CRT primitives so it has no dependency on Output::Source or the converted Windows env block; the Rust extern signature matches the C++ definition ([[noreturn]] ↔ -> !, two const char*).
Other factors
This PR has been through seven-plus review iterations; every prior finding (test anti-patterns, stale comment cross-references, description/diff mismatch after the haswell-build removal, comment-cop verbosity flags) has been addressed and the threads resolved. The one candidate issue raised this run — that the SSE4.2 wording is technically imprecise on genuine Nehalem (westmere kernel also needs PCLMULQDQ) — was examined and ruled out as a diagnostic-wording nit that doesn't affect correctness. No CODEOWNERS cover the touched paths. The test file follows repo conventions: test.concurrent, Promise.all pipe drain, {...bunEnv, ...}, skipIf(isArm64) with the single-implementation rationale, and content assertions before exit-code assertions.
…is CPU Bun's x64 builds are compiled with -march=nehalem, which makes simdutf drop its scalar fallback on the assumption that the SSE4.2 kernel can always run. On a host without SSE4.2 (QEMU's default TCG vCPU, very old hardware) simdutf's dispatcher installs an unsupported_implementation stub whose functions all return 0/false. Bun and WebKit trust those answers, so valid files fail UTF-8 validation and the scan loops built on first_non_ascii never advance: running any script hangs or dies with no useful output. Probe simdutf once at the top of main() by validating a single ASCII byte (the stub unconditionally returns false) and, if it is the stub, print which instruction set is missing and exit 134. The probe runs before the crash handler and the Windows environment conversion, both of which route strings through simdutf, so the diagnostic is written via the C runtime rather than Output. The regression test reaches the same stub by pointing SIMDUTF_FORCE_IMPLEMENTATION at an implementation name that does not exist. Fixes #30613 Fixes #14745
ad9b670 to
0cf6ac1
Compare
There was a problem hiding this comment.
LGTM — the descoped diff is small and all prior review threads are resolved.
Checked that has_any_implementation() cannot false-negative on a working CPU (validate_ascii(b"a") is true on every real simdutf kernel; only the unsupported_implementation stub returns false), and the second regression test guards this on every CI lane. Verified the probe's placement before bun_crash_handler::init() and convert_env_to_wtf8() matches the stated ordering constraint, and that bun_abort_missing_simd using CRT getenv/fprintf is correct given Output isn't initialized yet. Test follows harness conventions (bunEnv spread, concurrent pipe drain, skipIf(isArm64) with a stated reason, positive assertions on stderr/exitCode/signalCode).
Extended reasoning...
Overview
Adds a one-shot startup probe in main() (src/bun_bin/lib.rs) that calls bun_simdutf_sys::simdutf::has_any_implementation() — a thin wrapper over the existing validate::ascii FFI — and aborts via a new C++ bun_abort_missing_simd (src/jsc/bindings/c-bindings.cpp) with a diagnostic naming the missing ISA when simdutf has installed its unsupported_implementation stub. Also adds the bun_simdutf_sys dep to bun_bin and a two-test regression file. Total ~+120 lines across 6 files.
Security risks
None. No user input reaches the new code; the probe validates a single hard-coded ASCII byte and the diagnostic prints only compile-time C string literals plus the value of one env var to stderr.
Level of scrutiny
Moderate-to-high, because the probe runs on the cold-start path of every Bun invocation and a false negative would make Bun refuse to start everywhere. That specific risk is what I focused on: validate_ascii("a", 1) returns true on every real simdutf kernel and false only from the stub, and the "runs normally" regression test exercises exactly this on every CI lane. The abort path is #[cold], diverges, and uses only CRT primitives (fprintf/getenv/exit), so it has no dependency on Bun's own initialization order — appropriate given it runs before Output and (on Windows) before the WTF-8 env conversion.
Other factors
This PR has been through five prior review passes from me; every thread is marked resolved. The earlier Rosetta-recovery / baseline-hint / bun_warn_avx_missing pieces were dropped after #34782 unified x64 on -march=nehalem (making them unreachable), and the title/description were rewritten to match. The remaining code comments were trimmed per the comment-cop bot. CI build #98222 passed on all lanes that ran (the two stuck darwin-14 aarch64 shards were a fleet issue unrelated to this diff). Given the small final surface, the explicit negative-and-positive test coverage, and the fully resolved review history, I don't see anything left for a human pass to add.
2 participants
Problem
bun app.jshangs forever, or fails with a boguserror: Invalid UTF-8 byte sequenceon a valid file, or (on the v1.3.9 release in the original report) segfaults after ~16 seconds and ~4 GB of allocations. There is no hint that the CPU is the problem.-march=nehalem(scripts/build/flags.ts), which makes simdutf compile out its scalar fallback on the assumption that its SSE4.2 kernel can always run. On a CPU without SSE4.2, simdutf's dispatcher finds no usable kernel and installs anunsupported_implementationstub whose functions all return 0/false. Bun trusts those answers:validate_utf8rejects every file, andfirst_non_ascii(src/bun_core/lib.rs) reports a non-ASCII byte at index 0 for any input longer than its 32-byte scalar fast path, so the scan loops built on it never advance.-marchflags (oven-sh/WebKit@596e48e); before that the scalar fallback was compiled in.Fix
bun_simdutf_sys::simdutf::has_any_implementation()validates one ASCII byte. This forces simdutf's lazy dispatch to run, and the stub'svalidate_asciiunconditionally returns false, so the result distinguishes a real kernel from the stub without touching simdutf internals.main()insrc/bun_bin/lib.rscalls it immediately after capturing argv, before the crash handler and the Windows environment conversion (both push strings through simdutf). On failure it callsbun_abort_missing_simd(src/jsc/bindings/c-bindings.cpp), which prints which instruction set is missing (SSE4.2 on x64, NEON on arm64), a VM hint, and the value ofSIMDUTF_FORCE_IMPLEMENTATIONif set, then exits 134. It uses the C runtime becauseOutputis not initialized yet.test/regression/issue/30613.test.ts: settingSIMDUTF_FORCE_IMPLEMENTATIONto an unknown name makes simdutf install the same stub as an unsupported CPU, so the test reproduces the bug on any x64 machine. Without the fix the child hangs or fails with the UTF-8 error; with it, stderr names the requirement and the exit code is 134. A second test confirms a normal run is unaffected. The forced-stub test is skipped on arm64, where simdutf compiles a single kernel and bypasses dispatch entirely.bun app.jsunder the forced stub hangs until killed;bun --versionand short-escripts still work because they stay under the 32-byte scalar path), andcargo clippy --workspace.Background
wtf/SIMDUTF.h, so it is compiled once, inside the prebuilt WebKit). It ships several kernels (icelake, haswell, westmere, fallback) and picks one at first use by reading CPUID. Kernels that the compile-time-marchproves redundant are removed: with-march=nehalem,__SSE4_2__is defined, the westmere kernel "can always run", and the scalar fallback is dropped.unsupported_implementationis simdutf's placeholder for "no kernel matched". It is also whatSIMDUTF_FORCE_IMPLEMENTATION=<unknown name>selects, which is what makes the bug testable without special hardware.main()is not the problem), but a CPU so old that nehalem codegen itself faults beforemain()would still die with SIGILL. Moving the check into a separately compiled object, as Exit on startup if SSE4.2 is not available #14745 suggests, would be a build-system change and is out of scope here.SIMDUTF_IMPLEMENTATION_FALLBACK=1; that is a separate oven-sh/WebKit change. This PR only makes the failure immediate and explicit.Earlier revisions of this PR
Previous iterations also added a Rosetta 2 recovery path, a
bun_warn_avx_missingsuppression under Rosetta, and a hint pointing users of the default (-march=haswell) x64 build at the baseline download. #34782 made every x64 build nehalem and removedbun_warn_avx_missingandEnvironment::BASELINE, which made all of that unreachable: under Rosetta 2 the translated CPUID still advertises SSE4.2 and PCLMULQDQ, so the westmere kernel matches and the stub is never installed. Those pieces were dropped when rebasing onto that change; the remaining diff is the startup check alone. Earlier.zigreference-file edits were likewise dropped when #32621 removed those files; the Rustfirst_non_asciihas no equivalent of the Zig slice underflow.Fixes #30613
Fixes #14745