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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
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 | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
📒 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
📒 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
📒 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. |
1 participant
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 ~10 VEX codegen sites inMacroAssemblerX86_64.hemit AVX once code tiers 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 missing baseline macOS WebKit (#290, oven-sh/bun#32511). AVX2 on Darwin is already detected correctly via
sysctlbyname("hw.optional.avx2_0")— only AVX is assumed, so Sandy/Ivy Bridge Macs are unaffected. Together the two changes are what make a pre-Haswell Mac work: #290 fixes startup, this fixes the JIT.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. 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.