diff --git a/.cargo/config.toml b/.cargo/config.toml index 3f62e06197..ff1fe946e0 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -27,3 +27,24 @@ global-min-publish-age = "7 days" # aborts loudly if the tables are missing (perry-runtime/src/eh.rs). [build] rustflags = ["-C", "force-unwind-tables=yes"] + +# #9486: x86_64 needs frame pointers, and cannot inherit them from `[build]`. +# +# `Error.stack` captures its frames by walking the `rbp` / `x29` chain, which +# is only a chain if every frame between `new Error` and the throwing JS +# function maintains one. Generated code always does — codegen tags it +# `"frame-pointer"="non-leaf"` — but the runtime is Rust, and on +# x86_64-unknown-linux-gnu rustc leaves `rbp` as a general-purpose +# callee-saved register. Measured: `rbp` inside `alloc_error` held `0x1`, so +# the walk had no root at all and every stack fell back to `at `. +# The AArch64 platform ABI reserves x29, which is why this is a x86_64-only +# knob and why the collector's own `fp_chain` walker was AArch64-only. +# +# `force-unwind-tables` is REPEATED here on purpose: cargo does not merge +# `target.*.rustflags` with `build.rustflags` — the target flags take +# precedence and the `[build]` list is dropped entirely for this target. Omit +# the repeat and every throw crossing a runtime frame is stranded (the runtime +# self-checks on the first `try` and aborts loudly, so the mistake is loud +# rather than silent — but it is still a mistake). +[target.'cfg(target_arch = "x86_64")'] +rustflags = ["-C", "force-unwind-tables=yes", "-C", "force-frame-pointers=yes"] diff --git a/changelog.d/9486-error-stack-frames.md b/changelog.d/9486-error-stack-frames.md new file mode 100644 index 0000000000..5d5ef05e82 --- /dev/null +++ b/changelog.d/9486-error-stack-frames.md @@ -0,0 +1,87 @@ +### 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 ()` + — V8's own spelling for a frame whose script position is unknown, which is + also the `name (location)` shape the stack-parsing libraries in real bundles + read a name out of. 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. + + **Two limits worth knowing.** Inlining removes frames: `a() → b() → c()` + where all three are small folds into one function, so the trace names the + frame that survives rather than all three. V8 keeps inlined frames because it + retains inlining metadata for deoptimization; an ahead-of-time compiler has + no such record, and the frames that matter in real traces — the ones across + `try`/`catch`, callbacks and module boundaries — are exactly the ones the + inliner does not fold. And a frame is only as good as the registry's + coverage: an address inside a function nothing registered resolves to + whichever registered function precedes it, so this change also registers + class constructors, static methods and accessors, which previously had no + name of their own and were the ones a neighbour's name leaked into (measured: + a `new Widget()` frame came out labelled `main`). + + Registering a name is gated on `LlModule::has_function`. `method_names` is + a DISPATCH registry, not an emission record — it carries keys this module + never defines a body for, and emitting a registration against one makes + module init reference an undefined global. The claude-code bundle found + exactly one, a getter (`UT7.__get_get_extensionName`) out of ~46k functions, + and failed to compile; nothing smaller than that bundle reproduced it. + +- **x86_64 builds now keep frame pointers.** The capture above walks the + `rbp` / `x29` chain, which is only a chain if every frame between + `new Error` and the throwing JS function maintains one. Generated code always + did; the runtime is Rust, and on `x86_64-unknown-linux-gnu` rustc leaves + `rbp` as a general-purpose callee-saved register — measured, `rbp` inside + `alloc_error` held `0x1`, so the walk had no root and every stack on that + target fell back to `at `. `.cargo/config.toml` now adds + `-C force-frame-pointers=yes` for x86_64 only; the AArch64 platform ABI + reserves `x29`, which is why the collector's own `fp_chain` walker was + AArch64-only and why this knob is not needed there. diff --git a/crates/perry-codegen/src/codegen/artifacts.rs b/crates/perry-codegen/src/codegen/artifacts.rs index 8ef5073852..ac8b9f2457 100644 --- a/crates/perry-codegen/src/codegen/artifacts.rs +++ b/crates/perry-codegen/src/codegen/artifacts.rs @@ -1745,6 +1745,116 @@ 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) + .filter(|sym| llmod.has_function(sym)) + .map(|sym| (sym.clone(), display)) + }) + .collect(); + user_fn_display_names.extend(body_symbol_display_names); + // #9486: class methods, under the `Class.method` label node uses for a + // prototype-method frame. `method_names` is the map codegen itself keyed + // the emitted `perry_method_*` symbols by, and the `__perry_wrap_*` + // generator earlier in this function walks exactly this pair of loops + // with the same `.get(...)` guard — so every symbol here is one this module + // definitely emitted, which is the condition the #318/#343 "use of + // undefined value" class turns on. Only the BODY symbol is registered: + // the wrapper address is what `fn.name` reads, and giving a method a + // `.name` it never had is a separate, observable change. + { + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + // `method_names` is a DISPATCH registry, not an emission record: it + // carries entries this module never defines a body for (an accessor + // reached only through a cross-module or typed path, a stale key from + // a shape that lowered elsewhere). Registering one of those emits a + // `js_register_function_name(ptr @perry_method_…)` against a symbol + // that does not exist, and the module fails to build with "reference + // to unknown global" — measured on the claude-code bundle, where + // exactly one getter (`UT7.__get_get_extensionName`) had a registry + // key and no definition out of ~46k functions. `has_function` is the + // authority on what this module actually emitted, so every name below + // goes through it. + let mut push_defined = |symbol: String, display: String| { + if symbol.is_empty() || display.is_empty() || !llmod.has_function(&symbol) { + return; + } + if seen.insert(symbol.clone()) { + user_fn_display_names.push((symbol, display)); + } + }; + for class in &hir.classes { + for method in &class.methods { + let Some(symbol) = method_names + .get(&(class.name.clone(), method.name.clone())) + .cloned() + else { + continue; + }; + push_defined(symbol, format!("{}.{}", class.name, method.name)); + } + for method in &class.static_methods { + let Some(symbol) = method_names + .get(&(class.name.clone(), method.name.clone())) + .cloned() + else { + continue; + }; + push_defined(symbol, format!("{}.{}", class.name, method.name)); + } + // Accessors are keyed with the `__get_` / `__set_` prefix + // `method_registry` gives them, and node labels their frames + // `get x` / `set x`. + for (accessors, prefix, label) in [ + (&class.getters, "__get_", "get"), + (&class.setters, "__set_", "set"), + ] { + for (prop, _) in accessors { + let Some(symbol) = method_names + .get(&(class.name.clone(), format!("{prefix}{prop}"))) + .cloned() + else { + continue; + }; + push_defined(symbol, format!("{label} {prop}")); + } + } + // The constructor is registered in `method_names` under the + // synthesized `_constructor` method name (method_registry.rs + // emits one for EVERY class, explicit or not), and node labels its + // frame `new `. + // + // Registering these is not only about naming THEIR frames. A + // registry entry names a function START and carries no end, so an + // address inside an UNREGISTERED function resolves to whatever + // registered function precedes it — measured: a `new Widget()` + // frame came out labelled `main`. Every emitted function this list + // covers is one that can no longer borrow a neighbour's name. + let ctor_key = (class.name.clone(), format!("{}_constructor", class.name)); + if let Some(symbol) = method_names.get(&ctor_key).cloned() { + push_defined(symbol, format!("new {}", class.name)); + } + } + } // (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-codegen/src/codegen/string_pool.rs b/crates/perry-codegen/src/codegen/string_pool.rs index fefcd27854..5e0eb6eb88 100644 --- a/crates/perry-codegen/src/codegen/string_pool.rs +++ b/crates/perry-codegen/src/codegen/string_pool.rs @@ -257,11 +257,27 @@ pub(super) fn emit_string_pool( // Each entry becomes one `js_register_function_name(, , // )` call inside the init function. See #1202. let mut user_fn_name_constants: Vec<(String, String, usize)> = Vec::new(); + // Deduplicated by CONTENT (#9486): the same display name is now registered + // against several symbols — a top-level function's wrapper and its body, + // a class method and its `__perry_wrap_*` twin — and `add_string_constant` + // mints a fresh `@.str.N` per call, so without this every extra + // registration also cost a duplicate copy of the name in rodata. + // Deterministic: the map only reuses a global the loop already minted in + // its (already sorted) input order, so emission order is unchanged (#7622). + let mut name_constant_cache: std::collections::HashMap<&str, (String, usize)> = + std::collections::HashMap::new(); for (wrapper_sym, display_name) in user_fn_display_names { if wrapper_sym.is_empty() || display_name.is_empty() { continue; } - let (const_name, byte_len) = llmod.add_string_constant(display_name); + let (const_name, byte_len) = match name_constant_cache.get(display_name.as_str()) { + Some(hit) => hit.clone(), + None => { + let minted = llmod.add_string_constant(display_name); + name_constant_cache.insert(display_name.as_str(), minted.clone()); + minted + } + }; user_fn_name_constants.push((wrapper_sym.clone(), const_name, byte_len)); } diff --git a/crates/perry-ext-mysql2/src/lib.rs b/crates/perry-ext-mysql2/src/lib.rs index 8436f0370b..3a21c0a6b0 100644 --- a/crates/perry-ext-mysql2/src/lib.rs +++ b/crates/perry-ext-mysql2/src/lib.rs @@ -1828,7 +1828,9 @@ mod tests { runtime_string((*error).message), "Invalid connection handle" ); - let stack = runtime_string((*error).stack); + // #9486: through the accessor — the field is null until the + // first read materialises the string. + let stack = runtime_string(perry_runtime::error::js_error_get_stack(error)); assert!(stack.contains("Error: Invalid connection handle")); } } diff --git a/crates/perry-ext-sharp/src/lib.rs b/crates/perry-ext-sharp/src/lib.rs index 8e75250da8..60f6e2dfe5 100644 --- a/crates/perry-ext-sharp/src/lib.rs +++ b/crates/perry-ext-sharp/src/lib.rs @@ -1103,7 +1103,9 @@ mod tests { perry_ffi::copy_string_from_raw(message), "Invalid sharp handle" ); - assert!(!(*error).stack.is_null()); + // #9486: through the accessor — the field is null until the + // first read materialises the string. + assert!(!perry_runtime::error::js_error_get_stack(error).is_null()); } } diff --git a/crates/perry-runtime/src/builtins/formatting.rs b/crates/perry-runtime/src/builtins/formatting.rs index 569b6574a5..1b18af8420 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 @@ -699,8 +721,16 @@ unsafe fn format_error_headline(error_ptr: *const crate::error::ErrorHeader) -> } } -unsafe fn format_error_stack_frame(error_ptr: *const crate::error::ErrorHeader) -> Option { - let stack = string_header_to_string((*error_ptr).stack, ""); +/// The one stack line `util.inspect` shows under an error's headline. +/// +/// #9486: through the accessor, never off the field — `alloc_error` leaves +/// `stack` null and the first read materialises it, so a direct field read +/// here made `console.log(err)` print no frame at all. It is called from +/// `format_error_value` as the LAST use of `error_ptr` on purpose: the +/// accessor allocates, and a moving scavenge during that allocation would +/// leave any later read of `error_ptr` pointing at from-space. +unsafe fn format_error_stack_frame(error_ptr: *mut crate::error::ErrorHeader) -> Option { + let stack = string_header_to_string(crate::error::js_error_get_stack(error_ptr), ""); stack .lines() .skip(1) @@ -758,7 +788,7 @@ unsafe fn format_error_value(error_ptr: *const crate::error::ErrorHeader, depth: } let mut out = headline; - if let Some(frame) = format_error_stack_frame(error_ptr) { + if let Some(frame) = format_error_stack_frame(error_ptr as *mut _) { out.push('\n'); out.push_str(&frame); out.push_str(" {"); 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..ad10a9885c 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,15 +193,24 @@ 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(), + }) }) } @@ -237,16 +261,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 +297,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,14 +916,24 @@ 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) } } @@ -1862,6 +1908,12 @@ 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; +pub(crate) use stack_frames::{ + capture_frames_payload, frames_payload_to_lines, materialize_error_stack, +}; + #[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..cbf8ef266d --- /dev/null +++ b/crates/perry-runtime/src/error_stack_frames.rs @@ -0,0 +1,656 @@ +//! #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. A resolved frame renders as ` at name ()` — +//! V8's own spelling for a frame whose script position is unknown, and the +//! shape the stack-parsing libraries in real bundles expect. + +use super::*; +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 nearest registered function starts more than this far below +/// it is not plausibly inside that function: the address belongs to an +/// unregistered one — runtime Rust code, a codegen thunk — that happens to +/// sort after it. Rejecting it is what keeps a native frame out of the trace +/// under some unrelated JS function's name, and dropping a frame is the right +/// way to be wrong here: a MISSING frame is visibly missing, while a +/// MIS-NAMED one sends the reader after the wrong function. +/// +/// 64 KiB of machine code is a very large single JS function and a very small +/// slice of the runtime, which is the asymmetry this number trades on. +const MAX_FUNCTION_SPAN: usize = 64 * 1024; + +/// `PERRY_ERROR_STACK_DIAG=1` prints, per `.stack` materialisation, what the +/// capture collected and what the resolver made of it. +/// +/// Parsed by VALUE, not by presence: `PERRY_GC_DIAG` was `var_os(..).is_some()` +/// for long enough that `PERRY_GC_DIAG=0` ENABLED diagnostics and silently +/// collapsed one arm of an A/B (fixed in #7993). A new knob does not get to +/// repeat that. +pub(crate) fn diag_enabled() -> bool { + static ON: OnceLock = OnceLock::new(); + *ON.get_or_init(|| { + matches!( + std::env::var("PERRY_ERROR_STACK_DIAG").ok().as_deref(), + Some("1") | Some("on") | Some("true") + ) + }) +} + +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 diag_stack_top() -> usize { + stack_top() + } + + pub(super) fn diag_frame_pointer() -> usize { + current_frame_pointer() + } + + pub(super) fn capture(out: &mut [usize; MAX_CAPTURED_FRAMES]) -> usize { + let top = stack_top(); + if top == 0 { + return 0; + } + // The low bound is this frame's own stack address. On aarch64 the + // platform ABI makes x29 a real frame pointer everywhere, so this is + // belt-and-braces; on x86_64 the runtime's own Rust frames may omit + // one, in which case `rbp` still holds the frame pointer of the + // innermost function that DID establish one — generated code always + // does — and that is exactly the frame the capture wants. What the + // bound rules out is the remaining case: an `rbp` holding something + // that is not a stack address at all, which would otherwise start the + // walk on fabricated frame records. + let probe = 0usize; + let low = &probe as *const usize as usize; + let mut n = 0usize; + let mut fp = current_frame_pointer(); + if fp < low { + return 0; + } + 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 + } + + pub(super) fn diag_stack_top() -> usize { + 0 + } + + pub(super) fn diag_frame_pointer() -> 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); + if diag_enabled() { + eprintln!( + "[stackdiag] capture: frames={n} top={:#x} fp0={:#x} pcs={:x?}", + walk::diag_stack_top(), + walk::diag_frame_pointer(), + &pcs[..n] + ); + } + 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 binary search already guarantees the NEXT registered function starts +/// above `ip`, so the only open question is whether `ip` is inside THIS +/// function or past its end — and the registry has no end addresses, it names +/// starts. [`MAX_FUNCTION_SPAN`] is that missing bound, and it is what stops +/// an address well 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. +/// +/// The residual is honest and worth stating: an address inside an +/// UNREGISTERED function that sits within the span of a registered one — a +/// codegen thunk, or runtime code the linker interleaved — resolves to the +/// preceding registered name. It is the same shape as the residual the +/// collector's own function table carries (`stack_maps_index.rs`: "a function +/// with no safepoints is absent … so an `ip` inside one resolves to the +/// previous mapped function"), and closing it needs a per-function code +/// extent, which Mach-O does not expose cheaply. +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 > MAX_FUNCTION_SPAN { + 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| { + if diag_enabled() { + eprintln!( + "[stackdiag] resolve: registry_entries={} first={:#x} last={:#x}", + index.entries.len(), + index.entries.first().map(|(a, _)| *a).unwrap_or(0), + index.entries.last().map(|(a, _)| *a).unwrap_or(0) + ); + for pc in &pcs { + let hit = name_for_ip(index, pc.saturating_sub(1)) + .and_then(|n| std::str::from_utf8(n).ok().map(|s| s.to_string())); + eprintln!("[stackdiag] ip={pc:#x} -> {hit:?}"); + } + } + 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'); + } + // `at ()`, not a bare `at `: V8 already + // spells an unknown script position `()`, and keeping + // the `name (location)` shape is what lets the stack-parsing + // libraries in real bundles (`error-stack-parser`, + // `source-map-support`) read the name out of the frame at all. + out.push_str(" at "); + out.push_str(name); + out.push_str(" ()"); + rendered += 1; + } + if rendered == 0 { + None + } else { + Some(out) + } + }) + .flatten() +} + +/// #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) = 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 = 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(), + } +} + +/// #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 +} + +#[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..5588420088 100644 --- a/crates/perry-runtime/src/exception.rs +++ b/crates/perry-runtime/src/exception.rs @@ -672,6 +672,58 @@ fn emit_uncaught_backtrace() { } } +/// Render the line `print_uncaught` emits for a native `ErrorHeader`. +/// +/// Extracted so the ORDER of the reads is testable, which is the whole point +/// of this function existing (#9486). Node formats an uncaught throw as +/// `: ` followed by the frames and no `Uncaught exception:` +/// prefix (#616), and `Error.stack` already starts with that head, so the +/// stack IS the report; the head is rebuilt only when there is no stack. +/// +/// # Every read off `eh` happens BEFORE the stack is materialised +/// +/// `js_error_get_stack` allocates: since #9486 the string is built on first +/// read, not at construction. That allocation is a GC point, and an +/// evacuating scavenge moves the error out from under `eh` — a plain Rust +/// local, which perry's collector neither scans nor pins, so afterwards it +/// names from-space. Reading `(*eh).message` there to resolve the `ERR_*` +/// code is an unsound read whose result is whatever the collector left in the +/// vacated cell, and the address it yields is then used as a side-table key. +/// Reading everything first costs nothing and needs no root, which is why +/// this is an ordering fix rather than a `RuntimeHandleScope`. +/// +/// (Before #9486 the stack came from a plain field load, so no GC point +/// existed between the reads at all.) +pub(crate) unsafe fn uncaught_native_error_report(eh: *mut crate::error::ErrorHeader) -> String { + let name_str = string_header_to_string((*eh).name); + let msg_str = string_header_to_string((*eh).message); + let code = crate::node_submodules::error_code_for_message((*eh).message); + // Nothing may read `eh` past this line. + let stack_str = string_header_to_string(crate::error::js_error_get_stack(eh)); + + let name_display = if name_str.is_empty() { + "Error" + } else { + &name_str + }; + if !stack_str.is_empty() { + match code { + Some(code) => { + let frames = stack_str + .split_once('\n') + .map(|(_, frames)| format!("\n{frames}")) + .unwrap_or_default(); + format!("{name_display} [{code}]: {msg_str}{frames}") + } + None => stack_str, + } + } else if msg_str.is_empty() { + name_display.to_string() + } else { + format!("{name_display}: {msg_str}") + } +} + /// Best-effort display of a thrown value for uncaught-exception reporting. /// Matches Node semantics roughly: Errors print `name: message` + stack, /// regular objects probe for `.message`/`.stack`, everything else goes @@ -690,42 +742,9 @@ pub(crate) fn print_uncaught(value: f64) { // program declares (`class_id == 2 == OBJECT_TYPE_ERROR`) as an // Error and print `name`/`message`/`stack` out of its field slots. if unsafe { crate::error::ptr_is_native_error(ptr) } { - // ErrorHeader: object_type, error_kind, message, name, stack, cause, errors - 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) }; - let name_display = if name_str.is_empty() { - "Error" - } else { - &name_str - }; - // Issue #616: Node formats an uncaught throw as - // : - // at - // ... - // (no `Uncaught exception:` prefix). Perry's `stack` field - // already starts with `: ` per Error.stack - // convention, so emit just the stack — matches Node format - // for this header. When the stack is empty (defensive), fall - // back to the bare `: ` line. - if !stack_str.is_empty() { - if let Some(code) = - crate::node_submodules::error_code_for_message(unsafe { (*eh).message }) - { - let frames = stack_str - .split_once('\n') - .map(|(_, frames)| format!("\n{frames}")) - .unwrap_or_default(); - eprintln!("{name_display} [{code}]: {msg_str}{frames}"); - } else { - eprintln!("{}", stack_str); - } - } else if msg_str.is_empty() { - eprintln!("{}", name_display); - } else { - eprintln!("{}: {}", name_display, msg_str); - } + eprintln!("{}", unsafe { + uncaught_native_error_report(ptr as *mut crate::error::ErrorHeader) + }); return; } if unsafe { @@ -962,4 +981,52 @@ mod tests { assert_eq!(outcome, Err(2.0)); assert_eq!(current_try_depth(), base); } + + /// #9486: the extracted uncaught report carries the `ERR_*` code and the + /// frames, in the shape `print_uncaught` emits. + /// + /// This function exists as a separate item so the ORDER of its reads is + /// reviewable: `js_error_get_stack` allocates now, so every read off the + /// raw `eh` has to happen before it. That ordering is held by the + /// function's structure and its doc comment, NOT by this test — the + /// harness can force a collection at a point of its own choosing + /// (`gc_collect_minor`) but cannot schedule one inside + /// `js_string_from_bytes`, and a test that cannot create the racing + /// collection cannot assert the race is handled. Two attempts at one were + /// discarded rather than shipped green-but-vacuous: the first suppressed + /// GC triggers so nothing moved at all, and the second moved the error but + /// asserted a rekey that does not happen (see below). + /// + /// What this does pin is the report itself: the code branch, the frame + /// tail, and the ` []: ` head. + /// + /// Adjacent finding, NOT fixed here (pre-existing, unrelated to #9486): + /// `register_error_code_pub` / `error_code_for_message` key on the + /// MESSAGE `StringHeader`'s address, while the `GcMoveHookKind::ErrorSideTables` + /// rekey in `node_submodules/diagnostics_gc.rs` rekeys by the ERROR's + /// address. Nothing rekeys the message key, so an error's `ERR_*` code is + /// dropped as soon as its message string is relocated — reproduced with a + /// forced minor collection while writing this test. + #[test] + fn the_uncaught_report_carries_the_error_code_and_the_frames() { + unsafe { + let msg = crate::string::js_string_from_bytes(b"boom".as_ptr(), 4); + let err = crate::error::js_error_new_with_message(msg); + crate::node_submodules::register_error_code_pub((*err).message, "ERR_TEST_9486"); + + let report = uncaught_native_error_report(err); + + let (head, frames) = report + .split_once('\n') + .unwrap_or_else(|| panic!("the report must carry frames; got {report:?}")); + assert_eq!( + head, "Error [ERR_TEST_9486]: boom", + "node's head for a coded error is ` []: `" + ); + assert!( + frames.contains(" at "), + "the frame tail must survive the code branch; got {frames:?}" + ); + } + } } 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-runtime/src/gc/tests/alloc.rs b/crates/perry-runtime/src/gc/tests/alloc.rs index 649390fd56..ff33d88154 100644 --- a/crates/perry-runtime/src/gc/tests/alloc.rs +++ b/crates/perry-runtime/src/gc/tests/alloc.rs @@ -672,6 +672,7 @@ fn alloc_malloc_kind_test_object(obj_type: u8) -> *mut u8 { cause: 0.0, errors: std::ptr::null_mut(), meta: std::ptr::null_mut(), + frames: std::ptr::null_mut(), }, ); } diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index 49a7391cb0..6b35535846 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -95,6 +95,7 @@ pub(super) unsafe fn alloc_old_test_error() -> *mut crate::error::ErrorHeader { cause: f64::from_bits(crate::value::TAG_UNDEFINED), errors: std::ptr::null_mut(), meta: std::ptr::null_mut(), + frames: std::ptr::null_mut(), }, ); ptr diff --git a/crates/perry-runtime/src/promise/native_async.rs b/crates/perry-runtime/src/promise/native_async.rs index ee46651c85..a23b578072 100644 --- a/crates/perry-runtime/src/promise/native_async.rs +++ b/crates/perry-runtime/src/promise/native_async.rs @@ -700,7 +700,9 @@ mod tests { assert!(crate::error::ptr_is_native_error(error as usize)); assert_eq!(string_bytes((*error).message), expected); - let stack = string_bytes((*error).stack); + // #9486: through the accessor, never off the field — `alloc_error` + // leaves `stack` null and the first read materialises it. + let stack = string_bytes(crate::error::js_error_get_stack(error as *mut _)); assert!( stack.starts_with(b"Error: ") && stack.windows(expected.len()).any(|w| w == expected), "Error.stack must include the rejection message: {}", diff --git a/crates/perry-runtime/src/promise/rejection.rs b/crates/perry-runtime/src/promise/rejection.rs index 7cdcb088d8..1233d7d824 100644 --- a/crates/perry-runtime/src/promise/rejection.rs +++ b/crates/perry-runtime/src/promise/rejection.rs @@ -181,8 +181,13 @@ fn describe_rejection_reason(v: f64) -> String { // Offset 0 is `class_id` now, and `OBJECT_TYPE_ERROR` is 2 — an // ordinary user class id. if unsafe { crate::error::ptr_is_native_error(ptr) } { - let eh = ptr as *const crate::error::ErrorHeader; - let stack = unsafe { crate::exception::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 = unsafe { + crate::exception::string_header_to_string(crate::error::js_error_get_stack( + ptr as *mut crate::error::ErrorHeader, + )) + }; return format!("error(0x{ptr:x}) stack={stack:?}"); } return format!("pointer(0x{ptr:x})"); @@ -464,8 +469,14 @@ fn print_unhandled_diagnostic(reason: f64) { // #8113: `GcHeader.obj_type == GC_TYPE_ERROR` (which subsumes the // band+plausibility gate above), not a raw offset-0 read. if unsafe { crate::error::ptr_is_native_error(ptr) } { - let eh = ptr as *const crate::error::ErrorHeader; - let stack_str = unsafe { crate::exception::string_header_to_string((*eh).stack) }; + // #9486: through the accessor — see above. This is the line that + // prints an unhandled rejection's trace, so a null read here is + // exactly the frameless report this issue is about. + let stack_str = unsafe { + crate::exception::string_header_to_string(crate::error::js_error_get_stack( + ptr as *mut crate::error::ErrorHeader, + )) + }; if !stack_str.is_empty() { eprintln!("Uncaught (in promise) {stack_str}"); return; 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..5a991d2fb7 --- /dev/null +++ b/crates/perry/tests/issue_9486_error_stack_frames.rs @@ -0,0 +1,221 @@ +//! 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") +} + +/// `perry-runtime` is an rlib; the archive the compiled fixture links against +/// comes from the `perry-runtime-static` staticlib wrapper, so that is the +/// package to build. Building the rlib instead leaves `PERRY_RUNTIME_DIR` +/// pointing at a directory with no `libperry_runtime.a` (clean tree: the +/// fixture fails to link) or a stale one (reused target dir: the fixture +/// links a runtime older than the compiler that emitted its object code, and +/// perry's own source-hash check rejects the pair). +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-static") + .output() + .expect("run cargo build -p perry-runtime-static"); + assert!( + build.status.success(), + "cargo build -p perry-runtime-static 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 `