[JSC] Windows: emit a .pdata/.xdata record over the LLInt and vmEntry* code - #442
[JSC] Windows: emit a .pdata/.xdata record over the LLInt and vmEntry* code#442robobun wants to merge 2 commits into
Conversation
The LLInt opcode handlers and the vmEntry* trampolines are linked into the image's .text with no RUNTIME_FUNCTION, because offlineasm emits no .seh_* directives. The Windows unwinder consults only the image's static .pdata for an in-image PC, so the dynamic function table registered for the JIT pool cannot help, and every unwind-table based stack walk (RtlVirtualUnwind, RtlCaptureStackBackTrace, SEH dispatch, ETW, WinDbg) stops or derails at the first interpreter frame. Every function in jsc_llint_begin..jsc_llint_end keeps the frame pointer on a CallFrame, the frame shape the JIT pool's unwind info already describes, so LowLevelInterpreter.cpp now assembles one RUNTIME_FUNCTION over that range with the same unwind codes registerJITUnwindInfo hand-encodes, and with the same language-specific handler, which gets a C name so the record can address it directly. The record is written out as data rather than with .seh_* directives because LLVM cannot compute an ARM64 SEH function length across the alignment directives in the offlineasm output (llvm/llvm-project#47432). On ARM64 the 18-bit FunctionLength field is guarded by an ADR whose reach matches it, so the build fails instead of emitting a bad record if the interpreter ever outgrows one entry.
|
Warning Review limit reached
Next review available in: 29 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Given that it hand-encodes Windows .pdata/.xdata unwind records and wires a language-specific SEH handler over the entire LLInt/vmEntry range on two architectures, and the real-Windows verification in the companion Bun PR is still pending, a human look would still be worthwhile.
Checked that the x64 UNWIND_INFO bytes and the ARM64 header/codes match what registerJITUnwindInfo already builds for the JIT pool (JITUnwindRecord::unwindInfo, arm64XdataHeader/arm64JITUnwindCodes).
Checked that dropping static on jscJITSEHHandler in favour of extern "C" keeps the same body (still returns ExceptionContinueSearch when no callback is set) and that the ENABLE(JIT) guard on the new asm matches the guard around the handler's definition.
Checked the ARM64 adr xzr overflow guard and FunctionLength shift/flag-bit layout against the 18-bit limit described in the comments.
Extended reasoning...
Overview
This PR emits a static Windows .pdata/.xdata RUNTIME_FUNCTION record covering the offlineasm-generated code range jsc_llint_begin..jsc_llint_end (LLInt opcode handlers, vmEntryToJavaScript/vmEntryToNative, wasm in-place interpreter) so that RtlLookupFunctionEntry, RtlVirtualUnwind, RtlCaptureStackBackTrace, SEH dispatch, and out-of-process profilers/debuggers can unwind through interpreter frames. It does this via hand-written module-level inline assembly in LowLevelInterpreter.cpp for both x86_64 and ARM64 (data .byte/.long/.rva rather than .seh_* directives, to work around llvm/llvm-project#47432 on ARM64). jscJITSEHHandler in ExecutableAllocator.cpp loses static and gains extern "C" so the record can reference it by name, and comments in ExecutableAllocator.{h,cpp} are updated to reflect that the embedder's setJITExceptionHandlerWin callback now also fires for LLInt/vmEntry frames.
Security risks
The change registers a language-specific SEH handler over a large region of executable image code. If the unwind codes were wrong, the worst realistic outcome is a mis-unwound stack during crash reporting or SEH dispatch — not attacker-controlled control flow, since the record is static, read-only image data and the handler is the existing jscJITSEHHandler which just forwards to an embedder-set callback or returns ExceptionContinueSearch. Making jscJITSEHHandler externally visible (extern "C", no longer file-static) slightly widens the symbol surface but does not change what it does. I don't see an injection, auth, or data-exposure surface here.
Level of scrutiny
High. This is low-level, platform-ABI-specific work: hand-encoded unwind opcodes for two CPU architectures, a build-time overflow guard implemented as a never-executed adr xzr instruction, and a behaviour change to when the embedder's SEH callback is invoked. A single wrong nibble in the .xdata bytes could break unwinding or SEH dispatch on one architecture only, and the companion Bun PR's Windows x64 / Windows 11 ARM64 tests are described as not yet run ("Results will be added here once the preview build is up"). This is well outside the "simple, mechanical, obvious" bar for auto-approval.
Other factors
The encoding is internally consistent with the existing dynamic-table path in ExecutableAllocator.cpp (the x64 bytes 09 04 02 05 04 03 01 50 match JITUnwindRecord::unwindInfo; the ARM64 header expression and code word E1 81 E4 E3 match arm64XdataHeader/arm64JITUnwindCodes), and the ENABLE(JIT) guard on the new asm block lines up with the guard around jscJITSEHHandler's definition, so there is no obvious link-time hole. The PR description is thorough and cites the LLVM bug that forces the hand-written approach. Still, given the domain (Windows SEH internals across two ISAs) and the pending real-hardware verification, a human reviewer with Windows unwinding expertise should sign off.
LTO collects the symbols a module's inline asm references by parsing it with a recording streamer, and .rva operands never reach it. With .rva a -flto build internalized jscJITSEHHandler and lld-link failed with an undefined symbol referenced from .xdata (the windows-amd64-lto build). .long symbol@IMGREL assembles to the same ADDR32NB relocation but goes through an ordinary data expression, which the scan records.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it hand-encodes Windows SEH unwind data (.pdata/.xdata bytes) for both x64 and ARM64, changes jscJITSEHHandler linkage, and broadens when the embedder SEH callback fires, a human look would still be worthwhile — and the description notes preview-build test results are still pending.
What was reviewed:
- Cross-checked the x64
UNWIND_INFObytes and ARM64.xdataheader/codes againstJITUnwindRecord/arm64XdataHeader/arm64JITUnwindCodesinExecutableAllocator.cpp— they match. - Confirmed
jscJITSEHHandleris only otherwise referenced via&jscJITSEHHandlerin the same TU, sostatic→extern "C"doesn't break existing callers. - Checked the
adr xzroverflow guard sits afterjsc_llint_end(preceded by a trap spacer perOFFLINE_ASM_END), so it's unreachable as claimed.
Extended reasoning...
Overview
This PR adds a static .pdata/.xdata RUNTIME_FUNCTION covering the offlineasm-generated LLInt/vmEntry* code range (jsc_llint_begin..jsc_llint_end) on Windows x64 and ARM64, so RtlLookupFunctionEntry/RtlVirtualUnwind can unwind through interpreter frames. It changes jscJITSEHHandler from file-static to extern "C" so the hand-written .xdata can reference it by RVA, updates the header comment on setJITExceptionHandlerWin, and moves an existing follow-up comment. The unwind bytes mirror the existing dynamic-table encoding for the JIT pool from #315.
Security risks
Low but non-trivial. Registering a language-specific SEH handler over a large code range means malformed unwind data could redirect exception dispatch. The bytes are constant and match the already-shipping JIT-pool encoding, and the handler defaults to ExceptionContinueSearch, so no new attack surface beyond what #315 already introduced. No user-controlled input reaches the encoding.
Level of scrutiny
High. This is hand-encoded platform ABI binary data embedded via inline asm, with architecture-specific bit-packing, LTO-specific relocation spelling (@IMGREL vs .rva), and a clever build-time overflow guard. Errors here would manifest as broken stack walks, SEH misdispatch, or link failures on specific toolchain configurations — hard to catch without the Windows CI matrix. The author's verification is unusually thorough, but they explicitly note that results against the preview build are still pending.
Other factors
- The change is additive and Windows-only (guarded by
OS(WINDOWS) && ENABLE(JIT) && (CPU(X86_64) || CPU(ARM64))); other platforms are unaffected. - There is a semantic behaviour change for embedders: the
setJITExceptionHandlerWincallback now also fires for LLInt/vmEntry*frames. The header comment documents it, but a human should confirm this is desirable for Bun's crash-reporting integration. - The companion Bun PR with the actual runtime tests is referenced but its results against this build aren't in yet.
- No prior human or bot review on this PR beyond a rate-limited CodeRabbit notice.
Preview Builds
|
The offlineasm code linked into bun.exe (LLInt, vmEntryToJavaScript and the other vmEntry* trampolines) had no .pdata entry, so every unwind table based stack walk stopped or derailed at the first interpreter frame: fault reports lost everything below the JS frames, non-fault reports ended at the JIT thunk, and a fault propagating out of an interpreted call with no JIT frame below it had no SEH catch point. oven-sh/WebKit#442 makes LowLevelInterpreter.cpp emit one .pdata/.xdata record over jsc_llint_begin..jsc_llint_end carrying the JIT pool's unwind codes and handler. This bumps WEBKIT_VERSION to it, updates the comments that described LLInt as uncovered, and adds Windows-only test hooks (jscInternals.llintCodeRange, jscInternals.unwindCurrentStack) plus tests that look both ends of the range up with RtlLookupFunctionEntry, walk through interpreted frames with RtlVirtualUnwind, and check that a jitless fault under an interpreted frame is reported through the record's handler.
1 participant
Problem
jsc_llint_begin..jsc_llint_end(every LLInt opcode handler,vmEntryToJavaScript,vmEntryToNativeand the othervmEntry*trampolines, the wasm interpreter), has noRUNTIME_FUNCTION: offlineasm emits no.seh_*directives, so nothing lands in.pdatafor it.RtlLookupFunctionEntryconsults only the image's static.pdatafor a PC inside a loaded image, so the dynamic function tableregisterJITUnwindInforegisters for the JIT pool (JSC(windows): register unwind info for the fixed JIT pool #315) cannot cover it;ExecutableAllocator.cpphas been carrying this as a noted follow-up since then.RtlCaptureStackBackTrace(what WTF'sStackTraceand Bun's non-fault crash reports use) returns nothing below the JIT thunk that called into native code;RtlVirtualUnwindbased walkers (Bun's fault reports, ETW/WPA, WinDbg, minidump tooling) treat the PC as a leaf and pop aCallFrameslot as the return address, losing the JS frames,vmEntryToJavaScriptand everything that entered JS (crash_handler(windows): unwind through LLInt and vmEntryToJavaScript frames and stop the walk at non-code PCs bun#38789 has the traces); and an exception that propagates up through an LLInt frame without a JIT frame below it (a C++ slow path called from LLInt, or jitless mode) has no language-specific handler, so SEH dispatch has to get through the same broken unwind to reach anything.Fix
LowLevelInterpreter.cppassembles, right after theLLIntAssembly.hinclude and underOS(WINDOWS) && ENABLE(JIT), one.pdataentry coveringjsc_llint_begin..jsc_llint_endplus the.xdatait points at:UNWIND_INFOversion 1,UNW_FLAG_EHANDLER, prolog 4, frame register rbp, codesUWOP_SET_FPREG@4 andUWOP_PUSH_NONVOL rbp@1. These are the bytesJITUnwindRecord::unwindInfoholds for the pool (09 04 02 05 04 03 01 50), and also exactly what LLVM emits for apush rbp; mov rbp, rspfunction with an@excepthandler.X = 1, one code wordset_fp, save_fplr_x 16, end, nop, i.e.arm64XdataHeader/arm64JITUnwindCodeswith the actual length; again byte-identical to LLVM's own output for that prologue.jscJITSEHHandler, which losesstaticand gains a C name so the record can name it; it is in the same image, so no thunk is needed (the pool records need one only because a dynamic table's RVAs are relative to the table base).functionPrologue(push fp/lr, fp = sp) and keeps the frame pointer on aCallFrame, whose first two slots are the caller's frame pointer and the return address. That is precisely what these unwind codes restore, and it is the same argument JSC(windows): register unwind info for the fixed JIT pool #315 already makes for the pool, whose code has the same prologue; the two records compose, so a walk goes JIT -> LLInt ->vmEntryToJavaScript-> the C++ caller (whose own.pdatatakes over; rbp/fp is restored by the codes, which is the only register C++ unwind info can depend on). The range starts with thejsc_llint_beginandllintPCRangeStarttrap instructions, so the 4-byte prologue the record claims never contains a real PC. Precision is the same as the pool records (and V8's / SpiderMonkey's equivalents): a PC sitting exactly on a prologue instruction inside the range unwinds one frame short, which is what the pool already accepts..seh_*directives: LLVM cannot compute an ARM64 SEH function length across alignment directives ([AArch64] UnwindInfo cannot be emitted if inline asm with align or fill are used llvm/llvm-project#47432, the bug the existing-fno-unwind-tablesforLowLevelInterpreterLibworks around); wrapping the offlineasm output in.seh_proc/.seh_endprocdies withLLVM ERROR: Failed to evaluate function length in SEH unwind infoin LLVM 21. The.longin the hand-written record is an ordinary data fixup, resolved after layout..long symbol@IMGRELrather than.rva symbol. Both assemble to the sameADDR32NBrelocation, but LTO finds the symbols a module's asm references by parsing the asm with a recording streamer, and.rvaoperands never reach it: the first revision used.rva, and thewindows-amd64-ltopreview job was the one job that failed. Reproduced outside the tree by compiling the block with/clang:-flto=thinand linking with lld-link:undefined symbol: jscJITSEHHandler, referenced by ...(.xdata)(the handler gets internalized). With@IMGRELthe bitcode's symbol table listsU jscJITSEHHandlerand the same LTO link succeeds on both x64 and ARM64, with the records intact in the linked image; non-LTO objects are byte-identical to the.rvaversion.FunctionLengthis 18 bits of instructions (1 MB); the blob is ~400 KB today (jsc_llint_end - jsc_llint_beginis 401,200 bytes in a Windows x64 debug build of Bun and 404,884 on Windows ARM64 debug, measured by the tests below). Instead of letting the field overflow silently into the flag bits one day, a never-executedadr xzr, jsc_llint_begin - 4is assembled right afterjsc_llint_end: ADR reaches exactly 2^18 instructions, so the build fails withfixup value out of rangeon that line once the range stops fitting in one record (checked at both sides of the boundary with llvm-mc: 0xFFFFC bytes assembles and encodesFunctionLength = 0x3FFFF, 0x100000 fails).setJITExceptionHandlerWinis now also invoked when dispatch reaches an LLInt /vmEntry*frame. It is the same callback with the same arguments, and onlyUNW_FLAG_EHANDLERis set (no unwind-phase calls), as for the pool. No API change; the header comment says so.clang-cl --target={x86_64,aarch64}-pc-windows-msvc /clang:-masm=att, the flags the real build uses, around a stand-in blob full of.balign/.aligndirectives) produces aRUNTIME_FUNCTIONfromjsc_llint_begintojsc_llint_endwithADDR32NBrelocations,llvm-readobj --unwinddecodes it as described above, and the.xdatabytes match what LLVM emits from.seh_*directives for the same prologue on both architectures..seh_procaround the same stand-in fails with the [JSC] Use code blocks syntax for Test262 documents WebKit/WebKit#47432 error on ARM64. The same block compiled with/clang:-flto=thin /clang:-fno-split-lto-unitand linked with lld-link links on both architectures with the handler resolved.dbb960f3(.rva) failed onlywindows-amd64-lto;644fda92(@IMGREL, the current head) built every lane and publishedautobuild-preview-pr-442-644fda92.f0f60fd2, then on this preview; results identical on both architectures:RtlLookupFunctionEntryonjsc_llint_beginand onjsc_llint_end - 4: null and null before; after, the same entry for both,BeginAddress == jsc_llint_begin, and nothing atjsc_llint_enditself (on x64 the entry'sEndAddressis exactlyjsc_llint_end; on ARM64 the word after the range is theadr, which has no entry, so the 18-bit length is exact). Range size 401,200 bytes (x64) / 404,884 (ARM64).RtlCaptureContext+RtlLookupFunctionEntry+RtlVirtualUnwindwalk started in a host function called through five interpreted functions: before, 3 frames (host function, host-call thunk in the pool, first LLInt return address, where the lookup fails); after, 50 frames: 7 frames at the interpreter's call return point,vmEntryToJavaScript's return point, 40 frames of JSC/Bun C++ and Rust below it, then kernel32 and ntdll.BUN_JSC_useJIT=0(no pool, so no pool record anywhere in the chain) with a fault inside ntdll called from interpreted JS: before, no output and exit code0xC0000005(dispatch derails on the LLInt frame and the unhandled-exception filter is never reached); after, the crash report from the callback installed throughsetJITExceptionHandlerWin.Background
.pdata/.xdata: on 64-bit Windows every non-leaf function needs aRUNTIME_FUNCTIONin the image's.pdatasection (x64: begin RVA, end RVA, unwind info RVA; ARM64: begin RVA plus unwind info RVA, the length lives in the unwind info) pointing at an.xdatarecord that lists, in reverse prologue order, how to undo the prologue.RtlLookupFunctionEntryfinds the record for a PC;RtlVirtualUnwindapplies it;RtlCaptureStackBackTrace, SEH dispatch and every profiler/debugger are built on those two. A function with no record is assumed to be a leaf whose return address is at[rsp](inlron ARM64). Records can also carry an exception-handler RVA (UNW_FLAG_EHANDLER/ theXbit); SEH dispatch calls that handler when the exception reaches a frame of the function.RtlAddGrowableFunctionTable, used byregisterJITUnwindInfo) cover memory outside any image; for a PC inside an image the lookup goes straight to that image's static table, which is why this has to be emitted at build time.LLIntAssembly.his one__asm__statement whose first and last labels arejsc_llint_begin/jsc_llint_end(OFFLINE_ASM_BEGIN/OFFLINE_ASM_ENDin this file, originally for lldb). It is generated GNU-syntax assembly assembled by clang as module-level inline asm, which is also where the.cfi_*block above the include comes from; this PR's block is the SEH counterpart of that block.functionPrologue(LowLevelInterpreter.asm;preserveCallerPCAndCFRis the same sequence) ispush cfr; move sp, cfron x64 andpush cfr, lr; move sp, cfr(stp fp, lr, [sp, #-16]!; mov fp, sp) on ARM64.cfris rbp / x29 and points at the currentCallFrame, whose layout starts withcallerFrameandreturnPC. Every function in the range, includingdoVMEntryand the wasm interpreter (InPlaceInterpreter.asmis part of the same output), enters through one of these two macros, andAssemblyHelpers::emitFunctionPrologueemits the same sequence in every JIT tier, which is what makes one set of unwind codes describe both the pool and this range.