diff --git a/e2e-tests/compile-fixtures.sh b/e2e-tests/compile-fixtures.sh index 9e69b4f5..2f5c7ffa 100755 --- a/e2e-tests/compile-fixtures.sh +++ b/e2e-tests/compile-fixtures.sh @@ -103,7 +103,7 @@ run_make_fixture partitioned_ranges_program all run_make_fixture partitioned_ranges_program \ all \ "CC=${GCC_BIN}" \ - "CFLAGS=-Wall -Wextra -gdwarf-5 -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition" \ + "CFLAGS=-Wall -Wextra -gdwarf-5 -gcolumn-info -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition" \ "BINARY=partitioned_ranges_program_gcc_dwarf5_sections" \ "OBJ=partitioned_ranges_program_gcc_dwarf5_sections.o" run_make_fixture partitioned_ranges_program \ diff --git a/e2e-tests/tests/common/mod.rs b/e2e-tests/tests/common/mod.rs index c7bcc040..a15cb90c 100644 --- a/e2e-tests/tests/common/mod.rs +++ b/e2e-tests/tests/common/mod.rs @@ -1879,7 +1879,7 @@ fn compile_partitioned_ranges_program(compiler: FixtureCompiler) -> anyhow::Resu FixtureCompiler::GccDwarf5FunctionSections => compile_c_make_fixture( "partitioned_ranges_program", compiler, - "-Wall -Wextra -gdwarf-5 -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition", + "-Wall -Wextra -gdwarf-5 -gcolumn-info -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition", ), FixtureCompiler::ClangDwarf5Rnglistx => compile_c_make_fixture( "partitioned_ranges_program", diff --git a/e2e-tests/tests/common_fixture_builds.rs b/e2e-tests/tests/common_fixture_builds.rs index 18a5daf5..12ece06f 100644 --- a/e2e-tests/tests/common_fixture_builds.rs +++ b/e2e-tests/tests/common_fixture_builds.rs @@ -44,7 +44,7 @@ fn compiler_specific_builds_keep_sibling_fixture_outputs() -> anyhow::Result<()> ( "partitioned_ranges_program", FixtureCompiler::GccDwarf5FunctionSections, - "-Wall -Wextra -gdwarf-5 -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition", + "-Wall -Wextra -gdwarf-5 -gcolumn-info -O3 -DNDEBUG -ffunction-sections -freorder-blocks-and-partition", ), ( "partitioned_ranges_program", diff --git a/e2e-tests/tests/dwarf_index_regressions.rs b/e2e-tests/tests/dwarf_index_regressions.rs index 829811a2..e915aaf7 100644 --- a/e2e-tests/tests/dwarf_index_regressions.rs +++ b/e2e-tests/tests/dwarf_index_regressions.rs @@ -9,7 +9,7 @@ use gimli::write::{ use gimli::Reader; use gimli::{Format, LineEncoding, SectionId}; use object::{Object, ObjectSection, ObjectSymbol}; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fs; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; @@ -86,6 +86,34 @@ fn find_symbol_address(binary_path: &std::path::Path, symbol_name: &str) -> anyh }) } +fn find_symbol_range( + binary_path: &std::path::Path, + symbol_name: &str, +) -> anyhow::Result> { + let bytes = std::fs::read(binary_path) + .map_err(|e| anyhow::anyhow!("Failed to read {}: {}", binary_path.display(), e))?; + let file = object::File::parse(&*bytes) + .map_err(|e| anyhow::anyhow!("Failed to parse {}: {}", binary_path.display(), e))?; + + let symbol = file + .symbols() + .find(|symbol| symbol.name().is_ok_and(|name| name == symbol_name)) + .with_context(|| { + format!( + "Symbol '{}' not found in {}", + symbol_name, + binary_path.display() + ) + })?; + anyhow::ensure!( + symbol.size() > 0, + "Symbol '{}' has no size in {}", + symbol_name, + binary_path.display() + ); + Ok(symbol.address()..symbol.address() + symbol.size()) +} + fn assert_native_index_queries( analyzer: &ghostscope_dwarf::DwarfAnalyzer, target: &Path, @@ -1125,6 +1153,47 @@ fn load_dwarf_from_binary(path: &Path) -> anyhow::Result, +) -> anyhow::Result>> { + let dwarf = load_dwarf_from_binary(binary_path)?; + let mut rows_by_address = BTreeMap::>::new(); + let mut units = dwarf.units(); + + while let Some(header) = units.next()? { + let unit = dwarf.unit(header)?; + let Some(ref line_program) = unit.line_program else { + continue; + }; + let (line_program, sequences) = line_program.clone().sequences()?; + + for sequence in sequences { + let mut rows = line_program.resume_from(&sequence); + while let Some((_, row)) = rows.next_row()? { + if row.end_sequence() || !address_range.contains(&row.address()) { + continue; + } + rows_by_address + .entry(row.address()) + .or_default() + .push(LineRowFlags { + is_stmt: row.is_stmt(), + prologue_end: row.prologue_end(), + }); + } + } + } + + Ok(rows_by_address) +} + fn dwarf_has_type_unit(path: &Path) -> anyhow::Result { let dwarf = load_dwarf_from_binary(path)?; let mut units = dwarf.units(); @@ -1708,7 +1777,7 @@ async fn assert_partitioned_ranges_lookup_resolves_primary_entry( binary_path: PathBuf, scenario: &str, ) -> anyhow::Result<()> { - let hot_addr = find_symbol_address(&binary_path, "partitioned_target")?; + let hot_range = find_symbol_range(&binary_path, "partitioned_target")?; let analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary_path).await?; let addrs = analyzer.lookup_function_addresses("partitioned_target"); @@ -1721,9 +1790,11 @@ async fn assert_partitioned_ranges_lookup_resolves_primary_entry( addrs[0].module_path, binary_path, "Resolved module should point at the partitioned fixture for {scenario}" ); - assert_eq!( - addrs[0].address, hot_addr, - "lookup_function_addresses should resolve to the primary entry address for {scenario}" + assert!( + hot_range.contains(&addrs[0].address), + "lookup_function_addresses should resolve inside the primary/hot range for {scenario}. \ + Hot range: {hot_range:x?}, result: {:?}", + addrs[0] ); Ok(()) @@ -2091,7 +2162,7 @@ async fn test_partitioned_ranges_lookup_prefers_hot_entry_over_cold_partition() init(); let binary_path = FIXTURES.get_test_binary("partitioned_ranges_program")?; - let hot_addr = find_symbol_address(&binary_path, "partitioned_target")?; + let hot_range = find_symbol_range(&binary_path, "partitioned_target")?; let cold_addr = find_symbol_address(&binary_path, "partitioned_target.cold")?; let analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary_path).await?; @@ -2106,9 +2177,11 @@ async fn test_partitioned_ranges_lookup_prefers_hot_entry_over_cold_partition() addrs[0].module_path, binary_path, "Resolved module should point at the partitioned fixture" ); - assert_eq!( - addrs[0].address, hot_addr, - "lookup_function_addresses should resolve to the entry/hot range" + assert!( + hot_range.contains(&addrs[0].address), + "lookup_function_addresses should resolve inside the hot range. \ + Hot range: {hot_range:x?}, result: {:?}", + addrs[0] ); assert_ne!( addrs[0].address, cold_addr, @@ -2118,6 +2191,58 @@ async fn test_partitioned_ranges_lookup_prefers_hot_entry_over_cold_partition() Ok(()) } +#[tokio::test] +async fn test_gcc_o3_duplicate_pc_rows_preserve_first_statement_address() -> anyhow::Result<()> { + init(); + + if !fixture_compiler_available(FixtureCompiler::GccDwarf5FunctionSections) { + eprintln!("Skipping GCC O3 line-row regression: gcc is unavailable"); + return Ok(()); + } + + let binary_path = FIXTURES.get_test_binary_with_compiler( + "partitioned_ranges_program", + FixtureCompiler::GccDwarf5FunctionSections, + )?; + let main_range = find_symbol_range(&binary_path, "main")?; + let rows_by_address = line_row_flags_in_range(&binary_path, main_range.clone())?; + + assert!( + rows_by_address + .values() + .flatten() + .all(|row| !row.prologue_end), + "fixture should exercise the is_stmt fallback without DW_LNS_set_prologue_end" + ); + + let (&expected_address, expected_rows) = rows_by_address + .range(main_range.start.saturating_add(1)..main_range.end) + .find(|(_, rows)| rows.iter().any(|row| row.is_stmt)) + .context("main has no statement row after its entry address")?; + assert!( + expected_rows.len() > 1 + && expected_rows.iter().any(|row| row.is_stmt) + && !expected_rows.last().is_some_and(|row| row.is_stmt), + "fixture must keep an is_stmt row hidden before a non-statement row at the same PC: \ + address=0x{expected_address:x}, rows={expected_rows:?}" + ); + + let analyzer = ghostscope_dwarf::DwarfAnalyzer::from_exec_path(&binary_path).await?; + let addrs = analyzer.lookup_function_addresses("main"); + assert_eq!( + addrs.len(), + 1, + "Expected a single resolved address for main. Results: {addrs:?}" + ); + assert_eq!(addrs[0].module_path, binary_path); + assert_eq!( + addrs[0].address, expected_address, + "function lookup must consider every line row at a PC when selecting the first statement" + ); + + Ok(()) +} + #[tokio::test] async fn test_partitioned_ranges_gcc_dwarf5_function_sections_preserve_offset_ranges( ) -> anyhow::Result<()> { diff --git a/ghostscope-dwarf/src/index/line_mapping.rs b/ghostscope-dwarf/src/index/line_mapping.rs index d63c5f89..fbef0eed 100644 --- a/ghostscope-dwarf/src/index/line_mapping.rs +++ b/ghostscope-dwarf/src/index/line_mapping.rs @@ -136,7 +136,7 @@ impl LineMappingTable { } // Stable sorting preserves insertion order for duplicate-address rows, - // including the "last row is representative" behavior. + // including the source-location lookup's preference for the last row. entries.sort_by_key(|entry| entry.address); let (address_group_addresses, address_group_starts, address_group_prefix_max_ends) = Self::build_address_groups(&entries); @@ -227,16 +227,18 @@ impl LineMappingTable { .chain(self.incremental_entries.get(&address).into_iter().flatten()) } - fn representative_entries_from( - &self, - address: u64, - inclusive: bool, - ) -> impl Iterator { - let mut compact_group = self.address_group_addresses.partition_point(|&candidate| { + /// Iterate every row in address order, including duplicate-PC rows. + /// + /// Statement and prologue markers belong to individual DWARF line rows. + /// GCC column information commonly emits a marked row followed by an + /// unmarked row at the same PC, so choosing one representative row would + /// discard the marker. + fn entries_from(&self, address: u64, inclusive: bool) -> impl Iterator { + let compact_start = self.entries.partition_point(|entry| { if inclusive { - candidate < address + entry.address < address } else { - candidate <= address + entry.address <= address } }); let lower_bound = if inclusive { @@ -244,48 +246,24 @@ impl LineMappingTable { } else { Bound::Excluded(address) }; + let mut compact = self.entries[compact_start..].iter().peekable(); let mut incremental = self .incremental_entries .range((lower_bound, Bound::Unbounded)) + .flat_map(|(_, entries)| entries) .peekable(); - std::iter::from_fn(move || loop { - let compact_address = self.address_group_addresses.get(compact_group).copied(); - let incremental_address = incremental.peek().map(|(address, _)| **address); - - match (compact_address, incremental_address) { - (Some(compact_address), Some(incremental_address)) - if compact_address < incremental_address => - { - let entries = self.group_entries(compact_group); - compact_group += 1; - if let Some(entry) = Self::representative_entry(entries) { - return Some((compact_address, entry)); - } - } - (Some(compact_address), Some(incremental_address)) - if compact_address == incremental_address => - { - compact_group += 1; - let (_, entries) = incremental.next().expect("peeked incremental line group"); - if let Some(entry) = Self::representative_entry(entries) { - return Some((incremental_address, entry)); - } - } - (_, Some(incremental_address)) => { - let (_, entries) = incremental.next().expect("peeked incremental line group"); - if let Some(entry) = Self::representative_entry(entries) { - return Some((incremental_address, entry)); - } - } - (Some(compact_address), None) => { - let entries = self.group_entries(compact_group); - compact_group += 1; - if let Some(entry) = Self::representative_entry(entries) { - return Some((compact_address, entry)); - } - } + std::iter::from_fn(move || { + let take_compact = match (compact.peek(), incremental.peek()) { + (Some(compact), Some(incremental)) => compact.address <= incremental.address, + (Some(_), None) => true, + (None, Some(_)) => false, (None, None) => return None, + }; + if take_compact { + compact.next() + } else { + incremental.next() } }) } @@ -308,7 +286,7 @@ impl LineMappingTable { (compact_entries.peek(), incremental_entries.peek()) { // Rows already present precede later lazy additions at duplicate - // addresses, preserving the representative-row ordering. + // addresses, preserving duplicate-row ordering. if compact.address <= incremental.address { merged.push(compact_entries.next().expect("peeked compact line entry")); } else { @@ -382,10 +360,6 @@ impl LineMappingTable { } } - fn representative_entry(entries: &[LineEntry]) -> Option<&LineEntry> { - entries.last() - } - fn active_representative_entry(entries: &[LineEntry], address: u64) -> Option<&LineEntry> { entries .iter() @@ -708,14 +682,24 @@ impl LineMappingTable { /// Find the first executable instruction address after function prologue /// Assumes the input address is a real function (not inlined) /// Returns the best breakpoint location for the function - pub fn find_first_executable_address(&self, function_start: u64) -> u64 { + pub fn find_first_executable_address(&self, function_start: u64, function_end: u64) -> u64 { tracing::debug!( - "LineMappingTable: finding first executable address for function at 0x{:x}", - function_start + "LineMappingTable: finding first executable address for function range [0x{:x}, 0x{:x})", + function_start, + function_end ); + if function_start >= function_end { + tracing::debug!( + "LineMappingTable: invalid or empty function range [0x{:x}, 0x{:x}), using its start", + function_start, + function_end + ); + return function_start; + } + // 1. Try DWARF prologue_end flag first - if let Some(addr) = self.find_prologue_end_from_dwarf(function_start) { + if let Some(addr) = self.find_prologue_end_from_dwarf(function_start, function_end) { tracing::info!( "LineMappingTable: found prologue_end at 0x{:x} (offset +{})", addr, @@ -725,7 +709,7 @@ impl LineMappingTable { } // 2. Fall back to is_stmt=true search - if let Some(addr) = self.find_next_stmt_address(function_start) { + if let Some(addr) = self.find_next_stmt_address(function_start, function_end) { tracing::info!( "LineMappingTable: using is_stmt=true address at 0x{:x} (offset +{})", addr, @@ -743,27 +727,31 @@ impl LineMappingTable { } /// Find prologue end using DWARF prologue_end flag - fn find_prologue_end_from_dwarf(&self, function_start: u64) -> Option { + fn find_prologue_end_from_dwarf(&self, function_start: u64, function_end: u64) -> Option { tracing::debug!( "LineMappingTable: searching for prologue_end=true after 0x{:x}", function_start ); - // Iterate through addresses starting from function_start - for (address, entry) in self.representative_entries_from(function_start, true) { + // A neighboring function can begin immediately after this range. Do + // not borrow its prologue marker when this function has none. + for entry in self + .entries_from(function_start, true) + .take_while(|entry| entry.address < function_end) + { if entry.prologue_end { tracing::debug!( "LineMappingTable: found prologue_end=true at 0x{:x} (line {}, file {})", - address, + entry.address, entry.line, entry.file_path ); tracing::debug!( "LineMappingTable: found prologue_end at 0x{:x} (offset +{})", - address, - address - function_start + entry.address, + entry.address - function_start ); - return Some(address); + return Some(entry.address); } } @@ -776,28 +764,32 @@ impl LineMappingTable { /// This is used for prologue detection following GDB's approach /// Find the next is_stmt=true address after the given function start address - fn find_next_stmt_address(&self, function_start: u64) -> Option { + fn find_next_stmt_address(&self, function_start: u64, function_end: u64) -> Option { tracing::debug!( "LineMappingTable: searching for next is_stmt=true address after 0x{:x}", function_start ); - // Look for the first is_stmt=true address after function_start - for (address, entry) in self.representative_entries_from(function_start, false) { + // Look for the first is_stmt=true address after function_start, but + // never cross into a neighboring function. + for entry in self + .entries_from(function_start, false) + .take_while(|entry| entry.address < function_end) + { if entry.is_stmt { tracing::debug!( "LineMappingTable: found is_stmt=true at 0x{:x} (line {}, file {})", - address, + entry.address, entry.line, entry.file_path ); - return Some(address); + return Some(entry.address); } else { // Extra diagnostics to understand why we didn't pick nearer addresses tracing::debug!( "LineMappingTable: skipping non-is_stmt at 0x{:x} (offset +{}, line {}, file {}, prologue_end={})", - address, - address.saturating_sub(function_start), + entry.address, + entry.address.saturating_sub(function_start), entry.line, entry.file_path, entry.prologue_end @@ -1206,6 +1198,40 @@ mod tests { assert_eq!(table.lookup_line(0x1fc0).map(|entry| entry.line), Some(63)); } + #[test] + fn prologue_search_checks_every_row_at_same_address() { + let scoped = crate::index::ScopedFileIndexManager::new(); + let mut prologue_end = line_entry(0x1010, "/src/main.c", 11, true); + prologue_end.prologue_end = true; + let table = LineMappingTable::from_entries_with_scoped_manager( + vec![ + line_entry(0x1000, "/src/main.c", 10, false), + prologue_end, + line_entry(0x1010, "/src/main.c", 11, false), + line_entry(0x1020, "/src/main.c", 12, true), + ], + &scoped, + ); + + assert_eq!(table.find_first_executable_address(0x1000, 0x1030), 0x1010); + } + + #[test] + fn statement_search_checks_every_row_at_same_address() { + let scoped = crate::index::ScopedFileIndexManager::new(); + let table = LineMappingTable::from_entries_with_scoped_manager( + vec![ + line_entry(0x1000, "/src/main.c", 10, false), + line_entry(0x1010, "/src/main.c", 11, true), + line_entry(0x1010, "/src/main.c", 11, false), + line_entry(0x1020, "/src/main.c", 12, true), + ], + &scoped, + ); + + assert_eq!(table.find_first_executable_address(0x1000, 0x1030), 0x1010); + } + #[test] fn prologue_search_merges_compact_and_incremental_addresses() { let scoped = crate::index::ScopedFileIndexManager::new(); @@ -1225,6 +1251,24 @@ mod tests { &scoped, )); - assert_eq!(table.find_first_executable_address(0x1000), 0x2000); + assert_eq!(table.find_first_executable_address(0x1000, 0x4000), 0x2000); + } + + #[test] + fn prologue_search_does_not_cross_the_function_range() { + let scoped = crate::index::ScopedFileIndexManager::new(); + let mut next_function_prologue = line_entry(0x1100, "/src/main.c", 20, true); + next_function_prologue.prologue_end = true; + let table = LineMappingTable::from_entries_with_scoped_manager( + vec![ + line_entry(0x1000, "/src/main.c", 10, false), + line_entry(0x1050, "/src/main.c", 11, false), + next_function_prologue, + ], + &scoped, + ); + + assert_eq!(table.find_first_executable_address(0x1000, 0x1100), 0x1000); + assert_eq!(table.find_first_executable_address(0x1100, 0x1100), 0x1100); } } diff --git a/ghostscope-dwarf/src/objfile/function_lookup.rs b/ghostscope-dwarf/src/objfile/function_lookup.rs index 4aca8703..876696ea 100644 --- a/ghostscope-dwarf/src/objfile/function_lookup.rs +++ b/ghostscope-dwarf/src/objfile/function_lookup.rs @@ -462,7 +462,7 @@ impl LoadedObjfile { .line_mapping .read() .expect("line mapping lock poisoned") - .find_first_executable_address(*start); + .find_first_executable_address(*start, *end); Self::selected_non_inline_probe_address(*start, *end, first_exec) }; let prefer_entry = self