diff --git a/e2e-tests/tests/cpp_script_execution.rs b/e2e-tests/tests/cpp_script_execution.rs index 4c23510..601948e 100644 --- a/e2e-tests/tests/cpp_script_execution.rs +++ b/e2e-tests/tests/cpp_script_execution.rs @@ -4,6 +4,24 @@ mod common; use common::{init, FIXTURES}; +const CPP_NESTED_MEMBER_TRACE_LINE: u32 = 44; + +async fn compile_cpp_complex_script( + script: &str, +) -> anyhow::Result { + let binary_path = FIXTURES.get_test_binary("cpp_complex_program")?; + let mut analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary_path) + .await + .map_err(|e| anyhow::anyhow!("failed to load DWARF for cpp_complex_program: {e}"))?; + let compile_options = ghostscope_compiler::CompileOptions { + binary_path_hint: Some(binary_path.to_string_lossy().into_owned()), + ..Default::default() + }; + + ghostscope_compiler::compile_script(script, &mut analyzer, None, Some(1), &compile_options) + .map_err(|e| anyhow::anyhow!("compile_script failed: {e}")) +} + async fn run_ghostscope_with_script_for_target( script_content: &str, timeout_secs: u64, @@ -31,6 +49,56 @@ async fn spawn_cpp_complex_program() -> anyhow::Result anyhow::Result<()> { + init(); + + let binary_path = FIXTURES.get_test_binary("cpp_complex_program")?; + let source_path = binary_path + .parent() + .ok_or_else(|| anyhow::anyhow!("cpp_complex_program has no parent directory"))? + .join("main.cpp"); + + let valid_script = format!( + r#" +trace {}:{CPP_NESTED_MEMBER_TRACE_LINE} {{ + print o.nested.shadow; +}} +"#, + source_path.display() + ); + let valid = compile_cpp_complex_script(&valid_script).await?; + assert!( + !valid.uprobe_configs.is_empty(), + "expected valid o.nested.shadow to compile; target_info={} failed_targets={:?}", + valid.target_info, + valid.failed_targets + ); + + let invalid_script = format!( + r#" +trace {}:{CPP_NESTED_MEMBER_TRACE_LINE} {{ + print o.shadow; +}} +"#, + source_path.display() + ); + if let Ok(invalid) = compile_cpp_complex_script(&invalid_script).await { + assert!( + invalid.uprobe_configs.is_empty(), + "expected o.shadow to be rejected because shadow is only a member of o.nested; target_info={} failed_targets={:?}", + invalid.target_info, + invalid.failed_targets + ); + assert!( + !invalid.failed_targets.is_empty(), + "expected at least one failed target for invalid o.shadow access" + ); + } + + Ok(()) +} + #[tokio::test] async fn test_cpp_script_print_globals() -> anyhow::Result<()> { init(); diff --git a/e2e-tests/tests/fixtures/cpp_complex_program/main.cpp b/e2e-tests/tests/fixtures/cpp_complex_program/main.cpp index bb584c1..83a32fb 100644 --- a/e2e-tests/tests/fixtures/cpp_complex_program/main.cpp +++ b/e2e-tests/tests/fixtures/cpp_complex_program/main.cpp @@ -2,6 +2,7 @@ #include #include #include +#include int g_counter = 0; const char* g_msg = "hello cpp"; @@ -10,6 +11,17 @@ static int s_internal = 123; namespace ns1 { struct Point { int x; int y; }; +struct Outer { + struct Nested { + int shadow; + int payload; + }; + + int tag; + Nested nested; + int tail; +}; + class Foo { public: static int s_val; @@ -22,6 +34,17 @@ int Foo::s_val = 7; __attribute__((noinline)) int add(int a, int b) { return a + b; } __attribute__((noinline)) int add(double a, double b) { return (int)(a + b); } +__attribute__((noinline)) int nested_member_probe(int v) { + volatile Outer outer = { + 101, + {202 + v, 303}, + 404, + }; + Outer* o = (Outer*)&outer; + volatile std::uintptr_t sink = (std::uintptr_t)o + (std::uintptr_t)o->nested.shadow; + return (int)sink; +} + // Variables purposely ending with ::h and ::h264 to validate demangled leaf handling int h = 5; int h264 = 7; @@ -40,6 +63,7 @@ int main() { acc += f.bar(i); acc += ns1::add(i, i+1); acc += ns1::add(1.5, 2.5); + acc += ns1::nested_member_probe(i); touch_globals(); std::this_thread::sleep_for(std::chrono::milliseconds(1000)); } diff --git a/e2e-tests/tests/optimized_inline_call_value_execution.rs b/e2e-tests/tests/optimized_inline_call_value_execution.rs index 05945b1..f305e47 100644 --- a/e2e-tests/tests/optimized_inline_call_value_execution.rs +++ b/e2e-tests/tests/optimized_inline_call_value_execution.rs @@ -9,11 +9,6 @@ use std::time::Duration; // Keep this on the first executable line before consume_pair() is called. const INLINE_BEFORE_CALL_TRACE_LINE: u32 = 20; // Keep this on the first executable line after consume_pair() returns. -// This is intentionally a negative regression point: at this PC the inline -// parameters have already fallen out of their own location-list coverage, so -// we only assert that GhostScope does not misreport them as consume_pair's -// argument registers. We do not expect post-call value recovery to work until -// full DW_OP_entry_value + caller-side call-site evaluation is implemented. const INLINE_AFTER_CALL_TRACE_LINE: u32 = 22; async fn spawn_inline_call_value_program( @@ -272,3 +267,71 @@ async fn test_optimized_inline_parameters_survive_internal_call_sites() -> anyho Ok(()) } + +#[tokio::test] +async fn test_entry_value_recovers_outer_parameter_inside_optimized_inline_after_internal_call( +) -> anyhow::Result<()> { + init(); + + let binary_path = FIXTURES.get_test_binary("inline_call_value_program")?; + let mut analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary_path).await?; + let addrs = analyzer.lookup_addresses_by_source_line( + "inline_call_value_program.c", + INLINE_AFTER_CALL_TRACE_LINE, + ); + anyhow::ensure!( + !addrs.is_empty(), + "No DWARF addresses found for inline_call_value_program.c:{INLINE_AFTER_CALL_TRACE_LINE}" + ); + for module_address in &addrs { + anyhow::ensure!( + analyzer.is_inline_at(module_address) == Some(true), + "Expected inline address at 0x{:x}", + module_address.address + ); + } + + let target = spawn_inline_call_value_program(&binary_path).await?; + let script = format!( + "trace inline_call_value_program.c:{INLINE_AFTER_CALL_TRACE_LINE} {{\n print \"POSTCALL:{{}}:{{}}\", seed, after_call;\n}}\n" + ); + let (exit_code, stdout, stderr) = + run_ghostscope_with_script_for_target(&script, 4, &target).await?; + target.terminate().await?; + + if should_skip_for_ebpf_env(exit_code, &stderr) { + return Ok(()); + } + + assert_eq!(exit_code, 0, "stderr={stderr} stdout={stdout}"); + assert!( + !stdout.contains("ExprError"), + "Expected exact entry_value recovery inside the inline body. STDOUT: {stdout}\nSTDERR: {stderr}" + ); + assert!( + !stdout.contains(""), + "Inline post-call entry_value should not be optimized out. STDOUT: {stdout}\nSTDERR: {stderr}" + ); + + let re = Regex::new(r"POSTCALL:([0-9-]+):([0-9-]+)")?; + let mut seen = 0; + for caps in re.captures_iter(&stdout) { + let seed: i64 = caps[1].parse()?; + let after_call: i64 = caps[2].parse()?; + let original_x = seed * 7; + let original_y = seed + 11; + let combined = (original_x + original_y) * (original_x - original_y); + assert_eq!( + after_call, + combined + 7, + "Expected seed/after_call to match wrapper(seed) on the first post-call line. STDOUT: {stdout}" + ); + seen += 1; + } + + assert!( + seen >= 2, + "Expected multiple post-call entry_value events. STDOUT: {stdout}\nSTDERR: {stderr}" + ); + Ok(()) +} diff --git a/ghostscope-dwarf/src/index/block_index.rs b/ghostscope-dwarf/src/index/block_index.rs index fba073a..2f4ec4e 100644 --- a/ghostscope-dwarf/src/index/block_index.rs +++ b/ghostscope-dwarf/src/index/block_index.rs @@ -172,29 +172,9 @@ impl FunctionBlocks { out } - /// Find the nearest caller-side call-site parameter binding whose return_pc - /// is at or before `pc` and whose callee entry register matches `register`. - pub fn entry_value_parameter_for_pc( - &self, - pc: u64, - register: u16, - ) -> Option<&CallSiteParameter> { - for (_, records) in self.call_sites.range(..=pc).rev() { - for record in records.iter().rev() { - if let Some(parameter) = record - .parameters - .iter() - .find(|parameter| parameter.callee_register == register) - { - return Some(parameter); - } - } - } - None - } - /// Collect all incoming caller-side call-site bindings for a callee - /// register. These are used for non-inline DW_OP_entry_value recovery. + /// register. These drive DW_OP_entry_value recovery without consulting + /// nested outgoing call sites inside the current function body. pub fn incoming_entry_value_parameters(&self, register: u16) -> Vec<(u64, &CallSiteParameter)> { let mut out = Vec::new(); for (return_pc, records) in &self.incoming_call_sites { @@ -208,17 +188,6 @@ impl FunctionBlocks { } out } - - /// True when `pc` is inside an inlined-subroutine scope in this function. - pub fn is_inline_context_at(&self, pc: u64) -> bool { - if !self.function_contains_pc(pc) { - return false; - } - self.block_path_for_pc(pc) - .into_iter() - .skip(1) - .any(|idx| self.nodes[idx].entry_pc.is_some()) - } } /// Global per-module block index @@ -1400,58 +1369,4 @@ mod tests { assert_eq!(incoming[0].call_origin, None); assert_eq!(incoming[0].call_target, Some(0x1200)); } - - #[test] - fn entry_value_parameter_lookup_uses_nearest_prior_return_pc() { - let mut function = FunctionBlocks::new(gimli::DebugInfoOffset(0), gimli::UnitOffset(0)); - function.call_sites.insert( - 0x1018, - vec![CallSiteRecord { - cu_offset: gimli::DebugInfoOffset(0), - die_offset: gimli::UnitOffset(1), - return_pc: 0x1018, - call_origin: None, - call_target: None, - parameters: vec![CallSiteParameter { - callee_register: 5, - caller_value_steps: vec![ComputeStep::PushConstant(11)], - }], - }], - ); - function.call_sites.insert( - 0x1030, - vec![CallSiteRecord { - cu_offset: gimli::DebugInfoOffset(0), - die_offset: gimli::UnitOffset(2), - return_pc: 0x1030, - call_origin: None, - call_target: None, - parameters: vec![CallSiteParameter { - callee_register: 5, - caller_value_steps: vec![ComputeStep::PushConstant(22)], - }], - }], - ); - - let parameter = function - .entry_value_parameter_for_pc(0x1034, 5) - .expect("nearest call-site parameter should be found"); - assert_eq!( - parameter.caller_value_steps, - vec![ComputeStep::PushConstant(22)] - ); - - let earlier = function - .entry_value_parameter_for_pc(0x1019, 5) - .expect("earlier call-site parameter should be found"); - assert_eq!( - earlier.caller_value_steps, - vec![ComputeStep::PushConstant(11)] - ); - - assert!( - function.entry_value_parameter_for_pc(0x1017, 5).is_none(), - "call sites after the current PC must not match" - ); - } } diff --git a/ghostscope-dwarf/src/index/cfi_index.rs b/ghostscope-dwarf/src/index/cfi_index.rs index a4630fc..116f7c6 100644 --- a/ghostscope-dwarf/src/index/cfi_index.rs +++ b/ghostscope-dwarf/src/index/cfi_index.rs @@ -154,7 +154,7 @@ impl CfiIndex { use gimli::Reader; let temp = expression.0.to_slice().ok(); let expr_bytes = temp.as_deref().unwrap_or(&[]); - let steps = self.parse_dwarf_expression(expr_bytes)?; + let steps = Self::parse_dwarf_expression(expr_bytes)?; CfaResult::Expression { steps } } }; @@ -355,7 +355,7 @@ impl CfiIndex { let expression = expr.get(&self.eh_frame)?; let temp = expression.0.to_slice().ok(); let expr_bytes = temp.as_deref().unwrap_or(&[]); - self.parse_dwarf_expression(expr_bytes) + Self::parse_dwarf_expression(expr_bytes) } fn default_register_rule(register: u16) -> Option> { @@ -368,7 +368,7 @@ impl CfiIndex { } /// Parse DWARF expression bytes into ComputeStep sequence - fn parse_dwarf_expression(&self, expr_bytes: &[u8]) -> Result> { + fn parse_dwarf_expression(expr_bytes: &[u8]) -> Result> { let mut steps = Vec::new(); let mut pc = 0; @@ -381,7 +381,7 @@ impl CfiIndex { 0x70..=0x8f => { let register = (opcode - 0x70) as u16; // Read SLEB128 offset - let (offset, bytes_read) = self.read_sleb128(&expr_bytes[pc..])?; + let (offset, bytes_read) = Self::read_sleb128(&expr_bytes[pc..])?; pc += bytes_read; steps.push(ComputeStep::LoadRegister(register)); @@ -392,7 +392,7 @@ impl CfiIndex { } // DW_OP_plus_uconst 0x23 => { - let (value, bytes_read) = self.read_uleb128(&expr_bytes[pc..])?; + let (value, bytes_read) = Self::read_uleb128(&expr_bytes[pc..])?; pc += bytes_read; steps.push(ComputeStep::PushConstant(value as i64)); steps.push(ComputeStep::Add); @@ -414,10 +414,15 @@ impl CfiIndex { 0x21 => steps.push(ComputeStep::Or), // DW_OP_xor 0x27 => steps.push(ComputeStep::Xor), + // DW_OP_nop + 0x96 => {} _ => { - debug!("Unhandled DWARF opcode 0x{:02x} in CFA expression", opcode); - // For now, skip unknown opcodes + return Err(anyhow!( + "unsupported DWARF opcode 0x{:02x} in CFA expression at byte offset {}", + opcode, + pc - 1 + )); } } } @@ -426,7 +431,7 @@ impl CfiIndex { } /// Read ULEB128 from byte slice - fn read_uleb128(&self, data: &[u8]) -> Result<(u64, usize)> { + fn read_uleb128(data: &[u8]) -> Result<(u64, usize)> { let mut result = 0u64; let mut shift = 0; let mut bytes_read = 0; @@ -444,7 +449,7 @@ impl CfiIndex { } /// Read SLEB128 from byte slice - fn read_sleb128(&self, data: &[u8]) -> Result<(i64, usize)> { + fn read_sleb128(data: &[u8]) -> Result<(i64, usize)> { let mut result = 0i64; let mut shift = 0; let mut bytes_read = 0; @@ -491,9 +496,22 @@ pub struct CfiStats { #[cfg(test)] mod tests { + use super::CfiIndex; + #[test] fn test_cfi_index_creation() { // This would need a real ELF file for testing // For now, just ensure the module compiles } + + #[test] + fn cfa_expression_rejects_unknown_opcode_after_valid_prefix() { + let error = CfiIndex::parse_dwarf_expression(&[0x70, 0x00, 0xff]) + .expect_err("unknown CFI expression opcode must not be skipped"); + + assert!( + error.to_string().contains("unsupported"), + "unexpected error: {error}" + ); + } } diff --git a/ghostscope-dwarf/src/objfile/access_planner.rs b/ghostscope-dwarf/src/objfile/access_planner.rs index 89aa193..d416ccb 100644 --- a/ghostscope-dwarf/src/objfile/access_planner.rs +++ b/ghostscope-dwarf/src/objfile/access_planner.rs @@ -159,11 +159,15 @@ impl<'dwarf> AccessPlanner<'dwarf> { let header_now2 = self.dwarf.unit_header(current_cu_off)?; let unit_now2 = self.dwarf.unit(header_now2)?; let def_die = unit_now2.entry(def_off)?; - // Scan members for the field - let mut entries = unit_now2.entries_at_offset(def_die.offset())?; - let _ = entries.next_entry()?; // self + // Only direct DW_TAG_member children belong to this aggregate. + // Nested class/struct DIEs may appear under a C++ aggregate, but + // their members are not direct members of the parent type. + let mut tree = unit_now2.entries_tree(Some(def_die.offset()))?; + let root = tree.root()?; + let mut children = root.children(); let mut found_member = false; - while let Some(e) = entries.next_dfs()? { + while let Some(child) = children.next()? { + let e = child.entry(); if e.tag() == gimli::DW_TAG_member { if let Some(attr) = e.attr(gimli::DW_AT_name) { if let Ok(s) = self.dwarf.attr_string(&unit_now2, attr.value()) { diff --git a/ghostscope-dwarf/src/parser/expression_evaluator.rs b/ghostscope-dwarf/src/parser/expression_evaluator.rs index cceaeb5..e063864 100644 --- a/ghostscope-dwarf/src/parser/expression_evaluator.rs +++ b/ghostscope-dwarf/src/parser/expression_evaluator.rs @@ -341,22 +341,36 @@ impl ExpressionEvaluator { let mut has_stack_value = false; // Parse all operations in the expression - while let Ok(op) = Operation::parse(&mut expression.0, encoding) { + while !expression.0.is_empty() { + let offset = expr_bytes.len() - expression.0.len(); + let op = Operation::parse(&mut expression.0, encoding).map_err(|error| { + anyhow::anyhow!( + "failed to parse DWARF expression operation at byte offset {}: {}", + offset, + error + ) + })?; if matches!(op, Operation::StackValue) { has_stack_value = true; debug!("Found DW_OP_stack_value - this is a computed value"); } match &op { - // TODO(entry_value): This is only a minimal fallback. - // For inline parameters, a correct DW_OP_entry_value implementation must - // walk back to the caller and evaluate the matching call_site_parameter's - // DW_AT_call_value / DW_AT_GNU_call_site_value. Merely stripping - // entry_value(reg) to a bare register only preserves the simplest cases - // where the parameter is still equivalent to the entry register. + // Lower supported DW_OP_entry_value forms through caller-side + // call-site metadata. This keeps optimized parameters usable + // after their entry registers have been clobbered. Operation::EntryValue { expression } => { let mut inner = *expression; let mut inner_ops: Vec> = Vec::new(); - while let Ok(iop) = Operation::parse(&mut inner, encoding) { + let inner_len = inner.len(); + while !inner.is_empty() { + let offset = inner_len - inner.len(); + let iop = Operation::parse(&mut inner, encoding).map_err(|error| { + anyhow::anyhow!( + "failed to parse DW_OP_entry_value inner expression operation at byte offset {}: {}", + offset, + error + ) + })?; inner_ops.push(iop); } if inner_ops.len() == 1 { @@ -527,18 +541,34 @@ impl ExpressionEvaluator { // This marks the result as a computed value, not a memory location // Already handled by has_stack_value flag } - ParsedOperation::Operation(Operation::Deref { size, .. }) => { + ParsedOperation::Operation(Operation::Deref { size, space, .. }) => { + if *space { + return Err(anyhow::anyhow!( + "unsupported DWARF expression operation: {:?}", + op + )); + } let mem_size = match size { 1 => MemoryAccessSize::U8, 2 => MemoryAccessSize::U16, 4 => MemoryAccessSize::U32, 8 => MemoryAccessSize::U64, - _ => MemoryAccessSize::U64, // Default + _ => { + return Err(anyhow::anyhow!( + "unsupported DWARF dereference size {} in operation: {:?}", + size, + op + )) + } }; steps.push(ComputeStep::Dereference { size: mem_size }); } + ParsedOperation::Operation(Operation::Nop) => {} _ => { - debug!("Unhandled operation in expression: {:?}", op); + return Err(anyhow::anyhow!( + "unsupported DWARF expression operation: {:?}", + op + )); } } } @@ -1297,18 +1327,6 @@ impl ExpressionEvaluator { let function_context = function_context.ok_or_else(|| { anyhow::anyhow!("DW_OP_entry_value requires function call-site context") })?; - if function_context.is_inline_context_at(current_pc) { - if let Some(parameter) = - function_context.entry_value_parameter_for_pc(current_pc, register) - { - return Self::materialize_caller_value_steps( - ¶meter.caller_value_steps, - current_pc, - cfi_index, - ); - } - } - Self::build_incoming_entry_value_lookup( current_pc, register, @@ -1615,6 +1633,14 @@ mod tests { use gimli::{Format, LittleEndian, Register, RunTimeEndian}; use std::sync::Arc; + fn test_encoding() -> gimli::Encoding { + gimli::Encoding { + format: gimli::Format::Dwarf32, + version: 5, + address_size: 8, + } + } + fn build_scanned_incoming_entry_value_fixture( register: u16, caller_value: u64, @@ -1703,7 +1729,7 @@ mod tests { } #[test] - fn entry_value_uses_nearest_call_site_parameter_steps_in_inline_context() { + fn entry_value_ignores_outgoing_call_sites_in_inline_context() { let mut function = FunctionBlocks { cu_offset: gimli::DebugInfoOffset(0), die_offset: gimli::UnitOffset(0), @@ -1758,15 +1784,20 @@ mod tests { }], ); - let steps = ExpressionEvaluator::resolve_entry_value_register( + let error = ExpressionEvaluator::resolve_entry_value_register( 0x1034, 5, None, Some(&function), None, ) - .expect("entry_value should resolve to the nearest call site"); - assert_eq!(steps, vec![ComputeStep::PushConstant(22)]); + .expect_err("inline entry_value must not reuse nested outgoing call-site bindings"); + assert!( + error + .to_string() + .contains("DW_OP_entry_value register recovery needs CFI"), + "unexpected error: {error}" + ); } #[test] @@ -1956,13 +1987,62 @@ mod tests { ); } + #[test] + fn multi_op_expression_rejects_invalid_opcode_after_valid_prefix() { + let expr_bytes = [ + constants::DW_OP_lit1.0, + 0xff, + constants::DW_OP_stack_value.0, + ]; + + let error = ExpressionEvaluator::parse_expression_with_context( + &expr_bytes, + RunTimeEndian::Little, + test_encoding(), + None, + 0, + None, + None, + None, + 0, + ) + .expect_err("invalid opcode after a valid prefix must not return a value"); + + assert!( + error.to_string().contains("DWARF expression"), + "unexpected error: {error}" + ); + } + + #[test] + fn multi_op_expression_rejects_unsupported_operation_after_valid_prefix() { + let expr_bytes = [ + constants::DW_OP_lit1.0, + constants::DW_OP_drop.0, + constants::DW_OP_stack_value.0, + ]; + + let error = ExpressionEvaluator::parse_expression_with_context( + &expr_bytes, + RunTimeEndian::Little, + test_encoding(), + None, + 0, + None, + None, + None, + 0, + ) + .expect_err("unsupported operation after a valid prefix must not return a value"); + + assert!( + error.to_string().contains("unsupported"), + "unexpected error: {error}" + ); + } + #[test] fn single_fbreg_fast_path_saturates_cfa_offset_addition() { - let encoding = gimli::Encoding { - format: gimli::Format::Dwarf32, - version: 5, - address_size: 8, - }; let get_cfa = |_address| { Ok(Some(CfaResult::RegisterPlusOffset { register: 7, @@ -1973,7 +2053,7 @@ mod tests { let result = ExpressionEvaluator::parse_expression_with_context( &[0x91, 0x0a], RunTimeEndian::Little, - encoding, + test_encoding(), None, 0, Some(&get_cfa), @@ -1995,16 +2075,11 @@ mod tests { #[test] fn big_endian_dw_op_addr_preserves_absolute_address() { - let encoding = gimli::Encoding { - format: gimli::Format::Dwarf32, - version: 5, - address_size: 8, - }; let expr_bytes = [0x03, 0, 0, 0, 0, 0, 0, 0x12, 0x34]; let result = ExpressionEvaluator::parse_expression_with_context( &expr_bytes, gimli::RunTimeEndian::Big, - encoding, + test_encoding(), None, 0, None,