diff --git a/samply/src/linux_shared/converter.rs b/samply/src/linux_shared/converter.rs index 120020d4..e10dcf62 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 480c6ef9..fe8acb22 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 0d4728b4..cf7ef8aa 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 e1309b42..4ba0c132 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]"); + } + } }