Add bun.sys.ucontext_t with bionic-correct layout on x86_64 Android#30393
Add bun.sys.ucontext_t with bionic-correct layout on x86_64 Android#30393robobun wants to merge 5 commits into
Conversation
|
Updated 2:59 AM PT - May 8th, 2026
❌ @robobun, your commit 72aba59 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30393That installs a local version of the PR into your bun-30393 --bun |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds Android x86_64–specific ChangesAndroid x86_64 ucontext_t Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
std.c.ucontext_t on .linux forwards to the glibc/musl layout with a 128-byte sigmask between uc_mcontext and fpregs_mem. bionic x86_64 puts a bare c_ulong (8 bytes) there instead, so __fpregs_mem starts 120 bytes earlier than the Zig stdlib expects. aarch64 is accidentally correct because bionic pads uc_sigmask back to 128 bytes on that arch. Nothing dereferences the signal handler's ucontext_t* today, but handleSegfaultPosix now takes ?*const bun.sys.ucontext_t so that if the crash handler ever reads register state from the saved context it does so from the correct offsets on x86_64 Android. Also adds zig build check-android[-debug], gated on -Dandroid_ndk_sysroot like check-freebsd, so the Android-only struct gets compile-checked with its comptime offset tripwire.
ead1da0 to
291dca8
Compare
Bun reports process.platform === "android" on bionic, not "linux", so the isLinux gate from harness skipped the test there and the bionic-offset branch was unreachable.
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/internal/ucontext-layout.test.ts`:
- Around line 29-35: The test currently derives the expected branch from the
value under test (layout!.android) which can hide regressions; change the
selection of the expected object to use the independent isAndroid flag (e.g.,
use isAndroid ? {...android:true} : {...android:false}) instead of
layout!.android, and add an explicit assertion that layout.android equals
isAndroid so the test fails if the reported platform differs from the external
isAndroid signal; update references to the expected object creation (the const
expected) and ensure you still assert other fields against layout.mcontext,
layout.sigmask, layout.fpregs_mem, and layout.sizeof as before.
🪄 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: fc5dd953-e7d0-489e-ac68-830d1a3af3b8
📒 Files selected for processing (1)
test/internal/ucontext-layout.test.ts
….android Deriving the expected values from the value under test would mask a regression where Zig-side Environment.isAndroid disagreed with the runtime platform.
Problem
std.c.ucontext_ton.linuxforwards tostd.os.linux.<arch>.ucontext_t, which is the glibc/musl layout:{flags, link, stack, mcontext, sigmask[128B], fpregs_mem[512B]}. bionic x86_64'sucontext_t(<sys/ucontext.h>) puts only a bareunion { sigset_t; sigset64_t; }— onec_ulong, 8 bytes — betweenuc_mcontextand__fpregs_mem, so__fpregs_memstarts 120 bytes earlier than the Zig stdlib expects.aarch64 is accidentally correct: bionic explicitly pads
uc_sigmaskback to 128 bytes there (char __padding[128 - sizeof(sigset_t)]).No current breakage —
handleSegfaultPosixtakes the ucontext arg as?*const anyopaqueand never dereferences it. This is proactive: if the crash handler ever starts reading register state from the saved context (e.g.uc_mcontext.gregs[REG_RIP]/REG_RSPinstead of the live siginfo), x86_64 Android would read the wrong offsets.Fix
Same pattern as #30389 (on which this is now based):
src/sys/sys.zig: addpub const ucontext_t— on x86_64 Android, anextern structmatching bionic<sys/ucontext.h>(__x86_64__), with comptime offset assertions (uc_mcontext @40,uc_sigmask @296,__fpregs_mem @304,sizeof == 816) and a tripwire that fires whenstd.c.ucontext_tshrinks to the bionic size so the workaround can be dropped. Everywhere else it's a transparent alias ofstd.posix.ucontext_t. Thesigmaskfield reusesbun.sys.sigset_t.src/crash_handler/crash_handler.zig:handleSegfaultPosix's third parameter is now?*const bun.sys.ucontext_t(still unused, but correctly typed for future use).@ptrCastat the one assignment site sinceSigaction.sigaction_fntypes the slot as?*anyopaque— same C ABI.test/internal/ucontext-layout.test.ts: pins@sizeOf/@offsetOf(bun.sys.ucontext_t, …)to the host-libc values on Linux x86_64 (glibc/musl on CI, bionic when run on device).src/test_runner/harness/recover.zigalso referencesstd.c.ucontext_t, but that's a separate problem (bionic has nosetcontext/getcontextat all — it would need thesetjmp/longjmppath like musl) and that file is currently unreferenced.Verification
bun run zig:check-allpasses all 16 targets. Intentionally breaking an offset assertion (@sizeOf == 817) correctly fails the x86_64-android compile.No Android runtime test is included — the misbehaviour only surfaces when the kernel fills the structure on a real x86_64 Android device, which CI does not run, and nothing in Bun currently dereferences it.