macOS: detect AVX at runtime instead of assuming every Mac has it - #292
macOS: detect AVX at runtime instead of assuming every Mac has it#292tobocop2 wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The previous release fixed the startup SIGILL (Nehalem WebKit) but still crashed once code tiered up, because JSC hardcodes AVX as present on macOS instead of checking the CPU. Rebuilt against oven-sh/WebKit#292. Verified on a real Mac Pro 5,1 (Xeon X5690, Westmere) and on an M1 under Rosetta 2, which also reports no AVX.
WalkthroughChangesThe x86_64 assembler now uses runtime AVX usability checks on Darwin and selects SSE or AVX probe trampolines consistently across platforms. Darwin AVX2 enablement is gated by validated AVX support. Cross-platform AVX probing
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp (1)
407-419: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRemove compile-time AVX hardcoding for JIT probes on Darwin. The explicit goal of this PR is to avoid
SIGILLon non-AVX Darwin systems (like Rosetta 2) by dynamically checking for AVX support. However, the JIT probe infrastructure still hardcodes AVX usage for Darwin at compile time. If a probe is inserted during execution on a non-AVX CPU, it will unconditionally executevmovapsand crash.Remove the
OS(DARWIN)branches entirely so that Darwin utilizes the exact same dual-trampoline and fallback logic as Windows and Linux:
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp#L407-L419: Remove the#if OS(DARWIN)branch and unconditionally use the#elseblock to generate bothctiMasmProbeTrampoline(SSE) andctiMasmProbeTrampolineAVX(AVX).Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp#L43-L46: Remove the#if !OS(DARWIN)guard soctiMasmProbeTrampolineAVXis declared on all platforms.Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp#L196-L232: Remove the#if !OS(DARWIN)guard so the SSEmovapssave/restore macros are available for the fallback trampoline.Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp#L467-L474: Remove the#if OS(DARWIN)branch so thatMacroAssembler::probe()dynamically selects the trampoline viasupportsAVX()instead of assuming AVX.🛠️ Proposed fix to remove AVX hardcoding for probes
--- a/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp +++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp @@ -40,9 +40,7 @@ JSC_DECLARE_NOEXCEPT_JIT_OPERATION(ctiMasmProbeTrampoline, void, ()); JSC_ANNOTATE_JIT_OPERATION_PROBE(ctiMasmProbeTrampoline); -#if !OS(DARWIN) JSC_DECLARE_NOEXCEPT_JIT_OPERATION(ctiMasmProbeTrampolineAVX, void, ()); JSC_ANNOTATE_JIT_OPERATION_PROBE(ctiMasmProbeTrampolineAVX); -#endif // The following are offsets for Probe::State fields accessed by the ctiMasmProbeTrampoline stub. @@ -193,9 +191,7 @@ "vmovaps " STRINGIZE_VALUE_OF(PROBE_CPU_XMM14_VECTOR_OFFSET) "(%rbp), %xmm14" "\n" \ "vmovaps " STRINGIZE_VALUE_OF(PROBE_CPU_XMM15_VECTOR_OFFSET) "(%rbp), %xmm15" "\n" -#if !OS(DARWIN) `#define` PROBE_XMM_SAVE_MOVAPS \ "movaps %xmm0, " STRINGIZE_VALUE_OF(PROBE_CPU_XMM0_VECTOR_OFFSET) "(%rsp)" "\n" \ "movaps %xmm1, " STRINGIZE_VALUE_OF(PROBE_CPU_XMM1_VECTOR_OFFSET) "(%rsp)" "\n" \ @@ -228,7 +224,6 @@ "movaps " STRINGIZE_VALUE_OF(PROBE_CPU_XMM13_VECTOR_OFFSET) "(%rbp), %xmm13" "\n" \ "movaps " STRINGIZE_VALUE_OF(PROBE_CPU_XMM14_VECTOR_OFFSET) "(%rbp), %xmm14" "\n" \ "movaps " STRINGIZE_VALUE_OF(PROBE_CPU_XMM15_VECTOR_OFFSET) "(%rbp), %xmm15" "\n" -#endif // !OS(DARWIN) `#if` COMPILER(MSVC) `#define` ASM_PREVIOUS_SECTION @@ -404,17 +399,11 @@ ASM_PREVIOUS_SECTION \ ); -#if OS(DARWIN) -// On macOS, all x86_64 CPUs support AVX. Use vmovaps unconditionally. -DEFINE_PROBE_TRAMPOLINE(ctiMasmProbeTrampoline, PROBE_XMM_SAVE_VMOVAPS, PROBE_XMM_RESTORE_VMOVAPS, - ctiMasmProbeTrampolineCopyLoop, ctiMasmProbeTrampolineProbeStateIsSafe, ctiMasmProbeTrampolineRestoreRegisters) -#else -// On Linux/Windows, AVX may not be available. Provide two trampolines: // - ctiMasmProbeTrampoline: uses movaps (SSE, always available on x86_64) // - ctiMasmProbeTrampolineAVX: uses vmovaps (requires AVX) DEFINE_PROBE_TRAMPOLINE(ctiMasmProbeTrampoline, PROBE_XMM_SAVE_MOVAPS, PROBE_XMM_RESTORE_MOVAPS, ctiMasmProbeTrampolineCopyLoop, ctiMasmProbeTrampolineProbeStateIsSafe, ctiMasmProbeTrampolineRestoreRegisters) DEFINE_PROBE_TRAMPOLINE(ctiMasmProbeTrampolineAVX, PROBE_XMM_SAVE_VMOVAPS, PROBE_XMM_RESTORE_VMOVAPS, ctiMasmProbeTrampolineAVXCopyLoop, ctiMasmProbeTrampolineAVXProbeStateIsSafe, ctiMasmProbeTrampolineAVXRestoreRegisters) -#endif // What code is emitted for the probe? @@ -464,15 +453,11 @@ push(RegisterID::eax); -#if OS(DARWIN) - move(TrustedImmPtr(reinterpret_cast<void*>(ctiMasmProbeTrampoline)), RegisterID::eax); -#else if (supportsAVX()) move(TrustedImmPtr(reinterpret_cast<void*>(ctiMasmProbeTrampolineAVX)), RegisterID::eax); else move(TrustedImmPtr(reinterpret_cast<void*>(ctiMasmProbeTrampoline)), RegisterID::eax); -#endif push(RegisterID::edx);🤖 Prompt for 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. In `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp` around lines 407 - 419, Remove Darwin-specific AVX hardcoding in MacroAssemblerX86_64.cpp: at lines 407-419, generate both ctiMasmProbeTrampoline and ctiMasmProbeTrampolineAVX unconditionally; at lines 43-46, declare the AVX trampoline on all platforms; at lines 196-232, expose the SSE movaps save/restore macros universally; and at lines 467-474, make MacroAssembler::probe() use supportsAVX() to select the SSE or AVX trampoline dynamically.
🤖 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.
Outside diff comments:
In `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp`:
- Around line 407-419: Remove Darwin-specific AVX hardcoding in
MacroAssemblerX86_64.cpp: at lines 407-419, generate both ctiMasmProbeTrampoline
and ctiMasmProbeTrampolineAVX unconditionally; at lines 43-46, declare the AVX
trampoline on all platforms; at lines 196-232, expose the SSE movaps
save/restore macros universally; and at lines 467-474, make
MacroAssembler::probe() use supportsAVX() to select the SSE or AVX trampoline
dynamically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0752f870-b085-4b08-8fe9-6586493e7157
📥 Commits
Reviewing files that changed from the base of the PR and between 4895f45 and 8c7838b4090ca4746a371f2a44d279a26954d61e.
📒 Files selected for processing (1)
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp
8c7838b to
49fdeac
Compare
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 `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp`:
- Around line 525-538: Update ctiMasmProbeTrampoline and its probe-selection
logic to remove the OS(DARWIN) special cases: do not emit or invoke the
vmovaps-based trampoline unconditionally on Darwin, and use the existing
standard fallback versus AVX dual-trampoline selection based on runtime AVX
support on every platform. Ensure probes on non-AVX Darwin systems use the
fallback trampoline.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 936fcb3a-dae8-47cc-a854-9d792e3d0d88
📥 Commits
Reviewing files that changed from the base of the PR and between 8c7838b4090ca4746a371f2a44d279a26954d61e and 49fdeacf20b24eb4a20842044c596dc7b2b1b764.
📒 Files selected for processing (1)
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp
49fdeac to
d73fe09
Compare
JSC assumed AVX on macOS in two places, not one. Fixing only the feature detection left the probe trampoline emitting vmovaps unconditionally, which WebAssembly reaches via BBQ loop OSR entry and tier-up — so WASM still SIGILLd. Caught in review on oven-sh/WebKit#292.
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 `@Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp`:
- Around line 511-524: Update Darwin’s s_avx2CheckState detection to require
s_avxCheckState is Set before accepting the hw.optional.avx2_0 sysctl result.
Match the existing non-Darwin gating logic while preserving the current fallback
when AVX2 or OS-enabled YMM state is unavailable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c730b687-9bbc-4455-b091-569032d90dd2
📥 Commits
Reviewing files that changed from the base of the PR and between 49fdeacf20b24eb4a20842044c596dc7b2b1b764 and d73fe0920dc390c7d316e75de10e587fc339f2c6.
📒 Files selected for processing (1)
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.cpp
JSC assumed every x86_64 Mac has AVX in two places, so it emitted VEX-encoded
instructions on CPUs that cannot decode them and died with SIGILL:
- collectCPUFeatures() hardcoded s_avxCheckState = Set on Darwin, so
supportsAVX() reported true and the ~10 VEX codegen sites in
MacroAssemblerX86_64.h emitted AVX once code tiered up.
- The probe trampoline used vmovaps unconditionally on Darwin, and probe()
selected it without consulting supportsAVX(). This is reachable from
WebAssembly (WasmBBQJIT loop OSR entry and tier-up, WasmOMGIRGenerator's
catch prologue), not only from the debug-gated OSR-exit paths.
oven-sh#198 fixed the first bug for Linux/Windows and guarded the new code with
#if !OS(DARWIN), leaving Darwin on the old assumption. Use the same runtime
checks and the same dual-trampoline selection on Darwin, and un-gate readXCR0()
so it is available there.
Also gate Darwin's AVX2 detection on the AVX result, matching the non-Darwin
path. AVX2 is VEX-encoded and needs OS-enabled YMM state, so now that AVX is a
runtime check the two must not be able to disagree.
The assumption holds for every Mac Apple shipped with a supported macOS (Sandy
Bridge, 2011, and later). It does not hold on an older Mac running current macOS
via OpenCore, nor under Rosetta 2, which reports no AVX.
Non-Darwin is unaffected: the preprocessed output of this file for
!OS(DARWIN) is byte-identical before and after.
Reported by Wolfgang Fahl on a Xeon X5690 (Westmere, no AVX).
d73fe09 to
384b31c
Compare
|
On the Description check — I think this one is a false positive, and I'd rather say so than reformat into something this fork doesn't use.
The convention this repo actually uses is a There's also no Bugzilla bug to link: this is a Darwin-only bug in oven-sh's fork, introduced by the Happy to add a |
|
@coderabbitai full review The last review covered |
|
I'll kick off a full review of the PR, paying particular attention to ✏️ Learnings added
✅ Action performedFull review finished. |
|
@Jarred-Sumner sorry to ping. I've got a bunch of downloads so far of my and the only thing blocking |
|
Bun's crash telemetry confirms this in production. Every "Illegal instruction" report from a macOS x86_64 process whose CPUID lacks AVX that has a symbolicated stack ends in Yarr JIT code: The emitting code is the Yarr vector scan: I arrived at the same diff independently (CPUID + OSXSAVE + XGETBV on Darwin, both probe trampolines on every OS, AVX2 gated on AVX) before finding this PR, so I have not opened a second one. The |
|
Two more groups from the same population (macOS x86_64, CPUID without AVX), not regexp related: BUN-4P1J and BUN-4D7V. Both die in one JIT frame directly under an LLInt call op, one of them while |
|
Confirmed: 259 sites in |
|
Independent hardware confirmation for this change, from the other end of the telemetry. I hit the same bug and, before finding this PR, arrived at the same conclusion and the same sysctl fix. Consider this a second implementation converging on yours rather than a competing one — #292 also covers AVX2 and the probe trampoline, so mine folds into it. Hardware: Mac Pro 5,1, Xeon X5690 — Westmere, SSE4.2, no AVX, no AVX2, no BMI. Build: bun 1.4.0 from source, Results, JIT enabled throughout, no And specifically the path in robobun's telemetry — Earlier measurements on the same machine put the fault in JIT-emitted memory at Happy to run further cases on this hardware if that helps the review. |
3 participants
Problem
JSC assumes every x86_64 Mac has AVX in two places, so it emits VEX-encoded instructions on CPUs that cannot decode them.
1. Feature detection.
collectCPUFeatures()never asks the CPU on Darwin:supportsAVX()then returnstrueand the 259 VEX codegen sites inMacroAssemblerX86_64.h, across 157 distinct functions, emit AVX. This includes theMath.*thunks inThunkGenerators.cpp, whichop_callreaches straight from the LLInt, so a crash does not need a tier-up.2. The probe trampoline. Darwin defines only a
vmovapstrampoline andprobe()selects it without consultingsupportsAVX():This one is reachable from WebAssembly, not just from debug paths —
WasmBBQJIT.cpp:3470(loop OSR entry),WasmBBQJIT.cpp:3623(tier-up), andWasmOMGIRGenerator.cpp:1529(catch prologue) all callprobe()with no option guard.#198 fixed the first bug for Linux/Windows and guarded the new code with
#if !OS(DARWIN), leaving Darwin on the old assumption.The assumption holds for every Mac Apple shipped with a supported macOS — Sandy Bridge (2011) and later all have AVX. It does not hold on a pre-AVX Mac running current macOS via OpenCore, nor under Rosetta 2.
This is independent of the build flags. AVX2 on Darwin is already detected correctly via
sysctlbyname("hw.optional.avx2_0"), and only AVX is assumed, so Sandy/Ivy Bridge Macs are unaffected (oven-sh/bun#32511).The build-flag half has since landed: 541f498 lowered every x64 lane to
-march=nehalem, sobun-webkit-macos-amd64.tar.gzis a true baseline now and startup works. This PR is the remaining half. A Mac with no AVX at all still crashes the moment JSC tiers up, because codegen picks VEX encodings at runtime no matter what-marchwas.3. AVX2 was ungated. Darwin took AVX2 straight from
sysctlbyname("hw.optional.avx2_0")without gating it on AVX, unlike the non-Darwin path. That was harmless while AVX was hardcodedSet; making AVX a runtime check is what makesavx=Clear, avx2=Setreachable, and AVX2 is VEX-encoded too.Fix
Delete the Darwin special-cases and use the code that already exists for every other platform: the CPUID + OSXSAVE + XCR0 check, the dual-trampoline selection, and the same AVX2 gating. Un-gate
readXCR0()so Darwin can call it. 16 insertions, 22 deletions — it removes more than it adds.readXCR0()executesxgetbv, only reachable whenOSXSAVEis set — exactly whenxgetbvis architecturally legal. That gate is unchanged from the Linux/Windows path.Verification
1. Non-Darwin is provably untouched
For
!OS(DARWIN)the preprocessor selected this same code before the change and selects it after. Preprocessed output of the whole file, before vs after:Byte-identical. This cannot regress Linux or Windows. After the patch, the preprocessed Darwin output differs from Linux only in the
sysctlbynameblock for BMI1/AVX2, which is legitimately Darwin-specific.2. On an AVX CPU the new check agrees with the old hardcode
The patch's exact logic, run on AVX hardware (Intel Xeon @ 2.20GHz):
So on every Mac that actually has AVX,
supportsAVX()is unchanged, the AVX trampoline is still selected, and codegen is identical. Only machines that crash today behave differently. The added cost is oneCPUID+ onexgetbv, once per process, inside the existingstd::call_once.3. It fixes the crash
Built
bun-webkit-macos-amd64-baselinefrom this branch (MACOS_ARCH=x86_64 MARCH_FLAG=-march=nehalem), then bundarwin-x64 --baseline=trueagainst it. That separate lane no longer exists: after 541f498 those are the settings the defaultmacos-amd64build uses, so the same archive comes out of an unmodified checkout. The archive now exports both trampolines (ctiMasmProbeTrampolineAVXis present on Darwin for the first time);libJavaScriptCore.adisassembles to 0%ymm(still a true baseline) with thexgetbvcheck present.Tested under Rosetta 2 on an Apple M1 (macOS 14.6.1), which reports no AVX — the same condition as a Westmere Mac:
bun --versionopencode --versionMeasured CPU state on that machine, confirming it is a genuine no-AVX target:
CPUID.1:ECXgivesAVX[28]=0 OSXSAVE[27]=0, andhw.optional.avx1_0/avx2_0/bmi1/bmi2all report 0.The middle column is why the trampoline fix is required rather than optional: fixing only
collectCPUFeatures()leaves WebAssembly crashing, becauseprobe()still routes to thevmovapstrampoline.A 14-case suite covering the YarrJIT SIMD regex paths this gates (alternation, char-class, backtracking, unicode, lookbehind), plus strings/UTF-8, JSON, DFG/FTL tier-up and typed arrays: all pass on the patched build; the unpatched build
SIGILLs partway through the same suite on the same machine.The binaries behind that table, for anyone who wants to reproduce it or has a pre-AVX Mac to test on — a bun built from this branch, plus an opencode compiled against it: https://github.com/tobocop2/opencode-bun-pre-avx2-mac/releases/latest
4. Blast radius
Ordering:
s_avxCheckStateis computed fromCPUID(0x1)in the block above thesysctlblock that now reads it, so the AVX2 gate sees a settled value.readXCR0,s_avxCheckState,s_avx2CheckState,ctiMasmProbeTrampolineAVXand thePROBE_XMM_*macros have no consumers outside this file.supportsAVX()has three: the codegen sites inMacroAssemblerX86_64.h,YarrJIT.cpp:6126/:6404, andCPU.cpp:149. The YarrJIT sites already degrade gracefully (if (!supportsAVX()) return std::nullopt;), so a correctfalseselects the existing non-SIMD path.ctiMasmProbeTrampolinechanges meaning on Darwin — it becomes themovapsvariant, with the AVX variant selected at runtime, matching every other platform. No code outside this file references either symbol (the other hits are ARMv7/ARM64's own same-named trampolines andstatic_asserttext).Credit
The trampoline half was caught by @coderabbitai in review on the first revision of this PR, which fixed only
collectCPUFeatures(). I had assumedprobe()was debug-only; it is not — the WebAssembly paths above reach it, and the WASM testSIGILLed until the trampolines were fixed too. The AVX2 gate was also caught in that review.Found and diagnosed by @WolfgangFahl on a Mac Pro 5,1 (Xeon X5690, Westmere, no AVX, macOS 14.7.8), in anomalyco/opencode#8345. A bun built entirely from source with
--baseline=true --webkit=localstill died under JIT load whileBUN_JSC_useJIT=0ran fine — which isolated it to runtime codegen rather than build flags. @lumik0 independently flaggedcollectCPUFeatures()as the suspect area in oven-sh/bun#26872.