Conversation
…t one byte before it Fault traces are seeded with the exact faulting pc as frame 0, but every frame was encoded (and locally symbolized) at addr - 1, which is only right for return addresses. Frame 0 of every fault report therefore pointed at the instruction before the fault; at the first instruction of a function that is the previous function or padding. StackTrace now records whether frame 0 is an exact pc, set from the fault seed, and StackTrace::symbol_address() applies the return-address step only where it belongs. The trace string, the debug-build llvm-symbolizer path and write_stack_trace all go through it. Trap-class signals whose pc already points past the instruction (int3 on x86_64, SIGABRT) keep the step; brk on aarch64 is reported on the instruction and is exact. Windows trace strings are unchanged since bun.report adjusts the frames after the first itself. Test hooks fault (ud2/udf) or trap (int3/brk) on the first instruction of a naked function, and a control reports the same entry as a return address, so the encoded offsets must differ by exactly one.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Status: reproduced on the debug build by symbolizing the fault pc of the new |
|
Updated 1:52 PM PT - Aug 11th, 2026
❌ @robobun, your commit dcc07fe has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37533That installs a local version of the PR into your bun-37533 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding on the trap test, I checked the sibling fault-variant test ("fault pc is not stepped back like a return address") for the same mismatch — it is fine, since both faultAtFunctionEntry and functionEntryAsReturnAddress reference fault_at_function_entry. The core symbol_address / signal_pc_is_exact logic and the Windows exact_pc: true sites also look correct.
Extended reasoning...
The inline finding covers the substantive issue: the trap test compares trap_at_function_entry's encoded entry against a control derived from fault_at_function_entry, so it asserts T == F for two distinct naked functions and will fail on every non-ASAN POSIX lane. I traced the fault-variant test through the same encoding path and confirmed both sides use fault_at_function_entry, so that assertion holds. The StackTrace::symbol_address centralization, the from_address → from_frame refactor across encode_trace_string / spawn_symbolizer / write_stack_trace, and the per-signal signal_pc_is_exact classification (SIGABRT false, SIGTRAP arch-gated, faults true) all read correctly; nothing further to add beyond the inline comment.
| test.skipIf(isASAN || isWindows)("trap pc resolves to the trapping instruction", async () => { | ||
| const [trap, asReturnAddress] = await Promise.all([ | ||
| frameZeroOf("trapAtFunctionEntry"), | ||
| frameZeroOf("functionEntryAsReturnAddress"), | ||
| ]); | ||
| expect(trap).toBe(asReturnAddress + 1); | ||
| }); |
There was a problem hiding this comment.
🔴 This test will fail on every non-ASAN POSIX CI lane: trap encodes trap_at_function_entry's entry (T), but asReturnAddress is hardcoded to fault_at_function_entry's entry (F) in js_function_entry_as_return_address, so trap == asReturnAddress + 1 asserts T == F for two distinct functions. Per the robobun note the author only ran under ASAN, where this test is skipped. Fix: have functionEntryAsReturnAddress take an argument selecting which naked function's entry to report (or add a second control hook for the trap function).
Extended reasoning...
What the assertion actually compares
The test asserts expect(trap).toBe(asReturnAddress + 1), where the two sides come from different naked functions:
trap = frameZeroOf("trapAtFunctionEntry")—js_trap_at_function_entrycallstrap_at_function_entry(), a separate#[unsafe(naked)]function whose entry address is T.asReturnAddress = frameZeroOf("functionEntryAsReturnAddress")—js_function_entry_as_return_address(crash_handler_jsc.rs) builds its one-frame trace fromlet frames = [fault_at_function_entry as *const () as usize], i.e. it hardcodesfault_at_function_entry's entry address F.
The Rust doc comment on the control hook says "Control for the two hooks above: the same entry address reported as an ordinary return-address frame", and the describe-block comment says "all three approaches crash with frame 0 holding the entry address of one and the same function" — but the control only wires fault_at_function_entry. For the trap variant that claim is false: frame 0 is T, the control encodes F, and T ≠ F.
Step-by-step trace
trap side, on x86_64 POSIX: int3 sits at T, so the kernel reports pc = T+1. signal_pc_is_exact(SIGTRAP) returns TRAP_LEAVES_PC_ON_INSTRUCTION = cfg!(target_arch = "aarch64") = false, so exact_pc = false and StackTrace::symbol_address(0) steps back to (T+1) − 1 = T. On aarch64: brk at T is reported with pc = T, signal_pc_is_exact(SIGTRAP) returns true, exact_pc = true, so symbol_address(0) = T. Either way StackLine::from_frame on POSIX encodes trap = T − base.
asReturnAddress side: first_frame_is_exact_pc: false, so symbol_address(0) = F − 1; encoded as asReturnAddress = (F − 1) − base, hence asReturnAddress + 1 = F − base.
Assertion: T − base == F − base ⇒ T == F.
Why T ≠ F
fault_at_function_entry and trap_at_function_entry are two independent #[unsafe(naked)] symbols with different bodies (ud2 vs int3; ud2 on x86_64, udf #0 vs brk #0 on aarch64). The instruction bytes differ, so identical-code-folding cannot merge them, and nothing else places them at the same address. The linker gives each its own entry; T ≠ F on every target this test runs on.
Why nothing catches it before CI
The test is gated test.skipIf(isASAN || isWindows). Per the PR description and the robobun comment, the author ran locally on Linux x64 ASAN debug ("16 pass, trap variant skipped under ASAN as intended"), so this assertion never executed. It runs — and fails — on the non-ASAN Linux/macOS x64 and aarch64 lanes, exactly the ones the PR says are meant to "exercise the int3 / brk distinction end to end".
Impact and fix
Concrete CI failure on multiple required lanes; merging as-is either breaks main or bounces on the merge-gate build. The simplest fix is to make the control hook parameterized — e.g. have functionEntryAsReturnAddress accept a selector ("fault" | "trap") and pick the corresponding naked function's address on the Rust side — and pass "trap" from this test. Alternatively, add a second control hook (trapFunctionEntryAsReturnAddress) that reports trap_at_function_entry as *const () as usize. The describe-block comment and the Rust doc comment on the control hook should also be corrected to reflect that the fault and trap hooks target two distinct functions.
|
Heads-up: #38838 changes the trace string to format 4, which inserts a header right after the 7-character sha (a VLQ field count, then per field a VLQ tag, a VLQ char count and that many chars) before the two feature VLQs. The test decoder added here reads the features directly after the sha, so whichever of the two lands second needs to skip that header first; the header loop in the |
This branch has not been deployed
1 participant
What
For a real fault (SIGSEGV/SIGBUS/SIGILL/SIGFPE, Windows access violation) frame 0 of the crash report is symbolized one instruction too early. Sentry group BUN-3K0V (macOS arm64) is a typical case: bun.report shows frame 0 as
SentinelLinkedList.h:61(setNext, line 240 ofSentinelLinkedList::remove), while thestrthat actually faults on address0x8is the next instruction, 4 bytes later, on line 241 (setPrev). Every fault report has this skew; it points people at the wrong statement, and when the fault is on the first instruction of a function it names the previous function (or nothing at all).Repro against the debug build with this branch's test hook, which faults on the first instruction of a function:
Cause
crash_handler()seeds a fault trace from the signal/exception context, soinstruction_addresses[0]is the exact faulting pc (capture_from_contextstoresout[0] = pc); frames 1.. are return addresses.StackLine::from_addressthen subtracted 1 from every address before computing the image-relative offset (macOSaddr - 1, Linuxsaturating_sub(1)). That step is correct for return addresses, which point one past their call, but for frame 0 of a fault trace it lands on the instruction before the fault. Both consumers applied it to frame 0:encode_trace_string(the trace string bun.report decodes, and bun.report passes POSIX addresses to llvm-symbolizer unchanged) and the debug-buildspawn_symbolizer;write_stack_trace(debug builds on macOS/Windows) had its own unconditionalreturn_address - 1. Panic traces are unaffected because all of their frames are return addresses.Fix
bun_core::StackTracegainsfirst_frame_is_exact_pc, andStackTrace::symbol_address(i)is the one place that decides which address to symbolize: frame 0 as is when it is an exact pc,addr - 1otherwise.from_addressbecomesfrom_frameand uses it, as doeswrite_stack_trace.TraceSeed::Faultcarriesexact_pc, set by the producer that knows the semantics.handle_segfault_posixdecides per signal (signal_pc_is_exact): faults are reported with pc on the instruction, so they are exact. Trap-class signals are not: the kernel reports an x86_64int3(what WTF'sCRASH()/RELEASE_ASSERTexecutes) with pc already past it, and SIGABRT on return from the kill(2) that raised it, so for those the- 1step stays and still lands on the trapping instruction, i.e. the current behavior for JSC release-assert crashes is preserved.brkon aarch64 is the one trap reported with pc on the instruction (which is also why a returning SIGTRAP handler re-traps there), so it is exact. Every codeclassify_exception_windowsaccepts is a fault whoseExceptionAddressis the faulting instruction, so the three Windows call sites passexact_pc: true.adjustBunAddressesin itsbackend/symbolize.ts); adjusting on the Windows side as well would step return addresses back twice.from_framedocuments this contract. (Windows panic traces are symbolized one past the call by that decoder heuristic; that is a bun.report-side item and is not touched here.)Why this is the right layer
The -1 is a property of what kind of address a frame holds, and only the code that seeds the trace knows that, so the flag travels with the trace and every consumer (wire encoding, local symbolizer, local printer) gets it from one helper. Storing
pc + 1at capture time instead would keep the consumers unchanged but would put a wrong address into the data that the Windows wire format, the WTF fallback printer, and the reason address all report verbatim.Tests
test/cli/run/run-crash-handler.test.ts, using three newbun:internal-for-testinghooks.faultAtFunctionEntryandtrapAtFunctionEntrycall naked functions whose first instruction isud2/udf #0andint3/brk #0respectively, so frame 0 of the report is exactly that function's entry;functionEntryAsReturnAddresscrashes with a one-frame trace holding the same entry address as an ordinary return-address frame.faultAtFunctionEntryinvokes the handler directly with the SIGILL context there, the same way the existingsegfaulthook does).::fault_at_function_entryas the first frame (before this change it is??, see above).Without the fix the old encoder subtracts 1 from the fault pc as well, so the fault run encodes the same offset as the control (in the run above:
0xDF08733for both instead of0xDF08734vs0xDF08733), and the first test fails; on the released binary the hooks do not exist, so both tests fail at the trace-string check. With the changebun bd test test/cli/run/run-crash-handler.test.tspasses (16 pass, trap variant skipped under ASAN as intended), andcargo check -p bun_crash_handlerpasses for the x86_64-pc-windows-msvc, aarch64-apple-darwin and x86_64-unknown-freebsd targets, which cover the platform-specific branches touched.