From f28424261898e6a945baf4d93eb48e868267e8fc Mon Sep 17 00:00:00 2001 From: not-matthias Date: Fri, 23 Oct 2026 16:37:55 +0200 Subject: [PATCH] feat(linux): mark truncated and failed sample stacks Expose incomplete DWARF stacks as synthetic root labels in Firefox profiles instead of silently dropping the truncation sentinel. Distinguish capture exhaustion from other unwind errors, and attach markers after frame-pointer recovery, root trimming, and recursion folding. Keep the existing cache validation policy. Refs COD-3911 --- samply/src/linux_shared/converter.rs | 250 ++++++++++++++++++++---- samply/src/shared/stack_converter.rs | 200 +++++++++++++++++-- samply/src/shared/types.rs | 21 +- samply/tests/stack_read_cache_replay.rs | 31 +++ 4 files changed, 441 insertions(+), 61 deletions(-) diff --git a/samply/src/linux_shared/converter.rs b/samply/src/linux_shared/converter.rs index 120020d43..e10dcf623 100644 --- a/samply/src/linux_shared/converter.rs +++ b/samply/src/linux_shared/converter.rs @@ -55,7 +55,7 @@ use crate::shared::process_sample_data::{ use crate::shared::prop_types::ProfileCreationProps; use crate::shared::synthetic_jit_library::SyntheticJitLibrary; use crate::shared::timestamp_converter::TimestampConverter; -use crate::shared::types::{StackFrame, StackMode}; +use crate::shared::types::{StackFrame, StackMarker, StackMode}; use crate::shared::unresolved_samples::{ UnresolvedSamples, UnresolvedStackHandle, UnresolvedStacks, }; @@ -700,6 +700,24 @@ where ) { stack.clear(); + // Frame-pointer unwinding (framehop's fallback for code without + // `.eh_frame`, and the kernel's FP callchain) can mistake a stack-local + // value for a saved return address. A genuine code address always lies + // in a known module, so trim trailing user frames from the root inward + // until one does (or we hit a non-user frame). + let trim_unmapped_root = |stack: &mut Vec| { + while let Some(last) = stack.last() { + if last.stack_mode() != Some(StackMode::User) { + break; + } + let addr = last.address(); + if addr != 0 && !unwinder.is_address_in_module(addr) { + stack.pop(); + } else { + break; + } + } + }; // Parse e.callchain into kernel frames and user FP frames. let mut callchain_buf = Vec::new(); let mut callchain_kernel_len = 0; @@ -727,13 +745,14 @@ where } callchain_kernel_len = callchain_buf.iter().take_while(|f| f.is_kernel()).count(); } + trim_unmapped_root(&mut callchain_buf); let (kernel_frames, fp_user_frames) = callchain_buf.split_at(callchain_kernel_len); // Start with kernel frames from the callchain. stack.extend_from_slice(kernel_frames); // Append the user stack with the help of DWARF unwinding. - let mut dwarf_truncated = false; + let mut dwarf_marker = None; let dwarf_start = stack.len(); if let (Some(regs), Some((user_stack, _))) = (&e.user_regs, e.user_stack) { let ustack_bytes = RawDataU64::from_raw_data::(user_stack); @@ -773,8 +792,15 @@ where let frame = match frames.next() { Ok(Some(frame)) => frame, Ok(None) => break, - Err(_) => { - dwarf_truncated = true; + Err(error) => { + dwarf_marker = Some(match error { + framehop::Error::CouldNotReadStack(addr) + if addr >= sample_window.end() => + { + StackMarker::Truncated + } + _ => StackMarker::UnwindFailed, + }); break; } }; @@ -798,25 +824,7 @@ where }); } } - - // Frame-pointer unwinding (framehop's fallback for code without - // `.eh_frame`, and the kernel's FP callchain) can mistake a stack-local - // value for a saved return address. A genuine code address always lies - // in a known module, so trim trailing user frames from the root inward - // until one does (or we hit a non-user frame). - let trim_unmapped_root = |stack: &mut Vec| { - while let Some(last) = stack.last() { - if last.stack_mode() != Some(StackMode::User) { - break; - } - let addr = last.address(); - if addr != 0 && !unwinder.is_address_in_module(addr) { - stack.pop(); - } else { - break; - } - } - }; + trim_unmapped_root(stack); // Reconcile the DWARF stack against the user portion of the kernel's FP // callchain. The two can disagree because DWARF unwinding falls back to @@ -841,25 +849,16 @@ where .zip(fp_user_frames.iter()) .take_while(|(a, b)| a.address() == b.address()) .count(); - if common == dwarf_len { - stack.extend_from_slice(&fp_user_frames[common..]); - } else if fp_user_frames.len() > dwarf_len && common >= 1 { + if fp_user_frames.len() > dwarf_len && (common == dwarf_len || common >= 1) { stack.truncate(dwarf_start + common); stack.extend_from_slice(&fp_user_frames[common..]); + dwarf_marker = None; } - } else if dwarf_truncated || stack.len() == dwarf_start { - // No usable callchain. Use what little we have, or mark truncation. - if stack.len() == dwarf_start && !fp_user_frames.is_empty() { - stack.extend_from_slice(fp_user_frames); - } else if dwarf_truncated { - stack.push(StackFrame::TruncatedStackMarker); - } + } else if stack.len() == dwarf_start && !fp_user_frames.is_empty() { + stack.extend_from_slice(fp_user_frames); + dwarf_marker = None; } - // The callchain frames appended above weren't run through DWARF, so an FP - // root can still slip in; trim trailing frames outside any known module. - trim_unmapped_root(&mut *stack); - if stack.is_empty() { if let Some(ip) = e.ip { stack.push(StackFrame::InstructionPointer(ip, e.cpu_mode.into())); @@ -870,6 +869,13 @@ where stack.pop(); } } + + // Mark only the final stack, after FP reconciliation and root trimming. + if !stack.is_empty() { + if let Some(marker) = dwarf_marker { + stack.push(StackFrame::StackMarker(marker)); + } + } } pub fn handle_mmap(&mut self, e: MmapRecord, timestamp: u64) { @@ -2159,3 +2165,173 @@ impl Marker for MmapMarker { self.0 } } + +#[cfg(test)] +mod tests { + use framehop::x86_64::{CacheX86_64, UnwinderX86_64}; + use linux_perf_event_reader::{CpuMode, RawData, Regs}; + + use super::*; + use crate::linux_shared::convert_regs::ConvertRegsX86_64; + + const SP: u64 = 0x1000; + const IP: u64 = 0x5000; + const CALLER: u64 = 0x6000; + const ROOT: u64 = 0x7000; + + fn sample_stack(bp: u64, words: &[u64], callchain: &[u64]) -> Vec { + let bytes = |words: &[u64]| { + words + .iter() + .flat_map(|word| word.to_le_bytes()) + .collect::>() + }; + // PERF_REG_X86_BP, SP, IP are consecutive in this order. + let registers = bytes(&[bp, SP, IP]); + let stack_bytes = bytes(words); + let callchain_bytes = bytes(callchain); + let sample = SampleRecord { + id: None, + addr: None, + stream_id: None, + raw: None, + ip: Some(IP), + timestamp: Some(0), + pid: Some(1), + tid: Some(1), + cpu: None, + period: None, + read: None, + user_regs: Some(Regs::new( + ConvertRegsX86_64::regs_mask(), + RawDataU64::from_raw_data::(RawData::Single(®isters)), + )), + user_stack: Some((RawData::Single(&stack_bytes), stack_bytes.len() as u64)), + callchain: Some(RawDataU64::from_raw_data::(RawData::Single( + &callchain_bytes, + ))), + phys_addr: None, + data_page_size: None, + code_page_size: None, + intr_regs: None, + cpu_mode: CpuMode::User, + }; + let mut unwinder = UnwinderX86_64::new(); + unwinder.add_module(Module::new( + "test".into(), + 0x4000..0x8000, + 0x4000, + ExplicitModuleSectionInfo { + base_svma: 0x4000, + text_svma: None, + text: None, + stubs_svma: None, + stub_helper_svma: None, + got_svma: None, + unwind_info: None, + eh_frame_svma: None, + eh_frame: None, + eh_frame_hdr_svma: None, + eh_frame_hdr: None, + debug_frame: None, + text_segment_svma: None, + text_segment: None, + }, + )); + let mut stack = Vec::new(); + Converter::>::get_sample_stack::( + &sample, + &unwinder, + &mut CacheX86_64::new(), + &mut HashMap::new(), + &mut stack, + true, + false, + ); + stack + } + + fn ip() -> StackFrame { + StackFrame::InstructionPointer(IP, StackMode::User) + } + + fn caller() -> StackFrame { + StackFrame::ReturnAddress(CALLER, StackMode::User) + } + + #[test] + fn capture_exhaustion_is_distinct_from_invalid_unwinding() { + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER], &[]), + vec![ + ip(), + caller(), + StackFrame::StackMarker(StackMarker::Truncated) + ] + ); + // A read below the captured stack is not capture exhaustion. + assert_eq!( + sample_stack(SP - 8, &[], &[]), + vec![ip(), StackFrame::StackMarker(StackMarker::UnwindFailed)] + ); + // A backwards frame pointer fails without a memory read. + assert_eq!( + sample_stack(SP - 16, &[], &[]), + vec![ip(), StackFrame::StackMarker(StackMarker::UnwindFailed)] + ); + assert_eq!(sample_stack(SP, &[0, CALLER], &[]), vec![ip(), caller()]); + } + + #[test] + fn fp_extension_resolves_dwarf_failure_but_equal_callchain_does_not() { + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER], &[IP, CALLER, ROOT]), + vec![ + ip(), + caller(), + StackFrame::ReturnAddress(ROOT, StackMode::User) + ] + ); + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER], &[IP, ROOT, CALLER]), + vec![ + ip(), + StackFrame::ReturnAddress(ROOT, StackMode::User), + caller() + ] + ); + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER], &[IP, CALLER]), + vec![ + ip(), + caller(), + StackFrame::StackMarker(StackMarker::Truncated) + ] + ); + } + + #[test] + fn diagnostic_markers_do_not_block_root_trimming_or_recursion_folding() { + const UNMAPPED_ADDRESS: u64 = 0xdead; + assert_eq!( + sample_stack(SP, &[SP + 16, UNMAPPED_ADDRESS], &[]), + vec![ip(), StackFrame::StackMarker(StackMarker::Truncated)] + ); + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER], &[IP, CALLER, UNMAPPED_ADDRESS]), + vec![ + ip(), + caller(), + StackFrame::StackMarker(StackMarker::Truncated) + ] + ); + assert_eq!( + sample_stack(SP, &[SP + 16, CALLER, SP + 32, CALLER], &[]), + vec![ + ip(), + caller(), + StackFrame::StackMarker(StackMarker::Truncated) + ] + ); + } +} diff --git a/samply/src/shared/stack_converter.rs b/samply/src/shared/stack_converter.rs index 480c6ef90..fe8acb223 100644 --- a/samply/src/shared/stack_converter.rs +++ b/samply/src/shared/stack_converter.rs @@ -2,12 +2,13 @@ use std::collections::VecDeque; use std::iter::{Cloned, Rev}; use fxprof_processed_profile::{ - FrameAddress, FrameFlags, FrameHandle, ProcessHandle, Profile, SubcategoryHandle, + Category, CategoryColor, FrameAddress, FrameFlags, FrameHandle, ProcessHandle, Profile, + SubcategoryHandle, }; use super::jit_category_manager::{JsFrame, JsName}; use super::lib_mappings::{AndroidArtInfo, LibMappingsHierarchy}; -use super::types::{StackFrame, StackMode}; +use super::types::{StackFrame, StackMarker, StackMode}; #[derive(Debug)] pub struct StackConverter { @@ -49,6 +50,7 @@ struct LibartFilteringIter<'c, I: Iterator> { struct ConvertedStackIterD> { inner: I, pending_frame_handle: Option, + pending_marker: Option, js_name_for_baseline_interpreter: Option, } @@ -70,20 +72,17 @@ impl> Iterator for FirstPassIter { } fn next(&mut self) -> Option { - loop { - let frame = self.0.next()?; - let (mode, lookup_address, from_ip) = match frame { - StackFrame::InstructionPointer(addr, mode) => (mode, addr, true), - StackFrame::ReturnAddress(addr, mode) => (mode, addr.saturating_sub(1), false), - StackFrame::AdjustedReturnAddress(addr, mode) => (mode, addr, false), - StackFrame::TruncatedStackMarker => continue, - }; - return Some(FirstPassFrameInfo { - mode, - lookup_address, - from_ip, - }); - } + let (mode, lookup_address, from_ip) = match self.0.next()? { + StackFrame::InstructionPointer(addr, mode) => (mode, addr, true), + StackFrame::ReturnAddress(addr, mode) => (mode, addr.saturating_sub(1), false), + StackFrame::AdjustedReturnAddress(addr, mode) => (mode, addr, false), + StackFrame::StackMarker(_) => unreachable!("stack markers must be root-most"), + }; + Some(FirstPassFrameInfo { + mode, + lookup_address, + from_ip, + }) } } @@ -196,17 +195,23 @@ impl> Iterator for LibartFilteringIter<' impl> ConvertedStackIterD { fn size_hint(&self) -> (usize, Option) { - // Use the slice length as the size hint. This is a bit of a lie, unfortunately. - // This iterator can yield more elements than self.inner if we add JS frames, - // or fewer elements if the original iterator contains TruncatedStackMarker frames. - // But it's a relatively good approximation. - self.inner.size_hint() + // Approximate the converted depth for stack elision. JS frames can expand + // the stack and ART filtering can shrink it. + let pending = + self.pending_frame_handle.is_some() as usize + self.pending_marker.is_some() as usize; + let (lower, upper) = self.inner.size_hint(); + (lower + pending, upper.map(|upper| upper + pending)) } fn next(&mut self, profile: &mut Profile) -> Option { if let Some(pending_frame_handle) = self.pending_frame_handle.take() { return Some(pending_frame_handle); } + if let Some(marker) = self.pending_marker.take() { + let category = profile.handle_for_category(Category("Unwinding", CategoryColor::Gray)); + let label = profile.handle_for_string(marker.label()); + return Some(profile.handle_for_frame_with_label(label, category, FrameFlags::empty())); + } let SecondPassFrameInfo { location, category, @@ -289,6 +294,14 @@ impl StackConverter { lib_mappings: &'a LibMappingsHierarchy, extra_first_frame: Option, ) -> ConvertedStackIter<'a> { + // Producers append markers after stack reconciliation. Keep the root marker + // outside JS/ART processing, immediately inside any extra label frame. + let (stack, pending_marker) = match stack.split_last() { + Some((StackFrame::StackMarker(marker), frames)) => { + (frames, (!frames.is_empty()).then_some(*marker)) + } + _ => (stack, None), + }; let pass1 = FirstPassIter(stack.iter().cloned().rev()); let pass2 = SecondPassIter { inner: pass1, @@ -306,8 +319,153 @@ impl StackConverter { let pass4 = ConvertedStackIterD { inner: pass3, pending_frame_handle: extra_first_frame, + pending_marker, js_name_for_baseline_interpreter: None, }; ConvertedStackIter(pass4) } } + +#[cfg(test)] +mod tests { + use fxprof_processed_profile::{ + Category, CategoryColor, CpuDelta, FrameFlags, ProcessHandle, Profile, ReferenceTimestamp, + SamplingInterval, SubcategoryHandle, ThreadHandle, Timestamp, + }; + use serde_json::Value; + + use super::super::lib_mappings::{LibMappingOpQueue, LibMappingsHierarchy}; + use super::super::stack_depth_limiting_frame_iter::StackDepthLimitingFrameIter; + use super::super::types::{StackFrame, StackMarker, StackMode}; + use super::StackConverter; + + fn new_profile() -> (Profile, ProcessHandle, ThreadHandle, SubcategoryHandle) { + let mut profile = Profile::new( + "test", + ReferenceTimestamp::from_millis_since_unix_epoch(0.0), + SamplingInterval::from_millis(1), + ); + let process = profile.add_process("test", 1, Timestamp::from_millis_since_reference(0.0)); + let thread = profile.add_thread( + process, + 1, + Timestamp::from_millis_since_reference(0.0), + true, + ); + let user_category = profile + .handle_for_category(Category("User", CategoryColor::Blue)) + .into(); + (profile, process, thread, user_category) + } + + fn serialized_stack_labels(profile: &Value, stack_index: usize) -> Vec { + let shared = &profile["shared"]; + let stack_table = &shared["stackTable"]; + let frame_table = &shared["frameTable"]; + let func_table = &shared["funcTable"]; + let mut stack_index = Some(stack_index); + let mut labels = Vec::new(); + while let Some(index) = stack_index { + let frame_index = stack_table["frame"][index].as_u64().unwrap() as usize; + let func_index = frame_table["func"][frame_index].as_u64().unwrap() as usize; + let name_index = func_table["name"][func_index].as_u64().unwrap() as usize; + labels.push( + shared["stringArray"][name_index] + .as_str() + .unwrap() + .to_string(), + ); + stack_index = stack_table["prefix"][index] + .as_u64() + .map(|index| index as usize); + } + labels.reverse(); + labels + } + + #[test] + fn markers_are_preserved_as_distinct_root_labels() { + let (mut profile, process, thread, user_category) = new_profile(); + let mappings = LibMappingsHierarchy::new(LibMappingOpQueue::default()); + let mut converter = StackConverter::new(user_category, user_category); + let extra_label_string = profile.handle_for_string("sample"); + let extra_label = profile.handle_for_frame_with_label( + extra_label_string, + user_category, + FrameFlags::empty(), + ); + for (index, marker, extra) in [ + (0, StackMarker::Truncated, Some(extra_label)), + (1, StackMarker::UnwindFailed, None), + (2, StackMarker::Truncated, None), + ] { + let frames = [ + StackFrame::InstructionPointer(0x1234, StackMode::User), + StackFrame::StackMarker(marker), + ]; + let mut converted = converter.convert_stack(process, &frames, &mappings, extra); + let stack_handle = profile.handle_for_stack_frames(|profile| converted.next(profile)); + profile.add_sample( + thread, + Timestamp::from_millis_since_reference(index as f64), + stack_handle, + CpuDelta::ZERO, + (index + 1) as i32, + ); + } + let ordinary_frames = [StackFrame::InstructionPointer(0x1234, StackMode::User)]; + let mut ordinary = converter.convert_stack(process, &ordinary_frames, &mappings, None); + let ordinary_stack = profile.handle_for_stack_frames(|profile| ordinary.next(profile)); + profile.add_sample( + thread, + Timestamp::from_millis_since_reference(3.0), + ordinary_stack, + CpuDelta::ZERO, + 4, + ); + + let serialized = serde_json::to_value(&profile).unwrap(); + let thread_json = &serialized["threads"][0]; + let samples = &thread_json["samples"]; + let sample_stacks = samples["stack"].as_array().unwrap(); + let labels: Vec<_> = sample_stacks + .iter() + .map(|stack| serialized_stack_labels(&serialized, stack.as_u64().unwrap() as usize)) + .collect(); + assert_eq!(labels[0], ["sample", "[truncated]", "0x1234"]); + assert_eq!(labels[1], ["[unwind_failed]", "0x1234"]); + assert_eq!(labels[2], ["[truncated]", "0x1234"]); + assert_eq!(labels[3], ["0x1234"]); + } + + #[test] + fn deep_stack_elision_preserves_the_diagnostic_root() { + let (mut profile, process, thread, user_category) = new_profile(); + let mappings = LibMappingsHierarchy::new(LibMappingOpQueue::default()); + let mut converter = StackConverter::new(user_category, user_category); + let mut frames: Vec<_> = (0..502) + .map(|address| StackFrame::InstructionPointer(address as u64, StackMode::User)) + .collect(); + frames.push(StackFrame::StackMarker(StackMarker::Truncated)); + let converted = converter.convert_stack(process, &frames, &mappings, None); + let mut converted = + StackDepthLimitingFrameIter::new(&mut profile, converted, user_category); + let stack_handle = profile.handle_for_stack_frames(|profile| converted.next(profile)); + profile.add_sample( + thread, + Timestamp::from_millis_since_reference(0.0), + stack_handle, + CpuDelta::ZERO, + 1, + ); + + let serialized = serde_json::to_value(&profile).unwrap(); + let stack_index = serialized["threads"][0]["samples"]["stack"][0] + .as_u64() + .unwrap() as usize; + let labels = serialized_stack_labels(&serialized, stack_index); + assert_eq!(labels.first().map(String::as_str), Some("[truncated]")); + assert!(labels.iter().any(|label| label.contains("frames elided"))); + assert_eq!(labels.last().map(String::as_str), Some("0x0")); + } +} diff --git a/samply/src/shared/types.rs b/samply/src/shared/types.rs index 0d4728b48..cf7ef8aa4 100644 --- a/samply/src/shared/types.rs +++ b/samply/src/shared/types.rs @@ -37,12 +37,27 @@ impl From for StackMode { } } +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum StackMarker { + Truncated, + UnwindFailed, +} + +impl StackMarker { + pub fn label(self) -> &'static str { + match self { + Self::Truncated => "[truncated]", + Self::UnwindFailed => "[unwind_failed]", + } + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum StackFrame { InstructionPointer(u64, StackMode), ReturnAddress(u64, StackMode), AdjustedReturnAddress(u64, StackMode), - TruncatedStackMarker, + StackMarker(StackMarker), } impl StackFrame { @@ -51,7 +66,7 @@ impl StackFrame { StackFrame::InstructionPointer(addr, _) => addr, StackFrame::ReturnAddress(addr, _) => addr, StackFrame::AdjustedReturnAddress(addr, _) => addr, - StackFrame::TruncatedStackMarker => 0, + StackFrame::StackMarker(_) => 0, } } @@ -60,7 +75,7 @@ impl StackFrame { StackFrame::InstructionPointer(_, stack_mode) => Some(stack_mode), StackFrame::ReturnAddress(_, stack_mode) => Some(stack_mode), StackFrame::AdjustedReturnAddress(_, stack_mode) => Some(stack_mode), - StackFrame::TruncatedStackMarker => None, + StackFrame::StackMarker(_) => None, } } diff --git a/samply/tests/stack_read_cache_replay.rs b/samply/tests/stack_read_cache_replay.rs index e1309b42e..4ba0c1324 100644 --- a/samply/tests/stack_read_cache_replay.rs +++ b/samply/tests/stack_read_cache_replay.rs @@ -121,6 +121,26 @@ fn sample_stacks(profile: &Value) -> Vec> { stacks } +fn sample_root_names(profile: &Value) -> Vec<&str> { + let shared = &profile["shared"]; + profile["threads"] + .as_array() + .unwrap() + .iter() + .flat_map(|thread| thread["samples"]["stack"].as_array().unwrap()) + .map(|stack| { + let mut stack = stack.as_u64().unwrap() as usize; + while let Some(prefix) = shared["stackTable"]["prefix"][stack].as_u64() { + stack = prefix as usize; + } + let frame = shared["stackTable"]["frame"][stack].as_u64().unwrap() as usize; + let func = shared["frameTable"]["func"][frame].as_u64().unwrap() as usize; + let name = shared["funcTable"]["name"][func].as_u64().unwrap() as usize; + shared["stringArray"][name].as_str().unwrap() + }) + .collect() +} + #[test] fn replayed_stacks_are_not_spliced_from_earlier_samples() { let dir = fixture_dir(); @@ -184,4 +204,15 @@ fn replayed_stacks_are_not_spliced_from_earlier_samples() { complete_a > 0, "no phase A stack was completed past the stack copy" ); + + // Phase B never visits shallower frames, so no matching snapshot can + // complete its deep stack. The exported root must expose that truncation. + for (stack, root_name) in stacks.iter().zip(sample_root_names(&profile)) { + if stack + .first() + .is_some_and(|addr| functions.b_leaf.contains(addr)) + { + assert_eq!(root_name, "[truncated]"); + } + } }