From bb28c89ddd8f32435c48f373adb2c4869aaab435 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 09:16:31 +0200 Subject: [PATCH 01/13] fix(runtime): real named frames in Error.stack (#9486) Capture the native return addresses on every `new Error` via a frame-pointer chain walk, and resolve them to JS function names on the first `.stack` read against the registry codegen already fills for `fn.name`. `alloc_error` no longer builds the string eagerly. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp --- changelog.d/9486-error-stack-frames.md | 53 ++ crates/perry-codegen/src/codegen/artifacts.rs | 24 + .../perry-runtime/src/builtins/formatting.rs | 22 + crates/perry-runtime/src/builtins/mod.rs | 3 +- crates/perry-runtime/src/error.rs | 166 +++++- .../perry-runtime/src/error_stack_frames.rs | 486 ++++++++++++++++++ .../perry-runtime/src/error_subclass_stack.rs | 14 +- crates/perry-runtime/src/exception.rs | 8 +- .../perry-runtime/src/gc/layout_slot_visit.rs | 5 + .../tests/issue_9486_error_stack_frames.rs | 196 +++++++ 10 files changed, 954 insertions(+), 23 deletions(-) create mode 100644 changelog.d/9486-error-stack-frames.md create mode 100644 crates/perry-runtime/src/error_stack_frames.rs create mode 100644 crates/perry/tests/issue_9486_error_stack_frames.rs diff --git a/changelog.d/9486-error-stack-frames.md b/changelog.d/9486-error-stack-frames.md new file mode 100644 index 0000000000..48b035b110 --- /dev/null +++ b/changelog.d/9486-error-stack-frames.md @@ -0,0 +1,53 @@ +### Fixed + +- **`Error.prototype.stack` now carries real, named frames.** Every stack a + compiled program printed was one line — ` at ` — where node + prints the call chain. `#9432`/`#9410` gave `.stack` its *existence*; nothing + ever looked at the native stack, so its *content* was a placeholder. That is + what made `claude doctor`'s raw-mode report and commander's parse-error + render useless (120 bytes of stderr against node's 14,573), and it is why + every divergence investigation that touched a compiled app had to reach for + gdb and a symbolized build. + + Two halves, and the split between them is the design: + + **Capture** is a frame-pointer chain walk on every `new Error` — two loads + per frame, no allocation, no symbolication. Codegen already tags generated + functions `"frame-pointer"="non-leaf"`, the property the collector's own + `fp_chain` walker relies on, so `[fp] = caller fp` / `[fp+8] = return + address` holds for JS frames. The captured addresses ride in a new + `ErrorHeader.frames` slot (a `StringHeader`, so it needs no new `GC_TYPE_*` + and no new rewrite-descriptor arm — one added `visit(...)` line in the + `GcRewriteDescriptorKind::Error` trace arm covers it). + + **Resolution** happens on the first `.stack` read and reuses the registry + codegen already fills: `js_register_function_name` records + `(compiled address, JS display name)` once per function at module init + (72,713 entries for the claude-code bundle) so `fn.name` and `[Function: f]` + work. That table is keyed by exact function start; a return address points + into the middle of a function, so the resolver snapshots it into an + address-sorted vector once and answers containment with a binary search. + Codegen now registers the same name against the function BODY symbol + (`perry_fn___`) as well as the wrapper — a direct call between + two compiled functions targets the body, so the wrapper address a closure + value carries is not what a return address points into. Both keys map to the + same name and `fn.name` still reads the wrapper key, so nothing that + consulted the registry before sees a different answer. + + Building `.stack` eagerly is what the fix REMOVES: `alloc_error` used to + decode its own message from UTF-8 and allocate two `String`s per + construction to produce a line almost no program ever reads. Constructing a + million errors without reading `.stack` is now cheaper than before, not more + expensive, and the symbolication — the part that costs — happens only for + errors whose `.stack` is actually read, then memoises into the `stack` slot. + + **Frames are named but not positioned.** A `file:line:col` needs a + per-return-address line table, an O(instructions) artifact against this + one's O(functions); a resolved frame renders as ` at `. Frames the + resolver cannot attribute to a registered JS function — the runtime's own, + between `new Error` and the throwing code — are elided rather than printed + as bare addresses, the way node elides its internals; a capture in which + nothing resolves falls back to the pre-fix single `` line, so no + program loses what it had. Windows and any target without a guaranteed + frame-pointer chain keep the old behavior rather than guess at a frame + shape the ABI does not promise. diff --git a/crates/perry-codegen/src/codegen/artifacts.rs b/crates/perry-codegen/src/codegen/artifacts.rs index 8ef5073852..855a0345ba 100644 --- a/crates/perry-codegen/src/codegen/artifacts.rs +++ b/crates/perry-codegen/src/codegen/artifacts.rs @@ -1745,6 +1745,30 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { .map(|sym| (format!("__perry_wrap_{}", sym), display)) }) .collect(); + // #9486: the same name against the function BODY symbol as well. + // + // The wrapper address above is what a closure VALUE carries, so it is what + // `fn.name` needs — but it is not what a return address on the native + // stack points into. A direct call from one compiled function to another + // targets `perry_fn___` itself, so an `Error.stack` frame + // resolves against the body or against nothing at all. Both keys map to + // the same name, and `fn.name` still reads the wrapper key it always did, + // so nothing that consulted this registry before sees a different answer. + let body_symbol_display_names: Vec<(String, String)> = hir + .functions + .iter() + .filter_map(|f| { + let display = hir.closure_display_names.get(&f.id).cloned().or_else(|| { + if f.name.is_empty() || f.name.starts_with('_') { + None + } else { + Some(f.name.clone()) + } + })?; + func_names.get(&f.id).map(|sym| (sym.clone(), display)) + }) + .collect(); + user_fn_display_names.extend(body_symbol_display_names); // (b) Closures bound to a top-level `let`/`const`. #2076: a named // function expression's own name takes precedence over the binding // name (`const bar = function namedBar(){}` ⇒ `"namedBar"`). diff --git a/crates/perry-runtime/src/builtins/formatting.rs b/crates/perry-runtime/src/builtins/formatting.rs index 569b6574a5..b9aced2aff 100644 --- a/crates/perry-runtime/src/builtins/formatting.rs +++ b/crates/perry-runtime/src/builtins/formatting.rs @@ -429,6 +429,28 @@ pub fn register_function_name_if_absent(func_ptr: usize, name: &str) { } } +/// #9486: how many `(function address, name)` pairs the registry currently +/// holds. Cheap enough to consult on every `.stack` read, so the stack-frame +/// resolver can tell a stale address-sorted snapshot from a current one +/// without cloning the table to compare it. +pub fn function_name_registry_len() -> Option { + function_name_registry().lock().ok().map(|map| map.len()) +} + +/// #9486: snapshot the registry as `(function address, name bytes)` pairs for +/// the `Error.stack` frame resolver to sort by address. +/// +/// The `Arc` clones make this a pointer copy per entry rather than a name +/// copy, and the lock is held only for the walk — resolution (a binary search +/// per frame) happens outside it, so a `.stack` read never blocks a +/// concurrent registration for longer than the snapshot itself. +pub fn function_name_registry_entries() -> Option)>> { + function_name_registry() + .lock() + .ok() + .map(|map| map.iter().map(|(k, v)| (*k, v.clone())).collect()) +} + /// Look up the codegen-registered JS name for a function pointer. /// /// Returns the name registered by `js_register_function_name` (keyed on the diff --git a/crates/perry-runtime/src/builtins/mod.rs b/crates/perry-runtime/src/builtins/mod.rs index 6ae2cba144..9765d91300 100644 --- a/crates/perry-runtime/src/builtins/mod.rs +++ b/crates/perry-runtime/src/builtins/mod.rs @@ -144,7 +144,8 @@ pub(crate) use console::{ pub(crate) use console::{test_console_instance_count, test_seed_console_instance}; pub use formatting::{ - function_name_for_ptr, function_source_for_func_ptr, function_source_for_ptr, js_array_print, + function_name_for_ptr, function_name_registry_entries, function_name_registry_len, + function_source_for_func_ptr, function_source_for_ptr, js_array_print, js_boxed_bigint_new, js_boxed_boolean_new, js_boxed_number_new, js_boxed_string_new, js_boxed_symbol_new, js_register_function_name, js_register_function_source, js_util_format, js_util_format_with_options, js_util_inspect, js_util_is_deep_strict_equal, diff --git a/crates/perry-runtime/src/error.rs b/crates/perry-runtime/src/error.rs index 0efcecc28a..444839ec77 100644 --- a/crates/perry-runtime/src/error.rs +++ b/crates/perry-runtime/src/error.rs @@ -120,6 +120,21 @@ pub struct ErrorHeader { /// there separately (#6812: a meta edge enumerated only on the rewrite /// path is invisible to marking). pub meta: *mut crate::object::ObjectMeta, + /// #9486: the native return addresses captured when this error was + /// constructed, ASCII-encoded by `stack_frames::encode_pcs`, optionally + /// followed by `\n` and the #5247 recorded call-site line. Null when the + /// platform has no frame-pointer chain to walk, or once `stack` has been + /// materialised. + /// + /// A `StringHeader` rather than a bespoke cell so it needs no new + /// `GC_TYPE_*`, no new rewrite-descriptor arm and no finalizer: it is + /// traced by the one added `visit(...)` line in the + /// `GcRewriteDescriptorKind::Error` arm of `gc/layout_slot_visit.rs`, + /// exactly like `stack` beside it. + /// + /// Appended LAST, for the reason `meta` documents above: every preceding + /// field keeps its offset. + pub frames: *mut StringHeader, } thread_local! { @@ -178,18 +193,64 @@ static KEEP_JS_SET_CALL_LOCATION: unsafe extern "C" fn(*const u8, usize, u32) = /// #5247: render the current call-location frame, or `` when no /// location was recorded (default builds, or a synthesized/offset-less site). fn current_stack_frame() -> String { + recorded_stack_frame().unwrap_or_else(|| " at ".to_string()) +} + +/// The #5247 call-site frame line, or `None` when no location was recorded. +/// +/// The `Option` is what lets #9486 capture this WITHOUT paying for a string in +/// a default build: `current_stack_frame`'s unconditional `""` +/// allocation happened on every `new Error`, and the recorded case only exists +/// under `--debug-symbols`. +fn recorded_stack_frame() -> Option { if let Some((file, line, column)) = RUNTIME_SOURCE_LOCATION.with(|slot| slot.borrow().clone()) { - return format!(" at {file}:{line}:{column}"); + return Some(format!(" at {file}:{line}:{column}")); } - CURRENT_CALL_LOCATION.with(|c| match c.get() { - Some((file_ptr, file_len, line)) => { + CURRENT_CALL_LOCATION.with(|c| { + c.get().map(|(file_ptr, file_len, line)| { let bytes = unsafe { std::slice::from_raw_parts(file_ptr as *const u8, file_len) }; format!(" at {}:{}", String::from_utf8_lossy(bytes), line) - } - None => " at ".to_string(), + }) }) } +/// #9486: build the `frames` payload for an error being constructed — the +/// encoded native return addresses, plus the recorded #5247 line when there is +/// one. Returns an empty vector when there is nothing to record, in which case +/// no string is allocated at all. +pub(crate) fn capture_frames_payload() -> Vec { + let (blob, len) = stack_frames::capture_encoded(); + let recorded = recorded_stack_frame(); + if len == 0 && recorded.is_none() { + return Vec::new(); + } + let recorded = recorded.unwrap_or_default(); + let mut out = Vec::with_capacity(len + recorded.len() + 1); + out.extend_from_slice(&blob[..len]); + if !recorded.is_empty() { + out.push(b'\n'); + out.extend_from_slice(recorded.as_bytes()); + } + out +} + +/// #9486: render the frame lines of a `.stack` from a captured `frames` +/// payload. The recorded #5247 call site comes first (it is the innermost +/// position we know), then the resolved native frames outward. +pub(crate) fn frames_payload_to_lines(payload: &[u8]) -> String { + let (blob, recorded) = match payload.iter().position(|b| *b == b'\n') { + Some(at) => (&payload[..at], std::str::from_utf8(&payload[at + 1..]).ok()), + None => (payload, None), + }; + let resolved = stack_frames::render_frames(blob); + match (recorded, resolved) { + (Some(line), Some(frames)) => format!("{line}\n{frames}"), + (Some(line), None) => line.to_string(), + (None, Some(frames)) => frames, + (None, None) => " at ".to_string(), + } +} + unsafe fn make_stack(name: &str, message: &str) -> *mut StringHeader { // Build a simple ": \n at :" string // (or "" when no #5247 source location is recorded). Real @@ -237,16 +298,23 @@ unsafe fn alloc_error( let error_name = js_string_from_bytes(name_bytes.as_ptr(), name_bytes.len() as u32); let error_name_handle = scope.root_string_ptr(error_name); - let message_ptr = message_handle.get_raw_const_ptr::() as *mut StringHeader; - let msg_str = { - let len = (*message_ptr).byte_len as usize; - let data = (message_ptr as *const u8).add(std::mem::size_of::()); - let bytes = std::slice::from_raw_parts(data, len); - std::str::from_utf8(bytes).unwrap_or("") + // #9486: `.stack` is NOT built here. The frames are captured — a + // frame-pointer chain walk, no allocation, no symbolication — and the + // string is formatted on first read (`js_error_get_stack`), the same + // capture-then-format-on-read split #9432 gave Error subclasses. Building + // it eagerly meant every `new Error` paid for a UTF-8 decode of its own + // message and two `String` allocations to produce a line almost no + // program ever looks at; paying for a SYMBOLICATED one would have been far + // worse. + let payload = capture_frames_payload(); + let frames_handle = if payload.is_empty() { + None + } else { + Some(scope.root_string_ptr(js_string_from_bytes( + payload.as_ptr(), + payload.len() as u32, + ))) }; - let name_str = std::str::from_utf8(name_bytes).unwrap_or("Error"); - let stack = make_stack(name_str, msg_str); - let stack_handle = scope.root_string_ptr(stack); let raw = crate::arena::arena_alloc_gc( std::mem::size_of::(), @@ -266,12 +334,17 @@ unsafe fn alloc_error( }; (*ptr).message = message_handle.get_raw_const_ptr::() as *mut StringHeader; (*ptr).name = error_name_handle.get_raw_const_ptr::() as *mut StringHeader; - (*ptr).stack = stack_handle.get_raw_const_ptr::() as *mut StringHeader; + // Null until `js_error_get_stack` materialises it (#9486). + (*ptr).stack = std::ptr::null_mut(); (*ptr).cause = f64::from_bits(TAG_UNDEFINED); (*ptr).errors = std::ptr::null_mut(); // No metadata record until something needs one; the GC treats a null meta // edge as absent. (*ptr).meta = std::ptr::null_mut(); + (*ptr).frames = match &frames_handle { + Some(handle) => handle.get_raw_const_ptr::() as *mut StringHeader, + None => std::ptr::null_mut(), + }; ptr } @@ -880,17 +953,73 @@ pub(crate) unsafe fn js_error_builtin_own_property_is_enumerable( } } -/// Get the stack property of an Error +/// Get the stack property of an Error. +/// +/// #9486: this is where `.stack` is BUILT. `alloc_error` stores only the +/// captured return addresses, so the first read of an error's `.stack` +/// formats the head from the error's current `name`/`message` (what V8 does — +/// a subclass constructor assigns `this.name` after `super()` returns) and +/// resolves the captured frames to names, then memoises the result into the +/// `stack` field. Every later read returns that string. +/// +/// This is the single choke point: nothing else may read `(*error).stack` +/// directly, because before this runs it is null. #[no_mangle] pub extern "C" fn js_error_get_stack(error: *mut ErrorHeader) -> *mut StringHeader { unsafe { if error.is_null() { return js_string_from_bytes(b"".as_ptr(), 0); } - (*error).stack + materialize_error_stack(error) } } +/// #9486: format-and-memoise half of [`js_error_get_stack`]. +pub(crate) unsafe fn materialize_error_stack(error: *mut ErrorHeader) -> *mut StringHeader { + if !(*error).stack.is_null() { + return (*error).stack; + } + let name = read_string_header_owned((*error).name); + let message = read_string_header_owned((*error).message); + let head = if name.is_empty() { + message + } else if message.is_empty() { + name + } else { + format!("{name}: {message}") + }; + let head = if head.is_empty() { + "Error".to_string() + } else { + head + }; + let payload = read_string_header_owned((*error).frames); + let text = format!("{head}\n{}", frames_payload_to_lines(payload.as_bytes())); + + // The string birth can collect, and `error` is a bare pointer: root it + // across the allocation and re-read it afterwards, or a moving scavenge + // leaves the memoising store writing into from-space. + let scope = crate::gc::RuntimeHandleScope::new(); + let error_handle = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(error as i64)); + let stack_ptr = js_string_from_bytes(text.as_ptr(), text.len() as u32); + let stack_handle = scope.root_string_ptr(stack_ptr); + let error = crate::value::js_nanbox_get_pointer(error_handle.get_nanbox_f64()) as *mut ErrorHeader; + let stack_ptr = stack_handle.get_raw_const_ptr::() as *mut StringHeader; + crate::gc::runtime_store_gc_heap_word_slot( + error as usize, + &(*error).stack as *const _ as usize, + stack_ptr as u64, + ); + // The capture has served its purpose; releasing it keeps a long-lived + // error from pinning 128 bytes of encoded addresses forever. + crate::gc::runtime_store_gc_heap_word_slot( + error as usize, + &(*error).frames as *const _ as usize, + 0, + ); + stack_ptr +} + fn throw_builtin_not_constructor(name: &'static str) -> ! { let message = format!("{name} is not a constructor"); let msg = js_string_from_bytes(message.as_ptr(), message.len() as u32); @@ -1862,6 +1991,9 @@ static KEEP_AGGREGATEERROR_NEW_FULL: extern "C" fn( #[used] static KEEP_ERROR_IS_ERROR: extern "C" fn(f64) -> f64 = js_error_is_error; +#[path = "error_stack_frames.rs"] +mod stack_frames; + #[path = "error_subclass_stack.rs"] mod subclass_stack; pub use subclass_stack::js_error_subclass_capture_stack; diff --git a/crates/perry-runtime/src/error_stack_frames.rs b/crates/perry-runtime/src/error_stack_frames.rs new file mode 100644 index 0000000000..89020e4964 --- /dev/null +++ b/crates/perry-runtime/src/error_stack_frames.rs @@ -0,0 +1,486 @@ +//! #9486 — real, named frames in `Error.prototype.stack`. +//! +//! Split out of `error.rs` to keep that file under the 2,000-line CI cap +//! (`scripts/check_file_size.sh`); included from there with +//! `#[path = "error_stack_frames.rs"] mod stack_frames;`, so `use super::*` +//! resolves against `error.rs`. +//! +//! # What was broken +//! +//! `current_stack_frame()` produced exactly one line — ` at ` +//! outside a `--debug-symbols` build — because nothing ever looked at the +//! native stack. Node prints the real call chain, so every `err.stack` a +//! compiled app rendered (cc's `doctor`, commander's parse-error report) lost +//! its diagnostic content entirely. +//! +//! # The two halves, and why each one is the cheap one +//! +//! **Capture** is a frame-pointer chain walk. Codegen tags every generated +//! function `"frame-pointer"="non-leaf"` (`perry-codegen/src/function.rs`), +//! which is the same property the collector's own `fp_chain` walker relies on +//! (`gc/roots/stack_maps.rs`), so `[fp] = caller fp` / `[fp+8] = return +//! address` holds for JS frames on both supported architectures. Two loads per +//! frame, no allocation, no symbolication — and it runs on EVERY `new Error`, +//! including the overwhelming majority whose `.stack` is never read. +//! +//! **Resolution** happens on first `.stack` read and reuses the registry +//! codegen already fills: `js_register_function_name` records +//! `(compiled address, JS display name)` once per function in +//! `__perry_init_strings_*` (72,713 entries for the claude-code bundle) so +//! `fn.name` and `[Function: f]` work. That table is keyed by exact function +//! address; a return address points into the MIDDLE of a function, so this +//! module snapshots it into an address-sorted vector once and answers +//! containment with a binary search. +//! +//! # Why the frames are named but not positioned +//! +//! A `file:line:col` would need a per-return-address line table — an +//! O(instructions) artifact, against this one's O(functions). The issue's bar +//! is frame COUNT and NAMES; positions are explicitly not byte-compared +//! against node. ` at name` is what a resolved frame renders as. + +use std::sync::Arc; +use std::sync::Mutex; +use std::sync::OnceLock; + +/// Native return addresses captured per construction. 16 words = 128 bytes of +/// encoded blob, enough to cover node's default `Error.stackTraceLimit` of 10 +/// JS frames plus the runtime frames between `new Error` and the throwing +/// function. +pub(crate) const MAX_CAPTURED_FRAMES: usize = 16; + +/// Rendered JS frames, matching V8's default `Error.stackTraceLimit`. +const RENDER_LIMIT: usize = 10; + +/// Encoded characters per captured address: 48 bits at 6 bits per character. +/// Both supported platforms keep user-space text well under 2^47. +const PC_CHARS: usize = 8; +const PC_BITS: u32 = 48; + +/// A frame whose containing function starts more than this far below it is not +/// plausibly inside that function: the address belongs to an unregistered +/// function (runtime Rust code, a codegen thunk) that happens to sort after a +/// registered one. Rejecting it is what keeps a native frame from being +/// reported under some unrelated JS function's name. +const MAX_FUNCTION_SPAN: usize = 1 << 20; + +const ALPHABET: &[u8; 64] = + b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+-"; + +fn decode_char(c: u8) -> Option { + let v = match c { + b'A'..=b'Z' => c - b'A', + b'a'..=b'z' => c - b'a' + 26, + b'0'..=b'9' => c - b'0' + 52, + b'+' => 62, + b'-' => 63, + _ => return None, + }; + Some(v as u64) +} + +/// Encode captured addresses into an ASCII blob. +/// +/// ASCII rather than the raw little-endian words for one reason: the blob is +/// carried in a `StringHeader` (the only GC cell shape an `ErrorHeader` field +/// and a closure capture slot can both already hold and trace), and a +/// `StringHeader` whose payload is arbitrary bytes is a UTF-8 hazard for every +/// generic string path that might ever touch it. Six bits per character costs +/// 8 bytes per address — the same as the raw word — so the safety is free. +pub(crate) fn encode_pcs(pcs: &[usize], out: &mut [u8; MAX_CAPTURED_FRAMES * PC_CHARS]) -> usize { + let mut n = 0usize; + for &pc in pcs.iter().take(MAX_CAPTURED_FRAMES) { + let v = pc as u64; + if v >> PC_BITS != 0 { + continue; + } + for i in 0..PC_CHARS { + let shift = PC_BITS - 6 * (i as u32 + 1); + out[n + i] = ALPHABET[((v >> shift) & 0x3f) as usize]; + } + n += PC_CHARS; + } + n +} + +/// Inverse of [`encode_pcs`]. A blob whose length is not a multiple of +/// [`PC_CHARS`], or that contains a character outside the alphabet, decodes to +/// nothing rather than to garbage addresses. +fn decode_pcs(blob: &[u8]) -> Vec { + if blob.is_empty() || blob.len() % PC_CHARS != 0 { + return Vec::new(); + } + let mut out = Vec::with_capacity(blob.len() / PC_CHARS); + for chunk in blob.chunks_exact(PC_CHARS) { + let mut v: u64 = 0; + for &c in chunk { + match decode_char(c) { + Some(bits) => v = (v << 6) | bits, + None => return Vec::new(), + } + } + out.push(v as usize); + } + out +} + +// --------------------------------------------------------------------------- +// Capture: the frame-pointer chain walk. +// --------------------------------------------------------------------------- + +#[cfg(all( + any(target_vendor = "apple", target_os = "linux"), + any(target_arch = "aarch64", target_arch = "x86_64") +))] +mod walk { + use super::MAX_CAPTURED_FRAMES; + + /// A frame record is two words and must be word-aligned; anything else is + /// a corrupt chain and abandons the walk, exactly as the collector's + /// `fp_chain::visit` does. + const FRAME_RECORD_ALIGN_MASK: usize = 0x7; + + #[cfg(target_arch = "aarch64")] + #[inline(always)] + fn current_frame_pointer() -> usize { + let fp: usize; + unsafe { + core::arch::asm!("mov {fp}, x29", fp = out(reg) fp, options(nomem, nostack)); + } + fp + } + + #[cfg(target_arch = "x86_64")] + #[inline(always)] + fn current_frame_pointer() -> usize { + let fp: usize; + unsafe { + core::arch::asm!("mov {fp}, rbp", fp = out(reg) fp, options(nomem, nostack)); + } + fp + } + + #[cfg(target_vendor = "apple")] + fn stack_top_uncached() -> usize { + unsafe extern "C" { + fn pthread_self() -> *mut core::ffi::c_void; + fn pthread_get_stackaddr_np(thread: *mut core::ffi::c_void) -> *mut core::ffi::c_void; + } + unsafe { pthread_get_stackaddr_np(pthread_self()) as usize } + } + + #[cfg(all(target_os = "linux", not(target_vendor = "apple")))] + fn stack_top_uncached() -> usize { + unsafe extern "C" { + fn pthread_self() -> usize; + fn pthread_getattr_np(thread: usize, attr: *mut u8) -> i32; + fn pthread_attr_getstack( + attr: *const u8, + stackaddr: *mut *mut core::ffi::c_void, + stacksize: *mut usize, + ) -> i32; + fn pthread_attr_destroy(attr: *mut u8) -> i32; + } + let mut attr = [0u8; 128]; + let mut addr: *mut core::ffi::c_void = core::ptr::null_mut(); + let mut size: usize = 0; + unsafe { + if pthread_getattr_np(pthread_self(), attr.as_mut_ptr()) != 0 { + return 0; + } + let ok = pthread_attr_getstack(attr.as_ptr(), &mut addr, &mut size) == 0; + pthread_attr_destroy(attr.as_mut_ptr()); + if !ok { + return 0; + } + } + (addr as usize).saturating_add(size) + } + + // The bound is a property of the thread, and `new Error` is frequent + // enough that two libc calls per construction would be the dominant cost + // of the capture. + crate::perry_thread_local! { + static STACK_TOP: std::cell::Cell = const { std::cell::Cell::new(0) }; + } + + fn stack_top() -> usize { + STACK_TOP.with(|c| { + let cached = c.get(); + if cached != 0 { + return cached; + } + let top = stack_top_uncached(); + c.set(top); + top + }) + } + + /// Collect return addresses from this frame outward, innermost first. + /// + /// Fails closed: any misaligned, non-increasing or out-of-bounds frame + /// pointer ends the walk and keeps whatever was collected before it, + /// rather than reading a word at a fabricated address. + pub(super) fn capture(out: &mut [usize; MAX_CAPTURED_FRAMES]) -> usize { + let top = stack_top(); + if top == 0 { + return 0; + } + let mut n = 0usize; + let mut fp = current_frame_pointer(); + while n < MAX_CAPTURED_FRAMES && fp != 0 { + if fp & FRAME_RECORD_ALIGN_MASK != 0 { + break; + } + match fp.checked_add(16) { + Some(end) if end <= top => {} + _ => break, + } + // SAFETY: `fp` is word-aligned and `fp..fp+16` lies inside this + // thread's stack, so both words of the frame record are readable. + let return_address = unsafe { *((fp + 8) as *const usize) }; + let caller_fp = unsafe { *(fp as *const usize) }; + if return_address == 0 { + break; + } + out[n] = return_address; + n += 1; + if caller_fp <= fp { + break; + } + fp = caller_fp; + } + n + } +} + +#[cfg(not(all( + any(target_vendor = "apple", target_os = "linux"), + any(target_arch = "aarch64", target_arch = "x86_64") +)))] +mod walk { + use super::MAX_CAPTURED_FRAMES; + + /// Windows and the non-frame-pointer targets keep the pre-#9486 behavior + /// (a single `` frame) rather than guess at a chain shape the + /// ABI does not guarantee. + pub(super) fn capture(_out: &mut [usize; MAX_CAPTURED_FRAMES]) -> usize { + 0 + } +} + +/// Capture the current native return addresses and encode them. +/// Returns `(buffer, len)`; `len == 0` means nothing was captured. +pub(crate) fn capture_encoded() -> ([u8; MAX_CAPTURED_FRAMES * PC_CHARS], usize) { + let mut pcs = [0usize; MAX_CAPTURED_FRAMES]; + let n = walk::capture(&mut pcs); + let mut blob = [0u8; MAX_CAPTURED_FRAMES * PC_CHARS]; + if n == 0 { + return (blob, 0); + } + let len = encode_pcs(&pcs[..n], &mut blob); + (blob, len) +} + +// --------------------------------------------------------------------------- +// Resolution: address -> JS display name. +// --------------------------------------------------------------------------- + +struct CodeSymbolIndex { + /// Registry size the snapshot was taken at. `register_function_name_if_absent` + /// can add entries after module init (symbol-keyed object literals, + /// `util.promisify`), so a changed length rebuilds rather than serving a + /// stale table. + source_len: usize, + /// `(function start address, display name)`, sorted by address. + entries: Vec<(usize, Arc<[u8]>)>, +} + +fn index_slot() -> &'static Mutex> { + static INDEX: OnceLock>> = OnceLock::new(); + INDEX.get_or_init(|| Mutex::new(None)) +} + +/// Resolve `ip` to the display name of the function containing it. +/// +/// The registry names function STARTS, and there is no end address to pair +/// with them, so containment is "the greatest start at or below `ip`, provided +/// `ip` is below the next start and within [`MAX_FUNCTION_SPAN`] of this one". +/// The span cap is what stops an address past the last registered function — +/// every runtime Rust frame, on a link layout that places the archives after +/// the generated objects — from being reported under that function's name. +fn name_for_ip(index: &CodeSymbolIndex, ip: usize) -> Option<&Arc<[u8]>> { + let at = index.entries.partition_point(|(addr, _)| *addr <= ip); + let at = at.checked_sub(1)?; + let (start, name) = &index.entries[at]; + if ip < *start || ip - *start > MAX_FUNCTION_SPAN { + return None; + } + if let Some((next, _)) = index.entries.get(at + 1) { + if ip >= *next { + return None; + } + } + Some(name) +} + +fn with_index(f: impl FnOnce(&CodeSymbolIndex) -> R) -> Option { + let mut slot = index_slot().lock().ok()?; + let current_len = crate::builtins::function_name_registry_len()?; + let stale = match slot.as_ref() { + Some(index) => index.source_len != current_len, + None => true, + }; + if stale { + let mut entries = crate::builtins::function_name_registry_entries()?; + entries.sort_unstable_by_key(|(addr, _)| *addr); + *slot = Some(CodeSymbolIndex { + source_len: current_len, + entries, + }); + } + slot.as_ref().map(f) +} + +/// Render captured frames as `.stack` frame lines, or `None` when nothing in +/// the capture resolved to a JS function. +/// +/// Unresolved frames are DROPPED rather than printed as bare addresses. Node +/// elides its own internal frames the same way, and the frames this drops are +/// exactly the runtime's: `js_error_new_with_message`, `alloc_error`, the +/// builtin that invoked a callback. A capture in which nothing resolves +/// returns `None` so the caller can fall back to the pre-#9486 single line +/// instead of producing a headed stack with no frames at all. +pub(crate) fn render_frames(blob: &[u8]) -> Option { + let pcs = decode_pcs(blob); + if pcs.is_empty() { + return None; + } + with_index(|index| { + let mut out = String::new(); + let mut rendered = 0usize; + for pc in &pcs { + if rendered >= RENDER_LIMIT { + break; + } + // A return address points AFTER the call instruction; on a tail + // position that byte can belong to the next function, so resolve + // the call site itself. + let Some(name) = name_for_ip(index, pc.saturating_sub(1)) else { + continue; + }; + let Ok(name) = std::str::from_utf8(name) else { + continue; + }; + if name.is_empty() { + continue; + } + if rendered > 0 { + out.push('\n'); + } + out.push_str(" at "); + out.push_str(name); + rendered += 1; + } + if rendered == 0 { + None + } else { + Some(out) + } + }) + .flatten() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pcs_round_trip_through_the_ascii_blob() { + let pcs = [0x1_0000_1234usize, 0x7fff_ffff_0000, 1, 0]; + let mut buf = [0u8; MAX_CAPTURED_FRAMES * PC_CHARS]; + let len = encode_pcs(&pcs, &mut buf); + assert_eq!(len, pcs.len() * PC_CHARS); + assert!( + buf[..len].iter().all(|b| b.is_ascii_graphic()), + "the blob rides in a StringHeader — every byte must be plain ASCII" + ); + assert_eq!(decode_pcs(&buf[..len]), pcs.to_vec()); + } + + #[test] + fn a_malformed_blob_decodes_to_nothing_rather_than_to_addresses() { + assert!(decode_pcs(b"AAA").is_empty(), "short blob"); + assert!(decode_pcs(b"AAAAAAA!").is_empty(), "character outside the alphabet"); + assert!(decode_pcs(b"").is_empty(), "empty blob"); + } + + /// The containment rule is the whole correctness story of the resolver: a + /// registry entry names a function START, and mis-reading the gap after + /// the last one is how a runtime frame would acquire a JS function's name. + #[test] + fn containment_rejects_addresses_past_a_function() { + let index = CodeSymbolIndex { + source_len: 2, + entries: vec![ + (0x1000, Arc::from(&b"first"[..])), + (0x2000, Arc::from(&b"second"[..])), + ], + }; + assert_eq!( + name_for_ip(&index, 0x1004).map(|n| n.to_vec()), + Some(b"first".to_vec()) + ); + assert_eq!( + name_for_ip(&index, 0x2000).map(|n| n.to_vec()), + Some(b"second".to_vec()) + ); + assert!( + name_for_ip(&index, 0x0fff).is_none(), + "below the first entry belongs to nobody" + ); + assert!( + name_for_ip(&index, 0x2000 + MAX_FUNCTION_SPAN + 1).is_none(), + "past the last entry by more than a function's plausible span is \ + a runtime frame, not `second`" + ); + } + + /// A capture in which no address resolves must not produce an empty frame + /// list — the caller has to be able to fall back. + #[test] + fn a_capture_with_no_resolvable_frame_renders_nothing() { + let mut buf = [0u8; MAX_CAPTURED_FRAMES * PC_CHARS]; + // Address 8, which no registry can plausibly contain. + let len = encode_pcs(&[8usize], &mut buf); + assert_eq!(render_frames(&buf[..len]), None); + } + + #[test] + fn the_walk_sees_more_than_one_frame() { + // The unit binary is Rust, not generated code, so nothing here + // RESOLVES — but the chain itself must be walkable, which is the + // half of the capture this crate can test on its own. + #[inline(never)] + fn innermost() -> usize { + let (_, len) = capture_encoded(); + len + } + #[inline(never)] + fn middle() -> usize { + std::hint::black_box(innermost()) + } + let len = std::hint::black_box(middle()); + if cfg!(all( + any(target_vendor = "apple", target_os = "linux"), + any(target_arch = "aarch64", target_arch = "x86_64") + )) { + assert!( + len >= 2 * PC_CHARS, + "the frame-pointer chain must yield at least two return \ + addresses from a two-deep call; got {} chars", + len + ); + } + } +} diff --git a/crates/perry-runtime/src/error_subclass_stack.rs b/crates/perry-runtime/src/error_subclass_stack.rs index 556df23556..3fd3f9d8d7 100644 --- a/crates/perry-runtime/src/error_subclass_stack.rs +++ b/crates/perry-runtime/src/error_subclass_stack.rs @@ -86,6 +86,10 @@ extern "C" fn error_subclass_stack_getter(closure: *const crate::closure::Closur crate::value::js_nanbox_get_pointer(prep) as *const crate::closure::ClosureHeader; return crate::closure::js_closure_call2(prep_ptr, receiver, structured); } + // #9486: capture slot 0 holds the ENCODED capture (native return + // addresses, plus the #5247 line when one was recorded), not a + // finished frame line — resolving addresses to names is the expensive + // half and belongs here, on read, not in the constructor. let frame = { let bits = crate::closure::js_closure_get_capture_bits(closure, 0); let ptr = (bits & crate::value::POINTER_MASK) as *const StringHeader; @@ -95,7 +99,7 @@ extern "C" fn error_subclass_stack_getter(closure: *const crate::closure::Closur { current_stack_frame() } else { - read_string_header_owned(ptr) + frames_payload_to_lines(read_string_header_owned(ptr).as_bytes()) } }; let head = error_subclass_stack_head(receiver); @@ -220,9 +224,11 @@ pub extern "C" fn js_error_subclass_capture_stack(this_val: f64) { return; } - // Capture the frame NOW — this is the whole point of installing at - // construction rather than formatting the string on first read. - let frame = current_stack_frame(); + // Capture the frames NOW — this is the whole point of installing at + // construction rather than formatting on first read. #9486 makes the + // captured value the raw return addresses; the getter turns them into + // named lines. + let frame = capture_frames_payload(); let frame_ptr = js_string_from_bytes(frame.as_ptr(), frame.len() as u32); if frame_ptr.is_null() { return; diff --git a/crates/perry-runtime/src/exception.rs b/crates/perry-runtime/src/exception.rs index 94f728fa5e..d02d9e5f64 100644 --- a/crates/perry-runtime/src/exception.rs +++ b/crates/perry-runtime/src/exception.rs @@ -694,7 +694,13 @@ pub(crate) fn print_uncaught(value: f64) { let eh = ptr as *const crate::error::ErrorHeader; let name_str = unsafe { string_header_to_string((*eh).name) }; let msg_str = unsafe { string_header_to_string((*eh).message) }; - let stack_str = unsafe { string_header_to_string((*eh).stack) }; + // #9486: through the accessor, never off the field — the + // field is null until the first read materialises it. + let stack_str = unsafe { + string_header_to_string(crate::error::js_error_get_stack( + ptr as *mut crate::error::ErrorHeader, + )) + }; let name_display = if name_str.is_empty() { "Error" } else { diff --git a/crates/perry-runtime/src/gc/layout_slot_visit.rs b/crates/perry-runtime/src/gc/layout_slot_visit.rs index 83b543afec..c86d890569 100644 --- a/crates/perry-runtime/src/gc/layout_slot_visit.rs +++ b/crates/perry-runtime/src/gc/layout_slot_visit.rs @@ -221,6 +221,11 @@ pub(super) unsafe fn visit_gc_rewrite_slot_descriptors( // the meta record (keeping it, and anything reachable only through // it, alive) and rewrites the edge when evacuation moves it. visit(fixed_slot(&mut (*error).meta as *mut _ as *mut u64)); + // #9486: the captured-frames blob. Same shape as `stack` beside + // it — a `StringHeader` edge, null until captured and null again + // once `.stack` has been materialised — so it needs exactly this + // one line and no new descriptor kind. + visit(fixed_slot(&mut (*error).frames as *mut _ as *mut u64)); } GcRewriteDescriptorKind::Map => { let map = user_ptr as *mut crate::map::MapHeader; diff --git a/crates/perry/tests/issue_9486_error_stack_frames.rs b/crates/perry/tests/issue_9486_error_stack_frames.rs new file mode 100644 index 0000000000..96be7c11a0 --- /dev/null +++ b/crates/perry/tests/issue_9486_error_stack_frames.rs @@ -0,0 +1,196 @@ +//! Regression test for #9486: `Error.prototype.stack` carries real, NAMED +//! frames instead of the single `at ` placeholder. +//! +//! What is asserted is STRUCTURE, not bytes. Perry compiles to native code and +//! has no per-instruction line table, so a frame renders as ` at ` +//! where node renders ` at (file:line:col)`; positions differ +//! legitimately and comparing them against node would be a test of the wrong +//! thing. What must hold is what the issue asks for: more than one frame, and +//! the function names in call order. +//! +//! Both halves of the mechanism are exercised: the frame-pointer capture at +//! construction and the address→name resolution on read. The fixture also +//! pins the re-throw contract — `throw e` inside a `catch` must NOT recapture, +//! so the innermost frame stays the one that first constructed the error. + +use std::path::PathBuf; +use std::process::Command; +use std::sync::Once; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn workspace_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../..") + .canonicalize() + .expect("canonicalize workspace root") +} + +fn target_debug_dir() -> PathBuf { + std::env::var_os("CARGO_TARGET_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| workspace_root().join("target")) + .join("debug") +} + +fn ensure_runtime_archive() { + static BUILD_RUNTIME: Once = Once::new(); + BUILD_RUNTIME.call_once(|| { + let cargo = std::env::var_os("CARGO").unwrap_or_else(|| "cargo".into()); + let build = Command::new(cargo) + .current_dir(workspace_root()) + .arg("build") + .arg("-p") + .arg("perry-runtime") + .output() + .expect("run cargo build -p perry-runtime"); + assert!( + build.status.success(), + "cargo build -p perry-runtime failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&build.stdout), + String::from_utf8_lossy(&build.stderr) + ); + }); +} + +fn runtime_dir() -> PathBuf { + if let Some(runtime_dir) = std::env::var_os("PERRY_RUNTIME_DIR") { + return PathBuf::from(runtime_dir); + } + ensure_runtime_archive(); + target_debug_dir() +} + +/// Prints one `