diff --git a/e2e-tests/tests/dwarf_index_regressions.rs b/e2e-tests/tests/dwarf_index_regressions.rs index 9a0ee345..c1ff8b28 100644 --- a/e2e-tests/tests/dwarf_index_regressions.rs +++ b/e2e-tests/tests/dwarf_index_regressions.rs @@ -359,6 +359,114 @@ fn test_read_uleb128_rejects_values_that_overflow_u64() { ); } +#[tokio::test] +async fn test_dwarf_lookup_errors_do_not_fall_back_to_globals() -> anyhow::Result<()> { + init(); + let Some(cc) = preferred_c_compiler() else { + eprintln!("Skipping DWARF lookup regression: no C compiler is available"); + return Ok(()); + }; + let dir = tempfile::tempdir()?; + let source = dir.path().join("lookup.c"); + let binary = dir.path().join("lookup"); + fs::write( + &source, + "int state = 11;\n\ + struct { int field; } cfg = {13};\n\ + int probe(void) { return state + cfg.field; }\n\ + int local_probe(int state) { return state; }\n\ + int main(void) { return probe() + local_probe(7); }\n", + )?; + run_command( + StdCommand::new(cc) + .args(["-g", "-O0"]) + .arg(&source) + .arg("-o") + .arg(&binary), + "compile DWARF lookup fixture", + )?; + + let analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary).await?; + let probe = analyzer.lookup_function_addresses("probe").remove(0); + let probe_context = analyzer.resolve_pc(&probe)?; + assert!(analyzer + .plan_variable_by_name(&probe_context, "state")? + .is_none()); + let local_probe = analyzer.lookup_function_addresses("local_probe").remove(0); + let local_context = analyzer.resolve_pc(&local_probe)?; + assert!(analyzer + .plan_variable_by_name(&local_context, "state")? + .is_some()); + + // The CRT entry point is executable but has no local DWARF scope in this + // fixture. Its failed local query must not bind the file-scope `state`. + let bytes = fs::read(&binary)?; + let object = object::File::parse(bytes.as_slice())?; + let entry_pc = object + .symbols() + .find(|symbol| symbol.name() == Ok("_start")) + .context("fixture has no CRT entry point")? + .address(); + let entry = ghostscope_dwarf::ModuleAddress::new(binary.clone(), entry_pc); + let entry_context = analyzer.resolve_pc(&entry)?; + let error = analyzer + .plan_variable_by_name(&entry_context, "state") + .expect_err("a failed scope query must not report a missing binding"); + assert!(matches!( + error, + ghostscope_dwarf::VariableLookupError::QueryFailed(_) + )); + + let options = ghostscope_compiler::CompileOptions { + binary_path_hint: Some(binary.to_string_lossy().into_owned()), + ..Default::default() + }; + for (expression, base, path) in [ + ( + "state", + "state", + ghostscope_dwarf::VariableAccessPath::default(), + ), + ( + "cfg.field", + "cfg", + ghostscope_dwarf::VariableAccessPath::fields(["field"]), + ), + ] { + // Prove that a global fallback would succeed, so the failure below + // specifically exercises the compiler's local-query error boundary. + assert!(analyzer + .plan_global_access_read_plan_at_address(&entry, base, &path)? + .is_some()); + + let valid = ghostscope_compiler::compile_script( + &format!("trace probe {{ print {expression}; }}"), + &analyzer, + None, + Some(1), + &options, + )?; + assert_eq!(valid.uprobe_configs.len(), 1); + assert!(valid.failed_targets.is_empty()); + + let invalid = ghostscope_compiler::compile_script( + &format!("trace 0x{entry_pc:x} {{ print {expression}; }}"), + &analyzer, + None, + Some(1), + &options, + ) + .expect_err("a local-query failure must reject scalar and member global fallback"); + assert!( + invalid + .to_string() + .contains("StrictIndex: no function found"), + "{expression}: {invalid}" + ); + } + Ok(()) +} + #[tokio::test] #[serial_test::serial] async fn test_gnu_pubnames_resolve_symbols_lazily_and_reject_corruption() -> anyhow::Result<()> { diff --git a/ghostscope-compiler/src/ebpf/dwarf_bridge.rs b/ghostscope-compiler/src/ebpf/dwarf_bridge.rs index 58af23cd..a2af60f4 100644 --- a/ghostscope-compiler/src/ebpf/dwarf_bridge.rs +++ b/ghostscope-compiler/src/ebpf/dwarf_bridge.rs @@ -1481,46 +1481,18 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { let module_address = ghostscope_dwarf::ModuleAddress::new(prefer_module.clone(), pc_address); - let pc_plan = match analyzer.resolve_pc(&module_address) { - Ok(pc_context) => match analyzer.plan_variable_by_name(&pc_context, var_name) { - Ok(Some(plan)) => { - debug!("Found DWARF variable '{}' via PC variable plan", var_name); - Some(plan) - } - Ok(None) => { - debug!( - "Variable '{}' not found in PC variable plan; trying global read plan", - var_name - ); - None - } - Err(err) => { - let message = err.to_string(); - if message.starts_with("Ambiguous variable") - || message.starts_with("Unavailable variable") - { - return Err(CodeGenError::DwarfError(message)); - } - debug!( - "PC variable plan lookup error for '{}': {message}; trying global read plan", - var_name - ); - None - } - }, - Err(err) => { - debug!( - "PC context resolution failed for '{}': {err}; trying global read plan", - var_name - ); - None - } - }; - - if pc_plan.is_some() { - return Ok(pc_plan); + let pc_context = analyzer + .resolve_pc(&module_address) + .map_err(|err| CodeGenError::DwarfError(err.to_string()))?; + if let Some(plan) = analyzer + .plan_variable_by_name(&pc_context, var_name) + .map_err(|err| CodeGenError::DwarfError(err.to_string()))? + { + debug!("Found DWARF variable '{}' via PC variable plan", var_name); + return Ok(Some(plan)); } + // Only a successful local lookup with no binding permits global fallback. if let Some((_global_module, plan)) = analyzer .plan_global_access_read_plan_at_address( &module_address, @@ -1587,28 +1559,15 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { let module_address = ghostscope_dwarf::ModuleAddress::new(prefer_module.clone(), pc_address); - match analyzer.resolve_pc(&module_address) { - Ok(pc_context) => { - match analyzer.plan_variable_access_by_name(&pc_context, base_name, access_path) { - Ok(Some(plan)) => { - debug!("Found DWARF access '{path_text}' via PC variable access plan"); - return Ok(Some(plan)); - } - Ok(None) => {} - Err(err) => { - let message = err.to_string(); - debug!( - "PC variable access plan lookup failed for '{path_text}': {message}" - ); - return Err(CodeGenError::DwarfError(message)); - } - } - } - Err(err) => { - debug!( - "PC context resolution failed for '{path_text}': {err}; trying global read plan" - ); - } + let pc_context = analyzer + .resolve_pc(&module_address) + .map_err(|err| CodeGenError::DwarfError(err.to_string()))?; + if let Some(plan) = analyzer + .plan_variable_access_by_name(&pc_context, base_name, access_path) + .map_err(|err| CodeGenError::DwarfError(err.to_string()))? + { + debug!("Found DWARF access '{path_text}' via PC variable access plan"); + return Ok(Some(plan)); } if let Some((_module_path, plan)) = analyzer diff --git a/ghostscope-dwarf/src/analyzer/plan_pc.rs b/ghostscope-dwarf/src/analyzer/plan_pc.rs index d05d7c4c..94dcb4cc 100644 --- a/ghostscope-dwarf/src/analyzer/plan_pc.rs +++ b/ghostscope-dwarf/src/analyzer/plan_pc.rs @@ -3,7 +3,8 @@ use crate::{ core::{demangle::RustSymbolHashDisplay, ModuleAddress, Provenance, Result}, semantics::{ AddressSpaceInfo, FunctionParameter, PcContext, PcLineInfo, PcRange, VariableAccessPath, - VariableAccessSegment, VariableReadPlan, VisibleVariable, VisibleVariablesResult, + VariableAccessSegment, VariableLookupError, VariableReadPlan, VisibleVariable, + VisibleVariablesResult, }, }; use std::path::Path; @@ -240,11 +241,13 @@ impl DwarfAnalyzer { /// Plan a visible variable by source name at a previously resolved PC context. /// /// Exact names are preferred over producer-synthesized names like `name@...`. + /// `Ok(None)` means no local binding was found and allows a global lookup. + /// Unavailable or ambiguous bindings and failed queries must not fall back. pub fn plan_variable_by_name( &self, ctx: &PcContext, name: &str, - ) -> Result> { + ) -> std::result::Result, VariableLookupError> { let VisibleVariablesResult { variables: visible_variables, diagnostics, @@ -271,7 +274,7 @@ impl DwarfAnalyzer { name: &str, visible_variables: Vec, diagnostics: &[crate::semantics::VariableQueryDiagnostic], - ) -> Result> { + ) -> std::result::Result, VariableLookupError> { let synthesized_prefix = format!("{name}@"); let matching_diagnostics = diagnostics .iter() @@ -302,11 +305,11 @@ impl DwarfAnalyzer { .iter() .max_by_key(|diagnostic| diagnostic.scope_depth) { - return Err(anyhow::anyhow!( - "Unavailable variable '{name}' at PC 0x{:x}: {}", + return Err(VariableLookupError::Unavailable { + name: name.to_string(), pc, - diagnostic.detail - )); + detail: diagnostic.detail.clone(), + }); } return Ok(None); } @@ -321,11 +324,11 @@ impl DwarfAnalyzer { .filter(|diagnostic| diagnostic.scope_depth > max_scope_depth) .max_by_key(|diagnostic| diagnostic.scope_depth) { - return Err(anyhow::anyhow!( - "Unavailable variable '{name}' at PC 0x{:x}: {}", + return Err(VariableLookupError::Unavailable { + name: name.to_string(), pc, - diagnostic.detail - )); + detail: diagnostic.detail.clone(), + }); } candidates.retain(|variable| variable.scope_depth == max_scope_depth); @@ -335,14 +338,14 @@ impl DwarfAnalyzer { candidates.dedup(); if candidates.len() > 1 { - let names = candidates - .iter() - .map(|variable| variable.name.as_str()) - .collect::>() - .join(", "); - return Err(anyhow::anyhow!( - "Ambiguous variable '{name}' at PC 0x{pc:x}: candidates [{names}]" - )); + return Err(VariableLookupError::Ambiguous { + name: name.to_string(), + pc, + candidates: candidates + .into_iter() + .map(|variable| variable.name) + .collect(), + }); } Ok(candidates.into_iter().next()) diff --git a/ghostscope-dwarf/src/analyzer/tests.rs b/ghostscope-dwarf/src/analyzer/tests.rs index 37280c9f..190ea9e6 100644 --- a/ghostscope-dwarf/src/analyzer/tests.rs +++ b/ghostscope-dwarf/src/analyzer/tests.rs @@ -1,7 +1,7 @@ use super::DwarfAnalyzer; use crate::{ core::{Availability, VariableLocation}, - semantics::VisibleVariable, + semantics::{VariableLookupError, VisibleVariable}, RuntimeTextSymbol, }; use std::path::{Path, PathBuf}; @@ -62,8 +62,58 @@ fn variable_selection_rejects_inner_diagnostic_over_outer_match() { ) .expect_err("inner unavailable variable should block outer fallback"); - assert!(err.to_string().contains("Unavailable variable 'state'")); - assert!(err.to_string().contains("DW_OP_bad is unsupported")); + assert!(matches!( + err, + VariableLookupError::Unavailable { name, pc: 0x1234, detail } + if name == "state" && detail == "DW_OP_bad is unsupported" + )); +} + +#[test] +fn variable_selection_rejects_unavailable_binding_without_an_outer_match() { + let err = DwarfAnalyzer::select_visible_variable_by_name( + 0x1234, + "state", + Vec::new(), + &[diagnostic("state", 2, "location cannot be evaluated")], + ) + .expect_err("an unavailable binding must not be treated as absent"); + + assert!(matches!( + err, + VariableLookupError::Unavailable { name, pc: 0x1234, detail } + if name == "state" && detail == "location cannot be evaluated" + )); +} + +#[test] +fn variable_selection_reports_ambiguous_bindings() { + let err = DwarfAnalyzer::select_visible_variable_by_name( + 0x1234, + "state", + vec![visible_var("state@one", 1), visible_var("state@two", 1)], + &[], + ) + .expect_err("multiple visible bindings must not permit global fallback"); + + assert!(matches!( + err, + VariableLookupError::Ambiguous { name, pc: 0x1234, candidates } + if name == "state" && candidates == ["state@one", "state@two"] + )); +} + +#[test] +fn variable_selection_returns_none_for_a_missing_binding() { + let selected = DwarfAnalyzer::select_visible_variable_by_name( + 0x1234, + "state", + vec![visible_var("other", 1)], + &[diagnostic("unrelated", 2, "location cannot be evaluated")], + ) + .expect("unrelated bindings and diagnostics must not block global lookup"); + + assert!(selected.is_none()); } #[test] diff --git a/ghostscope-dwarf/src/lib.rs b/ghostscope-dwarf/src/lib.rs index e4106432..fee1d43f 100644 --- a/ghostscope-dwarf/src/lib.rs +++ b/ghostscope-dwarf/src/lib.rs @@ -61,10 +61,10 @@ pub use semantics::{ ValueAdapterReport, ValueAdapterStage, ValueCapturePlan, ValueHashTableField, ValueNestedFieldPlan, ValueNestedHashTableFieldPlan, ValueNestedPlan, ValueNestedVariantCondition, ValueNestedVariantFieldPlan, ValueReadPlan, ValueReadPlanOptions, - VariableAccessPath, VariableAccessSegment, VariableLoweringKind, VariableLoweringPlan, - VariableMaterialization, VariableMaterializationPlan, VariablePlan, VariableQueryDiagnostic, - VariableReadPlan, VisibleVariable, VisibleVariablesResult, DEFAULT_VALUE_ADAPTER_NESTING_DEPTH, - MAX_VALUE_ADAPTER_NESTING_DEPTH, + VariableAccessPath, VariableAccessSegment, VariableLookupError, VariableLoweringKind, + VariableLoweringPlan, VariableMaterialization, VariableMaterializationPlan, VariablePlan, + VariableQueryDiagnostic, VariableReadPlan, VisibleVariable, VisibleVariablesResult, + DEFAULT_VALUE_ADAPTER_NESTING_DEPTH, MAX_VALUE_ADAPTER_NESTING_DEPTH, }; pub use semantics::{ diff --git a/ghostscope-dwarf/src/semantics/mod.rs b/ghostscope-dwarf/src/semantics/mod.rs index 7306b3e7..29a763dd 100644 --- a/ghostscope-dwarf/src/semantics/mod.rs +++ b/ghostscope-dwarf/src/semantics/mod.rs @@ -52,7 +52,7 @@ pub(crate) use variable_plan::PlanError; pub use variable_plan::{ AddressOrigin, LvalueAddressPlan, PlannedAddress, PlannedAddressKind, PlannedValue, RuntimeComputedExpr, RuntimeComputedKind, VariableAccessPath, VariableAccessSegment, - VariableLoweringKind, VariableLoweringPlan, VariableMaterialization, + VariableLookupError, VariableLoweringKind, VariableLoweringPlan, VariableMaterialization, VariableMaterializationPlan, VariablePlan, VariableQueryDiagnostic, VariableReadPlan, VisibleVariable, VisibleVariablesResult, }; diff --git a/ghostscope-dwarf/src/semantics/variable_plan/mod.rs b/ghostscope-dwarf/src/semantics/variable_plan/mod.rs index 33e35c46..be65af4b 100644 --- a/ghostscope-dwarf/src/semantics/variable_plan/mod.rs +++ b/ghostscope-dwarf/src/semantics/variable_plan/mod.rs @@ -26,6 +26,29 @@ pub struct VisibleVariable { pub is_artificial: bool, } +/// Failure to establish which local variable a source name denotes at a PC. +/// Only a successful lookup returning `None` permits global-name fallback. +#[derive(Debug, thiserror::Error)] +pub enum VariableLookupError { + #[error("Unavailable variable '{name}' at PC 0x{pc:x}: {detail}")] + Unavailable { + name: String, + pc: u64, + detail: String, + }, + #[error( + "Ambiguous variable '{name}' at PC 0x{pc:x}: candidates [{candidates}]", + candidates = .candidates.join(", ") + )] + Ambiguous { + name: String, + pc: u64, + candidates: Vec, + }, + #[error(transparent)] + QueryFailed(#[from] anyhow::Error), +} + /// Diagnostic produced while answering a PC-sensitive variable query. #[derive(Debug, Clone, PartialEq)] pub struct VariableQueryDiagnostic {