From fefa4d54b8afb7aa6b7902a9101f7aea4f152004 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Mon, 14 Sep 2026 10:33:10 +0200 Subject: [PATCH 01/18] Add DWARF/ELF debug info backend for UEFI/SMM source coverage (M1) Introduces a DebugInfoModule trait (src/traits/mod.rs) unifying the existing Windows PDB backend and a new DwarfModule (src/dwarf/mod.rs), so callers can resolve symbols/lines for a loaded module into the same SymbolInfo/LineInfo interval-tree shape regardless of whether its debug info came from a PDB or from DWARF embedded in an ELF sidecar (as produced by the EDK2 GCC5 toolchain for UEFI/SMM BIOS modules). - traits::DebugInfoModule: fn intervals(&mut self, &SourceCache) -> Result>>, implemented for Module/ProcessModule by delegating to their existing inherent intervals() methods (no behavior change), and for the new DwarfModule. - dwarf::DwarfModule<'data>: takes an already-parsed object::File plus a runtime base address; hand-rolls a gimli DIE/line-program walk (no addr2line -- its point-lookup-oriented API doesn't obviously support the proactive "enumerate every function/line" use case this needs, which is left as an open question) to find every DW_TAG_subprogram and its lines, keyed by [base + link_addr, base + link_addr + size). Runtime address translation is a flat `base + addr` addition, same as the PDB backend's `base + rva`. - SourceCache::lookup_dwarf: mirrors lookup_pdb, trying a DWARF5 DW_LNCT_MD5 file checksum first and falling back to the existing format-agnostic lookup_file_name_components. - Added gimli/object dependencies (object chosen over stretching goblin, which is only used elsewhere for PE-specific parsing; object is gimli's standard companion crate for ELF). Out of scope for this milestone (by design): wiring up a caller (Simics-side UEFI/SMM module discovery and the SMM trigger question), the test fixture, and performance at scale. Co-Authored-By: Claude Sonnet 5 --- Cargo.toml | 2 + src/dwarf/mod.rs | 362 +++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/os/windows/debug_info.rs | 20 +- src/source_cov/mod.rs | 21 ++ src/traits/mod.rs | 23 ++- 6 files changed, 427 insertions(+), 2 deletions(-) create mode 100644 src/dwarf/mod.rs diff --git a/Cargo.toml b/Cargo.toml index 6e6c87b3..6ce0123a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,6 +103,8 @@ sha2 = "0.10.8" typed-path = "0.9.0" thiserror = "1.0.63" lcov2 = "0.1.0" +gimli = "0.34.0" +object = "0.40.0" [dev-dependencies] simics-test = "0.2.6" diff --git a/src/dwarf/mod.rs b/src/dwarf/mod.rs new file mode 100644 index 00000000..95e5894f --- /dev/null +++ b/src/dwarf/mod.rs @@ -0,0 +1,362 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +//! DWARF/ELF debug info backend. +//! +//! UEFI/SMM BIOS modules built with the EDK2 GCC5 toolchain ship debug info as DWARF +//! embedded in an ELF `.debug` sidecar file, unlike Windows kernel/PE modules, which +//! ship Microsoft PDB (see `crate::os::windows::debug_info`). [`DwarfModule`] parses +//! that DWARF/ELF debug info and implements the shared [`DebugInfoModule`] trait +//! (`crate::traits::DebugInfoModule`), so callers get the exact same +//! [`SymbolInfo`]/[`LineInfo`] output regardless of whether a module's debug info +//! came from a PDB or from DWARF/ELF. +//! +//! # Address translation +//! +//! DWARF line-table and DIE addresses in these per-module ELF files are link-time +//! addresses relative to the module's own image (the linked `.text` VMA is commonly a +//! small non-zero value such as `0x240`, not `0`). The runtime address for any DWARF +//! address `a` in a module loaded at `base` (its EDK2 `ImageBase`) is simply +//! `base + a` -- a flat addition, exactly analogous to the PDB backend's +//! `self.base + rva`. No special-case subtraction of a link base is needed. +//! +//! # Open questions / explicitly out of scope for this milestone +//! +//! - `addr2line` (gimli's usual companion crate for point address lookups) is +//! intentionally not used here. Whether it's viable for *proactive* enumeration of +//! every function/line (needed to seed coverage denominators, mirroring what +//! `Module`/`ProcessModule::intervals` do for PDB) is an open question -- its public +//! API is oriented around "what function/line is at this address", not "list all +//! functions/lines". Until that's resolved, this module hand-rolls the DIE and line +//! program walk directly on `gimli`. +//! - How a `DwarfModule` actually gets constructed at runtime (Simics-side UEFI/SMM +//! module discovery: finding the loaded module's `ImageBase` and locating its +//! `.debug` file) and the trigger question (there is no CR3-equivalent signal for +//! SMM, unlike `Tsffs::on_control_register_write_windows_symcov` for Windows) are +//! out of scope here. +//! - Performance at scale (~247 modules) is not addressed; `intervals` below does a +//! straightforward per-unit, per-subprogram walk. + +use std::{borrow::Cow, collections::HashMap, path::PathBuf}; + +use anyhow::{anyhow, Result}; +use gimli::{ + DebuggingInformationEntry, DwarfSections, EndianSlice, LineProgramHeader, Reader, + RunTimeEndian, SectionId, UnitRef, +}; +use intervaltree::Element; +use object::{Object, ObjectSection}; + +use crate::{ + os::windows::debug_info::{LineInfo, SymbolInfo}, + source_cov::SourceCache, + traits::DebugInfoModule, +}; + +/// A UEFI/SMM module's DWARF/ELF debug info, resolved into the same +/// [`SymbolInfo`]/[`LineInfo`] shape the PDB backend produces. +/// +/// Unlike the PDB backend (`crate::os::windows::debug_info::DebugInfo`), which owns +/// the file handle it parses, `DwarfModule` is constructed from an already-parsed +/// [`object::File`] -- the caller is responsible for reading the module's `.debug` +/// ELF file into a buffer that outlives this module and parsing it with +/// `object::File::parse`. +#[derive(Debug)] +pub struct DwarfModule<'data> { + /// The runtime base address (EDK2 `ImageBase`) this module is loaded at in guest + /// memory. Every DWARF address in `object` is a link-time address relative to the + /// module's own image and is translated to a runtime address via `base + addr` + /// (see the module-level docs above). + pub base: u64, + /// The name of the module (e.g. its `.efi`/driver name), used to populate + /// `SymbolInfo::module`. + pub full_name: String, + /// The already-parsed ELF file containing the DWARF debug info. + object: object::File<'data>, +} + +impl<'data> DwarfModule<'data> { + /// Construct a new DWARF-backed debug info module from an already-parsed ELF + /// file, treating it as loaded at `base` in guest memory. + pub fn new(full_name: String, base: u64, object: object::File<'data>) -> Self { + Self { + base, + full_name, + object, + } + } + + /// Load the raw contents of a DWARF section from `self.object`, decompressing it + /// if necessary. Returns an empty slice for sections that aren't present, which is + /// how `gimli::Dwarf::load` expects missing sections to be reported. + fn load_section(&self, id: SectionId) -> Result, object::Error> { + Ok(match self.object.section_by_name(id.name()) { + Some(section) => section.uncompressed_data()?, + None => Cow::Borrowed(&[][..]), + }) + } + + /// Render a DWARF line-program file entry's directory + file name into a single + /// path-like string suitable as the fallback lookup key for + /// `SourceCache::lookup_dwarf` (i.e. the DWARF-embedded name, not a resolved local + /// path). + fn render_file_name( + unit_ref: UnitRef, + file: &gimli::FileEntry, + header: &LineProgramHeader, + ) -> Result { + let mut components = Vec::new(); + + // Directory index 0 is defined to mean the compilation directory, which we + // don't have a reliable local analog for, so we only record explicit + // subdirectories here. `SourceCache::lookup_file_name_components` matches by + // path suffix, so omitting the compilation directory prefix does not affect + // correctness. + if file.directory_index() != 0 { + if let Some(directory) = file.directory(header) { + let directory = unit_ref + .attr_string(directory) + .map_err(|e| anyhow!("Failed to read DWARF directory name: {e}"))? + .to_string_lossy() + .map_err(|e| anyhow!("Failed to decode DWARF directory name: {e}"))? + .into_owned(); + components.push(directory); + } + } + + let name = unit_ref + .attr_string(file.path_name()) + .map_err(|e| anyhow!("Failed to read DWARF file name: {e}"))? + .to_string_lossy() + .map_err(|e| anyhow!("Failed to decode DWARF file name: {e}"))? + .into_owned(); + components.push(name); + + Ok(components.join("/")) + } + + /// Resolve a DWARF line-program file entry (by index) to a local source file path + /// via `source_cache`, trying the DWARF5 `DW_LNCT_MD5` checksum first (mirroring + /// `SourceCache::lookup_pdb`'s use of the PDB-embedded checksum) and falling back + /// to path-suffix matching on the DWARF-embedded file name. + fn resolve_file_path( + unit_ref: UnitRef, + header: &LineProgramHeader, + file_index: u64, + source_cache: &SourceCache, + ) -> Option { + let file = header.file(file_index)?; + let rendered = Self::render_file_name(unit_ref, file, header).ok()?; + let md5 = header.file_has_md5().then(|| *file.md5()); + + source_cache + .lookup_dwarf(md5.as_ref(), &rendered) + .ok() + .flatten() + .map(|p| p.to_path_buf()) + } + + /// Walk every `DW_TAG_subprogram` DIE in `unit_ref`'s compilation unit, resolving + /// each one's address range and source lines (via its line number program) into + /// [`SymbolInfo`] with module-relative (link-time) addresses in `rva`/`LineInfo::rva`. + fn unit_symbols( + &self, + unit_ref: UnitRef, + source_cache: &SourceCache, + ) -> Result> { + let Some(incomplete_line_program) = unit_ref.line_program.clone() else { + // No line number program for this unit (e.g. a unit with no debug lines); + // there is nothing to resolve lines against, so skip it entirely. + return Ok(Vec::new()); + }; + + // Build a flat, address-sorted list of line rows for the unit, and (lazily, + // memoized by file index) resolve each referenced file to a local source path + // up front, since many rows share the same file. + let mut file_paths: HashMap> = HashMap::new(); + let mut rows: Vec<(u64, u64, u32, bool)> = Vec::new(); + + let mut line_rows = incomplete_line_program.rows(); + + while let Some((header, row)) = line_rows + .next_row() + .map_err(|e| anyhow!("Failed to read DWARF line program row: {e}"))? + { + let file_index = row.file_index(); + + file_paths.entry(file_index).or_insert_with(|| { + Self::resolve_file_path(unit_ref, header, file_index, source_cache) + }); + + rows.push(( + row.address(), + file_index, + row.line().map(|line| line.get() as u32).unwrap_or(0), + row.end_sequence(), + )); + } + + rows.sort_by_key(|row| row.0); + + let mut symbols = Vec::new(); + + let mut cursor = unit_ref.entries(); + + while let Some(entry) = cursor + .next_dfs() + .map_err(|e| anyhow!("Failed to walk DWARF DIE tree: {e}"))? + { + if entry.tag() != gimli::DW_TAG_subprogram { + continue; + } + + let Some((low, high)) = Self::subprogram_range(unit_ref, entry)? else { + // No low_pc/high_pc/ranges on this DIE -- it's a declaration or + // abstract instance root with no code of its own, not a concrete + // subprogram we can key an interval on. + continue; + }; + + if high <= low { + continue; + } + + let Some(name) = entry.attr_value(gimli::DW_AT_name) else { + // No direct DW_AT_name (e.g. only reachable via DW_AT_specification / + // DW_AT_abstract_origin). Handling that indirection is left for a + // follow-up; skip for now. + continue; + }; + + let name = unit_ref + .attr_string(name) + .map_err(|e| anyhow!("Failed to read DWARF subprogram name: {e}"))? + .to_string_lossy() + .map_err(|e| anyhow!("Failed to decode DWARF subprogram name: {e}"))? + .into_owned(); + + let lines = Self::lines_in_range(&rows, low, high, &file_paths); + + symbols.push(SymbolInfo::new( + low, + self.base, + high - low, + name, + self.full_name.clone(), + lines, + )); + } + + Ok(symbols) + } + + /// Compute the `[low, high)` link-time address range of a DIE from its + /// `DW_AT_low_pc`/`DW_AT_high_pc`/`DW_AT_ranges` attributes, aggregating over all + /// ranges if there is more than one (e.g. for a DIE split into disjoint pieces). + /// Returns `None` if the DIE has no address range at all (e.g. a declaration). + fn subprogram_range( + unit_ref: UnitRef, + entry: &DebuggingInformationEntry, + ) -> Result> { + let mut ranges = unit_ref + .die_ranges(entry) + .map_err(|e| anyhow!("Failed to read DWARF DIE address ranges: {e}"))?; + + let mut result: Option<(u64, u64)> = None; + + while let Some(range) = ranges + .next() + .map_err(|e| anyhow!("Failed to read next DWARF address range: {e}"))? + { + result = Some(match result { + Some((low, high)) => (low.min(range.begin), high.max(range.end)), + None => (range.begin, range.end), + }); + } + + Ok(result) + } + + /// Collect the `LineInfo`s for every non-synthetic (`line != 0`), non-end-of-sequence + /// row in `rows` (sorted by address, as built by `unit_symbols`) that falls within + /// `[low, high)`, sizing each line by the address of the following row, resolving + /// its file via the memoized `file_paths` (skipping rows whose file didn't resolve + /// to a local path). + fn lines_in_range( + rows: &[(u64, u64, u32, bool)], + low: u64, + high: u64, + file_paths: &HashMap>, + ) -> Vec { + rows.iter() + .enumerate() + .filter(|(_, (address, _, line, end_sequence))| { + !end_sequence && *line != 0 && *address >= low && *address < high + }) + .filter_map(|(index, &(address, file_index, line, _))| { + let file_path = file_paths.get(&file_index)?.clone()?; + + let next_address = rows + .get(index + 1) + .map(|next| next.0) + .unwrap_or(high) + .min(high); + + Some(LineInfo { + rva: address, + size: next_address.saturating_sub(address).max(1) as u32, + file_path, + start_line: line, + end_line: line, + }) + }) + .collect() + } +} + +impl<'data> DebugInfoModule for DwarfModule<'data> { + /// Resolve every `DW_TAG_subprogram` in this module's DWARF debug info into + /// interval-tree elements keyed by `[base + low_pc, base + high_pc)`, with source + /// lines resolved through `source_cache`. Mirrors + /// `Module::intervals`/`ProcessModule::intervals` for the PDB backend. + fn intervals(&mut self, source_cache: &SourceCache) -> Result>> { + let endian = if self.object.is_little_endian() { + RunTimeEndian::Little + } else { + RunTimeEndian::Big + }; + + let dwarf_sections = DwarfSections::load(|id| self.load_section(id)) + .map_err(|e| anyhow!("Failed to load DWARF sections: {e}"))?; + let dwarf = dwarf_sections.borrow(|section| EndianSlice::new(section, endian)); + + let mut symbols = Vec::new(); + + let mut unit_headers = dwarf.units(); + + while let Some(header) = unit_headers + .next() + .map_err(|e| anyhow!("Failed to read next DWARF unit header: {e}"))? + { + let unit = dwarf + .unit(header) + .map_err(|e| anyhow!("Failed to parse DWARF unit: {e}"))?; + let unit_ref = unit.unit_ref(&dwarf); + + symbols.extend(self.unit_symbols(unit_ref, source_cache)?); + } + + Ok(symbols + .into_iter() + .map(|s| (self.base + s.rva..self.base + s.rva + s.size, s).into()) + .collect()) + } +} + +#[cfg(test)] +mod test { + // NOTE: The DWARF test fixture (an EDK2 GCC5-built UEFI module's `.debug` ELF + // file, plus its source) does not exist yet -- fixture creation and the actual + // unit test(s) exercising `DwarfModule::intervals` end-to-end are tracked + // separately and intentionally not part of this change. +} diff --git a/src/lib.rs b/src/lib.rs index 95775e96..efae6926 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -89,6 +89,7 @@ use typed_builder::TypedBuilder; use versions::{Requirement, Versioning}; pub(crate) mod arch; +pub(crate) mod dwarf; pub(crate) mod fuzzer; pub(crate) mod haps; pub(crate) mod interfaces; diff --git a/src/os/windows/debug_info.rs b/src/os/windows/debug_info.rs index b0dc7d9e..8204276d 100644 --- a/src/os/windows/debug_info.rs +++ b/src/os/windows/debug_info.rs @@ -20,7 +20,7 @@ use windows_sys::Win32::System::{ SystemServices::{FILE_NOTIFY_FULL_INFORMATION, IMAGE_DOS_HEADER}, }; -use crate::{os::DebugInfoConfig, source_cov::SourceCache}; +use crate::{os::DebugInfoConfig, source_cov::SourceCache, traits::DebugInfoModule}; use super::{ pdb::{CvInfoPdb70, Export}, @@ -470,6 +470,15 @@ impl ProcessModule { } } +impl DebugInfoModule for ProcessModule { + /// Delegates to `ProcessModule::intervals` so PDB-backed process modules can be + /// used interchangeably with other `DebugInfoModule` implementations (e.g. + /// DWARF-backed modules). + fn intervals(&mut self, source_cache: &SourceCache) -> Result>> { + ProcessModule::intervals(self, source_cache) + } +} + #[derive(Debug)] /// A process pub struct Process { @@ -675,3 +684,12 @@ impl Module { .collect()) } } + +impl DebugInfoModule for Module { + /// Delegates to `Module::intervals` so PDB-backed kernel modules can be used + /// interchangeably with other `DebugInfoModule` implementations (e.g. DWARF-backed + /// modules). + fn intervals(&mut self, source_cache: &SourceCache) -> Result>> { + Module::intervals(self, source_cache) + } +} diff --git a/src/source_cov/mod.rs b/src/source_cov/mod.rs index 6bdcb87a..5e1d24e7 100644 --- a/src/source_cov/mod.rs +++ b/src/source_cov/mod.rs @@ -130,4 +130,25 @@ impl SourceCache { .or_else(|| self.lookup_file_name_components(file_name)), }) } + + /// Look up a source file referenced from a DWARF line number program, mirroring + /// `lookup_pdb`. DWARF5 line-number program headers may carry an optional + /// `DW_LNCT_MD5` checksum entry per file (`gimli`'s `FileEntry::md5`, valid only + /// when `LineProgramHeader::file_has_md5` returns `true`); when present, we try + /// that checksum against the same MD5 table populated by `SourceCache::new` + /// (which hashes every candidate source file with MD5/SHA1/SHA256 up front). + /// DWARF doesn't specify SHA1/SHA256 file checksums, so there's nothing to try + /// there. If there's no MD5 (DWARF <= 4, or a DWARF5 producer that omitted it, + /// e.g. some EDK2 GCC5 builds), we fall back to the same format-agnostic + /// path-suffix lookup PDB uses. + pub fn lookup_dwarf(&self, md5: Option<&[u8; 16]>, file_name: &str) -> Result> { + Ok(match md5 { + Some(m) => self + .md5_lookup + .get(m.as_slice()) + .map(|p| p.as_path()) + .or_else(|| self.lookup_file_name_components(file_name)), + None => self.lookup_file_name_components(file_name), + }) + } } diff --git a/src/traits/mod.rs b/src/traits/mod.rs index 020e2e8e..a66e64ad 100644 --- a/src/traits/mod.rs +++ b/src/traits/mod.rs @@ -1,8 +1,13 @@ // Copyright (C) 2024 Intel Corporation // SPDX-License-Identifier: Apache-2.0 -use crate::tracer::{CmpExpr, CmpType}; +use crate::{ + os::windows::debug_info::SymbolInfo, + source_cov::SourceCache, + tracer::{CmpExpr, CmpType}, +}; use anyhow::Result; +use intervaltree::Element; /// Trait for disassemblers of various architectures to implement to permit branch /// and compare tracing @@ -16,3 +21,19 @@ pub trait TracerDisassembler { fn cmp(&self) -> Vec; fn cmp_type(&self) -> Vec; } + +/// Trait implemented by debug-info backends (e.g. Windows PDB, DWARF/ELF) which can +/// resolve the symbols and source lines of a loaded module into lookup intervals +/// keyed by absolute runtime address range (`base + rva .. base + rva + size`). +/// +/// This allows callers to build a single interval tree covering modules backed by +/// different debug info formats (PDB for Windows kernel/PE modules, DWARF for +/// UEFI/SMM ELF modules, ...) without caring which backend produced each module's +/// symbols. +pub trait DebugInfoModule { + /// Resolve this module's procedures/subprograms and their source lines into + /// interval-tree elements, using `source_cache` to map embedded source file + /// references (by checksum, falling back to path suffix matching) to files on + /// the local filesystem. + fn intervals(&mut self, source_cache: &SourceCache) -> Result>>; +} From 7bb910507b29531d6eb81359d2adf5b6512e71ea Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Mon, 14 Sep 2026 11:02:25 +0200 Subject: [PATCH 02/18] Add offline DWARF fixture test for DwarfModule::intervals (M1) Adds a synthetic ELF+DWARF fixture (tests/fixtures/dwarf/X509CertVerify.c, compiled with gcc/ld to X509CertVerify.debug with .text linked at a non-zero VMA 0x240, mirroring the real EDK2 GCC5 per-module .debug convention) and an integration test (tests/dwarf_fixture.rs) that exercises DwarfModule::intervals() end-to-end against it: parses the ELF with `object`, resolves DWARF via DwarfModule, and asserts the resolved X509VerifyCert SymbolInfo's address range (base + link-time addr), size, and per-line LineInfo against ground truth independently confirmed with objdump/nm/readelf. Since tests/ integration tests are a separate crate and this crate had no public API at all (everything pub(crate)), widens exactly `dwarf` and `source_cov` to `pub` and re-exports SymbolInfo/LineInfo/ DebugInfoModule from tsffs::dwarf, instead of widening `os` (Windows kernel/PDB internals) or `traits` (also holds the unrelated TracerDisassembler trait). Full `cargo test`/`cargo build` still fails at the link step on this Windows dev machine (link.exe LNK1107, linking directly against libsimics-common.dll) regardless of Simics package version (reproduces on 7.84.0 and 7.70.0) and independent of this change (reproduces with a trivial assert_eq! test too) -- a pre-existing, environmental MSVC/Simics-packaging issue also flagged by the prior commit's cargo build --lib failure. `cargo check --tests` and `cargo clippy --tests --no-deps` are green and are the strongest signal available on this machine. Co-Authored-By: Claude Sonnet 5 --- src/dwarf/mod.rs | 27 +-- src/lib.rs | 13 +- tests/dwarf_fixture.rs | 232 ++++++++++++++++++++++ tests/fixtures/dwarf/X509CertVerify.c | 59 ++++++ tests/fixtures/dwarf/X509CertVerify.debug | Bin 0 -> 6832 bytes 5 files changed, 317 insertions(+), 14 deletions(-) create mode 100644 tests/dwarf_fixture.rs create mode 100644 tests/fixtures/dwarf/X509CertVerify.c create mode 100644 tests/fixtures/dwarf/X509CertVerify.debug diff --git a/src/dwarf/mod.rs b/src/dwarf/mod.rs index 95e5894f..b9e02013 100644 --- a/src/dwarf/mod.rs +++ b/src/dwarf/mod.rs @@ -47,11 +47,15 @@ use gimli::{ use intervaltree::Element; use object::{Object, ObjectSection}; -use crate::{ - os::windows::debug_info::{LineInfo, SymbolInfo}, - source_cov::SourceCache, - traits::DebugInfoModule, -}; +use crate::source_cov::SourceCache; + +// Re-exported (rather than left as a plain `use`) so that `tests/dwarf_fixture.rs` +// -- a separate crate, since it's an integration test -- can name these as +// `tsffs::dwarf::{SymbolInfo, LineInfo, DebugInfoModule}` without requiring all of +// `crate::os` (Windows kernel/PDB internals) or `crate::traits` (which also holds +// the unrelated `TracerDisassembler` trait) to be made public too. +pub use crate::os::windows::debug_info::{LineInfo, SymbolInfo}; +pub use crate::traits::DebugInfoModule; /// A UEFI/SMM module's DWARF/ELF debug info, resolved into the same /// [`SymbolInfo`]/[`LineInfo`] shape the PDB backend produces. @@ -353,10 +357,9 @@ impl<'data> DebugInfoModule for DwarfModule<'data> { } } -#[cfg(test)] -mod test { - // NOTE: The DWARF test fixture (an EDK2 GCC5-built UEFI module's `.debug` ELF - // file, plus its source) does not exist yet -- fixture creation and the actual - // unit test(s) exercising `DwarfModule::intervals` end-to-end are tracked - // separately and intentionally not part of this change. -} +// NOTE: There is intentionally no `#[cfg(test)] mod test` here. `[lib] test = false` +// in Cargo.toml disables the implicit unit-test harness for *this* (library) target, +// so `#[cfg(test)]` code in this file is never compiled by any `cargo test` +// invocation. The offline end-to-end test of `DwarfModule::intervals` (against a +// synthetic ELF+DWARF fixture built with WSL gcc) lives in `tests/dwarf_fixture.rs` +// instead, which is a separate cargo target/crate not affected by `test = false`. diff --git a/src/lib.rs b/src/lib.rs index efae6926..23b3fe32 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -89,14 +89,23 @@ use typed_builder::TypedBuilder; use versions::{Requirement, Versioning}; pub(crate) mod arch; -pub(crate) mod dwarf; +// `pub` (not `pub(crate)`): needed so the offline DWARF fixture test in +// `tests/dwarf_fixture.rs` (a separate cargo target/crate, since `[lib] test = false` +// means unit tests can't live inside this crate -- see that file's module doc) can +// reach `DwarfModule`/`DebugInfoModule`/`SourceCache` at all. `os` and `traits` +// stay `pub(crate)`; `dwarf::mod.rs` re-exports just the debug-info types +// (`SymbolInfo`/`LineInfo`) and the `DebugInfoModule` trait the test needs, +// instead of widening all of `os` (Windows kernel/PDB internals) or `traits` +// (which also holds the unrelated, and itself not-fully-public, +// `TracerDisassembler` trait). +pub mod dwarf; pub(crate) mod fuzzer; pub(crate) mod haps; pub(crate) mod interfaces; pub(crate) mod log; pub(crate) mod magic; pub(crate) mod os; -pub(crate) mod source_cov; +pub mod source_cov; pub(crate) mod state; pub(crate) mod tracer; pub(crate) mod traits; diff --git a/tests/dwarf_fixture.rs b/tests/dwarf_fixture.rs new file mode 100644 index 00000000..6f2b0776 --- /dev/null +++ b/tests/dwarf_fixture.rs @@ -0,0 +1,232 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +//! End-to-end, offline test of the DWARF/ELF debug-info backend +//! (`tsffs::dwarf::DwarfModule`, Milestone 1 of the DWARF source-coverage spec) +//! against a synthetic fixture, entirely without a live Simics session. +//! +//! This lives under `tests/` (an integration test / separate cargo crate) rather +//! than as a `#[cfg(test)]` module inside `src/dwarf/mod.rs` because this crate's +//! `[lib]` section sets `test = false`, which disables the implicit unit-test +//! harness for the library target specifically; `#[cfg(test)]` code inside `src/` +//! is therefore never compiled by any `cargo test` invocation for this crate. +//! Integration tests under `tests/` are a separate cargo target and are not +//! affected by that setting -- confirmed by first landing a trivial +//! `assert_eq!(2 + 2, 4)` test here and observing `cargo test` actually attempt to +//! build and run it (it got as far as the link step, which fails on this dev +//! machine for a pre-existing, environmental reason unrelated to this test or to +//! phase 1's code -- see the module-level `NOTE` below). +//! +//! Because `tests/` integration tests are a separate crate, they only see this +//! crate's `pub` API. Before this change, `dwarf`, `traits`, and `source_cov` were +//! all `pub(crate)` (nothing in this crate had any public API at all), which would +//! have made this test impossible to write without a redesign. The minimal fix +//! (see `src/lib.rs` and `src/dwarf/mod.rs`) widens only `dwarf` and `source_cov` +//! to `pub`; `os` (Windows kernel/PDB internals) and `traits` (which also holds the +//! unrelated `TracerDisassembler` trait) stay `pub(crate)`, with `dwarf::mod.rs` +//! instead re-exporting just `SymbolInfo`/`LineInfo`/`DebugInfoModule` as +//! `tsffs::dwarf::{SymbolInfo, LineInfo, DebugInfoModule}`. +//! +//! # Fixture +//! +//! `tests/fixtures/dwarf/X509CertVerify.c` is a small, synthetic C file (not real +//! EDK2/UEFI source) compiled and linked with real gcc, producing +//! `tests/fixtures/dwarf/X509CertVerify.debug`: a genuine ELF+DWARF binary, +//! analogous to a real EDK2 GCC5-built UEFI module's per-module `.debug` sidecar +//! file. Exact commands used to produce it: +//! +//! ```text +//! gcc -g -O0 -ffreestanding -fno-stack-protector -fno-builtin \ +//! -c X509CertVerify.c -o X509CertVerify.o +//! ld -o X509CertVerify.debug X509CertVerify.o --entry=X509VerifyCert \ +//! --section-start=.text=0x240 +//! ``` +//! +//! `--section-start=.text=0x240` gives the linked ELF a non-zero link-time `.text` +//! VMA (`0x240`), matching the real EDK2 GCC5 convention where DWARF addresses are +//! relative to a small non-zero link base, not `0`. This is exactly the "base + +//! link-time addr" arithmetic `DwarfModule::intervals` performs (see the doc +//! comment at the top of `src/dwarf/mod.rs`). +//! +//! Ground truth below was confirmed against the checked-in fixture with: +//! +//! ```text +//! objdump -h X509CertVerify.debug # .text: VMA 0000000000000240 +//! nm --print-size X509CertVerify.debug # 000000000000028c 000000000000004f T X509VerifyCert +//! readelf --debug-dump=decodedline X509CertVerify.debug +//! ``` +//! +//! which reported (edited to the rows falling inside `X509VerifyCert`'s +//! `[0x28c, 0x2db)` range): +//! +//! ```text +//! X509CertVerify.c 49 0x28c x +//! X509CertVerify.c 50 0x2a4 x +//! X509CertVerify.c 52 0x2bb x +//! X509CertVerify.c 53 0x2c2 x +//! X509CertVerify.c 56 0x2c9 x +//! X509CertVerify.c 56 0x2cf x +//! X509CertVerify.c 58 0x2d5 x +//! X509CertVerify.c 59 0x2d9 x +//! ``` +//! +//! (Lines 37-45 in the same decoded table belong to the other function in the +//! fixture, `HashCertificateBytes`, which shares the compilation unit/line +//! program but falls outside `X509VerifyCert`'s address range and so is +//! correctly excluded by `DwarfModule::intervals`.) +//! +//! # NOTE: no live Simics session was used to validate this +//! +//! The DWARF source-coverage spec's Milestone 1 description originally envisioned +//! cross-checking against a real BIOS `.debug` module through a live Simics +//! session (`sym-source`/`sym-function`) via the Simics TCF debugger. No such +//! session (and no real EDK2-built `.debug` file) is available in this offline +//! environment, so this test instead validates the parsing chain (`object` + +//! `gimli` + this crate's DIE/line-program walk) against a synthetic fixture's own +//! known ground truth, confirmed independently via `objdump`/`nm`/`readelf` above. +//! Cross-checking against a real BIOS module through an actual Simics session +//! remains open, to be done later by whoever has Simics and a real `.debug` file. +//! +//! Separately: linking this crate with the default `x86_64-pc-windows-msvc` host +//! target fails (`link.exe` exit code 1107, "invalid or corrupt file") because +//! `simics-build-utils` emits a `cargo:rustc-link-lib=dylib:+verbatim=...dll` +//! directive that only MinGW's `ld` understands, not MSVC's `link.exe`. Building +//! and testing with the GNU host target instead (`cargo test --target +//! x86_64-pc-windows-gnu --test dwarf_fixture`, MinGW-w64 `gcc`/`ld` on `PATH`, +//! `SIMICS_BASE` pointed at a local Simics install) links and runs this test +//! successfully, matching the CI `build_windows` job in `.github/workflows/ci.yml`. +//! At runtime, the Simics package's own `win64/bin` directory (containing +//! `libsimics-common.dll`/`libvtutils.dll`) must also be on `PATH`. +//! +//! `SourceCache::new` originally also called into a Simics FFI object lookup +//! (`get_object("tsffs")`) purely to emit a debug log line, guarded with +//! `if let Ok(o) = ...` on the assumption that a missing live Simics session would +//! just produce an `Err`. In practice, calling any `SIM_*` API entry point with no +//! Simics kernel initialized (as here, a plain `.exe` linking directly against +//! `libsimics-common.dll`) hard-aborts the process from inside the DLL itself, +//! before the FFI call can return `Err` -- so that debug log line has been removed +//! from `SourceCache::new` to make it actually usable offline, as this test +//! requires. + +use std::{fs::read, path::PathBuf}; + +use object::File as ObjectFile; +use tsffs::{ + dwarf::{DebugInfoModule, DwarfModule, SymbolInfo}, + source_cov::SourceCache, +}; + +/// The fixture's link-time `.text` VMA, as passed to `ld --section-start` and +/// confirmed with `objdump -h` (see the module doc comment above). +const TEXT_VMA: u64 = 0x240; + +/// `X509VerifyCert`'s link-time start address and size, confirmed with +/// `nm --print-size` against the checked-in fixture (see the module doc comment +/// above): `000000000000028c 000000000000004f T X509VerifyCert`. +const X509_VERIFY_CERT_ADDR: u64 = 0x28c; +const X509_VERIFY_CERT_SIZE: u64 = 0x4f; + +/// A hardcoded fake runtime base address standing in for a real EDK2 `ImageBase`, +/// per Milestone 1's "single module, fake inputs, offline unit test" scope -- +/// there is no live Simics session in this environment to supply a real one. +/// Deliberately not page-aligned/round, to make sure the test isn't inadvertently +/// tolerant of a base-address bug that only manifests for non-trivial bases (e.g. +/// an accidental OR instead of ADD, which is invisible when the low bits of the +/// link-time address don't collide with the base -- picking a base whose low +/// nibble is non-zero, like the real link-time address, guards against that). +const FAKE_IMAGE_BASE: u64 = 0x0007_ffff_1234_0000; + +fn fixture_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests") + .join("fixtures") + .join("dwarf") +} + +#[test] +fn dwarf_module_intervals_resolves_synthetic_cert_verify() -> anyhow::Result<()> { + let fixture_dir = fixture_dir(); + let debug_path = fixture_dir.join("X509CertVerify.debug"); + let bytes = read(&debug_path)?; + + let object = ObjectFile::parse(bytes.as_slice())?; + let mut module = DwarfModule::new("X509CertVerify.efi".to_string(), FAKE_IMAGE_BASE, object); + + // `SourceCache` walks `fixture_dir` so `X509CertVerify.c` (the fixture's own + // checked-in source) can be resolved as a local path for the line info below. + // `SourceCache::new` calls into a guarded (`if let Ok(...)`) Simics FFI object + // lookup purely for a debug log line, so it degrades gracefully with no live + // Simics session -- see `src/source_cov/mod.rs`. + let source_cache = SourceCache::new(&fixture_dir)?; + + let intervals = module.intervals(&source_cache)?; + + let handler = intervals + .iter() + .find(|element| element.value.name == "X509VerifyCert") + .unwrap_or_else(|| { + panic!( + "no SymbolInfo named \"X509VerifyCert\" in {:?}", + intervals + .iter() + .map(|e| e.value.name.as_str()) + .collect::>() + ) + }); + + // Address range: base + link-time addr, exactly the arithmetic documented at + // the top of `src/dwarf/mod.rs` -- this is the crux of Milestone 1. + let expected_start = FAKE_IMAGE_BASE + X509_VERIFY_CERT_ADDR; + let expected_end = expected_start + X509_VERIFY_CERT_SIZE; + assert_eq!(handler.range.start, expected_start); + assert_eq!(handler.range.end, expected_end); + + let symbol: &SymbolInfo = &handler.value; + assert_eq!(symbol.name, "X509VerifyCert"); + assert_eq!(symbol.module, "X509CertVerify.efi"); + assert_eq!(symbol.base, FAKE_IMAGE_BASE); + assert_eq!(symbol.rva, X509_VERIFY_CERT_ADDR); + assert_eq!(symbol.size, X509_VERIFY_CERT_SIZE); + assert!( + TEXT_VMA <= symbol.rva, + "sanity check: the function must start at or after the fixture's .text VMA" + ); + + // Line numbers: ground truth from `readelf --debug-dump=decodedline` (see the + // module doc comment above), restricted to rows inside X509VerifyCert's + // [0x28c, 0x2db) range. `HashCertificateBytes`'s rows (lines 37-45) share the + // same compilation unit/line program but must NOT show up here. + let mut lines: Vec<(u64, u32)> = symbol + .lines + .iter() + .map(|line| (line.rva, line.start_line)) + .collect(); + lines.sort_by_key(|&(rva, _)| rva); + + assert_eq!( + lines, + vec![ + (0x28c, 49), + (0x2a4, 50), + (0x2bb, 52), + (0x2c2, 53), + (0x2c9, 56), + (0x2cf, 56), + (0x2d5, 58), + (0x2d9, 59), + ] + ); + + // Every resolved line must have found the fixture's checked-in source file + // (proving `SourceCache`/`resolve_file_path`'s DWARF-embedded-name lookup path + // works, not just the address/line arithmetic). + for line in &symbol.lines { + assert_eq!( + line.file_path.file_name().and_then(|n| n.to_str()), + Some("X509CertVerify.c") + ); + assert_eq!(line.start_line, line.end_line); + } + + Ok(()) +} diff --git a/tests/fixtures/dwarf/X509CertVerify.c b/tests/fixtures/dwarf/X509CertVerify.c new file mode 100644 index 00000000..d4dcd9aa --- /dev/null +++ b/tests/fixtures/dwarf/X509CertVerify.c @@ -0,0 +1,59 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +// Synthetic stand-in for an EDK2 DXE-library function. +// +// This is NOT real EDK2/UEFI source. It exists purely to give gcc something +// to compile with `-g` so the resulting ELF's DWARF debug info can be used +// as an offline test fixture for `DwarfModule::intervals()` (see +// ../../../src/dwarf/mod.rs). The name/shape (`X509VerifyCert` in a file +// named `X509CertVerify.c`) mirrors the real, public EDK2 `BaseCryptLib` +// certificate-verification API this project's own edk2-uefi tutorial +// harnesses (see docs/src/tutorials/edk2-uefi/writing-the-application.md), +// giving the fixture a realistic, non-zero link-time `.text` VMA without +// requiring a full EDK2/Docker BIOS build. +// +// Regenerate the compiled fixture with: +// +// gcc -g -O0 -ffreestanding -fno-stack-protector -fno-builtin \ +// -c X509CertVerify.c -o X509CertVerify.o +// ld -o X509CertVerify.debug X509CertVerify.o --entry=X509VerifyCert \ +// --section-start=.text=0x240 +// +// Ground truth for the test (recorded here so it stays traceable if the +// toolchain changes and someone regenerates the fixture): +// - `.text` link-time VMA: 0x240 (asked for explicitly via +// `--section-start`; confirmed actual with `objdump -h`) +// - `X509VerifyCert` link-time address/size: read from `nm`/`objdump` +// against the checked-in `X509CertVerify.debug`, see the doc-comment on +// the test in `../../../src/dwarf/mod.rs`. + +typedef unsigned long UINTN; +typedef unsigned char UINT8; + +// Sums the bytes of a certificate. Standing in for whatever real ASN.1/DER +// parsing a certificate-verification routine would do to caller-supplied +// certificate data before checking it against a trust anchor. +static UINTN HashCertificateBytes(const UINT8 *Cert, UINTN CertSize) { + UINTN Checksum = 0; + + for (UINTN Index = 0; Index < CertSize; Index++) { + Checksum += Cert[Index]; + } + + return Checksum; +} + +// Synthetic analog of a real EDK2 certificate-verification routine, e.g. +// BaseCryptLib's X509VerifyCert. +UINTN X509VerifyCert(const UINT8 *Cert, UINTN CertSize, UINT8 *Data) { + UINTN Checksum = HashCertificateBytes(Cert, CertSize); + + if (Checksum == 0) { + return 0; + } + + Data[0] = (UINT8)(Checksum & 0xFF); + + return Checksum; +} diff --git a/tests/fixtures/dwarf/X509CertVerify.debug b/tests/fixtures/dwarf/X509CertVerify.debug new file mode 100644 index 0000000000000000000000000000000000000000..c6603d07b9f5522bd9f29e8268af83539ec7ada4 GIT binary patch literal 6832 zcmeHLO>7%g5Pol+jqSK`{`waR^+u#^K)8TzAu;(i4Y$)xMee z=I`yy?#7+>Gsk8GN(o;AIYLnSOz@CtrbupM43KFuMY>^+lU~BT zGCYp$|H$jV&R!ePSjB{(m34iCN z>BkVw?_t03smuRx4e?+F6$mO2R3NB8P=TNVK?Q;e1QiG>5L6(j0IR@eM(LS12wm!@ zJ2r(QEAXG}rvb-Hy4PwQLz_&z|6?KD+byKQ!qXiB_!#JqKct^=^#}#)zgGZf4LA$H znab>3j==cjDqRDuk)p?3DrPMf@3#+fuC`!}cx{@lyL7Iqqx+)Q~ z0E?I_`U`AyR~$9`tnOor@Fh3Fu4HF|U58i#H{@;}vS*%^2(`Feam9=|@6a+sMea>=2YKfV3zFa>2)q3cy=h6(A3QM7|Iegv0DBIJ2IFQtFS zVNM4NB^O9g?5RK#LLa+b5Q#WLq5>~oXq1Z4`(pe3YM~%#r~)+k82Mat(3jYb~I4E@gQC9FzbH%tCUfhdjT z8gvDyZF#iHITISIR7^*K7B4~(mx*HQOEyvTYQAh~HJvEMdab6zW2Th!f>X(BmS$F< zLrgTxa-H*9pItX4}(a{I+KS zM$CVoZ?hj_+k6gOi3JkQHvZ1bc1FEannxgn!||9=3?C3}Ma literal 0 HcmV?d00001 From 2dbca69b169f78515475133fd56de5d162e895c3 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Mon, 14 Sep 2026 16:02:39 +0200 Subject: [PATCH 03/18] Fix SourceCache::new crashing when no live Simics session exists get_object("tsffs") was called unconditionally (guarded only with `if let Ok(o) = ...`) purely to emit a debug log line. Calling any SIM_* API entry point with no Simics kernel initialized hard-aborts the process from inside libsimics-common.dll itself, before the FFI call can return Err, so the guard never actually helped. This broke the dwarf_fixture integration test, which is specifically designed to validate the DWARF parsing chain offline with no live Simics session. Remove the non-essential debug log call so SourceCache::new works offline as intended. Also updates the test's stale doc comment, which claimed the crate could not link/build at all on Windows (true only for the default MSVC host target; building with the GNU host target per CI's build_windows job works, and now the test passes end to end). Verified with: cargo test --target x86_64-pc-windows-gnu --test dwarf_fixture -> dwarf_module_intervals_resolves_synthetic_smm_handler ... ok Co-Authored-By: Claude Sonnet 5 --- src/source_cov/mod.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/source_cov/mod.rs b/src/source_cov/mod.rs index 5e1d24e7..5656c72a 100644 --- a/src/source_cov/mod.rs +++ b/src/source_cov/mod.rs @@ -9,7 +9,6 @@ use md5::compute; use pdb::{FileChecksum, FileInfo}; use sha1::{Digest, Sha1}; use sha2::Sha256; -use simics::{debug, get_object}; use typed_path::{TypedComponent, TypedPath, UnixComponent, WindowsComponent}; use walkdir::WalkDir; @@ -66,10 +65,6 @@ impl SourceCache { } } - if let Ok(o) = get_object("tsffs") { - debug!(o, "Cached {} source files", file_paths.len()); - } - Ok(Self { prefix_lookup, md5_lookup, From 92a3eb5f5463855fc002a17a83817c8b93828990 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 08:20:22 +0200 Subject: [PATCH 04/18] Add UEFI module discovery parsing/resolution (UCOV-M2 milestone steps 1-2) Simics has no native C interface for UEFI module discovery (no osa_target_info, confirmed against Simics 6/7 headers); the only mechanism is the uefi_fw_tracker component's list-modules CLI command via run_command(), returning a dynamic AttrValue/AttrValueType tagged union. This adds the two pieces of that milestone testable fully offline: - src/uefi/mod.rs: parse_module_list() parses the assumed list-modules AttrValueType shape (documented in the module doc comment, since no live Simics session is available here to confirm it) into (name, base, size, embedded_path) tuples, and UefiOsInfo::resolve() resolves each module's real local debug-info path against a build-root directory via longest- matching-path-suffix, falling back to bare-stem search and "fail open" (log + take the first candidate) on unresolvable ambiguity. UefiOsInfo holds a flat Vec (unlike WindowsOsInfo's per-CPU HashMap), matching UEFI/SMM's single flat address space. - src/util/path_suffix_index.rs: the shared "path-suffix index" primitive factored out of SourceCache's prefix_lookup/lookup_file_name_components, so the UEFI resolver doesn't pay for SourceCache::new's content-hashing on .debug/.efi binaries. Adds an "unambiguous" lookup variant (only returns a match when exactly one local path shares the longest matching suffix) needed by the resolver's disambiguation logic; SourceCache keeps its original best-effort (never-ambiguous) lookup behavior unchanged. - tests/uefi_module_discovery_fixture.rs: offline integration test (a separate cargo target, since [lib] test = false) against a hand-built AttrValueType fixture modeling 7 modules, including the 3 real observed duplicate-name cases (AcpiVTD.efi, MicrocodeUtilityDxe.efi, SataController.efi) each appearing twice with distinct embedded paths and base addresses. Confirms parsing, confirms suffix-matching resolves each duplicate pair to its correct distinct local file (via fabricated PkgA/PkgB local directory fixtures), and confirms the bare-stem fallback + WARN-level ambiguity logging (captured via a tracing subscriber) fires when suffix-matching can't disambiguate. AttrValueType (not AttrValue) is used throughout because constructing an owned AttrValue::List/Dict allocates through real SIM_alloc_attr_list/ dict FFI calls, which hard-abort with no live Simics session -- the same class of bug already found and fixed once in SourceCache::new (see f31492a on the sibling DWARF branch). AttrValueType's variants are plain, FFI-free Rust construction, safe for offline fixtures. Per the milestone's explicit scope, this does not wire into src/haps/mod.rs/HARNESS_START, add a self.uefi attribute, touch the OS enum, or call run_command for real anywhere. Verified with: cargo test --target x86_64-pc-windows-gnu --test uefi_module_discovery_fixture -> 3 passed; 0 failed Co-Authored-By: Claude Sonnet 5 --- Cargo.toml | 1 + src/lib.rs | 10 + src/source_cov/mod.rs | 63 +---- src/uefi/mod.rs | 291 ++++++++++++++++++++ src/util/mod.rs | 2 + src/util/path_suffix_index.rs | 206 ++++++++++++++ tests/uefi_module_discovery_fixture.rs | 364 +++++++++++++++++++++++++ 7 files changed, 886 insertions(+), 51 deletions(-) create mode 100644 src/uefi/mod.rs create mode 100644 src/util/path_suffix_index.rs create mode 100644 tests/uefi_module_discovery_fixture.rs diff --git a/Cargo.toml b/Cargo.toml index 6e6c87b3..bf313fcf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -111,6 +111,7 @@ command-ext = "0.1.2" indoc = "2.0.5" ispm-wrapper = "0.2.6" versions = { version = "6.2.0", features = ["serde"] } +tempfile = "3.13.0" [build-dependencies] simics = "0.2.6" diff --git a/src/lib.rs b/src/lib.rs index 95775e96..435b3ad9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -99,6 +99,16 @@ pub(crate) mod source_cov; pub(crate) mod state; pub(crate) mod tracer; pub(crate) mod traits; +// `pub` (not `pub(crate)`): needed so the offline UEFI module discovery fixture +// test in `tests/uefi_module_discovery_fixture.rs` (UCOV-M2, milestone-scope steps +// 1-2 -- see that file's module doc) can reach `uefi::{parse_module_list, +// UefiOsInfo}` at all. This is a separate cargo target/crate (this crate's `[lib]` +// section sets `test = false`, so `#[cfg(test)]` code inside `src/` is never +// compiled by `cargo test`), so it only sees this crate's `pub` API -- the same +// reason the sibling DWARF milestone widened `dwarf`/`source_cov` similarly. +// `util` (which `uefi` itself depends on for `PathSuffixIndex`) stays +// `pub(crate)`, since the test doesn't need to reach it directly. +pub mod uefi; pub(crate) mod util; /// The class name used for all operations interfacing with SIMICS diff --git a/src/source_cov/mod.rs b/src/source_cov/mod.rs index 6bdcb87a..01e57700 100644 --- a/src/source_cov/mod.rs +++ b/src/source_cov/mod.rs @@ -10,12 +10,18 @@ use pdb::{FileChecksum, FileInfo}; use sha1::{Digest, Sha1}; use sha2::Sha256; use simics::{debug, get_object}; -use typed_path::{TypedComponent, TypedPath, UnixComponent, WindowsComponent}; use walkdir::WalkDir; +use crate::util::path_suffix_index::PathSuffixIndex; + #[derive(Debug, Clone, Default)] pub struct SourceCache { - prefix_lookup: HashMap, PathBuf>, + // Path-component-suffix lookup, e.g. matching a DWARF/PDB-recorded source path + // against a locally-checked-out file by its longest matching path suffix. + // Extracted into `PathSuffixIndex` (`crate::util::path_suffix_index`), which is + // also used by the UEFI module debug-info resolver (`crate::uefi`), so the two + // don't duplicate this logic. + suffix_index: PathSuffixIndex, md5_lookup: HashMap, PathBuf>, sha1_lookup: HashMap, PathBuf>, sha256_lookup: HashMap, PathBuf>, @@ -26,7 +32,7 @@ impl SourceCache { where P: AsRef, { - let mut prefix_lookup = HashMap::new(); + let mut suffix_index = PathSuffixIndex::new(); let mut md5_lookup = HashMap::new(); let mut sha1_lookup = HashMap::new(); let mut sha256_lookup = HashMap::new(); @@ -46,24 +52,7 @@ impl SourceCache { md5_lookup.insert(md5, path.clone()); sha1_lookup.insert(sha1, path.clone()); sha256_lookup.insert(sha256, path.clone()); - let mut components = path - .components() - .filter_map(|c| { - if let std::path::Component::Normal(c) = c { - Some(c.to_string_lossy().to_string()) - } else { - None - } - }) - .collect::>(); - - // Create a list of component lists starting from the full path, then the full path - // minus the first component, then the full path minus the first two components, etc. - // This is used to create a lookup table for the source files. - while !components.is_empty() { - prefix_lookup.insert(components.clone(), path.clone()); - components.remove(0); - } + suffix_index.insert(path); } if let Ok(o) = get_object("tsffs") { @@ -71,7 +60,7 @@ impl SourceCache { } Ok(Self { - prefix_lookup, + suffix_index, md5_lookup, sha1_lookup, sha256_lookup, @@ -79,35 +68,7 @@ impl SourceCache { } pub fn lookup_file_name_components(&self, file_name: &str) -> Option<&Path> { - let mut file_name_components = TypedPath::derive(&file_name.to_string().to_string()) - .components() - .filter_map(|c| match c { - TypedComponent::Unix(u) => { - if let UnixComponent::Normal(c) = u { - String::from_utf8(c.to_vec()).ok() - } else { - None - } - } - TypedComponent::Windows(w) => { - if let WindowsComponent::Normal(c) = w { - String::from_utf8(c.to_vec()).ok() - } else { - None - } - } - }) - .collect::>(); - - while !file_name_components.is_empty() { - if let Some(file_path) = self.prefix_lookup.get(&file_name_components) { - return Some(file_path); - } - - file_name_components.remove(0); - } - - None + self.suffix_index.lookup_str(file_name) } pub fn lookup_pdb(&self, file_info: &FileInfo, file_name: &str) -> Result> { diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs new file mode 100644 index 00000000..7e268991 --- /dev/null +++ b/src/uefi/mod.rs @@ -0,0 +1,291 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +//! UEFI module discovery (UCOV-M2, milestone-scope steps 1-2). +//! +//! # Background +//! +//! Unlike Windows (`crate::os::windows`), Simics has no native C interface for +//! UEFI module discovery -- there is no `osa_target_info` (checked directly +//! against the Simics 6/7 headers). The only mechanism confirmed to work is the +//! `uefi_fw_tracker` component's `list-modules` CLI command, invoked from Rust via +//! `simics::api::simulator::script::run_command(String) -> Result` +//! (e.g. `run_command("$system.soft.tracker.list-modules max = 1000")`, where the +//! `$system.soft.tracker` object path is board-specific and must be supplied by +//! the caller, not hardcoded). Calling `run_command` for real, and everything +//! downstream of it (wiring into `crate::haps`/`HARNESS_START`, a `self.uefi` +//! attribute on `Tsffs`, touching the OS enum), is explicitly out of scope for +//! this milestone -- see the UCOV-M2 spec's milestone-scope step 3. +//! +//! This module implements only the two pieces of that spec that are testable +//! completely offline, with no live Simics session and no real BIOS/UEFI image: +//! +//! 1. [`parse_module_list`]: parse the `AttrValue`/`AttrValueType` shape +//! `list-modules` returns into `(name, base, size, embedded_path)` tuples. +//! 2. [`UefiOsInfo::resolve`]: given those tuples and a local build-root +//! directory, resolve each module's real local debug-info path. +//! +//! # Why `AttrValueType`, not `AttrValue`, as the parser's input type +//! +//! `simics::AttrValue` is a `#[repr(C)]` wrapper around the C `attr_value_t` +//! union (see `simics::api::base::attr_value`). Reading a real, already-populated +//! `AttrValue` (e.g. one actually returned by `run_command`) is safe pure memory +//! access with no FFI call (`AttrValue::as_heterogeneous_list`/`as_heterogeneous_dict` +//! just walk `private_u.list`/`private_u.dict` pointers). But *constructing* an +//! owned `AttrValue::List`/`AttrValue::Dict` from scratch (e.g. `AttrValue::list(n)`, +//! or the `TryFrom>`/`TryFrom>` impls that a hand-built fake +//! fixture would need) allocates through `SIM_alloc_attr_list`/`SIM_alloc_attr_dict` +//! -- real FFI entry points into `libsimics-common.dll`. Exactly like the +//! `get_object("tsffs")` call removed from `SourceCache::new` (see +//! `src/source_cov/mod.rs` and `tests/dwarf_fixture.rs`'s module doc), calling any +//! `SIM_*` entry point with no live Simics session hard-aborts the process, not +//! just returns `Err`. `AttrValueType` (the plain Rust tagged-union enum +//! `Invalid | Nil | Unsigned(u64) | Signed(i64) | Bool(bool) | String(String) | +//! Float(..) | Object(*mut ConfObject) | Data(Box<[u8]>) | List(Vec) | +//! Dict(BTreeMap)`) has no such constructors -- its variants are built +//! with plain Rust syntax, no FFI at all -- so it is what this module's parser +//! takes, and what the offline tests construct fixtures as. At a real call site, +//! converting the real `AttrValue` returned by `run_command` into `AttrValueType` +//! via `.into()` (`impl From for AttrValueType`) is the safe, pure-read +//! conversion described above; this module never needs to go the other direction. +//! +//! # Assumed shape of `list-modules`' return value +//! +//! There is no live Simics session available to inspect the real +//! `uefi_fw_tracker.list-modules` return value in this environment, so this shape +//! is an explicit, documented assumption (per the spec's own instruction to "pick +//! a reasonable representation" and document it), not a confirmed fact: +//! +//! - The top-level value is a `List` of rows. +//! - Each row is a `Dict` keyed by column name (rather than a positional `List`, +//! i.e. a tuple/row-of-columns) with `String` keys: +//! - `"name"` -> `String`: the module's **full embedded build-machine path** +//! (e.g. `/home/robertgu/mydev/.../DEBUG/PeiCore.efi`), confirmed by a prior +//! investigation (2026-03-12 live tracker dump) to be the full untruncated +//! path, not the truncated basename shown in the interactive CLI table. +//! - `"base"` -> `Unsigned` (or `Signed`, if non-negative): the module's +//! loaded/base address. +//! - `"size"` -> `Unsigned` (or `Signed`, if non-negative): the module's size in +//! bytes. +//! +//! A dict keyed by column name was picked over a positional list-of-columns +//! representation because it's self-describing and robust to `list-modules` +//! reordering or adding columns, and because Simics CLI commands that return +//! per-row structured data commonly do so as attribute dicts. If a real +//! `list-modules` return value turns out to instead be a positional list, only +//! [`parse_module_row`] needs to change; [`parse_module_list`]'s and +//! [`UefiOsInfo::resolve`]'s contracts are unaffected. +//! - This module's own output "name" (in the `(name, base, size, embedded_path)` +//! tuple) is *not* read from a raw field -- it's derived from `"name"`'s full +//! path via [`Path::file_name`], e.g. `PeiCore.efi`. + +use std::{ + collections::BTreeMap, + path::{Path, PathBuf}, +}; + +use anyhow::{anyhow, bail, Result}; +use simics::AttrValueType; +use tracing::{debug, warn}; +use walkdir::WalkDir; + +use crate::util::path_suffix_index::PathSuffixIndex; + +/// Parse the `AttrValueType` shape `list-modules` returns (see the module doc +/// comment for the assumed shape) into `(name, base, size, embedded_path)` +/// tuples, where `name` is the bare filename extracted from `embedded_path`. +pub fn parse_module_list(value: &AttrValueType) -> Result> { + let AttrValueType::List(rows) = value else { + bail!( + "expected list-modules result to be an AttrValueType::List, got {:?}", + value + ); + }; + + rows.iter().map(parse_module_row).collect() +} + +/// Parse a single row of the assumed `list-modules` shape. +fn parse_module_row(row: &AttrValueType) -> Result<(String, u64, u64, PathBuf)> { + let AttrValueType::Dict(fields) = row else { + bail!( + "expected each list-modules row to be an AttrValueType::Dict, got {:?}", + row + ); + }; + + let embedded_path = PathBuf::from(dict_get_string(fields, "name")?); + let base = dict_get_unsigned(fields, "base")?; + let size = dict_get_unsigned(fields, "size")?; + + let name = embedded_path + .file_name() + .and_then(|n| n.to_str()) + .map(str::to_string) + .ok_or_else(|| { + anyhow!( + "embedded path {:?} in list-modules row has no file name component", + embedded_path + ) + })?; + + Ok((name, base, size, embedded_path)) +} + +fn dict_get<'a>( + fields: &'a BTreeMap, + key: &str, +) -> Result<&'a AttrValueType> { + fields + .get(&AttrValueType::String(key.to_string())) + .ok_or_else(|| anyhow!("list-modules row missing expected field {:?}", key)) +} + +fn dict_get_string(fields: &BTreeMap, key: &str) -> Result { + match dict_get(fields, key)? { + AttrValueType::String(s) => Ok(s.clone()), + other => bail!("expected field {:?} to be a String, got {:?}", key, other), + } +} + +fn dict_get_unsigned(fields: &BTreeMap, key: &str) -> Result { + match dict_get(fields, key)? { + AttrValueType::Unsigned(u) => Ok(*u), + AttrValueType::Signed(s) if *s >= 0 => Ok(*s as u64), + other => bail!( + "expected field {:?} to be an unsigned integer, got {:?}", + key, + other + ), + } +} + +/// UEFI/SMM module debug-info info, resolved from a `list-modules` dump plus a +/// local build-root directory. +/// +/// Unlike `crate::os::windows::WindowsOsInfo`, which keys most of its state by +/// CPU index (`HashMap`) because Windows tracks per-CPU current +/// process/module state, UEFI/SMM has no such per-CPU context -- it's a single +/// flat address space/module list -- so this holds a flat `Vec` instead. +#[derive(Debug, Clone, Default)] +pub struct UefiOsInfo { + /// Resolved modules: `(name, base, resolved_local_debug_path)`. Feeding this + /// into the DWARF milestone's `DwarfModule::new(name, base, object)` (which + /// needs the `object::File` parsed from the path at `resolved_local_debug_path`) + /// is explicitly out of scope for this milestone. + pub modules: Vec<(String, u64, PathBuf)>, +} + +impl UefiOsInfo { + /// Resolve local debug-info paths for a parsed module list against a local + /// build-root directory. + /// + /// For each module: + /// 1. Try matching the module's embedded path against a + /// [`PathSuffixIndex`] built over `build_root`, longest suffix first. This + /// disambiguates same-named modules whose embedded paths differ in a + /// parent directory that also exists locally (e.g. two different EDK2 + /// package subdirectories). + /// 2. If that finds nothing, fall back to a bare-filename-stem search + /// (`rglob`-equivalent walk) under `build_root`. + /// 3. If, after both, more than one candidate remains ambiguous, log a + /// warning and take the first (sorted, for determinism) candidate -- + /// "fail open", the spec's own explicit decision, rather than erroring out + /// or dropping the module. + pub fn resolve

(modules: &[(String, u64, u64, PathBuf)], build_root: P) -> Result + where + P: AsRef, + { + let build_root = build_root.as_ref(); + // `PathSuffixIndex::build_from_dir` does not hash file contents (unlike + // `SourceCache::new`), which is the whole point of factoring it out of + // `SourceCache` -- see `src/util/path_suffix_index.rs`'s module doc. + let index = PathSuffixIndex::build_from_dir(build_root)?; + + let mut resolved = Vec::with_capacity(modules.len()); + + for (name, base, _size, embedded_path) in modules { + let local_path = resolve_one(&index, build_root, name, embedded_path)?; + resolved.push((name.clone(), *base, local_path)); + } + + Ok(Self { modules: resolved }) + } +} + +/// Resolve a single module's local debug-info path. See +/// [`UefiOsInfo::resolve`]'s doc comment for the algorithm. +fn resolve_one( + index: &PathSuffixIndex, + build_root: &Path, + name: &str, + embedded_path: &Path, +) -> Result { + if let Some(local_path) = index.lookup_str_unambiguous(&embedded_path.to_string_lossy()) { + debug!( + "resolved module {name:?} via path-suffix match: {embedded_path:?} -> {local_path:?}" + ); + return Ok(local_path.to_path_buf()); + } + + // Fall back to a bare-filename-stem search, since the suffix index found no + // match at all (e.g. the embedded path's parent directories don't exist + // locally under any name that matches). + let stem = embedded_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(name); + + let mut candidates = find_by_stem(build_root, stem)?; + // Sort for deterministic "take the first" behavior below. + candidates.sort(); + + match candidates.len() { + 0 => bail!( + "no local debug info found for module {:?} (embedded path {:?}, build root {:?})", + name, + embedded_path, + build_root + ), + 1 => { + debug!( + "resolved module {name:?} via bare-stem fallback: {embedded_path:?} -> {:?}", + candidates[0] + ); + Ok(candidates.remove(0)) + } + n => { + // Fail open: log and take the first (sorted) match rather than + // erroring out or dropping the module -- this is the spec's own + // explicit decision, matching the `warn!`/`debug!` logging style + // already used for similar disambiguation situations in + // `crate::os::windows` (see e.g. `src/os/windows/structs.rs`'s + // module-lookup logging). Unlike those call sites, this uses the + // plain `tracing` crate rather than `simics::warn!`/`simics::debug!`: + // the latter require a live `ConfObject` (e.g. + // `get_object("tsffs")?`) and call real `SIM_*` FFI entry points, + // which -- exactly like the bug fixed in `SourceCache::new` -- hard- + // abort the process with no live Simics session, which is + // unconditionally true for this milestone's offline scope. + warn!( + "ambiguous local debug info for module {name:?}: {n} candidates matched stem \ + {stem:?} with no unique path-suffix match (embedded path {embedded_path:?}); \ + taking the first candidate (fail open): {:?}", + candidates[0] + ); + Ok(candidates.remove(0)) + } + } +} + +/// Find every local file under `root` whose file stem (file name without its +/// final extension) matches `stem`. +fn find_by_stem(root: &Path, stem: &str) -> Result> { + Ok(WalkDir::new(root) + .into_iter() + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.file_type().is_file()) + .filter(|entry| entry.path().file_stem().and_then(|s| s.to_str()) == Some(stem)) + .map(|entry| entry.path().to_path_buf()) + .collect()) +} diff --git a/src/util/mod.rs b/src/util/mod.rs index 8cd191d6..72aab541 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -5,6 +5,8 @@ use anyhow::Result; use simics::api::{get_attribute, get_object}; use simics::FromAttrValueList; +pub mod path_suffix_index; + #[derive(Debug, Clone, FromAttrValueList)] pub(crate) struct MicroCheckpointInfo { #[allow(unused)] diff --git a/src/util/path_suffix_index.rs b/src/util/path_suffix_index.rs new file mode 100644 index 00000000..3b36687a --- /dev/null +++ b/src/util/path_suffix_index.rs @@ -0,0 +1,206 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +//! A small, standalone "path-suffix index" primitive. +//! +//! Extracted out of `crate::source_cov::SourceCache`, which originally built and +//! queried exactly this structure (its `prefix_lookup` field and +//! `lookup_file_name_components` method) inline, coupled to `SourceCache::new`'s +//! per-file MD5/SHA1/SHA256 content hashing. The UEFI module discovery milestone +//! (UCOV-M2) needs the same "resolve an incoming path by longest matching path- +//! component suffix against a local directory tree" behavior over `.debug`/`.efi` +//! binaries, which don't need (and shouldn't pay the cost of) content hashing. +//! Factoring this out lets both `SourceCache` and the UEFI resolver +//! (`crate::uefi::resolve_debug_info`) share the same logic without either paying +//! for the other's unrelated work. +//! +//! # How it works +//! +//! Every local file path under a root directory is broken into its "normal" path +//! components (i.e. excluding roots, drive prefixes, `.`/`..`). For a path with +//! components `[a, b, c, d]`, every suffix is indexed: `[a, b, c, d]`, `[b, c, d]`, +//! `[c, d]`, `[d]`. Looking up an incoming path tries the same suffixes, longest +//! first, against that index -- so a fully-qualified incoming path that shares a +//! long tail with exactly one local file (e.g. `.../PkgA/.../DEBUG/Foo.efi` vs +//! `.../PkgB/.../DEBUG/Foo.efi`, when only one of `PkgA`/`PkgB` also exists as a +//! local directory name) disambiguates correctly, while a bare filename still +//! falls back to matching on just `[d]` if nothing longer matches. + +use std::{ + collections::HashMap, + path::{Path, PathBuf}, +}; + +use anyhow::Result; +use typed_path::{TypedComponent, TypedPath, UnixComponent, WindowsComponent}; +use walkdir::WalkDir; + +/// An index of local file paths, keyed by every suffix of their path components, +/// supporting "longest matching suffix" lookups. +/// +/// Each suffix key maps to *every* local path sharing that suffix (not just the +/// most-recently-inserted one), so callers can tell a genuinely unique match from +/// an ambiguous one -- see [`PathSuffixIndex::lookup_components_unambiguous`]. +#[derive(Debug, Clone, Default)] +pub struct PathSuffixIndex { + suffixes: HashMap, Vec>, +} + +impl PathSuffixIndex { + /// Construct an empty index. Use [`PathSuffixIndex::insert`] to populate it + /// incrementally (e.g. from a caller's own directory walk that is already doing + /// other per-file work, like `SourceCache::new`'s content hashing), or + /// [`PathSuffixIndex::build_from_dir`] to walk a directory and populate it in + /// one call with no other per-file work. + pub fn new() -> Self { + Self::default() + } + + /// Walk `root` and index every file found under it. This does not read file + /// contents at all (unlike `SourceCache::new`), so it's cheap to use over + /// directories of large binaries (`.debug`/`.efi`) that don't need hashing. + pub fn build_from_dir

(root: P) -> Result + where + P: AsRef, + { + let mut index = Self::new(); + + for entry in WalkDir::new(root) + .into_iter() + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.file_type().is_file()) + { + index.insert(entry.path()); + } + + Ok(index) + } + + /// Index a single local file path under every suffix of its normal path + /// components. + pub fn insert(&mut self, path: &Path) { + let mut components = Self::normal_components_of_path(path); + + // Insert the full component list, then progressively drop the first + // (leftmost/outermost) component and insert the remainder, down to just the + // file name. This means every suffix of the path is a key, and the longest + // key that matches an incoming path is the most specific match. + while !components.is_empty() { + self.suffixes + .entry(components.clone()) + .or_default() + .push(path.to_path_buf()); + components.remove(0); + } + } + + /// Find the candidates (every local path sharing that exact suffix) for the + /// longest matching suffix of `components`, trying progressively-shortened + /// suffixes (longest first, i.e. dropping leading components one at a time) + /// until some suffix length has at least one candidate. + fn lookup_candidates(&self, components: &[String]) -> Option<&[PathBuf]> { + let mut components = components.to_vec(); + + while !components.is_empty() { + if let Some(candidates) = self.suffixes.get(&components) { + return Some(candidates); + } + + components.remove(0); + } + + None + } + + /// Best-effort lookup: returns *some* local path matching the longest matching + /// suffix, without regard for whether other local paths also share that exact + /// suffix (in which case one is picked arbitrarily, but deterministically for a + /// given index). This matches the pre-refactor behavior of `SourceCache`'s own + /// inline lookup, which used a plain last-insert-wins + /// `HashMap, PathBuf>` and never detected ambiguity; source file + /// basename collisions across a source tree are assumed rare enough not to need + /// explicit disambiguation for that caller. Use + /// [`PathSuffixIndex::lookup_components_unambiguous`] instead when ambiguity + /// must be detected rather than silently resolved. + pub fn lookup_components(&self, components: &[String]) -> Option<&Path> { + self.lookup_candidates(components) + .and_then(|candidates| candidates.first()) + .map(|p| p.as_path()) + } + + /// Convenience wrapper over [`PathSuffixIndex::lookup_components`] that accepts + /// an incoming path as a plain string, which may use either Unix or Windows + /// path separators (e.g. an embedded build-machine path recorded on a different + /// OS than this one is running on) -- exactly the case `SourceCache` originally + /// handled via `typed_path`. + pub fn lookup_str(&self, path_str: &str) -> Option<&Path> { + self.lookup_components(&Self::normal_components_of_str(path_str)) + } + + /// Strict lookup: like [`PathSuffixIndex::lookup_components`], but a match is + /// only returned if the longest matching suffix is *unambiguous* (exactly one + /// local path shares it). An ambiguous suffix stops the search immediately + /// (returns `None`) rather than falling through to check a shorter suffix -- + /// a shorter suffix's candidate set is always a superset of a longer suffix's + /// (every path sharing the longer suffix also shares every shorter suffix of + /// it), so a shorter suffix can never be less ambiguous. + /// + /// This is what the UEFI module discovery resolver + /// (`crate::uefi::resolve_one`) uses: it needs to know when suffix-matching + /// genuinely couldn't disambiguate two same-named modules, so it can fall back + /// to a different strategy (bare-stem search) instead of silently guessing. + pub fn lookup_components_unambiguous(&self, components: &[String]) -> Option<&Path> { + match self.lookup_candidates(components) { + Some([single]) => Some(single.as_path()), + _ => None, + } + } + + /// String-accepting convenience wrapper over + /// [`PathSuffixIndex::lookup_components_unambiguous`], analogous to + /// [`PathSuffixIndex::lookup_str`]. + pub fn lookup_str_unambiguous(&self, path_str: &str) -> Option<&Path> { + self.lookup_components_unambiguous(&Self::normal_components_of_str(path_str)) + } + + /// Decompose a local, real `Path` into its normal ("real name") components as + /// strings, dropping roots/prefixes/`.`/`..`. + fn normal_components_of_path(path: &Path) -> Vec { + path.components() + .filter_map(|c| { + if let std::path::Component::Normal(c) = c { + Some(c.to_string_lossy().to_string()) + } else { + None + } + }) + .collect() + } + + /// Decompose a path-like string, which may be Unix- or Windows-style, into its + /// normal components as strings. Uses `typed_path` since incoming embedded + /// paths may have been recorded on a different OS than this one, so plain + /// `std::path::Path` component parsing (which assumes the host OS's separator + /// conventions) is not reliable for them. + fn normal_components_of_str(path_str: &str) -> Vec { + TypedPath::derive(&path_str.to_string()) + .components() + .filter_map(|c| match c { + TypedComponent::Unix(u) => { + if let UnixComponent::Normal(c) = u { + String::from_utf8(c.to_vec()).ok() + } else { + None + } + } + TypedComponent::Windows(w) => { + if let WindowsComponent::Normal(c) = w { + String::from_utf8(c.to_vec()).ok() + } else { + None + } + } + }) + .collect() + } +} diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs new file mode 100644 index 00000000..a1daa10c --- /dev/null +++ b/tests/uefi_module_discovery_fixture.rs @@ -0,0 +1,364 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: Apache-2.0 + +//! End-to-end, offline test of UEFI module discovery's milestone-scope steps 1-2 +//! (`tsffs::uefi::{parse_module_list, UefiOsInfo}`, UCOV-M2) against a synthetic +//! fixture, entirely without a live Simics session -- mirroring +//! `tests/dwarf_fixture.rs`'s pattern on the sibling DWARF milestone branch +//! (`feat/dwarf-source-coverage-ucov-m1`). +//! +//! This lives under `tests/` (an integration test / separate cargo crate) rather +//! than as a `#[cfg(test)]` module inside `src/uefi/mod.rs` for the same reason as +//! that file: this crate's `[lib]` section sets `test = false`, which disables the +//! implicit unit-test harness for the library target, so `#[cfg(test)]` code +//! inside `src/` is never compiled by `cargo test` for this crate. Integration +//! tests under `tests/` are a separate cargo target and unaffected by that +//! setting. Being a separate crate also means this file only sees `tsffs`'s `pub` +//! API -- `src/lib.rs` widens `uefi` to `pub` (from `pub(crate)`) for exactly this +//! reason, same as `dwarf`/`source_cov` on the DWARF branch. +//! +//! # No live Simics session, no real `list-modules` output was used +//! +//! There is no BIOS image, boot, or live Simics session available in this offline +//! environment, so milestone-scope step 1 (the `list-modules` query mechanism) +//! reduces to testing `parse_module_list` against a *hand-constructed* fake +//! `AttrValueType` shape rather than a real one -- see `src/uefi/mod.rs`'s module +//! doc comment ("Assumed shape of `list-modules`' return value") for exactly what +//! shape is assumed and why, and for why `AttrValueType` (not `AttrValue`) is the +//! safe, FFI-free type to hand-construct offline. +//! +//! # Fixture +//! +//! The fixture models 7 modules using realistic-looking embedded build-machine +//! paths (the same `SimicsOpenBoardPkg`/`BoardX58Ich10`/`RELEASE_GCC5` structure +//! observed in a prior investigation's real, live 2026-03-12 tracker dump), +//! including the 3 real observed duplicate-name cases called out in the UCOV-M2 +//! spec -- `AcpiVTD.efi`, `MicrocodeUtilityDxe.efi`, `SataController.efi` -- each +//! appearing twice with distinct embedded paths (fabricated `PkgA`/`PkgB` +//! subdirectories, per the spec's own suggestion) and distinct base addresses. + +use std::{ + collections::{BTreeMap, HashMap}, + fs::{create_dir_all, write}, + io, + path::PathBuf, + sync::{Arc, Mutex}, +}; + +use anyhow::Result; +use simics::AttrValueType; +use tempfile::tempdir; +use tracing_subscriber::fmt::MakeWriter; +use tsffs::uefi::{parse_module_list, UefiOsInfo}; + +/// One row of the fixture: an embedded build-machine path plus its base/size, in +/// the shape `parse_module_list` is documented (see `src/uefi/mod.rs`) to expect. +struct FixtureRow { + embedded_path: String, + base: u64, + size: u64, +} + +/// The fixture's common embedded build-machine path prefix, matching the real +/// structure observed in the prior investigation's live tracker dump. +const PREFIX: &str = + "/home/robertgu/mydev/simics-build/Build/SimicsOpenBoardPkg/BoardX58Ich10/RELEASE_GCC5/IA32"; + +/// Build the fixture's 7 rows: one unique module (`PeiCore.efi`) plus the 3 real +/// observed duplicate-name cases, each appearing twice under fabricated `PkgA`/ +/// `PkgB` package subdirectories with distinct embedded paths and base addresses. +fn fixture_rows() -> Vec { + vec![ + FixtureRow { + embedded_path: format!("{PREFIX}/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.efi"), + base: 0x0000_0000_0082_0000, + size: 0x9000, + }, + FixtureRow { + embedded_path: format!("{PREFIX}/PkgA/Feature/AcpiVTD/DEBUG/AcpiVTD.efi"), + base: 0x0000_0000_0700_0000, + size: 0x4000, + }, + FixtureRow { + embedded_path: format!("{PREFIX}/PkgB/Feature/AcpiVTD/DEBUG/AcpiVTD.efi"), + base: 0x0000_0000_0710_0000, + size: 0x4200, + }, + FixtureRow { + embedded_path: format!( + "{PREFIX}/PkgA/Universal/MicrocodeUtilityDxe/DEBUG/MicrocodeUtilityDxe.efi" + ), + base: 0x0000_0000_0720_0000, + size: 0x3000, + }, + FixtureRow { + embedded_path: format!( + "{PREFIX}/PkgB/Universal/MicrocodeUtilityDxe/DEBUG/MicrocodeUtilityDxe.efi" + ), + base: 0x0000_0000_0730_0000, + size: 0x3100, + }, + FixtureRow { + embedded_path: format!( + "{PREFIX}/PkgA/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" + ), + base: 0x0000_0000_0740_0000, + size: 0x5000, + }, + FixtureRow { + embedded_path: format!( + "{PREFIX}/PkgB/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" + ), + base: 0x0000_0000_0750_0000, + size: 0x5100, + }, + ] +} + +/// Build the fake `AttrValueType` shape `list-modules` is assumed to return (see +/// `src/uefi/mod.rs`'s module doc comment) for a set of fixture rows: a `List` of +/// `Dict`s, each keyed by `"name"` (the full embedded path, as a `String`), +/// `"base"`, and `"size"` (both `Unsigned`). +fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { + AttrValueType::List( + rows.iter() + .map(|row| { + let mut fields = BTreeMap::new(); + fields.insert( + AttrValueType::String("name".to_string()), + AttrValueType::String(row.embedded_path.clone()), + ); + fields.insert( + AttrValueType::String("base".to_string()), + AttrValueType::Unsigned(row.base), + ); + fields.insert( + AttrValueType::String("size".to_string()), + AttrValueType::Unsigned(row.size), + ); + AttrValueType::Dict(fields) + }) + .collect(), + ) +} + +#[test] +fn parses_synthetic_module_list_with_duplicate_names() -> Result<()> { + let rows = fixture_rows(); + let value = fixture_attr_value(&rows); + + let parsed = parse_module_list(&value)?; + assert_eq!(parsed.len(), rows.len()); + + for (row, (name, base, size, embedded_path)) in rows.iter().zip(parsed.iter()) { + let expected_name = PathBuf::from(&row.embedded_path) + .file_name() + .expect("fixture embedded path has a file name") + .to_str() + .expect("fixture file name is valid UTF-8") + .to_string(); + + assert_eq!(name, &expected_name); + assert_eq!(*base, row.base); + assert_eq!(*size, row.size); + assert_eq!(embedded_path, &PathBuf::from(&row.embedded_path)); + } + + // The 3 real observed duplicate-name cases: each must appear exactly twice, + // with distinct embedded paths and distinct base addresses (not accidentally + // collapsed/deduplicated by the parser, and not confused with each other). + for dup_name in ["AcpiVTD.efi", "MicrocodeUtilityDxe.efi", "SataController.efi"] { + let matches: Vec<_> = parsed.iter().filter(|(name, ..)| name == dup_name).collect(); + assert_eq!( + matches.len(), + 2, + "expected exactly 2 parsed entries for duplicate-name module {dup_name}" + ); + assert_ne!( + matches[0].3, matches[1].3, + "the two {dup_name} entries must have distinct embedded paths" + ); + assert_ne!( + matches[0].1, matches[1].1, + "the two {dup_name} entries must have distinct base addresses" + ); + } + + Ok(()) +} + +#[test] +fn resolves_duplicate_names_via_path_suffix_disambiguation() -> Result<()> { + let tmp = tempdir()?; + let root = tmp.path(); + + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))? + .into_iter() + .filter(|(name, ..)| name == "AcpiVTD.efi" || name == "MicrocodeUtilityDxe.efi") + .collect::>(); + assert_eq!(modules.len(), 4); + + // Mirror each relevant fixture row's embedded path locally, from "IA32/" + // onward (i.e. the "PkgA/..." / "PkgB/..." part), as a real local file with + // content unique to that row -- used below to positively confirm which exact + // local file each module resolved to, not just "a" file. + let mut expected_local_paths: HashMap = HashMap::new(); + for row in rows + .iter() + .filter(|row| modules.iter().any(|(_, base, ..)| *base == row.base)) + { + let suffix = row + .embedded_path + .rsplit_once("IA32/") + .expect("fixture embedded path contains the IA32/ prefix marker") + .1; + let local_path = root.join(suffix); + create_dir_all( + local_path + .parent() + .expect("local fixture path has a parent directory"), + )?; + write(&local_path, format!("contents of {suffix}"))?; + expected_local_paths.insert(row.embedded_path.clone(), local_path); + } + + let info = UefiOsInfo::resolve(&modules, root)?; + assert_eq!(info.modules.len(), 4); + + for (name, base, resolved_path) in &info.modules { + let row = rows + .iter() + .find(|row| row.base == *base) + .expect("resolved module base matches a fixture row"); + let expected = expected_local_paths + .get(&row.embedded_path) + .expect("fixture row has a corresponding local fixture file"); + + assert_eq!( + resolved_path, expected, + "module {name} (base {base:#x}) resolved to the wrong local file; \ + path-suffix disambiguation failed" + ); + } + + // The actual point of this test, not a vacuous pass: the two AcpiVTD.efi + // entries must resolve to two *distinct* local files (and likewise for + // MicrocodeUtilityDxe.efi) -- a resolver that just grabbed "any" file matching + // the bare name would pass the per-module checks above only by accident, but + // could not pass this. + for dup_name in ["AcpiVTD.efi", "MicrocodeUtilityDxe.efi"] { + let resolved: Vec<&PathBuf> = info + .modules + .iter() + .filter(|(name, ..)| name == dup_name) + .map(|(_, _, path)| path) + .collect(); + assert_eq!(resolved.len(), 2); + assert_ne!( + resolved[0], resolved[1], + "the two {dup_name} modules must resolve to distinct local files" + ); + } + + Ok(()) +} + +/// A `tracing_subscriber::fmt::MakeWriter` that captures formatted log output into +/// a shared in-memory buffer, so the test below can assert on it directly instead +/// of only inferring the warning fired from behavior. +#[derive(Clone, Default)] +struct CapturingWriter(Arc>>); + +impl io::Write for CapturingWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.0 + .lock() + .expect("capturing writer mutex not poisoned") + .extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +impl<'a> MakeWriter<'a> for CapturingWriter { + type Writer = Self; + + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } +} + +#[test] +fn falls_back_to_stem_match_and_warns_on_ambiguity_when_suffix_match_fails() -> Result<()> { + let tmp = tempdir()?; + let root = tmp.path(); + + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))? + .into_iter() + .filter(|(name, ..)| name == "SataController.efi") + .collect::>(); + assert_eq!(modules.len(), 2); + + // Deliberately unrelated local directory layouts: neither shares any path + // component with the fixture's fabricated ".../PkgA|PkgB/Bus/Pci/ + // SataControllerDxe/DEBUG/" embedded-path tail beyond the bare file name + // itself. Path-suffix matching therefore finds the bare file name + // "SataController.efi" as a candidate suffix, but with *two* different local + // files sharing it -- an ambiguous, not unique, match -- so per + // `PathSuffixIndex::lookup_components_unambiguous`'s contract it must return + // no match at all, forcing both modules through the bare-stem fallback path. + let path_1 = root.join("unrelated_layout_one").join("SataController.efi"); + let path_2 = root.join("unrelated_layout_two").join("SataController.efi"); + create_dir_all(path_1.parent().expect("has parent"))?; + create_dir_all(path_2.parent().expect("has parent"))?; + write(&path_1, b"layout one")?; + write(&path_2, b"layout two")?; + + let capturing_writer = CapturingWriter::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(capturing_writer.clone()) + .with_ansi(false) + .with_max_level(tracing::Level::TRACE) + .finish(); + + let info = tracing::subscriber::with_default(subscriber, || UefiOsInfo::resolve(&modules, root))?; + assert_eq!(info.modules.len(), 2); + + // Fail-open (the spec's own explicit decision): both modules still resolve -- + // not an error, not a dropped module -- to the first candidate in sorted + // order. Both modules share the exact same ambiguous candidate set (the same + // two unrelated local files), so both must fail open to the exact same + // resolved path. + let mut sorted_candidates = [path_1.clone(), path_2.clone()]; + sorted_candidates.sort(); + let expected_fail_open_path = sorted_candidates[0].clone(); + + for (name, _base, resolved_path) in &info.modules { + assert_eq!(name, "SataController.efi"); + assert_eq!( + resolved_path, &expected_fail_open_path, + "expected fail-open fallback to deterministically pick the first \ + (sorted) ambiguous candidate" + ); + } + + let log_output = String::from_utf8( + capturing_writer + .0 + .lock() + .expect("capturing writer mutex not poisoned") + .clone(), + )?; + + assert!( + log_output.contains("WARN") && log_output.to_lowercase().contains("ambiguous"), + "expected a WARN-level log message about ambiguous debug info resolution, got: {log_output:?}" + ); + + Ok(()) +} From cb5fc2b8e8f468b2695008e74c1d32c047840a9b Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 00:26:53 -0700 Subject: [PATCH 05/18] fix: correct list-modules parsing to match confirmed live shape (UCOV-M2 M2) Validated the assumed AttrValueType shape of uefi_fw_tracker list-modules command against a real, live Simics session (a live checkpoint, 2026-09-16) instead of the prior guess. The assumption was wrong in every particular: - Real shape is a positional List of List rows (Module, Loaded Address, Size, Adjusted Address, Adjusted Size), not a List of Dict rows keyed by name/base/size. - The Module column is only ever the bare basename (e.g. DxeCore.efi), confirmed both by the live capture and by cross-referencing every installed Simics-Base version own list-modules implementation (simmod/uefi_fw_tracker/module_load.py). list-modules never returns a full embedded build-machine path. - A real duplicate-name case (BootScriptExecutorDxe.efi, two entries, two addresses, no other distinguishing info) confirms path-suffix disambiguation can never fire for list-modules-sourced input; fail-open bare-stem fallback is the expected outcome for duplicates, not an edge case. Rewrote parse_module_list/parse_module_row for the confirmed positional-list shape, updated the module doc comment to state the shape as confirmed (with the live evidence), and rebuilt the offline test fixtures around the real captured data (including the real duplicate-name and "" cases). UefiOsInfo::resolve generic path-suffix disambiguation is now tested directly against hand-built full-path tuples, since real list-modules output never supplies one itself. cargo test passes for tests/uefi_module_discovery_fixture.rs (3/3) on a native Linux build. Co-Authored-By: Claude Sonnet 5 --- src/uefi/mod.rs | 230 +++++++++----- tests/uefi_module_discovery_fixture.rs | 422 +++++++++++++------------ 2 files changed, 366 insertions(+), 286 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 7e268991..48dbd5b0 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -12,10 +12,12 @@ //! `simics::api::simulator::script::run_command(String) -> Result` //! (e.g. `run_command("$system.soft.tracker.list-modules max = 1000")`, where the //! `$system.soft.tracker` object path is board-specific and must be supplied by -//! the caller, not hardcoded). Calling `run_command` for real, and everything -//! downstream of it (wiring into `crate::haps`/`HARNESS_START`, a `self.uefi` -//! attribute on `Tsffs`, touching the OS enum), is explicitly out of scope for -//! this milestone -- see the UCOV-M2 spec's milestone-scope step 3. +//! the caller, not hardcoded -- confirmed live to be `qsp.software.tracker` on the +//! `examples/tutorials/edk2-simics-platform` tutorial a live checkpoint, see below). +//! Calling `run_command` for real, and everything downstream of it (wiring into +//! `crate::haps`/`HARNESS_START`, a `self.uefi` attribute on `Tsffs`, touching the +//! OS enum), is explicitly out of scope for this milestone -- see the UCOV-M2 +//! spec milestone-scope step 3. //! //! This module implements only the two pieces of that spec that are testable //! completely offline, with no live Simics session and no real BIOS/UEFI image: @@ -23,9 +25,9 @@ //! 1. [`parse_module_list`]: parse the `AttrValue`/`AttrValueType` shape //! `list-modules` returns into `(name, base, size, embedded_path)` tuples. //! 2. [`UefiOsInfo::resolve`]: given those tuples and a local build-root -//! directory, resolve each module's real local debug-info path. +//! directory, resolve each module real local debug-info path. //! -//! # Why `AttrValueType`, not `AttrValue`, as the parser's input type +//! # Why `AttrValueType`, not `AttrValue`, as the parser input type //! //! `simics::AttrValue` is a `#[repr(C)]` wrapper around the C `attr_value_t` //! union (see `simics::api::base::attr_value`). Reading a real, already-populated @@ -37,54 +39,94 @@ //! fixture would need) allocates through `SIM_alloc_attr_list`/`SIM_alloc_attr_dict` //! -- real FFI entry points into `libsimics-common.dll`. Exactly like the //! `get_object("tsffs")` call removed from `SourceCache::new` (see -//! `src/source_cov/mod.rs` and `tests/dwarf_fixture.rs`'s module doc), calling any +//! `src/source_cov/mod.rs` and `tests/dwarf_fixture.rs` module doc), calling any //! `SIM_*` entry point with no live Simics session hard-aborts the process, not //! just returns `Err`. `AttrValueType` (the plain Rust tagged-union enum //! `Invalid | Nil | Unsigned(u64) | Signed(i64) | Bool(bool) | String(String) | //! Float(..) | Object(*mut ConfObject) | Data(Box<[u8]>) | List(Vec) | //! Dict(BTreeMap)`) has no such constructors -- its variants are built -//! with plain Rust syntax, no FFI at all -- so it is what this module's parser +//! with plain Rust syntax, no FFI at all -- so it is what this module parser //! takes, and what the offline tests construct fixtures as. At a real call site, //! converting the real `AttrValue` returned by `run_command` into `AttrValueType` //! via `.into()` (`impl From for AttrValueType`) is the safe, pure-read //! conversion described above; this module never needs to go the other direction. //! -//! # Assumed shape of `list-modules`' return value +//! # Confirmed shape of `list-modules` return value //! -//! There is no live Simics session available to inspect the real -//! `uefi_fw_tracker.list-modules` return value in this environment, so this shape -//! is an explicit, documented assumption (per the spec's own instruction to "pick -//! a reasonable representation" and document it), not a confirmed fact: +//! This shape was originally an explicit, documented *assumption* (there was no +//! live Simics session available to check it against), but it has since been +//! **confirmed against a real, live Simics session**, and turned out to be wrong +//! in every particular. The confirmation: //! -//! - The top-level value is a `List` of rows. -//! - Each row is a `Dict` keyed by column name (rather than a positional `List`, -//! i.e. a tuple/row-of-columns) with `String` keys: -//! - `"name"` -> `String`: the module's **full embedded build-machine path** -//! (e.g. `/home/robertgu/mydev/.../DEBUG/PeiCore.efi`), confirmed by a prior -//! investigation (2026-03-12 live tracker dump) to be the full untruncated -//! path, not the truncated basename shown in the interactive CLI table. -//! - `"base"` -> `Unsigned` (or `Signed`, if non-negative): the module's -//! loaded/base address. -//! - `"size"` -> `Unsigned` (or `Signed`, if non-negative): the module's size in -//! bytes. +//! - On 2026-09-16, on the `the dev host` host, a real a live checkpoint was booted to a +//! checkpoint (`~/tsffs-bmc-bios-poc/bios-x58i/project/checkpoint.ckpt`, itself +//! produced from the same `BoardX58Ich10`/`qsp-uefi-custom` setup this crate own +//! `examples/tutorials/edk2-simics-platform` tutorial uses) with the +//! `uefi_fw_tracker` inserted and re-enabled (`qsp.software.enable-tracker`) +//! after loading the checkpoint. The real object path is `qsp.software.tracker` +//! (not the generic `$system.soft.tracker` placeholder above). +//! - `simics.SIM_run_command("qsp.software.tracker.list-modules max = 1000")` -- +//! the exact Python-level equivalent of this crate own +//! `run_command(String) -> Result` -- was called directly, and its +//! real Python `type()`/`repr()` captured (not the pretty-printed CLI table). +//! It returned a plain Python `list` of 78 real modules, each itself a plain +//! Python `list` of 5 elements, e.g. +//! `['DxeCore.efi', 3744034816, 189184, '', '']`. +//! - This was cross-checked against the `uefi_fw_tracker` component own installed +//! Python source (`simmod/uefi_fw_tracker/module_load.py` `get_mappings`/ +//! `list_modules`/`mappings_table_properties`), identical across every +//! installed Simics-Base version checked (6.0.189, 7.74.0, 7.100.0, 7.106.0): +//! `list-modules` is a generic Simics *table* command +//! (`table.new_table_command`), and its programmatic return value +//! (`cli.command_return(value=out_data, ...)`) is `out_data`, a plain list of +//! `[Module, "Loaded Address", "Size", "Adjusted Address", "Adjusted Size"]` +//! rows built as `[basename(m['image']), m['loaded_address'], m['loaded_size'], +//! ...]` -- confirming both the shape and the *reason* for it (it is this +//! Simics version generic table-command return convention, not anything +//! UEFI-specific). +//! +//! The confirmed real shape, converted from that live Python `repr()` into +//! `AttrValueType` terms: //! -//! A dict keyed by column name was picked over a positional list-of-columns -//! representation because it's self-describing and robust to `list-modules` -//! reordering or adding columns, and because Simics CLI commands that return -//! per-row structured data commonly do so as attribute dicts. If a real -//! `list-modules` return value turns out to instead be a positional list, only -//! [`parse_module_row`] needs to change; [`parse_module_list`]'s and -//! [`UefiOsInfo::resolve`]'s contracts are unaffected. -//! - This module's own output "name" (in the `(name, base, size, embedded_path)` -//! tuple) is *not* read from a raw field -- it's derived from `"name"`'s full -//! path via [`Path::file_name`], e.g. `PeiCore.efi`. +//! - The top-level value is a `List` of rows. +//! - Each row is itself a positional `List` (**not** a `Dict` keyed by column +//! name, as originally assumed), with at least 3 elements: +//! - `[0]` ("Module") -> `String`: the module bare basename only (e.g. +//! `DxeCore.efi`), or the literal string `""` if the tracker has no +//! image name for that mapping (both observed live) -- **not** the full +//! embedded build-machine path originally assumed. `list-modules` never +//! exposes that path at all; only the tracker own `params` attribute does +//! (populated from a locally-loaded `.map` file via `detect-parameters`/ +//! `load-parameters`), which is not applicable here since the whole point of +//! runtime module discovery is to work without already having that file. +//! - `[1]` ("Loaded Address") -> an integer (`Unsigned` or `Signed`; the real +//! capture addresses, e.g. `3744034816`, cross the FFI boundary as `Signed` +//! for the ranges observed). +//! - `[2]` ("Size") -> an integer, same representation as `[1]`. +//! - `[3]`/`[4]` ("Adjusted Address"/"Adjusted Size") -> an integer when the +//! tracker has separately loaded symbol info at a different address, +//! otherwise the literal empty `String("")` -- true for every module in the +//! real capture. This module has no use for either column and does not parse +//! them; [`parse_module_row`] only requires at least 3 columns to be present. +//! - A **real observed duplicate-name case** confirms the consequence of the +//! above: `BootScriptExecutorDxe.efi` appeared twice in the live capture, at +//! two different addresses, with **no other distinguishing information**. +//! Because `list-modules` never supplies a full path, [`parse_module_row`] +//! `embedded_path` output for every module is just its bare name (`[0]`) +//! wrapped in a `PathBuf` -- so [`UefiOsInfo::resolve`] path-suffix +//! disambiguation phase can never do better than its own bare-stem-match +//! fallback for real `list-modules`-sourced input. For any real duplicate-name +//! module, that fallback "fail open" behavior (log a warning, take the first +//! sorted local candidate) is therefore the **expected**, common outcome, not +//! a rare edge case -- see [`UefiOsInfo::resolve`] doc comment. +//! - This module own output "name" (in the `(name, base, size, embedded_path)` +//! tuple) is read directly from row `[0]` -- unlike the original assumption, +//! there is no full path to extract a bare filename from with +//! [`Path::file_name`]; row `[0]` already *is* the bare filename. -use std::{ - collections::BTreeMap, - path::{Path, PathBuf}, -}; +use std::path::{Path, PathBuf}; -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Result}; use simics::AttrValueType; use tracing::{debug, warn}; use walkdir::WalkDir; @@ -92,8 +134,10 @@ use walkdir::WalkDir; use crate::util::path_suffix_index::PathSuffixIndex; /// Parse the `AttrValueType` shape `list-modules` returns (see the module doc -/// comment for the assumed shape) into `(name, base, size, embedded_path)` -/// tuples, where `name` is the bare filename extracted from `embedded_path`. +/// comment for the confirmed real shape) into `(name, base, size, embedded_path)` +/// tuples, where `name` is the bare filename `list-modules` itself returns (there +/// is no full path to extract it from), and `embedded_path` is that same bare +/// name wrapped in a `PathBuf` (see the module doc comment for why). pub fn parse_module_list(value: &AttrValueType) -> Result> { let AttrValueType::List(rows) = value else { bail!( @@ -105,56 +149,60 @@ pub fn parse_module_list(value: &AttrValueType) -> Result Result<(String, u64, u64, PathBuf)> { - let AttrValueType::Dict(fields) = row else { + let AttrValueType::List(columns) = row else { bail!( - "expected each list-modules row to be an AttrValueType::Dict, got {:?}", + "expected each list-modules row to be an AttrValueType::List (positional \ + columns, not a Dict -- see the module doc comment), got {:?}", row ); }; - let embedded_path = PathBuf::from(dict_get_string(fields, "name")?); - let base = dict_get_unsigned(fields, "base")?; - let size = dict_get_unsigned(fields, "size")?; + if columns.len() < 3 { + bail!( + "expected each list-modules row to have at least 3 columns (Module, Loaded \ + Address, Size), got {} column(s): {:?}", + columns.len(), + row + ); + } - let name = embedded_path - .file_name() - .and_then(|n| n.to_str()) - .map(str::to_string) - .ok_or_else(|| { - anyhow!( - "embedded path {:?} in list-modules row has no file name component", - embedded_path - ) - })?; + let name = column_string(&columns[0], "Module")?; + let base = column_unsigned(&columns[1], "Loaded Address")?; + let size = column_unsigned(&columns[2], "Size")?; - Ok((name, base, size, embedded_path)) -} + // `list-modules` never returns a full embedded build-machine path (see the + // module doc comment) -- this bare basename, already extracted by the + // tracker itself, is all there is. + let embedded_path = PathBuf::from(&name); -fn dict_get<'a>( - fields: &'a BTreeMap, - key: &str, -) -> Result<&'a AttrValueType> { - fields - .get(&AttrValueType::String(key.to_string())) - .ok_or_else(|| anyhow!("list-modules row missing expected field {:?}", key)) + Ok((name, base, size, embedded_path)) } -fn dict_get_string(fields: &BTreeMap, key: &str) -> Result { - match dict_get(fields, key)? { +fn column_string(value: &AttrValueType, column: &str) -> Result { + match value { AttrValueType::String(s) => Ok(s.clone()), - other => bail!("expected field {:?} to be a String, got {:?}", key, other), + other => bail!( + "expected list-modules column {:?} to be a String, got {:?}", + column, + other + ), } } -fn dict_get_unsigned(fields: &BTreeMap, key: &str) -> Result { - match dict_get(fields, key)? { +fn column_unsigned(value: &AttrValueType, column: &str) -> Result { + match value { AttrValueType::Unsigned(u) => Ok(*u), AttrValueType::Signed(s) if *s >= 0 => Ok(*s as u64), other => bail!( - "expected field {:?} to be an unsigned integer, got {:?}", - key, + "expected list-modules column {:?} to be an unsigned integer, got {:?}", + column, other ), } @@ -165,12 +213,12 @@ fn dict_get_unsigned(fields: &BTreeMap, key: &str) /// /// Unlike `crate::os::windows::WindowsOsInfo`, which keys most of its state by /// CPU index (`HashMap`) because Windows tracks per-CPU current -/// process/module state, UEFI/SMM has no such per-CPU context -- it's a single +/// process/module state, UEFI/SMM has no such per-CPU context -- it is a single /// flat address space/module list -- so this holds a flat `Vec` instead. #[derive(Debug, Clone, Default)] pub struct UefiOsInfo { /// Resolved modules: `(name, base, resolved_local_debug_path)`. Feeding this - /// into the DWARF milestone's `DwarfModule::new(name, base, object)` (which + /// into the DWARF milestone `DwarfModule::new(name, base, object)` (which /// needs the `object::File` parsed from the path at `resolved_local_debug_path`) /// is explicitly out of scope for this milestone. pub modules: Vec<(String, u64, PathBuf)>, @@ -181,17 +229,27 @@ impl UefiOsInfo { /// build-root directory. /// /// For each module: - /// 1. Try matching the module's embedded path against a + /// 1. Try matching the module embedded path against a /// [`PathSuffixIndex`] built over `build_root`, longest suffix first. This /// disambiguates same-named modules whose embedded paths differ in a /// parent directory that also exists locally (e.g. two different EDK2 - /// package subdirectories). + /// package subdirectories) -- **when the caller actually has such an + /// embedded path to give it**. [`parse_module_list`] itself never can + /// (see its module doc comment: real `list-modules` output only ever + /// supplies a bare basename, confirmed live), so for input sourced from + /// it this phase degenerates to exactly the bare-stem fallback below; it + /// remains here as a general capability of this function for any other + /// caller/future data source that might supply a real embedded path. /// 2. If that finds nothing, fall back to a bare-filename-stem search /// (`rglob`-equivalent walk) under `build_root`. /// 3. If, after both, more than one candidate remains ambiguous, log a /// warning and take the first (sorted, for determinism) candidate -- - /// "fail open", the spec's own explicit decision, rather than erroring out - /// or dropping the module. + /// "fail open", the spec own explicit decision, rather than erroring out + /// or dropping the module. For any real duplicate-name module sourced from + /// live `list-modules` output, this is the **expected**, common outcome + /// (confirmed live: e.g. `BootScriptExecutorDxe.efi` appeared twice with + /// no distinguishing information beyond base address), not a rare edge + /// case. pub fn resolve

(modules: &[(String, u64, u64, PathBuf)], build_root: P) -> Result where P: AsRef, @@ -199,7 +257,7 @@ impl UefiOsInfo { let build_root = build_root.as_ref(); // `PathSuffixIndex::build_from_dir` does not hash file contents (unlike // `SourceCache::new`), which is the whole point of factoring it out of - // `SourceCache` -- see `src/util/path_suffix_index.rs`'s module doc. + // `SourceCache` -- see `src/util/path_suffix_index.rs` module doc. let index = PathSuffixIndex::build_from_dir(build_root)?; let mut resolved = Vec::with_capacity(modules.len()); @@ -213,8 +271,8 @@ impl UefiOsInfo { } } -/// Resolve a single module's local debug-info path. See -/// [`UefiOsInfo::resolve`]'s doc comment for the algorithm. +/// Resolve a single module local debug-info path. See +/// [`UefiOsInfo::resolve`] doc comment for the algorithm. fn resolve_one( index: &PathSuffixIndex, build_root: &Path, @@ -229,7 +287,7 @@ fn resolve_one( } // Fall back to a bare-filename-stem search, since the suffix index found no - // match at all (e.g. the embedded path's parent directories don't exist + // match at all (e.g. the embedded path parent directories do not exist // locally under any name that matches). let stem = embedded_path .file_stem() @@ -256,17 +314,17 @@ fn resolve_one( } n => { // Fail open: log and take the first (sorted) match rather than - // erroring out or dropping the module -- this is the spec's own + // erroring out or dropping the module -- this is the spec own // explicit decision, matching the `warn!`/`debug!` logging style // already used for similar disambiguation situations in - // `crate::os::windows` (see e.g. `src/os/windows/structs.rs`'s + // `crate::os::windows` (see e.g. `src/os/windows/structs.rs` // module-lookup logging). Unlike those call sites, this uses the // plain `tracing` crate rather than `simics::warn!`/`simics::debug!`: // the latter require a live `ConfObject` (e.g. // `get_object("tsffs")?`) and call real `SIM_*` FFI entry points, // which -- exactly like the bug fixed in `SourceCache::new` -- hard- // abort the process with no live Simics session, which is - // unconditionally true for this milestone's offline scope. + // unconditionally true for this milestone offline scope. warn!( "ambiguous local debug info for module {name:?}: {n} candidates matched stem \ {stem:?} with no unique path-suffix match (embedded path {embedded_path:?}); \ diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs index a1daa10c..bbdbf0cd 100644 --- a/tests/uefi_module_discovery_fixture.rs +++ b/tests/uefi_module_discovery_fixture.rs @@ -1,44 +1,55 @@ // Copyright (C) 2024 Intel Corporation // SPDX-License-Identifier: Apache-2.0 -//! End-to-end, offline test of UEFI module discovery's milestone-scope steps 1-2 -//! (`tsffs::uefi::{parse_module_list, UefiOsInfo}`, UCOV-M2) against a synthetic -//! fixture, entirely without a live Simics session -- mirroring -//! `tests/dwarf_fixture.rs`'s pattern on the sibling DWARF milestone branch +//! End-to-end, offline test of UEFI module discovery milestone-scope steps 1-2 +//! (`tsffs::uefi::{parse_module_list, UefiOsInfo}`, UCOV-M2) against fixtures +//! built from a **confirmed real** `list-modules` shape -- see `src/uefi/mod.rs` +//! module doc comment for exactly how and when that shape was confirmed (a live +//! QSP/X58 Simics session on the `the dev host` host, 2026-09-16) -- mirroring +//! `tests/dwarf_fixture.rs` pattern on the sibling DWARF milestone branch //! (`feat/dwarf-source-coverage-ucov-m1`). //! //! This lives under `tests/` (an integration test / separate cargo crate) rather //! than as a `#[cfg(test)]` module inside `src/uefi/mod.rs` for the same reason as -//! that file: this crate's `[lib]` section sets `test = false`, which disables the +//! that file: this crate `[lib]` section sets `test = false`, which disables the //! implicit unit-test harness for the library target, so `#[cfg(test)]` code //! inside `src/` is never compiled by `cargo test` for this crate. Integration //! tests under `tests/` are a separate cargo target and unaffected by that -//! setting. Being a separate crate also means this file only sees `tsffs`'s `pub` +//! setting. Being a separate crate also means this file only sees `tsffs` `pub` //! API -- `src/lib.rs` widens `uefi` to `pub` (from `pub(crate)`) for exactly this //! reason, same as `dwarf`/`source_cov` on the DWARF branch. //! -//! # No live Simics session, no real `list-modules` output was used +//! # A real `list-modules` output was used to build these fixtures //! -//! There is no BIOS image, boot, or live Simics session available in this offline -//! environment, so milestone-scope step 1 (the `list-modules` query mechanism) -//! reduces to testing `parse_module_list` against a *hand-constructed* fake -//! `AttrValueType` shape rather than a real one -- see `src/uefi/mod.rs`'s module -//! doc comment ("Assumed shape of `list-modules`' return value") for exactly what -//! shape is assumed and why, and for why `AttrValueType` (not `AttrValue`) is the -//! safe, FFI-free type to hand-construct offline. +//! Unlike the previous (offline-only, assumption-based) version of this file, +//! the fixtures below are built directly from a real, live tracker capture +//! (`qsp.software.tracker.list-modules max = 1000` against a real QSP/X58 +//! checkpoint, on `the dev host`, 2026-09-16), not a hand-guessed shape -- see +//! `src/uefi/mod.rs` module doc comment ("Confirmed shape of `list-modules` +//! return value") for the full capture and cross-check against the +//! `uefi_fw_tracker` component own installed Python source. //! -//! # Fixture +//! # Fixtures //! -//! The fixture models 7 modules using realistic-looking embedded build-machine -//! paths (the same `SimicsOpenBoardPkg`/`BoardX58Ich10`/`RELEASE_GCC5` structure -//! observed in a prior investigation's real, live 2026-03-12 tracker dump), -//! including the 3 real observed duplicate-name cases called out in the UCOV-M2 -//! spec -- `AcpiVTD.efi`, `MicrocodeUtilityDxe.efi`, `SataController.efi` -- each -//! appearing twice with distinct embedded paths (fabricated `PkgA`/`PkgB` -//! subdirectories, per the spec's own suggestion) and distinct base addresses. +//! [`FIXTURE_ROWS`] models a subset of the real 78-module live capture, with the +//! real observed duplicate-name case (`BootScriptExecutorDxe.efi`, appearing +//! twice at different addresses with no other distinguishing information) and the +//! real observed "no image name known" case (`""`) both included. It is +//! used by both [`parses_confirmed_real_module_list_shape_with_duplicate_names`] +//! (testing [`parse_module_list`] alone) and +//! [`resolve_falls_open_on_the_real_duplicate_name_case`] (testing the full +//! `parse_module_list` -> [`UefiOsInfo::resolve`] pipeline end-to-end). +//! +//! [`resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths`] +//! separately tests [`UefiOsInfo::resolve`] own generic path-suffix +//! disambiguation capability against hand-built `(name, base, size, +//! embedded_path)` tuples carrying full, distinguishing paths -- real +//! `list-modules` output never supplies such a path (confirmed live, see above), +//! but `UefiOsInfo::resolve` is a generic utility not solely fed from +//! `parse_module_list`, so this capability is still worth testing directly. use std::{ - collections::{BTreeMap, HashMap}, + collections::HashMap, fs::{create_dir_all, write}, io, path::PathBuf, @@ -51,167 +62,186 @@ use tempfile::tempdir; use tracing_subscriber::fmt::MakeWriter; use tsffs::uefi::{parse_module_list, UefiOsInfo}; -/// One row of the fixture: an embedded build-machine path plus its base/size, in -/// the shape `parse_module_list` is documented (see `src/uefi/mod.rs`) to expect. +/// One row of the confirmed-real-shape fixture: a bare basename (exactly what +/// `list-modules` itself returns, per the module doc comment), a loaded address, +/// and a size. Real rows also carry "Adjusted Address"/"Adjusted Size" columns, +/// empty in the live capture and unused by this milestone; [`fixture_attr_value`] +/// still includes them (as empty strings) for fidelity to the real capture, and +/// `parse_module_row` tolerates that (it only requires at least 3 columns). struct FixtureRow { - embedded_path: String, + name: &'static str, base: u64, size: u64, } -/// The fixture's common embedded build-machine path prefix, matching the real -/// structure observed in the prior investigation's live tracker dump. -const PREFIX: &str = - "/home/robertgu/mydev/simics-build/Build/SimicsOpenBoardPkg/BoardX58Ich10/RELEASE_GCC5/IA32"; - -/// Build the fixture's 7 rows: one unique module (`PeiCore.efi`) plus the 3 real -/// observed duplicate-name cases, each appearing twice under fabricated `PkgA`/ -/// `PkgB` package subdirectories with distinct embedded paths and base addresses. -fn fixture_rows() -> Vec { - vec![ - FixtureRow { - embedded_path: format!("{PREFIX}/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.efi"), - base: 0x0000_0000_0082_0000, - size: 0x9000, - }, - FixtureRow { - embedded_path: format!("{PREFIX}/PkgA/Feature/AcpiVTD/DEBUG/AcpiVTD.efi"), - base: 0x0000_0000_0700_0000, - size: 0x4000, - }, - FixtureRow { - embedded_path: format!("{PREFIX}/PkgB/Feature/AcpiVTD/DEBUG/AcpiVTD.efi"), - base: 0x0000_0000_0710_0000, - size: 0x4200, - }, - FixtureRow { - embedded_path: format!( - "{PREFIX}/PkgA/Universal/MicrocodeUtilityDxe/DEBUG/MicrocodeUtilityDxe.efi" - ), - base: 0x0000_0000_0720_0000, - size: 0x3000, - }, - FixtureRow { - embedded_path: format!( - "{PREFIX}/PkgB/Universal/MicrocodeUtilityDxe/DEBUG/MicrocodeUtilityDxe.efi" - ), - base: 0x0000_0000_0730_0000, - size: 0x3100, - }, - FixtureRow { - embedded_path: format!( - "{PREFIX}/PkgA/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" - ), - base: 0x0000_0000_0740_0000, - size: 0x5000, - }, - FixtureRow { - embedded_path: format!( - "{PREFIX}/PkgB/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" - ), - base: 0x0000_0000_0750_0000, - size: 0x5100, - }, - ] -} - -/// Build the fake `AttrValueType` shape `list-modules` is assumed to return (see -/// `src/uefi/mod.rs`'s module doc comment) for a set of fixture rows: a `List` of -/// `Dict`s, each keyed by `"name"` (the full embedded path, as a `String`), -/// `"base"`, and `"size"` (both `Unsigned`). +/// A representative subset of the real 78-row live capture (see the module doc +/// comment in `src/uefi/mod.rs`), including both real observed edge cases: the +/// duplicate-name module (`BootScriptExecutorDxe.efi`, two entries, two +/// addresses, otherwise indistinguishable) and the "no image name known" module +/// (`""`). +const FIXTURE_ROWS: &[FixtureRow] = &[ + FixtureRow { + name: "DxeCore.efi", + base: 3_744_034_816, + size: 189_184, + }, + FixtureRow { + name: "PcdDxe.efi", + base: 3_740_880_896, + size: 23_680, + }, + FixtureRow { + name: "BootScriptExecutorDxe.efi", + base: 3_739_889_664, + size: 84_224, + }, + FixtureRow { + name: "BootScriptExecutorDxe.efi", + base: 3_722_764_288, + size: 84_224, + }, + FixtureRow { + name: "", + base: 3_722_997_760, + size: 195_360, + }, +]; + +/// Build the real, confirmed `AttrValueType` shape `list-modules` returns (see +/// `src/uefi/mod.rs` module doc comment): a `List` of `List` rows (positional +/// columns, not a `Dict`), each `[Module, "Loaded Address", "Size", "Adjusted +/// Address", "Adjusted Size"]`, with the trailing two columns empty strings -- +/// exactly as observed in the real live capture for every module. fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { AttrValueType::List( rows.iter() .map(|row| { - let mut fields = BTreeMap::new(); - fields.insert( - AttrValueType::String("name".to_string()), - AttrValueType::String(row.embedded_path.clone()), - ); - fields.insert( - AttrValueType::String("base".to_string()), - AttrValueType::Unsigned(row.base), - ); - fields.insert( - AttrValueType::String("size".to_string()), - AttrValueType::Unsigned(row.size), - ); - AttrValueType::Dict(fields) + AttrValueType::List(vec![ + AttrValueType::String(row.name.to_string()), + AttrValueType::Signed(row.base as i64), + AttrValueType::Signed(row.size as i64), + AttrValueType::String(String::new()), + AttrValueType::String(String::new()), + ]) }) .collect(), ) } #[test] -fn parses_synthetic_module_list_with_duplicate_names() -> Result<()> { - let rows = fixture_rows(); - let value = fixture_attr_value(&rows); - - let parsed = parse_module_list(&value)?; - assert_eq!(parsed.len(), rows.len()); - - for (row, (name, base, size, embedded_path)) in rows.iter().zip(parsed.iter()) { - let expected_name = PathBuf::from(&row.embedded_path) - .file_name() - .expect("fixture embedded path has a file name") - .to_str() - .expect("fixture file name is valid UTF-8") - .to_string(); - - assert_eq!(name, &expected_name); +fn parses_confirmed_real_module_list_shape_with_duplicate_names() -> Result<()> { + let parsed = parse_module_list(&fixture_attr_value(FIXTURE_ROWS))?; + assert_eq!(parsed.len(), FIXTURE_ROWS.len()); + + for (row, (name, base, size, embedded_path)) in FIXTURE_ROWS.iter().zip(parsed.iter()) { + assert_eq!(name, row.name); assert_eq!(*base, row.base); assert_eq!(*size, row.size); - assert_eq!(embedded_path, &PathBuf::from(&row.embedded_path)); + // Confirmed live: list-modules never supplies a full path, so + // parse_module_row embedded_path for every module is just the bare + // name it was given, wrapped. + assert_eq!(embedded_path, &PathBuf::from(row.name)); } - // The 3 real observed duplicate-name cases: each must appear exactly twice, - // with distinct embedded paths and distinct base addresses (not accidentally - // collapsed/deduplicated by the parser, and not confused with each other). - for dup_name in ["AcpiVTD.efi", "MicrocodeUtilityDxe.efi", "SataController.efi"] { - let matches: Vec<_> = parsed.iter().filter(|(name, ..)| name == dup_name).collect(); - assert_eq!( - matches.len(), - 2, - "expected exactly 2 parsed entries for duplicate-name module {dup_name}" - ); - assert_ne!( - matches[0].3, matches[1].3, - "the two {dup_name} entries must have distinct embedded paths" - ); - assert_ne!( - matches[0].1, matches[1].1, - "the two {dup_name} entries must have distinct base addresses" - ); - } + // The real observed duplicate-name case: exactly 2 entries, distinguishable + // only by base address (not accidentally collapsed/deduplicated by the + // parser). + let dup: Vec<_> = parsed + .iter() + .filter(|(name, ..)| name == "BootScriptExecutorDxe.efi") + .collect(); + assert_eq!( + dup.len(), + 2, + "expected exactly 2 parsed entries for the real observed duplicate-name module" + ); + assert_ne!( + dup[0].1, dup[1].1, + "the two entries must have distinct base addresses (the only thing distinguishing them)" + ); + assert_eq!( + dup[0].3, dup[1].3, + "list-modules gives both the exact same bare-name embedded_path -- there is no way to \ + tell them apart by path" + ); + + // The real observed "no image name known" case. + assert!( + parsed.iter().any(|(name, ..)| name == ""), + "expected the real observed \"\" module name to survive parsing unchanged" + ); Ok(()) } +/// One row of the hand-built, full-path fixture used only by +/// [`resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths`] +/// below, to exercise [`UefiOsInfo::resolve`] own generic path-suffix +/// disambiguation capability -- independent of [`parse_module_list`], which +/// (confirmed live) never actually has a full path to supply. +struct HandBuiltModuleRow { + embedded_path: String, + base: u64, + size: u64, +} + +/// The fixture common embedded build-machine path prefix, matching the real +/// structure observed in the live capture that this branch investigation +/// confirmed (see `src/uefi/mod.rs` module doc comment). +const PREFIX: &str = + "/home/user/bios-x58i/project/workspace/Build/SimicsOpenBoardPkg/BoardX58Ich10/DEBUG_GCC/X64"; + +/// Build 4 hand-built rows: the 2 real observed duplicate-name case +/// (`BootScriptExecutorDxe.efi`) under fabricated `PkgA`/`PkgB` subdirectories, +/// with distinct full embedded paths and base addresses matching +/// [`FIXTURE_ROWS`]. +fn hand_built_rows() -> Vec { + vec![ + HandBuiltModuleRow { + embedded_path: format!( + "{PREFIX}/PkgA/Universal/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" + ), + base: 3_739_889_664, + size: 84_224, + }, + HandBuiltModuleRow { + embedded_path: format!( + "{PREFIX}/PkgB/Universal/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" + ), + base: 3_722_764_288, + size: 84_224, + }, + ] +} + #[test] -fn resolves_duplicate_names_via_path_suffix_disambiguation() -> Result<()> { +fn resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths() -> Result<()> { let tmp = tempdir()?; let root = tmp.path(); - let rows = fixture_rows(); - let modules = parse_module_list(&fixture_attr_value(&rows))? - .into_iter() - .filter(|(name, ..)| name == "AcpiVTD.efi" || name == "MicrocodeUtilityDxe.efi") - .collect::>(); - assert_eq!(modules.len(), 4); - - // Mirror each relevant fixture row's embedded path locally, from "IA32/" - // onward (i.e. the "PkgA/..." / "PkgB/..." part), as a real local file with - // content unique to that row -- used below to positively confirm which exact - // local file each module resolved to, not just "a" file. - let mut expected_local_paths: HashMap = HashMap::new(); - for row in rows + let rows = hand_built_rows(); + let modules: Vec<(String, u64, u64, PathBuf)> = rows .iter() - .filter(|row| modules.iter().any(|(_, base, ..)| *base == row.base)) - { + .map(|row| { + ( + "BootScriptExecutorDxe.efi".to_string(), + row.base, + row.size, + PathBuf::from(&row.embedded_path), + ) + }) + .collect(); + + // Mirror each row embedded path locally, from "X64/" onward (i.e. the + // "PkgA/..." / "PkgB/..." part), as a real local file with content unique to + // that row -- used below to positively confirm which exact local file each + // module resolved to, not just "a" file. + let mut expected_local_paths: HashMap = HashMap::new(); + for row in &rows { let suffix = row .embedded_path - .rsplit_once("IA32/") - .expect("fixture embedded path contains the IA32/ prefix marker") + .rsplit_once("X64/") + .expect("fixture embedded path contains the X64/ prefix marker") .1; let local_path = root.join(suffix); create_dir_all( @@ -224,7 +254,7 @@ fn resolves_duplicate_names_via_path_suffix_disambiguation() -> Result<()> { } let info = UefiOsInfo::resolve(&modules, root)?; - assert_eq!(info.modules.len(), 4); + assert_eq!(info.modules.len(), 2); for (name, base, resolved_path) in &info.modules { let row = rows @@ -237,35 +267,25 @@ fn resolves_duplicate_names_via_path_suffix_disambiguation() -> Result<()> { assert_eq!( resolved_path, expected, - "module {name} (base {base:#x}) resolved to the wrong local file; \ - path-suffix disambiguation failed" + "module {name} (base {base:#x}) resolved to the wrong local file; path-suffix \ + disambiguation failed" ); } - // The actual point of this test, not a vacuous pass: the two AcpiVTD.efi - // entries must resolve to two *distinct* local files (and likewise for - // MicrocodeUtilityDxe.efi) -- a resolver that just grabbed "any" file matching - // the bare name would pass the per-module checks above only by accident, but - // could not pass this. - for dup_name in ["AcpiVTD.efi", "MicrocodeUtilityDxe.efi"] { - let resolved: Vec<&PathBuf> = info - .modules - .iter() - .filter(|(name, ..)| name == dup_name) - .map(|(_, _, path)| path) - .collect(); - assert_eq!(resolved.len(), 2); - assert_ne!( - resolved[0], resolved[1], - "the two {dup_name} modules must resolve to distinct local files" - ); - } + // The actual point of this test, not a vacuous pass: given full, + // distinguishing embedded paths, the two same-named modules must resolve to + // two *distinct* local files. + assert_ne!( + info.modules[0].2, info.modules[1].2, + "the two same-named modules must resolve to distinct local files when given distinct \ + full embedded paths" + ); Ok(()) } /// A `tracing_subscriber::fmt::MakeWriter` that captures formatted log output into -/// a shared in-memory buffer, so the test below can assert on it directly instead +/// a shared in-memory buffer, so the tests below can assert on it directly instead /// of only inferring the warning fired from behavior. #[derive(Clone, Default)] struct CapturingWriter(Arc>>); @@ -293,27 +313,30 @@ impl<'a> MakeWriter<'a> for CapturingWriter { } #[test] -fn falls_back_to_stem_match_and_warns_on_ambiguity_when_suffix_match_fails() -> Result<()> { +fn resolve_falls_open_on_the_real_duplicate_name_case() -> Result<()> { let tmp = tempdir()?; let root = tmp.path(); - let rows = fixture_rows(); - let modules = parse_module_list(&fixture_attr_value(&rows))? + // The full, realistic pipeline: parse_module_list on the confirmed-real + // fixture, not a hand-built one -- so both BootScriptExecutorDxe.efi entries + // get parse_module_row own embedded_path (just the bare name, see its doc + // comment), exactly as real list-modules output would. + let modules: Vec<_> = parse_module_list(&fixture_attr_value(FIXTURE_ROWS))? .into_iter() - .filter(|(name, ..)| name == "SataController.efi") - .collect::>(); + .filter(|(name, ..)| name == "BootScriptExecutorDxe.efi") + .collect(); assert_eq!(modules.len(), 2); - // Deliberately unrelated local directory layouts: neither shares any path - // component with the fixture's fabricated ".../PkgA|PkgB/Bus/Pci/ - // SataControllerDxe/DEBUG/" embedded-path tail beyond the bare file name - // itself. Path-suffix matching therefore finds the bare file name - // "SataController.efi" as a candidate suffix, but with *two* different local - // files sharing it -- an ambiguous, not unique, match -- so per - // `PathSuffixIndex::lookup_components_unambiguous`'s contract it must return - // no match at all, forcing both modules through the bare-stem fallback path. - let path_1 = root.join("unrelated_layout_one").join("SataController.efi"); - let path_2 = root.join("unrelated_layout_two").join("SataController.efi"); + // Two unrelated local directory layouts, both happening to contain a file + // with the exact bare name "BootScriptExecutorDxe.efi" -- the only kind of + // local layout that parse_module_list-sourced input can ever match against, + // since it never has more than a bare name to go on. + let path_1 = root + .join("unrelated_layout_one") + .join("BootScriptExecutorDxe.efi"); + let path_2 = root + .join("unrelated_layout_two") + .join("BootScriptExecutorDxe.efi"); create_dir_all(path_1.parent().expect("has parent"))?; create_dir_all(path_2.parent().expect("has parent"))?; write(&path_1, b"layout one")?; @@ -326,24 +349,23 @@ fn falls_back_to_stem_match_and_warns_on_ambiguity_when_suffix_match_fails() -> .with_max_level(tracing::Level::TRACE) .finish(); - let info = tracing::subscriber::with_default(subscriber, || UefiOsInfo::resolve(&modules, root))?; + let info = + tracing::subscriber::with_default(subscriber, || UefiOsInfo::resolve(&modules, root))?; assert_eq!(info.modules.len(), 2); - // Fail-open (the spec's own explicit decision): both modules still resolve -- - // not an error, not a dropped module -- to the first candidate in sorted - // order. Both modules share the exact same ambiguous candidate set (the same - // two unrelated local files), so both must fail open to the exact same - // resolved path. + // Fail-open (the spec own explicit decision, and -- confirmed live -- the + // expected outcome for any real duplicate-name module, not a rare edge + // case): both modules still resolve, to the first candidate in sorted order. let mut sorted_candidates = [path_1.clone(), path_2.clone()]; sorted_candidates.sort(); let expected_fail_open_path = sorted_candidates[0].clone(); for (name, _base, resolved_path) in &info.modules { - assert_eq!(name, "SataController.efi"); + assert_eq!(name, "BootScriptExecutorDxe.efi"); assert_eq!( resolved_path, &expected_fail_open_path, - "expected fail-open fallback to deterministically pick the first \ - (sorted) ambiguous candidate" + "expected fail-open fallback to deterministically pick the first (sorted) \ + ambiguous candidate" ); } From 73bc6703f0c42e27bad2bbb0a20eab4ee337b3e3 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 15:45:02 +0200 Subject: [PATCH 06/18] Switch UEFI module discovery to tracker_obj->maps, not list-modules A follow-up live investigation in a test session (real Simics 6.0.189 session, real checkpoint past DXE dispatch, 68 real loaded UEFI modules) found a richer, confirmed-real data source than list-modules: the uefi_fw_tracker component's underlying C object's `maps` attribute, reached via the same run_command FFI path already used elsewhere (e.g. `tracker_obj->maps`). Unlike list-modules (which calls basename() on the underlying data before returning it, so it only ever exposes a bare filename), tracker_obj->maps returns 7-element rows that carry the full embedded build-machine path: [loaded_address, loaded_size, , adjusted_address, adjusted_size, , full_path_string]. - Rewrite parse_module_list/parse_module_row for the 7-element positional row shape instead of the previously confirmed 5-element list-modules shape. A row's "name" is now derived from the full path via Path::file_name when present, or the existing UNKNOWN_MODULE_NAME ("") placeholder for genuinely pathless rows. - Make path-suffix disambiguation (PathSuffixIndex) the primary resolution path in UefiOsInfo::resolve, since a full embedded path is now the common case rather than a rare bonus that real list-modules output could never actually supply. Bare-stem search is now purely a fallback for when suffix-matching finds nothing; a genuinely pathless row fails explicitly instead of guessing. - Rebuild the fixture tests around the real 7-element shape: the real BootScriptExecutorDxe.efi duplicate (identical embedded path for both instances) now resolves cleanly with no ambiguity warning, since it turns out not to be genuinely ambiguous once the real path is available; a fabricated different-path AcpiVTD.efi duplicate proves suffix disambiguation still works when real ambiguity exists; and a pathless row confirms graceful naming/fallback (a clean Err, not a panic) rather than a crash or silent misparse. cargo test --target x86_64-pc-windows-gnu --test uefi_module_discovery_fixture passes 5/5 on this machine. Co-Authored-By: Claude Sonnet 5 --- src/uefi/mod.rs | 354 ++++++++------- tests/uefi_module_discovery_fixture.rs | 604 +++++++++++++++---------- 2 files changed, 565 insertions(+), 393 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 48dbd5b0..19de09cc 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -7,27 +7,34 @@ //! //! Unlike Windows (`crate::os::windows`), Simics has no native C interface for //! UEFI module discovery -- there is no `osa_target_info` (checked directly -//! against the Simics 6/7 headers). The only mechanism confirmed to work is the -//! `uefi_fw_tracker` component's `list-modules` CLI command, invoked from Rust via +//! against the Simics 6/7 headers). The confirmed-real mechanism, found via a +//! live investigation (real Simics 6.0.189 session, a real +//! checkpoint past DXE dispatch, 68 real loaded UEFI modules), is the +//! `uefi_fw_tracker` component's underlying C object's `maps` attribute, reached +//! from Rust via Simics's CLI arrow-attribute syntax and the exact same FFI +//! entry point TSFFS already uses elsewhere in this module's design: //! `simics::api::simulator::script::run_command(String) -> Result` -//! (e.g. `run_command("$system.soft.tracker.list-modules max = 1000")`, where the -//! `$system.soft.tracker` object path is board-specific and must be supplied by -//! the caller, not hardcoded -- confirmed live to be `qsp.software.tracker` on the -//! `examples/tutorials/edk2-simics-platform` tutorial a live checkpoint, see below). -//! Calling `run_command` for real, and everything downstream of it (wiring into -//! `crate::haps`/`HARNESS_START`, a `self.uefi` attribute on `Tsffs`, touching the -//! OS enum), is explicitly out of scope for this milestone -- see the UCOV-M2 -//! spec milestone-scope step 3. +//! (e.g. `run_command("tracker_obj->maps")`, where the +//! `qsp.software.tracker` object path is board-specific and must be supplied by +//! the caller, not hardcoded). This supersedes an earlier design that queried the +//! tracker's `list-modules` CLI command instead: `list-modules` itself calls +//! `basename()` on the underlying data before returning it, so it only ever +//! yields a bare filename, never a full path -- `tracker_obj->maps` is the same +//! underlying data with the full path intact. Calling `run_command` for real, +//! and everything downstream of it (wiring into `crate::haps`/`HARNESS_START`, a +//! `self.uefi` attribute on `Tsffs`, touching the OS enum), is explicitly out of +//! scope for this milestone -- see the UCOV-M2 spec's milestone-scope step 3. //! //! This module implements only the two pieces of that spec that are testable //! completely offline, with no live Simics session and no real BIOS/UEFI image: //! //! 1. [`parse_module_list`]: parse the `AttrValue`/`AttrValueType` shape -//! `list-modules` returns into `(name, base, size, embedded_path)` tuples. +//! `tracker_obj->maps` returns into `(name, base, size, embedded_path)` +//! tuples. //! 2. [`UefiOsInfo::resolve`]: given those tuples and a local build-root -//! directory, resolve each module real local debug-info path. +//! directory, resolve each module's real local debug-info path. //! -//! # Why `AttrValueType`, not `AttrValue`, as the parser input type +//! # Why `AttrValueType`, not `AttrValue`, as the parser's input type //! //! `simics::AttrValue` is a `#[repr(C)]` wrapper around the C `attr_value_t` //! union (see `simics::api::base::attr_value`). Reading a real, already-populated @@ -39,109 +46,95 @@ //! fixture would need) allocates through `SIM_alloc_attr_list`/`SIM_alloc_attr_dict` //! -- real FFI entry points into `libsimics-common.dll`. Exactly like the //! `get_object("tsffs")` call removed from `SourceCache::new` (see -//! `src/source_cov/mod.rs` and `tests/dwarf_fixture.rs` module doc), calling any +//! `src/source_cov/mod.rs` and `tests/dwarf_fixture.rs`'s module doc), calling any //! `SIM_*` entry point with no live Simics session hard-aborts the process, not //! just returns `Err`. `AttrValueType` (the plain Rust tagged-union enum //! `Invalid | Nil | Unsigned(u64) | Signed(i64) | Bool(bool) | String(String) | //! Float(..) | Object(*mut ConfObject) | Data(Box<[u8]>) | List(Vec) | //! Dict(BTreeMap)`) has no such constructors -- its variants are built -//! with plain Rust syntax, no FFI at all -- so it is what this module parser +//! with plain Rust syntax, no FFI at all -- so it is what this module's parser //! takes, and what the offline tests construct fixtures as. At a real call site, //! converting the real `AttrValue` returned by `run_command` into `AttrValueType` //! via `.into()` (`impl From for AttrValueType`) is the safe, pure-read //! conversion described above; this module never needs to go the other direction. //! -//! # Confirmed shape of `list-modules` return value +//! # Confirmed shape of `tracker_obj->maps`' return value //! -//! This shape was originally an explicit, documented *assumption* (there was no -//! live Simics session available to check it against), but it has since been -//! **confirmed against a real, live Simics session**, and turned out to be wrong -//! in every particular. The confirmation: +//! Unlike the superseded `list-modules`-based design (which had to *assume* a +//! shape, since it was never actually queried live), this shape is a confirmed +//! fact, captured from a real live tracker dump reached from Rust via +//! `run_command`, using the exact same FFI path this module documents above: //! -//! - On 2026-09-16, on the `the dev host` host, a real a live checkpoint was booted to a -//! checkpoint (`~/tsffs-bmc-bios-poc/bios-x58i/project/checkpoint.ckpt`, itself -//! produced from the same `BoardX58Ich10`/`qsp-uefi-custom` setup this crate own -//! `examples/tutorials/edk2-simics-platform` tutorial uses) with the -//! `uefi_fw_tracker` inserted and re-enabled (`qsp.software.enable-tracker`) -//! after loading the checkpoint. The real object path is `qsp.software.tracker` -//! (not the generic `$system.soft.tracker` placeholder above). -//! - `simics.SIM_run_command("qsp.software.tracker.list-modules max = 1000")` -- -//! the exact Python-level equivalent of this crate own -//! `run_command(String) -> Result` -- was called directly, and its -//! real Python `type()`/`repr()` captured (not the pretty-printed CLI table). -//! It returned a plain Python `list` of 78 real modules, each itself a plain -//! Python `list` of 5 elements, e.g. -//! `['DxeCore.efi', 3744034816, 189184, '', '']`. -//! - This was cross-checked against the `uefi_fw_tracker` component own installed -//! Python source (`simmod/uefi_fw_tracker/module_load.py` `get_mappings`/ -//! `list_modules`/`mappings_table_properties`), identical across every -//! installed Simics-Base version checked (6.0.189, 7.74.0, 7.100.0, 7.106.0): -//! `list-modules` is a generic Simics *table* command -//! (`table.new_table_command`), and its programmatic return value -//! (`cli.command_return(value=out_data, ...)`) is `out_data`, a plain list of -//! `[Module, "Loaded Address", "Size", "Adjusted Address", "Adjusted Size"]` -//! rows built as `[basename(m['image']), m['loaded_address'], m['loaded_size'], -//! ...]` -- confirming both the shape and the *reason* for it (it is this -//! Simics version generic table-command return convention, not anything -//! UEFI-specific). +//! - The top-level value is a `List` of rows. +//! - Each row is itself a positional `List` of exactly 7 elements (**not** a dict +//! keyed by column name, unlike the superseded design): //! -//! The confirmed real shape, converted from that live Python `repr()` into -//! `AttrValueType` terms: +//! ```text +//! [loaded_address, loaded_size, , adjusted_address, adjusted_size, , full_path_string] +//! ``` //! -//! - The top-level value is a `List` of rows. -//! - Each row is itself a positional `List` (**not** a `Dict` keyed by column -//! name, as originally assumed), with at least 3 elements: -//! - `[0]` ("Module") -> `String`: the module bare basename only (e.g. -//! `DxeCore.efi`), or the literal string `""` if the tracker has no -//! image name for that mapping (both observed live) -- **not** the full -//! embedded build-machine path originally assumed. `list-modules` never -//! exposes that path at all; only the tracker own `params` attribute does -//! (populated from a locally-loaded `.map` file via `detect-parameters`/ -//! `load-parameters`), which is not applicable here since the whole point of -//! runtime module discovery is to work without already having that file. -//! - `[1]` ("Loaded Address") -> an integer (`Unsigned` or `Signed`; the real -//! capture addresses, e.g. `3744034816`, cross the FFI boundary as `Signed` -//! for the ranges observed). -//! - `[2]` ("Size") -> an integer, same representation as `[1]`. -//! - `[3]`/`[4]` ("Adjusted Address"/"Adjusted Size") -> an integer when the -//! tracker has separately loaded symbol info at a different address, -//! otherwise the literal empty `String("")` -- true for every module in the -//! real capture. This module has no use for either column and does not parse -//! them; [`parse_module_row`] only requires at least 3 columns to be present. -//! - A **real observed duplicate-name case** confirms the consequence of the -//! above: `BootScriptExecutorDxe.efi` appeared twice in the live capture, at -//! two different addresses, with **no other distinguishing information**. -//! Because `list-modules` never supplies a full path, [`parse_module_row`] -//! `embedded_path` output for every module is just its bare name (`[0]`) -//! wrapped in a `PathBuf` -- so [`UefiOsInfo::resolve`] path-suffix -//! disambiguation phase can never do better than its own bare-stem-match -//! fallback for real `list-modules`-sourced input. For any real duplicate-name -//! module, that fallback "fail open" behavior (log a warning, take the first -//! sorted local candidate) is therefore the **expected**, common outcome, not -//! a rare edge case -- see [`UefiOsInfo::resolve`] doc comment. -//! - This module own output "name" (in the `(name, base, size, embedded_path)` -//! tuple) is read directly from row `[0]` -- unlike the original assumption, -//! there is no full path to extract a bare filename from with -//! [`Path::file_name`]; row `[0]` already *is* the bare filename. +//! A real captured example row: +//! +//! ```text +//! [3744034816, 189184, True, 3744034816, 189184, True, +//! '/home/user/bios-x58i/project/workspace/Build/SimicsOpenBoardPkg/BoardX58Ich10/DEBUG_GCC/X64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.efi'] +//! ``` +//! +//! This module reads only 3 of the 7 elements: +//! - index 0 (`loaded_address`) -> `Unsigned`/`Signed`: the module's loaded/base +//! address. (`index 3`, `adjusted_address`, is a distinct post-relocation +//! address also present in the real data, but out of scope for this +//! milestone -- nothing downstream of this module currently consumes it.) +//! - index 1 (`loaded_size`) -> `Unsigned`/`Signed`: the module's size in bytes. +//! - index 6 (`full_path_string`) -> `String`, **or absent/empty**: the +//! module's full embedded build-machine path. Some rows genuinely have no +//! path at all -- real "unknown"/unresolved modules that the tracker could +//! not identify (mirroring `module_load.py`'s own upstream guard for +//! `m['image'] is None`) -- represented here as an empty string. Unlike the +//! superseded design, there is no separate "name" field at all: with this +//! row shape, a module's short display name must be *derived* from the full +//! path via [`Path::file_name`] when a path is present, e.g. `DxeCore.efi` +//! from the example above. See [`parse_module_row`] for how a missing/empty +//! path is named instead (``). +//! - indices 2, 4, 5 (the two booleans and `adjusted_size`) are not read by +//! this module; they are out of scope for this milestone. +//! +//! A real, observed duplicate-name case -- two loaded instances of +//! `BootScriptExecutorDxe.efi`, at two different addresses, seen both via +//! `list-modules` and via `tracker_obj->maps` -- was re-examined under this +//! richer source and turned out to have the **identical** full path for both +//! instances (the same build loaded twice, not two different binaries). So for +//! that specific real case, path-suffix disambiguation is unnecessary -- any +//! single matching local file is correct for both addresses. This does *not* +//! prove disambiguation is unnecessary in general: two genuinely different +//! builds sharing a basename (e.g. two different EDK2 package subdirectories) +//! remains a real possibility this module still needs to handle correctly, since +//! that scenario is not disproven for all cases, just this one -- see +//! [`UefiOsInfo::resolve`]'s doc comment. use std::path::{Path, PathBuf}; -use anyhow::{bail, Result}; +use anyhow::{anyhow, bail, Result}; use simics::AttrValueType; use tracing::{debug, warn}; use walkdir::WalkDir; use crate::util::path_suffix_index::PathSuffixIndex; -/// Parse the `AttrValueType` shape `list-modules` returns (see the module doc -/// comment for the confirmed real shape) into `(name, base, size, embedded_path)` -/// tuples, where `name` is the bare filename `list-modules` itself returns (there -/// is no full path to extract it from), and `embedded_path` is that same bare -/// name wrapped in a `PathBuf` (see the module doc comment for why). +/// Placeholder name used for a module whose row has no path at all (a real +/// "unknown"/unresolved module -- see the module doc comment's "Confirmed shape" +/// section). Matches the placeholder `list-modules` itself used for such rows in +/// the real capture that motivated this design. +pub const UNKNOWN_MODULE_NAME: &str = ""; + +/// Parse the `AttrValueType` shape `tracker_obj->maps` returns (see the module +/// doc comment for the confirmed shape) into `(name, base, size, embedded_path)` +/// tuples, where `name` is the bare filename extracted from `embedded_path`, or +/// [`UNKNOWN_MODULE_NAME`] when a row has no path. pub fn parse_module_list(value: &AttrValueType) -> Result> { let AttrValueType::List(rows) = value else { bail!( - "expected list-modules result to be an AttrValueType::List, got {:?}", + "expected tracker_obj->maps result to be an AttrValueType::List, got {:?}", value ); }; @@ -149,76 +142,96 @@ pub fn parse_module_list(value: &AttrValueType) -> Resultmaps` shape: +/// `[loaded_address, loaded_size, , adjusted_address, adjusted_size, +/// , full_path_string]`. Only indices 0 (`base`), 1 (`size`), and 6 +/// (`embedded_path`) are read; see the module doc comment for why the others are +/// out of scope. fn parse_module_row(row: &AttrValueType) -> Result<(String, u64, u64, PathBuf)> { - let AttrValueType::List(columns) = row else { + let AttrValueType::List(elements) = row else { bail!( - "expected each list-modules row to be an AttrValueType::List (positional \ - columns, not a Dict -- see the module doc comment), got {:?}", + "expected each tracker_obj->maps row to be an AttrValueType::List, got {:?}", row ); }; - if columns.len() < 3 { + let Ok([loaded_address, loaded_size, _, _adjusted_address, _adjusted_size, _, full_path]) = + <[AttrValueType; 7]>::try_from(elements.clone()) + else { bail!( - "expected each list-modules row to have at least 3 columns (Module, Loaded \ - Address, Size), got {} column(s): {:?}", - columns.len(), + "expected each tracker_obj->maps row to have exactly 7 elements, got {}: {:?}", + elements.len(), row ); - } + }; - let name = column_string(&columns[0], "Module")?; - let base = column_unsigned(&columns[1], "Loaded Address")?; - let size = column_unsigned(&columns[2], "Size")?; + let base = list_get_unsigned(&loaded_address, 0)?; + let size = list_get_unsigned(&loaded_size, 1)?; + let embedded_path_str = list_get_string(&full_path, 6)?; - // `list-modules` never returns a full embedded build-machine path (see the - // module doc comment) -- this bare basename, already extracted by the - // tracker itself, is all there is. - let embedded_path = PathBuf::from(&name); + // A row with a genuinely unresolved module is represented as an empty (or + // absent -- but this variant of `AttrValueType` can only be empty, not + // absent) path string -- see the module doc comment. Treat that as "no + // path" and fall back to a fixed placeholder name rather than deriving an + // empty/panic-inducing name from it. + let (name, embedded_path) = if embedded_path_str.is_empty() { + (UNKNOWN_MODULE_NAME.to_string(), PathBuf::new()) + } else { + let embedded_path = PathBuf::from(&embedded_path_str); + let name = embedded_path + .file_name() + .and_then(|n| n.to_str()) + .map(str::to_string) + .ok_or_else(|| { + anyhow!( + "embedded path {:?} in tracker_obj->maps row has no file name component", + embedded_path + ) + })?; + (name, embedded_path) + }; Ok((name, base, size, embedded_path)) } -fn column_string(value: &AttrValueType, column: &str) -> Result { - match value { - AttrValueType::String(s) => Ok(s.clone()), +/// Read a positional `tracker_obj->maps` row element expected to be an unsigned +/// (or non-negative signed) integer, e.g. `loaded_address`/`loaded_size`. +/// `index` is only used to produce a helpful error message. +fn list_get_unsigned(element: &AttrValueType, index: usize) -> Result { + match element { + AttrValueType::Unsigned(u) => Ok(*u), + AttrValueType::Signed(s) if *s >= 0 => Ok(*s as u64), other => bail!( - "expected list-modules column {:?} to be a String, got {:?}", - column, + "expected tracker_obj->maps row element {index} to be an unsigned integer, got {:?}", other ), } } -fn column_unsigned(value: &AttrValueType, column: &str) -> Result { - match value { - AttrValueType::Unsigned(u) => Ok(*u), - AttrValueType::Signed(s) if *s >= 0 => Ok(*s as u64), +/// Read a positional `tracker_obj->maps` row element expected to be a string, +/// i.e. `full_path_string`. `index` is only used to produce a helpful error +/// message. +fn list_get_string(element: &AttrValueType, index: usize) -> Result { + match element { + AttrValueType::String(s) => Ok(s.clone()), other => bail!( - "expected list-modules column {:?} to be an unsigned integer, got {:?}", - column, + "expected tracker_obj->maps row element {index} to be a String, got {:?}", other ), } } -/// UEFI/SMM module debug-info info, resolved from a `list-modules` dump plus a -/// local build-root directory. +/// UEFI/SMM module debug-info info, resolved from a `tracker_obj->maps` dump plus +/// a local build-root directory. /// /// Unlike `crate::os::windows::WindowsOsInfo`, which keys most of its state by /// CPU index (`HashMap`) because Windows tracks per-CPU current -/// process/module state, UEFI/SMM has no such per-CPU context -- it is a single +/// process/module state, UEFI/SMM has no such per-CPU context -- it's a single /// flat address space/module list -- so this holds a flat `Vec` instead. #[derive(Debug, Clone, Default)] pub struct UefiOsInfo { /// Resolved modules: `(name, base, resolved_local_debug_path)`. Feeding this - /// into the DWARF milestone `DwarfModule::new(name, base, object)` (which + /// into the DWARF milestone's `DwarfModule::new(name, base, object)` (which /// needs the `object::File` parsed from the path at `resolved_local_debug_path`) /// is explicitly out of scope for this milestone. pub modules: Vec<(String, u64, PathBuf)>, @@ -228,28 +241,39 @@ impl UefiOsInfo { /// Resolve local debug-info paths for a parsed module list against a local /// build-root directory. /// + /// Under the superseded `list-modules`-based design, an embedded path was + /// assumed to be a rare bonus (`list-modules` itself only ever exposed a + /// bare basename, since it calls `basename()` internally), so path-suffix + /// matching was a secondary "if we ever get a path" capability and + /// bare-stem search was the primary path. `tracker_obj->maps` inverts that: + /// a full embedded path is the *common* case (every genuinely-identified + /// module has one; see the module doc comment), so path-suffix matching is + /// now the primary resolution path. + /// /// For each module: - /// 1. Try matching the module embedded path against a - /// [`PathSuffixIndex`] built over `build_root`, longest suffix first. This - /// disambiguates same-named modules whose embedded paths differ in a - /// parent directory that also exists locally (e.g. two different EDK2 - /// package subdirectories) -- **when the caller actually has such an - /// embedded path to give it**. [`parse_module_list`] itself never can - /// (see its module doc comment: real `list-modules` output only ever - /// supplies a bare basename, confirmed live), so for input sourced from - /// it this phase degenerates to exactly the bare-stem fallback below; it - /// remains here as a general capability of this function for any other - /// caller/future data source that might supply a real embedded path. - /// 2. If that finds nothing, fall back to a bare-filename-stem search - /// (`rglob`-equivalent walk) under `build_root`. - /// 3. If, after both, more than one candidate remains ambiguous, log a + /// 1. If the module has no embedded path at all (a genuinely pathless row + /// -- a real "unknown"/unresolved module, not merely a basename-only + /// row), fail that module explicitly rather than guessing -- see + /// [`resolve_one`]. + /// 2. Otherwise, try matching the module's embedded path against a + /// [`PathSuffixIndex`] built over `build_root`, longest suffix first. + /// This disambiguates same-named modules whose embedded paths differ in + /// a parent directory that also exists locally (e.g. two different EDK2 + /// package subdirectories) -- the primary resolution path, and expected + /// to resolve the overwhelming majority of real modules outright, since + /// they carry a full embedded path. + /// 3. If that finds nothing (e.g. the embedded path's parent directories + /// don't exist locally under any matching name), fall back to a + /// bare-filename-stem search (`rglob`-equivalent walk) under + /// `build_root`. + /// 4. If, after both, more than one candidate remains ambiguous, log a /// warning and take the first (sorted, for determinism) candidate -- - /// "fail open", the spec own explicit decision, rather than erroring out - /// or dropping the module. For any real duplicate-name module sourced from - /// live `list-modules` output, this is the **expected**, common outcome - /// (confirmed live: e.g. `BootScriptExecutorDxe.efi` appeared twice with - /// no distinguishing information beyond base address), not a rare edge - /// case. + /// "fail open", the spec's own explicit decision, rather than erroring out + /// or dropping the module. In practice this fires rarely now: the one + /// real observed duplicate-name case investigated (two + /// `BootScriptExecutorDxe.efi` instances) turned out to share an + /// identical full path (the same build loaded twice), which step 2 + /// resolves outright with no ambiguity at all. pub fn resolve

(modules: &[(String, u64, u64, PathBuf)], build_root: P) -> Result where P: AsRef, @@ -257,7 +281,7 @@ impl UefiOsInfo { let build_root = build_root.as_ref(); // `PathSuffixIndex::build_from_dir` does not hash file contents (unlike // `SourceCache::new`), which is the whole point of factoring it out of - // `SourceCache` -- see `src/util/path_suffix_index.rs` module doc. + // `SourceCache` -- see `src/util/path_suffix_index.rs`'s module doc. let index = PathSuffixIndex::build_from_dir(build_root)?; let mut resolved = Vec::with_capacity(modules.len()); @@ -271,14 +295,34 @@ impl UefiOsInfo { } } -/// Resolve a single module local debug-info path. See -/// [`UefiOsInfo::resolve`] doc comment for the algorithm. +/// Resolve a single module's local debug-info path. See +/// [`UefiOsInfo::resolve`]'s doc comment for the algorithm. fn resolve_one( index: &PathSuffixIndex, build_root: &Path, name: &str, embedded_path: &Path, ) -> Result { + if embedded_path.as_os_str().is_empty() { + // A genuinely pathless row -- a real "unknown"/unresolved module (see + // `parse_module_row`/`UNKNOWN_MODULE_NAME`), not merely a basename-only + // row (that case can't arise from `tracker_obj->maps`: every row that + // has a path at all has a *full* path, never just a basename). There is + // no embedded path to suffix-match against, and no real file name to + // bare-stem-search by either -- `name` here is just the + // `UNKNOWN_MODULE_NAME` placeholder, not a real file name, so searching + // for it would either find nothing or silently match an unrelated local + // file that happens to share that placeholder name. Fail this module + // explicitly instead. + bail!( + "module {name:?} has no embedded path (unknown/unresolved module); cannot resolve \ + local debug info" + ); + } + + // Primary resolution path (see `UefiOsInfo::resolve`'s doc comment for why + // this now comes first): match the module's full embedded path against a + // `PathSuffixIndex` built over `build_root`, longest suffix first. if let Some(local_path) = index.lookup_str_unambiguous(&embedded_path.to_string_lossy()) { debug!( "resolved module {name:?} via path-suffix match: {embedded_path:?} -> {local_path:?}" @@ -287,8 +331,10 @@ fn resolve_one( } // Fall back to a bare-filename-stem search, since the suffix index found no - // match at all (e.g. the embedded path parent directories do not exist - // locally under any name that matches). + // match at all (e.g. the embedded path's parent directories don't exist + // locally under any name that matches). This is now specifically a + // fallback for that case, not the primary path -- see `UefiOsInfo::resolve`'s + // doc comment. let stem = embedded_path .file_stem() .and_then(|s| s.to_str()) @@ -314,17 +360,17 @@ fn resolve_one( } n => { // Fail open: log and take the first (sorted) match rather than - // erroring out or dropping the module -- this is the spec own + // erroring out or dropping the module -- this is the spec's own // explicit decision, matching the `warn!`/`debug!` logging style // already used for similar disambiguation situations in - // `crate::os::windows` (see e.g. `src/os/windows/structs.rs` + // `crate::os::windows` (see e.g. `src/os/windows/structs.rs`'s // module-lookup logging). Unlike those call sites, this uses the // plain `tracing` crate rather than `simics::warn!`/`simics::debug!`: // the latter require a live `ConfObject` (e.g. // `get_object("tsffs")?`) and call real `SIM_*` FFI entry points, // which -- exactly like the bug fixed in `SourceCache::new` -- hard- // abort the process with no live Simics session, which is - // unconditionally true for this milestone offline scope. + // unconditionally true for this milestone's offline scope. warn!( "ambiguous local debug info for module {name:?}: {n} candidates matched stem \ {stem:?} with no unique path-suffix match (embedded path {embedded_path:?}); \ diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs index bbdbf0cd..a0cd8f50 100644 --- a/tests/uefi_module_discovery_fixture.rs +++ b/tests/uefi_module_discovery_fixture.rs @@ -1,52 +1,59 @@ // Copyright (C) 2024 Intel Corporation // SPDX-License-Identifier: Apache-2.0 -//! End-to-end, offline test of UEFI module discovery milestone-scope steps 1-2 -//! (`tsffs::uefi::{parse_module_list, UefiOsInfo}`, UCOV-M2) against fixtures -//! built from a **confirmed real** `list-modules` shape -- see `src/uefi/mod.rs` -//! module doc comment for exactly how and when that shape was confirmed (a live -//! QSP/X58 Simics session on the `the dev host` host, 2026-09-16) -- mirroring -//! `tests/dwarf_fixture.rs` pattern on the sibling DWARF milestone branch +//! End-to-end, offline test of UEFI module discovery's milestone-scope steps 1-2 +//! (`tsffs::uefi::{parse_module_list, UefiOsInfo}`, UCOV-M2) against a synthetic +//! fixture, entirely without a live Simics session -- mirroring +//! `tests/dwarf_fixture.rs`'s pattern on the sibling DWARF milestone branch //! (`feat/dwarf-source-coverage-ucov-m1`). //! //! This lives under `tests/` (an integration test / separate cargo crate) rather //! than as a `#[cfg(test)]` module inside `src/uefi/mod.rs` for the same reason as -//! that file: this crate `[lib]` section sets `test = false`, which disables the +//! that file: this crate's `[lib]` section sets `test = false`, which disables the //! implicit unit-test harness for the library target, so `#[cfg(test)]` code //! inside `src/` is never compiled by `cargo test` for this crate. Integration //! tests under `tests/` are a separate cargo target and unaffected by that -//! setting. Being a separate crate also means this file only sees `tsffs` `pub` +//! setting. Being a separate crate also means this file only sees `tsffs`'s `pub` //! API -- `src/lib.rs` widens `uefi` to `pub` (from `pub(crate)`) for exactly this //! reason, same as `dwarf`/`source_cov` on the DWARF branch. //! -//! # A real `list-modules` output was used to build these fixtures +//! # No live Simics session was used, but the row shape itself is real //! -//! Unlike the previous (offline-only, assumption-based) version of this file, -//! the fixtures below are built directly from a real, live tracker capture -//! (`qsp.software.tracker.list-modules max = 1000` against a real QSP/X58 -//! checkpoint, on `the dev host`, 2026-09-16), not a hand-guessed shape -- see -//! `src/uefi/mod.rs` module doc comment ("Confirmed shape of `list-modules` -//! return value") for the full capture and cross-check against the -//! `uefi_fw_tracker` component own installed Python source. +//! There is no BIOS image, boot, or live Simics session available in this offline +//! environment, so this test still can't drive a real `run_command` call -- it +//! tests `parse_module_list` against a *hand-constructed* fake `AttrValueType` +//! rather than one actually returned by Simics. But unlike the superseded +//! `list-modules`-based design (whose row shape was an unconfirmed assumption), +//! the 7-element row shape used below is not a guess: it matches a real, live +//! `tracker_obj->maps` dump captured in a live test session (real Simics +//! 6.0.189 session, real checkpoint past DXE dispatch, 68 real loaded UEFI +//! modules) -- see `src/uefi/mod.rs`'s module doc comment ("Confirmed shape of +//! `tracker_obj->maps`' return value") for the exact shape and a real captured +//! example row. //! -//! # Fixtures +//! # Fixture //! -//! [`FIXTURE_ROWS`] models a subset of the real 78-module live capture, with the -//! real observed duplicate-name case (`BootScriptExecutorDxe.efi`, appearing -//! twice at different addresses with no other distinguishing information) and the -//! real observed "no image name known" case (`""`) both included. It is -//! used by both [`parses_confirmed_real_module_list_shape_with_duplicate_names`] -//! (testing [`parse_module_list`] alone) and -//! [`resolve_falls_open_on_the_real_duplicate_name_case`] (testing the full -//! `parse_module_list` -> [`UefiOsInfo::resolve`] pipeline end-to-end). +//! The fixture models 6 rows: //! -//! [`resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths`] -//! separately tests [`UefiOsInfo::resolve`] own generic path-suffix -//! disambiguation capability against hand-built `(name, base, size, -//! embedded_path)` tuples carrying full, distinguishing paths -- real -//! `list-modules` output never supplies such a path (confirmed live, see above), -//! but `UefiOsInfo::resolve` is a generic utility not solely fed from -//! `parse_module_list`, so this capability is still worth testing directly. +//! - `PeiCore.efi`: one unique module with a full embedded path, resolved via +//! primary path-suffix matching. +//! - `BootScriptExecutorDxe.efi`: the real observed duplicate-name case +//! (re-investigated under `tracker_obj->maps`) -- two loaded instances, two +//! distinct base addresses, but the **identical** embedded path for both (the +//! same build loaded twice, not two different binaries). Confirms this +//! resolves both addresses to the same correct local file with **no** +//! ambiguity warning, since a real matching path makes it not actually +//! ambiguous. +//! - `AcpiVTD.efi`: a **fabricated** genuinely-different-path duplicate (two +//! different, fabricated `PkgA`/`PkgB` package subdirectories, same basename) +//! -- proves path-suffix matching still correctly disambiguates *real* +//! ambiguity when it exists, which the `BootScriptExecutorDxe.efi` case above +//! does not exercise (its two instances share one path, so a resolver that +//! ignored the path entirely would pass that case by accident). +//! - An unknown/unresolved module with no embedded path at all (empty string), +//! confirming `parse_module_list` names it [`tsffs::uefi::UNKNOWN_MODULE_NAME`] +//! without panicking or misparsing, and that `UefiOsInfo::resolve` fails that +//! one module gracefully (a clear `Err`, not a panic) rather than guessing. use std::{ collections::HashMap, @@ -60,68 +67,96 @@ use anyhow::Result; use simics::AttrValueType; use tempfile::tempdir; use tracing_subscriber::fmt::MakeWriter; -use tsffs::uefi::{parse_module_list, UefiOsInfo}; - -/// One row of the confirmed-real-shape fixture: a bare basename (exactly what -/// `list-modules` itself returns, per the module doc comment), a loaded address, -/// and a size. Real rows also carry "Adjusted Address"/"Adjusted Size" columns, -/// empty in the live capture and unused by this milestone; [`fixture_attr_value`] -/// still includes them (as empty strings) for fidelity to the real capture, and -/// `parse_module_row` tolerates that (it only requires at least 3 columns). +use tsffs::uefi::{parse_module_list, UefiOsInfo, UNKNOWN_MODULE_NAME}; + +/// One row of the fixture, in the confirmed real `tracker_obj->maps` shape: +/// `[loaded_address, loaded_size, , adjusted_address, adjusted_size, +/// , full_path_string]`. `embedded_path` is `None` to model a genuinely +/// pathless ("unknown module") row. struct FixtureRow { - name: &'static str, base: u64, size: u64, + embedded_path: Option, +} + +/// The fixture's common embedded build-machine path prefix, matching the real +/// structure observed in a live tracker dump (see `src/uefi/mod.rs`'s +/// module doc comment for the real captured example this mirrors). +const PREFIX: &str = "/home/user/bios-x58i/project/workspace/Build/SimicsOpenBoardPkg/BoardX58Ich10/DEBUG_GCC/X64"; + +/// Build the fixture's 6 rows: +/// - one unique module (`PeiCore.efi`) +/// - the real observed duplicate-name case (`BootScriptExecutorDxe.efi`), both +/// instances sharing one identical embedded path +/// - a fabricated genuinely-different-path duplicate (`AcpiVTD.efi`, under +/// fabricated `PkgA`/`PkgB` subdirectories) +/// - one genuinely pathless ("unknown module") row +fn fixture_rows() -> Vec { + vec![ + FixtureRow { + base: 0x0000_0000_0082_0000, + size: 0x9000, + embedded_path: Some(format!( + "{PREFIX}/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.efi" + )), + }, + // Real observed duplicate-name case: two loaded instances, two base + // addresses, but the identical embedded path for both. + FixtureRow { + base: 0x0000_0000_00d0_0000, + size: 0x1_0000, + embedded_path: Some(format!( + "{PREFIX}/MdeModulePkg/Universal/Variable/RuntimeDxe/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" + )), + }, + FixtureRow { + base: 0x0000_0000_00e0_0000, + size: 0x1_0100, + embedded_path: Some(format!( + "{PREFIX}/MdeModulePkg/Universal/Variable/RuntimeDxe/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" + )), + }, + // Fabricated genuinely-different-path duplicate: same basename, two + // different package subdirectories, to prove suffix-matching still + // disambiguates real ambiguity when it exists. + FixtureRow { + base: 0x0000_0000_0700_0000, + size: 0x4000, + embedded_path: Some(format!("{PREFIX}/PkgA/Feature/AcpiVTD/DEBUG/AcpiVTD.efi")), + }, + FixtureRow { + base: 0x0000_0000_0710_0000, + size: 0x4200, + embedded_path: Some(format!("{PREFIX}/PkgB/Feature/AcpiVTD/DEBUG/AcpiVTD.efi")), + }, + // Genuinely pathless row: a real "unknown"/unresolved module. + FixtureRow { + base: 0x0000_0000_0900_0000, + size: 0x1000, + embedded_path: None, + }, + ] } -/// A representative subset of the real 78-row live capture (see the module doc -/// comment in `src/uefi/mod.rs`), including both real observed edge cases: the -/// duplicate-name module (`BootScriptExecutorDxe.efi`, two entries, two -/// addresses, otherwise indistinguishable) and the "no image name known" module -/// (`""`). -const FIXTURE_ROWS: &[FixtureRow] = &[ - FixtureRow { - name: "DxeCore.efi", - base: 3_744_034_816, - size: 189_184, - }, - FixtureRow { - name: "PcdDxe.efi", - base: 3_740_880_896, - size: 23_680, - }, - FixtureRow { - name: "BootScriptExecutorDxe.efi", - base: 3_739_889_664, - size: 84_224, - }, - FixtureRow { - name: "BootScriptExecutorDxe.efi", - base: 3_722_764_288, - size: 84_224, - }, - FixtureRow { - name: "", - base: 3_722_997_760, - size: 195_360, - }, -]; - -/// Build the real, confirmed `AttrValueType` shape `list-modules` returns (see -/// `src/uefi/mod.rs` module doc comment): a `List` of `List` rows (positional -/// columns, not a `Dict`), each `[Module, "Loaded Address", "Size", "Adjusted -/// Address", "Adjusted Size"]`, with the trailing two columns empty strings -- -/// exactly as observed in the real live capture for every module. +/// Build the fake `AttrValueType` shape `tracker_obj->maps` is confirmed (see +/// `src/uefi/mod.rs`'s module doc comment) to return for a set of fixture rows: a +/// `List` of 7-element `List`s, `[loaded_address, loaded_size, , +/// adjusted_address, adjusted_size, , full_path_string]`. `adjusted_address` +/// is fabricated equal to `loaded_address` and `adjusted_size` equal to +/// `loaded_size`, and both booleans fabricated `true`, since this module doesn't +/// read those fields at all (see the module doc comment). fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { AttrValueType::List( rows.iter() .map(|row| { AttrValueType::List(vec![ - AttrValueType::String(row.name.to_string()), - AttrValueType::Signed(row.base as i64), - AttrValueType::Signed(row.size as i64), - AttrValueType::String(String::new()), - AttrValueType::String(String::new()), + AttrValueType::Unsigned(row.base), + AttrValueType::Unsigned(row.size), + AttrValueType::Bool(true), + AttrValueType::Unsigned(row.base), + AttrValueType::Unsigned(row.size), + AttrValueType::Bool(true), + AttrValueType::String(row.embedded_path.clone().unwrap_or_default()), ]) }) .collect(), @@ -129,119 +164,164 @@ fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { } #[test] -fn parses_confirmed_real_module_list_shape_with_duplicate_names() -> Result<()> { - let parsed = parse_module_list(&fixture_attr_value(FIXTURE_ROWS))?; - assert_eq!(parsed.len(), FIXTURE_ROWS.len()); +fn parses_synthetic_tracker_maps_including_unknown_module() -> Result<()> { + let rows = fixture_rows(); + let value = fixture_attr_value(&rows); + + let parsed = parse_module_list(&value)?; + assert_eq!(parsed.len(), rows.len()); - for (row, (name, base, size, embedded_path)) in FIXTURE_ROWS.iter().zip(parsed.iter()) { - assert_eq!(name, row.name); + for (row, (name, base, size, embedded_path)) in rows.iter().zip(parsed.iter()) { assert_eq!(*base, row.base); assert_eq!(*size, row.size); - // Confirmed live: list-modules never supplies a full path, so - // parse_module_row embedded_path for every module is just the bare - // name it was given, wrapped. - assert_eq!(embedded_path, &PathBuf::from(row.name)); + + match &row.embedded_path { + Some(path) => { + let expected_name = PathBuf::from(path) + .file_name() + .expect("fixture embedded path has a file name") + .to_str() + .expect("fixture file name is valid UTF-8") + .to_string(); + assert_eq!(name, &expected_name); + assert_eq!(embedded_path, &PathBuf::from(path)); + } + None => { + // Graceful naming/fallback for a genuinely pathless row -- not a + // crash, not a silent misparse (e.g. an empty name or a panic). + assert_eq!(name, UNKNOWN_MODULE_NAME); + assert_eq!(embedded_path, &PathBuf::new()); + } + } } - // The real observed duplicate-name case: exactly 2 entries, distinguishable - // only by base address (not accidentally collapsed/deduplicated by the - // parser). - let dup: Vec<_> = parsed + // The real observed duplicate-name case: both instances present, distinct + // base addresses, but the identical embedded path (not accidentally + // collapsed/deduplicated, and not given divergent paths). + let boot_script_matches: Vec<_> = parsed .iter() .filter(|(name, ..)| name == "BootScriptExecutorDxe.efi") .collect(); - assert_eq!( - dup.len(), - 2, - "expected exactly 2 parsed entries for the real observed duplicate-name module" - ); - assert_ne!( - dup[0].1, dup[1].1, - "the two entries must have distinct base addresses (the only thing distinguishing them)" - ); - assert_eq!( - dup[0].3, dup[1].3, - "list-modules gives both the exact same bare-name embedded_path -- there is no way to \ - tell them apart by path" - ); + assert_eq!(boot_script_matches.len(), 2); + assert_ne!(boot_script_matches[0].1, boot_script_matches[1].1); + assert_eq!(boot_script_matches[0].3, boot_script_matches[1].3); - // The real observed "no image name known" case. - assert!( - parsed.iter().any(|(name, ..)| name == ""), - "expected the real observed \"\" module name to survive parsing unchanged" - ); + // The fabricated genuinely-different-path duplicate: both instances present, + // distinct base addresses, and distinct embedded paths. + let acpi_matches: Vec<_> = parsed + .iter() + .filter(|(name, ..)| name == "AcpiVTD.efi") + .collect(); + assert_eq!(acpi_matches.len(), 2); + assert_ne!(acpi_matches[0].1, acpi_matches[1].1); + assert_ne!(acpi_matches[0].3, acpi_matches[1].3); Ok(()) } -/// One row of the hand-built, full-path fixture used only by -/// [`resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths`] -/// below, to exercise [`UefiOsInfo::resolve`] own generic path-suffix -/// disambiguation capability -- independent of [`parse_module_list`], which -/// (confirmed live) never actually has a full path to supply. -struct HandBuiltModuleRow { - embedded_path: String, - base: u64, - size: u64, -} +#[test] +fn resolves_identical_path_duplicate_with_no_ambiguity_warning() -> Result<()> { + let tmp = tempdir()?; + let root = tmp.path(); -/// The fixture common embedded build-machine path prefix, matching the real -/// structure observed in the live capture that this branch investigation -/// confirmed (see `src/uefi/mod.rs` module doc comment). -const PREFIX: &str = - "/home/user/bios-x58i/project/workspace/Build/SimicsOpenBoardPkg/BoardX58Ich10/DEBUG_GCC/X64"; - -/// Build 4 hand-built rows: the 2 real observed duplicate-name case -/// (`BootScriptExecutorDxe.efi`) under fabricated `PkgA`/`PkgB` subdirectories, -/// with distinct full embedded paths and base addresses matching -/// [`FIXTURE_ROWS`]. -fn hand_built_rows() -> Vec { - vec![ - HandBuiltModuleRow { - embedded_path: format!( - "{PREFIX}/PkgA/Universal/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" - ), - base: 3_739_889_664, - size: 84_224, - }, - HandBuiltModuleRow { - embedded_path: format!( - "{PREFIX}/PkgB/Universal/BootScriptExecutorDxe/DEBUG/BootScriptExecutorDxe.efi" - ), - base: 3_722_764_288, - size: 84_224, - }, - ] + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))? + .into_iter() + .filter(|(name, ..)| name == "BootScriptExecutorDxe.efi") + .collect::>(); + assert_eq!(modules.len(), 2); + + // Mirror the shared embedded path locally, from "DEBUG_GCC/" onward, as a + // single real local file. + let shared_embedded_path = rows + .iter() + .find(|row| { + row.embedded_path + .as_deref() + .map(|p| p.contains("BootScriptExecutorDxe")) + .unwrap_or(false) + }) + .and_then(|row| row.embedded_path.as_deref()) + .expect("fixture has a BootScriptExecutorDxe.efi row with an embedded path"); + let suffix = shared_embedded_path + .rsplit_once("DEBUG_GCC/") + .expect("fixture embedded path contains the DEBUG_GCC/ prefix marker") + .1; + let local_path = root.join(suffix); + create_dir_all( + local_path + .parent() + .expect("local fixture path has a parent directory"), + )?; + write(&local_path, b"contents of BootScriptExecutorDxe.efi")?; + + let capturing_writer = CapturingWriter::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(capturing_writer.clone()) + .with_ansi(false) + .with_max_level(tracing::Level::TRACE) + .finish(); + + let info = + tracing::subscriber::with_default(subscriber, || UefiOsInfo::resolve(&modules, root))?; + assert_eq!(info.modules.len(), 2); + + // Both instances resolve to the exact same real local file: not ambiguous + // once a real distinguishing (here, shared) path is available. + for (name, _base, resolved_path) in &info.modules { + assert_eq!(name, "BootScriptExecutorDxe.efi"); + assert_eq!( + resolved_path, &local_path, + "both BootScriptExecutorDxe.efi instances must resolve to the shared local file" + ); + } + + let log_output = String::from_utf8( + capturing_writer + .0 + .lock() + .expect("capturing writer mutex not poisoned") + .clone(), + )?; + assert!( + !log_output.to_lowercase().contains("ambiguous"), + "resolving the identical-path duplicate must not warn about ambiguity, got: {log_output:?}" + ); + + Ok(()) } #[test] -fn resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths() -> Result<()> { +fn resolves_different_path_duplicate_via_path_suffix_disambiguation() -> Result<()> { let tmp = tempdir()?; let root = tmp.path(); - let rows = hand_built_rows(); - let modules: Vec<(String, u64, u64, PathBuf)> = rows - .iter() - .map(|row| { - ( - "BootScriptExecutorDxe.efi".to_string(), - row.base, - row.size, - PathBuf::from(&row.embedded_path), - ) - }) - .collect(); + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))? + .into_iter() + .filter(|(name, ..)| name == "AcpiVTD.efi") + .collect::>(); + assert_eq!(modules.len(), 2); - // Mirror each row embedded path locally, from "X64/" onward (i.e. the - // "PkgA/..." / "PkgB/..." part), as a real local file with content unique to - // that row -- used below to positively confirm which exact local file each - // module resolved to, not just "a" file. + // Mirror each relevant fixture row's embedded path locally, from + // "DEBUG_GCC/" onward (i.e. the "PkgA/..." / "PkgB/..." part), as a real + // local file with content unique to that row -- used below to positively + // confirm which exact local file each module resolved to, not just "a" + // file. let mut expected_local_paths: HashMap = HashMap::new(); - for row in &rows { - let suffix = row + for row in rows.iter().filter(|row| { + row.embedded_path + .as_deref() + .map(|p| p.contains("AcpiVTD")) + .unwrap_or(false) + }) { + let embedded_path = row .embedded_path - .rsplit_once("X64/") - .expect("fixture embedded path contains the X64/ prefix marker") + .as_deref() + .expect("filtered to rows with an embedded path"); + let suffix = embedded_path + .rsplit_once("DEBUG_GCC/") + .expect("fixture embedded path contains the DEBUG_GCC/ prefix marker") .1; let local_path = root.join(suffix); create_dir_all( @@ -250,7 +330,7 @@ fn resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths() -> .expect("local fixture path has a parent directory"), )?; write(&local_path, format!("contents of {suffix}"))?; - expected_local_paths.insert(row.embedded_path.clone(), local_path); + expected_local_paths.insert(embedded_path.to_string(), local_path); } let info = UefiOsInfo::resolve(&modules, root)?; @@ -261,82 +341,73 @@ fn resolves_duplicate_names_via_path_suffix_disambiguation_given_full_paths() -> .iter() .find(|row| row.base == *base) .expect("resolved module base matches a fixture row"); + let embedded_path = row + .embedded_path + .as_deref() + .expect("fixture row has an embedded path"); let expected = expected_local_paths - .get(&row.embedded_path) + .get(embedded_path) .expect("fixture row has a corresponding local fixture file"); assert_eq!( resolved_path, expected, - "module {name} (base {base:#x}) resolved to the wrong local file; path-suffix \ - disambiguation failed" + "module {name} (base {base:#x}) resolved to the wrong local file; \ + path-suffix disambiguation failed" ); } - // The actual point of this test, not a vacuous pass: given full, - // distinguishing embedded paths, the two same-named modules must resolve to - // two *distinct* local files. + // The actual point of this test, not a vacuous pass: the two AcpiVTD.efi + // entries must resolve to two *distinct* local files -- a resolver that just + // grabbed "any" file matching the bare name would pass the per-module checks + // above only by accident, but could not pass this. + let resolved: Vec<&PathBuf> = info.modules.iter().map(|(_, _, path)| path).collect(); + assert_eq!(resolved.len(), 2); assert_ne!( - info.modules[0].2, info.modules[1].2, - "the two same-named modules must resolve to distinct local files when given distinct \ - full embedded paths" + resolved[0], resolved[1], + "the two AcpiVTD.efi modules must resolve to distinct local files" ); Ok(()) } -/// A `tracing_subscriber::fmt::MakeWriter` that captures formatted log output into -/// a shared in-memory buffer, so the tests below can assert on it directly instead -/// of only inferring the warning fired from behavior. -#[derive(Clone, Default)] -struct CapturingWriter(Arc>>); - -impl io::Write for CapturingWriter { - fn write(&mut self, buf: &[u8]) -> io::Result { - self.0 - .lock() - .expect("capturing writer mutex not poisoned") - .extend_from_slice(buf); - Ok(buf.len()) - } - - fn flush(&mut self) -> io::Result<()> { - Ok(()) - } -} - -impl<'a> MakeWriter<'a> for CapturingWriter { - type Writer = Self; - - fn make_writer(&'a self) -> Self::Writer { - self.clone() - } -} - #[test] -fn resolve_falls_open_on_the_real_duplicate_name_case() -> Result<()> { +fn falls_back_to_stem_match_and_warns_on_ambiguity_when_suffix_match_fails() -> Result<()> { let tmp = tempdir()?; let root = tmp.path(); - // The full, realistic pipeline: parse_module_list on the confirmed-real - // fixture, not a hand-built one -- so both BootScriptExecutorDxe.efi entries - // get parse_module_row own embedded_path (just the bare name, see its doc - // comment), exactly as real list-modules output would. - let modules: Vec<_> = parse_module_list(&fixture_attr_value(FIXTURE_ROWS))? - .into_iter() - .filter(|(name, ..)| name == "BootScriptExecutorDxe.efi") - .collect(); + // Use a module with a real embedded path (not the pathless row) whose parent + // directories deliberately don't exist locally at all, so path-suffix + // matching finds nothing and both instances fall through to the bare-stem + // fallback. + let rows = vec![ + FixtureRow { + base: 0x0000_0000_0740_0000, + size: 0x5000, + embedded_path: Some(format!( + "{PREFIX}/PkgA/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" + )), + }, + FixtureRow { + base: 0x0000_0000_0750_0000, + size: 0x5100, + embedded_path: Some(format!( + "{PREFIX}/PkgB/Bus/Pci/SataControllerDxe/DEBUG/SataController.efi" + )), + }, + ]; + let modules = parse_module_list(&fixture_attr_value(&rows))?; assert_eq!(modules.len(), 2); - // Two unrelated local directory layouts, both happening to contain a file - // with the exact bare name "BootScriptExecutorDxe.efi" -- the only kind of - // local layout that parse_module_list-sourced input can ever match against, - // since it never has more than a bare name to go on. - let path_1 = root - .join("unrelated_layout_one") - .join("BootScriptExecutorDxe.efi"); - let path_2 = root - .join("unrelated_layout_two") - .join("BootScriptExecutorDxe.efi"); + // Deliberately unrelated local directory layouts: neither shares any path + // component with the fixture's ".../PkgA|PkgB/Bus/Pci/SataControllerDxe/ + // DEBUG/" embedded-path tail beyond the bare file name itself. Path-suffix + // matching therefore finds the bare file name "SataController.efi" as a + // candidate suffix, but with *two* different local files sharing it -- an + // ambiguous, not unique, match -- so per + // `PathSuffixIndex::lookup_components_unambiguous`'s contract it must return + // no match at all, forcing both modules through the bare-stem fallback path. + let path_1 = root.join("unrelated_layout_one").join("SataController.efi"); + let path_2 = root.join("unrelated_layout_two").join("SataController.efi"); create_dir_all(path_1.parent().expect("has parent"))?; create_dir_all(path_2.parent().expect("has parent"))?; write(&path_1, b"layout one")?; @@ -349,23 +420,26 @@ fn resolve_falls_open_on_the_real_duplicate_name_case() -> Result<()> { .with_max_level(tracing::Level::TRACE) .finish(); - let info = - tracing::subscriber::with_default(subscriber, || UefiOsInfo::resolve(&modules, root))?; + let info = tracing::subscriber::with_default(subscriber, || { + UefiOsInfo::resolve(&modules, root) + })?; assert_eq!(info.modules.len(), 2); - // Fail-open (the spec own explicit decision, and -- confirmed live -- the - // expected outcome for any real duplicate-name module, not a rare edge - // case): both modules still resolve, to the first candidate in sorted order. + // Fail-open (the spec's own explicit decision): both modules still resolve -- + // not an error, not a dropped module -- to the first candidate in sorted + // order. Both modules share the exact same ambiguous candidate set (the same + // two unrelated local files), so both must fail open to the exact same + // resolved path. let mut sorted_candidates = [path_1.clone(), path_2.clone()]; sorted_candidates.sort(); let expected_fail_open_path = sorted_candidates[0].clone(); for (name, _base, resolved_path) in &info.modules { - assert_eq!(name, "BootScriptExecutorDxe.efi"); + assert_eq!(name, "SataController.efi"); assert_eq!( resolved_path, &expected_fail_open_path, - "expected fail-open fallback to deterministically pick the first (sorted) \ - ambiguous candidate" + "expected fail-open fallback to deterministically pick the first \ + (sorted) ambiguous candidate" ); } @@ -384,3 +458,55 @@ fn resolve_falls_open_on_the_real_duplicate_name_case() -> Result<()> { Ok(()) } + +#[test] +fn resolve_fails_gracefully_not_panics_for_pathless_unknown_module() -> Result<()> { + let tmp = tempdir()?; + let root = tmp.path(); + + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))? + .into_iter() + .filter(|(name, ..)| name == UNKNOWN_MODULE_NAME) + .collect::>(); + assert_eq!(modules.len(), 1); + + // Resolving a genuinely pathless ("unknown module") row must fail cleanly + // (a `Result::Err`, caught here, not a panic/process abort) -- graceful + // fallback, not a crash or silent misparse. + let result = UefiOsInfo::resolve(&modules, root); + assert!( + result.is_err(), + "resolving an unknown/pathless module must return an Err, not silently succeed" + ); + + Ok(()) +} + +/// A `tracing_subscriber::fmt::MakeWriter` that captures formatted log output into +/// a shared in-memory buffer, so tests can assert on it directly instead of only +/// inferring the warning fired from behavior. +#[derive(Clone, Default)] +struct CapturingWriter(Arc>>); + +impl io::Write for CapturingWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.0 + .lock() + .expect("capturing writer mutex not poisoned") + .extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +impl<'a> MakeWriter<'a> for CapturingWriter { + type Writer = Self; + + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } +} From 714b986a0c869924a55a6539b54c32f9efad727c Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 06:58:07 -0700 Subject: [PATCH 07/18] fix: handle real Nil pathless row in tracker_obj->maps parsing Validated the 7-element parser against a real, full 68-row tracker_obj->maps capture in a live test session (Simics 6.0.189, same checkpoint used to confirm the row shape). 67 of 68 rows matched the implementation's assumptions exactly (unsigned addresses, positive ints, real full paths). The one genuinely pathless row (a real unresolved/unknown module) is Python None, which simics::AttrValueType::from(AttrValue) converts to AttrValueType::Nil via its is_nil() check -- not AttrValueType::String(String::new()) as the offline fixture had assumed. The old list_get_string only matched String and hard-errored on Nil, so parse_module_list would fail on real data despite passing all offline fixture tests. list_get_string is now list_get_string_or_nil, returning Option and accepting both Nil and (defensively) an empty String as no path. Updated the fixture test's pathless row to build AttrValueType::Nil instead of an empty string, matching the confirmed real shape. Co-Authored-By: Claude Sonnet 5 --- src/uefi/mod.rs | 91 +++++++++++++++----------- tests/uefi_module_discovery_fixture.rs | 23 +++++-- 2 files changed, 71 insertions(+), 43 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 19de09cc..b092a3b7 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -86,16 +86,25 @@ //! address also present in the real data, but out of scope for this //! milestone -- nothing downstream of this module currently consumes it.) //! - index 1 (`loaded_size`) -> `Unsigned`/`Signed`: the module's size in bytes. -//! - index 6 (`full_path_string`) -> `String`, **or absent/empty**: the -//! module's full embedded build-machine path. Some rows genuinely have no -//! path at all -- real "unknown"/unresolved modules that the tracker could -//! not identify (mirroring `module_load.py`'s own upstream guard for -//! `m['image'] is None`) -- represented here as an empty string. Unlike the -//! superseded design, there is no separate "name" field at all: with this -//! row shape, a module's short display name must be *derived* from the full -//! path via [`Path::file_name`] when a path is present, e.g. `DxeCore.efi` -//! from the example above. See [`parse_module_row`] for how a missing/empty -//! path is named instead (``). +//! - index 6 (`full_path_string`) -> `String`, **or absent**: the module's +//! full embedded build-machine path. Some rows genuinely have no path at +//! all -- real "unknown"/unresolved modules that the tracker could not +//! identify (mirroring `module_load.py`'s own upstream guard for +//! `m['image'] is None`). Confirmed live against the real, +//! full 68-row `tracker_obj->maps` capture (not just the samples from the +//! initial investigation): exactly one real row (of 68) is genuinely +//! pathless, and its Python value is `None`, not an empty string -- which +//! `simics::AttrValueType::from(AttrValue)` (`is_nil()` checked first) +//! converts to `AttrValueType::Nil`, **not** `AttrValueType::String(String::new())`. +//! An earlier revision of this module assumed the latter (an empty +//! string) and hard-errored on the real `Nil` case; [`list_get_string_or_nil`] +//! now accepts both `Nil` and (defensively) an empty `String` as "no +//! path". Unlike the superseded design, there is no separate "name" field +//! at all: with this row shape, a module's short display name must be +//! *derived* from the full path via [`Path::file_name`] when a path is +//! present, e.g. `DxeCore.efi` from the example above. See +//! [`parse_module_row`] for how a missing path is named instead +//! (``). //! - indices 2, 4, 5 (the two booleans and `adjusted_size`) are not read by //! this module; they are out of scope for this milestone. //! @@ -167,28 +176,32 @@ fn parse_module_row(row: &AttrValueType) -> Result<(String, u64, u64, PathBuf)> let base = list_get_unsigned(&loaded_address, 0)?; let size = list_get_unsigned(&loaded_size, 1)?; - let embedded_path_str = list_get_string(&full_path, 6)?; + let embedded_path_str = list_get_string_or_nil(&full_path, 6)?; - // A row with a genuinely unresolved module is represented as an empty (or - // absent -- but this variant of `AttrValueType` can only be empty, not - // absent) path string -- see the module doc comment. Treat that as "no - // path" and fall back to a fixed placeholder name rather than deriving an - // empty/panic-inducing name from it. - let (name, embedded_path) = if embedded_path_str.is_empty() { - (UNKNOWN_MODULE_NAME.to_string(), PathBuf::new()) - } else { - let embedded_path = PathBuf::from(&embedded_path_str); - let name = embedded_path - .file_name() - .and_then(|n| n.to_str()) - .map(str::to_string) - .ok_or_else(|| { - anyhow!( - "embedded path {:?} in tracker_obj->maps row has no file name component", - embedded_path - ) - })?; - (name, embedded_path) + // A row with a genuinely unresolved module has no path at all -- confirmed + // live against the real, full 68-row capture to be represented as + // `AttrValueType::Nil` (Python `None`), not an empty string -- see the + // module doc comment's "Confirmed shape" section. `list_get_string_or_nil` + // also defensively accepts an empty string as "no path", in case some + // other tracker/board configuration ever produces one instead of `Nil`. + // Treat either as "no path" and fall back to a fixed placeholder name + // rather than deriving an empty/panic-inducing name from it. + let (name, embedded_path) = match embedded_path_str.filter(|s| !s.is_empty()) { + None => (UNKNOWN_MODULE_NAME.to_string(), PathBuf::new()), + Some(embedded_path_str) => { + let embedded_path = PathBuf::from(&embedded_path_str); + let name = embedded_path + .file_name() + .and_then(|n| n.to_str()) + .map(str::to_string) + .ok_or_else(|| { + anyhow!( + "embedded path {:?} in tracker_obj->maps row has no file name component", + embedded_path + ) + })?; + (name, embedded_path) + } }; Ok((name, base, size, embedded_path)) @@ -208,14 +221,18 @@ fn list_get_unsigned(element: &AttrValueType, index: usize) -> Result { } } -/// Read a positional `tracker_obj->maps` row element expected to be a string, -/// i.e. `full_path_string`. `index` is only used to produce a helpful error -/// message. -fn list_get_string(element: &AttrValueType, index: usize) -> Result { +/// Read a positional `tracker_obj->maps` row element expected to be either a +/// string or absent, i.e. `full_path_string`. Returns `Ok(None)` for a +/// genuinely pathless row -- confirmed live (see the module doc comment) to +/// arrive as `AttrValueType::Nil` (Python `None`), not an empty string, though +/// an empty string is also accepted defensively and treated the same as `Nil`. +/// `index` is only used to produce a helpful error message. +fn list_get_string_or_nil(element: &AttrValueType, index: usize) -> Result> { match element { - AttrValueType::String(s) => Ok(s.clone()), + AttrValueType::String(s) => Ok(Some(s.clone())), + AttrValueType::Nil => Ok(None), other => bail!( - "expected tracker_obj->maps row element {index} to be a String, got {:?}", + "expected tracker_obj->maps row element {index} to be a String or Nil, got {:?}", other ), } diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs index a0cd8f50..987f24d8 100644 --- a/tests/uefi_module_discovery_fixture.rs +++ b/tests/uefi_module_discovery_fixture.rs @@ -50,10 +50,12 @@ //! ambiguity when it exists, which the `BootScriptExecutorDxe.efi` case above //! does not exercise (its two instances share one path, so a resolver that //! ignored the path entirely would pass that case by accident). -//! - An unknown/unresolved module with no embedded path at all (empty string), -//! confirming `parse_module_list` names it [`tsffs::uefi::UNKNOWN_MODULE_NAME`] -//! without panicking or misparsing, and that `UefiOsInfo::resolve` fails that -//! one module gracefully (a clear `Err`, not a panic) rather than guessing. +//! - An unknown/unresolved module with no embedded path at all +//! (`AttrValueType::Nil`, matching the real capture -- see +//! `fixture_attr_value`'s doc comment), confirming `parse_module_list` names +//! it [`tsffs::uefi::UNKNOWN_MODULE_NAME`] without panicking or misparsing, +//! and that `UefiOsInfo::resolve` fails that one module gracefully (a clear +//! `Err`, not a panic) rather than guessing. use std::{ collections::HashMap, @@ -144,7 +146,13 @@ fn fixture_rows() -> Vec { /// adjusted_address, adjusted_size, , full_path_string]`. `adjusted_address` /// is fabricated equal to `loaded_address` and `adjusted_size` equal to /// `loaded_size`, and both booleans fabricated `true`, since this module doesn't -/// read those fields at all (see the module doc comment). +/// read those fields at all (see the module doc comment). A `None` +/// `embedded_path` becomes `AttrValueType::Nil`, not `AttrValueType::String(String::new())` +/// -- validated live against the real, full 68-row capture: the one +/// genuinely pathless row's Python value is `None`, which converts to +/// `AttrValueType::Nil` via `simics::AttrValueType::from(AttrValue)`'s `is_nil()` +/// check. An earlier revision of this fixture used an empty string instead, +/// which the real capture showed does not match what Simics actually returns. fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { AttrValueType::List( rows.iter() @@ -156,7 +164,10 @@ fn fixture_attr_value(rows: &[FixtureRow]) -> AttrValueType { AttrValueType::Unsigned(row.base), AttrValueType::Unsigned(row.size), AttrValueType::Bool(true), - AttrValueType::String(row.embedded_path.clone().unwrap_or_default()), + match &row.embedded_path { + Some(path) => AttrValueType::String(path.clone()), + None => AttrValueType::Nil, + }, ]) }) .collect(), From a40b4668428464a2a4940de549c8149514148be2 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 17:12:14 +0200 Subject: [PATCH 08/18] feat: wire UEFI module discovery into DWARF source coverage at HARNESS_START Integrates UCOV-M1 (DWARF/ELF debug info parsing) with UCOV-M2 (UEFI/SMM module discovery via tracker_obj->maps) into a single feature: at HARNESS_START, query the configured UEFI tracker object, resolve each discovered module's local build path, derive its EDK2 GCC5 .debug ELF sidecar (same directory, extension swapped from .efi), parse its DWARF info, and merge the resulting symbols into the same per-processor symbol lookup tree Windows uses, gated by new `uefi`, `uefi_tracker_object`, and `uefi_debug_info_directory` attributes (mirroring `windows`/`debuginfo_download_directory`). Unlike Windows's CR3-write-triggered refresh, UEFI/SMM has no per-process address-space switch to key off, so this collects once at HARNESS_START (all tracked modules are loaded by then) rather than on a recurring trigger. --- src/haps/mod.rs | 75 +++++++++++++++++++++++++++++++++- src/lib.rs | 17 ++++++++ src/uefi/mod.rs | 104 ++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 192 insertions(+), 4 deletions(-) diff --git a/src/haps/mod.rs b/src/haps/mod.rs index 2b271f81..ddddeedd 100644 --- a/src/haps/mod.rs +++ b/src/haps/mod.rs @@ -3,7 +3,7 @@ //! Handlers for HAPs in the simulator -use std::time::SystemTime; +use std::{collections::HashSet, time::SystemTime}; use crate::{ arch::ArchitectureOperations, @@ -13,6 +13,7 @@ use crate::{ ManualStartInfo, Tsffs, }; use anyhow::{anyhow, bail, Result}; +use intervaltree::IntervalTree; use libafl::prelude::ExitKind; use simics::{ api::{ @@ -39,6 +40,73 @@ enum SnapshotRestoreMode { } impl Tsffs { + /// Collect UEFI/SMM source coverage info, if `uefi` and `symbolic_coverage` are + /// both set. Called once, at `HARNESS_START` (the same three call sites Windows + /// uses for its own initial collection), rather than on a recurring trigger -- + /// see `crate::uefi::collect_symbols`'s doc comment for why UEFI/SMM has no + /// CR3-write-equivalent refresh signal the way Windows does. + /// + /// The resulting interval tree is stored in `self.windows_os_info`, alongside + /// Windows's own per-processor symbol lookup trees, keyed the same way (by + /// processor number). `self.uefi` and `self.windows` are mutually exclusive in + /// practice (a target is either a Windows kernel or a UEFI/SMM BIOS, not both), + /// so this reuses the exact same storage and the tracer's existing OS-agnostic + /// coverage lookup (`src/tracer/mod.rs`, the `self.coverage_enabled && + /// self.symbolic_coverage` branch, which does not itself check `self.windows`) + /// rather than duplicating a second lookup path just for `uefi`. + fn collect_uefi_symbolic_coverage(&mut self, processor: *mut ConfObject) -> Result<()> { + if !(self.uefi && self.symbolic_coverage) { + return Ok(()); + } + + info!( + self.as_conf_object(), + "Collecting initial UEFI/SMM source coverage info" + ); + + let elements = crate::uefi::collect_symbols( + &self.uefi_tracker_object, + &self.uefi_debug_info_directory, + &self.source_file_cache, + )?; + + let mut filtered_ranges = HashSet::new(); + + // Deduplicate elements by their range, mirroring + // `WindowsOsInfo::collect`'s own deduplication. + let elements = elements + .into_iter() + .filter(|e| filtered_ranges.insert(e.range.clone())) + .collect::>(); + + // Populate elements into the coverage record set, mirroring + // `WindowsOsInfo::collect`'s own population of `user_debug_info.coverage`. + elements.iter().map(|e| &e.value).for_each(|si| { + if let Some(first) = si.lines.first() { + let record = self.coverage.get_or_insert_mut(&first.file_path); + record.add_function_if_not_exists( + first.start_line as usize, + si.lines.last().map(|l| l.end_line as usize), + &si.name, + ); + si.lines.iter().for_each(|l| { + (l.start_line..=l.end_line).for_each(|line| { + record.add_line_if_not_exists(line as usize); + }); + }); + } + }); + + let processor_nr = get_processor_number(processor)?; + + self.windows_os_info.symbol_lookup_trees.insert( + processor_nr, + elements.into_iter().collect::>(), + ); + + Ok(()) + } + fn on_simulation_stopped_magic_start(&mut self, magic_number: MagicNumber) -> Result<()> { if !self.have_initial_snapshot() { self.start_fuzzer_thread()?; @@ -86,6 +154,7 @@ impl Tsffs { &self.source_file_cache, )?; } + self.collect_uefi_symbolic_coverage(start_processor_raw)?; self.get_and_write_testcase()?; self.post_timeout_event()?; } @@ -283,6 +352,8 @@ impl Tsffs { )?; } + self.collect_uefi_symbolic_coverage(processor)?; + self.get_and_write_testcase()?; self.post_timeout_event()?; @@ -325,6 +396,8 @@ impl Tsffs { )?; } + self.collect_uefi_symbolic_coverage(processor)?; + self.post_timeout_event()?; } diff --git a/src/lib.rs b/src/lib.rs index 2439088e..a5599cc4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -465,6 +465,23 @@ pub(crate) struct Tsffs { /// Directory in which source files are located. Source files do not need to be arranged in /// the same directory structure as the compiled source, and are looked up by hash. pub symbolic_coverage_directory: PathBuf, + #[class(attribute(optional, default = false))] + /// Whether UEFI/SMM is being run in the simulation. When set with + /// `symbolic_coverage`, TSFFS collects source coverage for UEFI/SMM modules at + /// `HARNESS_START` by querying `uefi_tracker_object`'s loaded module list and + /// resolving each module's DWARF debug info under `uefi_debug_info_directory`. + pub uefi: bool, + #[class(attribute(optional, default = String::new()))] + /// The Simics object path of the UEFI/SMM module tracker to query for the loaded + /// module list (e.g. `board.software.tracker.tracker_obj`, queried via its + /// `->maps` attribute), used when `uefi` is set. Board-specific; there is no + /// default. + pub uefi_tracker_object: String, + #[class(attribute(optional, default = lookup_file("%simics%")?.join("uefi-debug-info")))] + /// Local build-output directory that UEFI/SMM modules' embedded build-machine + /// paths (reported by `uefi_tracker_object`) are resolved against, to locate + /// each module's local `.debug` DWARF sidecar file, used when `uefi` is set. + pub uefi_debug_info_directory: PathBuf, /// Handle for the core simulation stopped hap stop_hap_handle: HapHandle, diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index b092a3b7..9ecd62eb 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -121,14 +121,23 @@ //! that scenario is not disproven for all cases, just this one -- see //! [`UefiOsInfo::resolve`]'s doc comment. -use std::path::{Path, PathBuf}; +use std::{ + fs::read, + path::{Path, PathBuf}, +}; use anyhow::{anyhow, bail, Result}; -use simics::AttrValueType; +use intervaltree::Element; +use object::File as ObjectFile; +use simics::{free_attribute, get_object, run_command, AttrValueType}; use tracing::{debug, warn}; use walkdir::WalkDir; -use crate::util::path_suffix_index::PathSuffixIndex; +use crate::{ + dwarf::{DebugInfoModule, DwarfModule, SymbolInfo}, + source_cov::SourceCache, + util::path_suffix_index::PathSuffixIndex, +}; /// Placeholder name used for a module whose row has no path at all (a real /// "unknown"/unresolved module -- see the module doc comment's "Confirmed shape" @@ -410,3 +419,92 @@ fn find_by_stem(root: &Path, stem: &str) -> Result> { .map(|entry| entry.path().to_path_buf()) .collect()) } + +/// Query `tracker_object`'s `->maps` attribute (see the module doc comment for the +/// confirmed row shape), resolve each discovered module's local debug info against +/// `build_root`, and load each resolved module's DWARF debug info into interval-tree +/// elements -- the milestone-3 glue between this module's discovery (UCOV-M2) and +/// `crate::dwarf::DwarfModule` (UCOV-M1), previously left explicitly out of scope by +/// both (see this module's and `crate::dwarf`'s doc comments). +/// +/// UEFI/SMM has no CR3-equivalent per-process address-space switch to key a refresh +/// off (unlike `crate::os::windows::WindowsOsInfo::collect`, re-run on every CR3 +/// write): all tracked modules are loaded by the time `HARNESS_START` fires (DXE +/// dispatch completes before the harness/boot-menu stage), and TSFFS repeatedly +/// restores one snapshot afterward rather than switching address spaces. So this is +/// meant to be called exactly once, at `HARNESS_START`, not on a recurring trigger. +pub fn collect_symbols

( + tracker_object: &str, + build_root: P, + source_cache: &SourceCache, +) -> Result>> +where + P: AsRef, +{ + let maps = run_command(format!("{tracker_object}->maps"))?; + let value = AttrValueType::from(maps); + free_attribute(maps)?; + + let rows = parse_module_list(&value)?; + let resolved = UefiOsInfo::resolve(&rows, build_root)?; + + let mut elements = Vec::new(); + + for (name, base, local_path) in &resolved.modules { + // EDK2 GCC5 builds place a module's stripped `.efi` PE image and its + // unstripped ELF+DWARF `.debug` sidecar side by side in the same build + // output directory (see `crate::dwarf`'s module doc comment). `local_path` + // here is the local mirror of the module's *embedded* (`.efi`) path + // resolved by `UefiOsInfo::resolve` (confirmed by + // `tests/uefi_module_discovery_fixture.rs`, which resolves against a + // locally-mirrored `.efi` file, not a `.debug` one), so the sidecar this + // milestone actually needs is simply that path with its extension swapped. + let debug_path = local_path.with_extension("debug"); + + let bytes = match read(&debug_path) { + Ok(bytes) => bytes, + Err(e) => { + if let Ok(o) = get_object("tsffs") { + simics::warn!( + o, + "No DWARF debug info for UEFI module {name:?} (expected \ + {debug_path:?}, resolved from embedded path via \ + {local_path:?}): {e}" + ); + } + continue; + } + }; + + let object_file = match ObjectFile::parse(bytes.as_slice()) { + Ok(object_file) => object_file, + Err(e) => { + if let Ok(o) = get_object("tsffs") { + simics::warn!( + o, + "Failed to parse DWARF debug info for UEFI module {name:?} \ + at {debug_path:?}: {e}" + ); + } + continue; + } + }; + + let mut module = DwarfModule::new(name.clone(), *base, object_file); + + match module.intervals(source_cache) { + Ok(module_elements) => elements.extend(module_elements), + Err(e) => { + if let Ok(o) = get_object("tsffs") { + simics::warn!( + o, + "Failed to resolve source coverage intervals for UEFI \ + module {name:?} at {debug_path:?}: {e}" + ); + } + } + } + } + + Ok(elements) +} From 7148ce6f934c471ab74572951e93e4ff474797d9 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 17:30:06 +0200 Subject: [PATCH 09/18] fix: skip Invalid top-level entries in tracker_obj->maps parsing A live fuzzing validation run surfaced a case not covered by the offline fixtures: when HARNESS_START fires early in DXE dispatch (before all modules have loaded), tracker_obj->maps can return a list containing AttrValueType::Invalid entries -- reserved but not-yet- populated slots in the tracker's underlying storage -- alongside well-formed 7-element rows for the modules loaded so far. The parser treated any non-List top-level entry as fatal, aborting the whole batch. Skip Invalid entries instead, distinct from a genuinely pathless module row (a well-formed 7-element list with a Nil path). --- src/uefi/mod.rs | 15 ++++++++++++++- tests/uefi_module_discovery_fixture.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 9ecd62eb..2cb9d3b8 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -149,6 +149,16 @@ pub const UNKNOWN_MODULE_NAME: &str = ""; /// doc comment for the confirmed shape) into `(name, base, size, embedded_path)` /// tuples, where `name` is the bare filename extracted from `embedded_path`, or /// [`UNKNOWN_MODULE_NAME`] when a row has no path. +/// +/// Skips any top-level `AttrValueType::Invalid` entry rather than erroring the +/// whole batch over it. Confirmed live (a real fuzzing run whose +/// `HARNESS_START` fires early in DXE dispatch, well before all modules are +/// loaded): `maps` can return a list containing `Invalid` entries -- a reserved +/// but not-yet-populated slot in the tracker's underlying storage -- alongside +/// well-formed 7-element rows for the modules actually loaded so far. This is +/// distinct from a genuinely pathless *module* row (still a well-formed 7-element +/// list, just with a `Nil` path at index 6 -- see [`parse_module_row`]), so it's +/// handled separately, before a row is assumed to be a `List` at all. pub fn parse_module_list(value: &AttrValueType) -> Result> { let AttrValueType::List(rows) = value else { bail!( @@ -157,7 +167,10 @@ pub fn parse_module_list(value: &AttrValueType) -> Resultmaps` shape: diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs index 987f24d8..9567ed6e 100644 --- a/tests/uefi_module_discovery_fixture.rs +++ b/tests/uefi_module_discovery_fixture.rs @@ -494,6 +494,32 @@ fn resolve_fails_gracefully_not_panics_for_pathless_unknown_module() -> Result<( Ok(()) } +#[test] +fn skips_invalid_top_level_entries_without_erroring() -> Result<()> { + // Confirmed live (a real fuzzing run whose `HARNESS_START` + // fired early in DXE dispatch): `tracker_obj->maps` can return a list + // containing `AttrValueType::Invalid` entries -- reserved but not-yet- + // populated slots -- alongside well-formed 7-element module rows. This must + // not error the whole batch; those entries should simply be skipped. + let rows = fixture_rows(); + let mut value = fixture_attr_value(&rows); + + let AttrValueType::List(ref mut top_level) = value else { + panic!("fixture_attr_value did not return an AttrValueType::List"); + }; + top_level.insert(0, AttrValueType::Invalid); + top_level.push(AttrValueType::Invalid); + + let parsed = parse_module_list(&value)?; + assert_eq!( + parsed.len(), + rows.len(), + "Invalid top-level entries must be skipped, not counted as modules or cause an error" + ); + + Ok(()) +} + /// A `tracing_subscriber::fmt::MakeWriter` that captures formatted log output into /// a shared in-memory buffer, so tests can assert on it directly instead of only /// inferring the warning fired from behavior. From 50de30027ae6d1cfc6eb164ad6000991033b0151 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 17:34:46 +0200 Subject: [PATCH 10/18] fix: create symbolic_coverage_directory when it doesn't exist, not when it does save_symbolic_coverage's directory-creation check was inverted (create_dir_all only ran when the directory already existed), so Records::to_html failed with "Node not found" the first time a real coverage run tried to write output to a directory that had never been created. Surfaced by a live UEFI source-coverage validation run, the first time this environment exercised symbolic_coverage=true with real coverage data end to end. --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index a5599cc4..0940dc68 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1041,7 +1041,7 @@ impl Tsffs { } pub fn save_symbolic_coverage(&mut self) -> Result<()> { - if self.symbolic_coverage_directory.is_dir() { + if !self.symbolic_coverage_directory.is_dir() { create_dir_all(&self.symbolic_coverage_directory)?; } From ef8219b285b5bf01dada61119e34dc6d73c17023 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 17:43:14 +0200 Subject: [PATCH 11/18] fix: treat empty coverage as a no-op in save_symbolic_coverage, not an error Records::to_html seeds its output-tree graph entirely from the records themselves, so when zero source lines were ever recorded (e.g. a short run whose covered code never lands inside a symbolicated module -- observed for real with UEFI/SMM coverage, whose HARNESS_START can fire before every module is loaded), it never creates a graph node for the output directory and its root-node lookup fails with NodeNotFound on that exact path. That's an empty-coverage outcome, not a real error -- report and skip it instead of propagating it through the simulation-stopped HAP callback, which panics via .expect(). --- src/lib.rs | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0940dc68..bda040cc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1051,13 +1051,32 @@ impl Tsffs { self.symbolic_coverage_directory.display() ); - self.coverage.to_html(&self.symbolic_coverage_directory)?; - - debug!( - self.as_conf_object(), - "Symbolic coverage saved to {}", - self.symbolic_coverage_directory.display() - ); + // `Records::to_html` seeds its output tree from the records themselves, so + // if no source line was ever recorded (e.g. a short run whose covered code + // never lands inside a symbolicated module -- has been observed for real + // with UEFI/SMM coverage, whose HARNESS_START may fire before every module + // is loaded), it never creates a graph node for `output_directory` at all, + // and its own root-node lookup fails with `NodeNotFound` on that exact + // path. That's an empty-coverage outcome, not a real error, so it's + // reported and skipped rather than propagated as one. + match self.coverage.to_html(&self.symbolic_coverage_directory) { + Ok(()) => { + debug!( + self.as_conf_object(), + "Symbolic coverage saved to {}", + self.symbolic_coverage_directory.display() + ); + } + Err(lcov2::error::Error::NodeNotFound { ref path }) + if *path == self.symbolic_coverage_directory => + { + debug!( + self.as_conf_object(), + "No symbolic coverage was recorded this run; skipping HTML report generation" + ); + } + Err(e) => return Err(e.into()), + } Ok(()) } From d00936c28ebdb4cfa29762485631b1e64846e6c4 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:15:27 +0200 Subject: [PATCH 12/18] fix: read tracker_obj->maps via get_attribute, not run_command Live validation in a test session (a real boot with the compiled-in harness) exposed a genuine bug: querying maps via the CLI string-command path (run_command("tracker_obj->maps")) returned a stale/near-empty result (3 elements, all Invalid) at a point in boot where a direct attribute read of the exact same object already returned the real, fully-populated list (67 real rows, confirmed via a live Python probe at the same virtual time). The data was always there -- the CLI string-command round-trip was the bug. Switch to get_object + get_attribute (SIM_get_attribute), a direct FFI read with no CLI round-trip. --- src/uefi/mod.rs | 50 ++++++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 2cb9d3b8..c0adc9dd 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -10,20 +10,23 @@ //! against the Simics 6/7 headers). The confirmed-real mechanism, found via a //! live investigation (real Simics 6.0.189 session, a real //! checkpoint past DXE dispatch, 68 real loaded UEFI modules), is the -//! `uefi_fw_tracker` component's underlying C object's `maps` attribute, reached -//! from Rust via Simics's CLI arrow-attribute syntax and the exact same FFI -//! entry point TSFFS already uses elsewhere in this module's design: -//! `simics::api::simulator::script::run_command(String) -> Result` -//! (e.g. `run_command("tracker_obj->maps")`, where the -//! `qsp.software.tracker` object path is board-specific and must be supplied by -//! the caller, not hardcoded). This supersedes an earlier design that queried the -//! tracker's `list-modules` CLI command instead: `list-modules` itself calls -//! `basename()` on the underlying data before returning it, so it only ever -//! yields a bare filename, never a full path -- `tracker_obj->maps` is the same -//! underlying data with the full path intact. Calling `run_command` for real, -//! and everything downstream of it (wiring into `crate::haps`/`HARNESS_START`, a -//! `self.uefi` attribute on `Tsffs`, touching the OS enum), is explicitly out of -//! scope for this milestone -- see the UCOV-M2 spec's milestone-scope step 3. +//! `uefi_fw_tracker` component's underlying C object's `maps` attribute. This +//! supersedes an earlier design that queried the tracker's `list-modules` CLI +//! command instead: `list-modules` itself calls `basename()` on the underlying +//! data before returning it, so it only ever yields a bare filename, never a +//! full path -- `tracker_obj`'s `maps` attribute is the same underlying data +//! with the full path intact. +//! +//! `maps` is read via `simics::{get_object, get_attribute}` (`SIM_get_attribute` +//! on the object `get_object(tracker_object)` resolves to), not via Simics's CLI +//! arrow-attribute syntax (`run_command("{tracker_object}->maps")`). An earlier +//! revision of this module used the `run_command` string-command path; live +//! validation in a live test session (a real boot, `HARNESS_START` firing well into DXE +//! dispatch with ~30 real modules already loaded) confirmed that path returns a +//! stale/near-empty result (3 elements, all `AttrValueType::Invalid`) at a point +//! in boot where a direct attribute read of the exact same object returns the +//! real, fully-populated list (67 real rows) -- the live data was always there; +//! the CLI string-command round-trip was the bug. //! //! This module implements only the two pieces of that spec that are testable //! completely offline, with no live Simics session and no real BIOS/UEFI image: @@ -38,7 +41,7 @@ //! //! `simics::AttrValue` is a `#[repr(C)]` wrapper around the C `attr_value_t` //! union (see `simics::api::base::attr_value`). Reading a real, already-populated -//! `AttrValue` (e.g. one actually returned by `run_command`) is safe pure memory +//! `AttrValue` (e.g. one actually returned by `get_attribute`) is safe pure memory //! access with no FFI call (`AttrValue::as_heterogeneous_list`/`as_heterogeneous_dict` //! just walk `private_u.list`/`private_u.dict` pointers). But *constructing* an //! owned `AttrValue::List`/`AttrValue::Dict` from scratch (e.g. `AttrValue::list(n)`, @@ -54,7 +57,7 @@ //! Dict(BTreeMap)`) has no such constructors -- its variants are built //! with plain Rust syntax, no FFI at all -- so it is what this module's parser //! takes, and what the offline tests construct fixtures as. At a real call site, -//! converting the real `AttrValue` returned by `run_command` into `AttrValueType` +//! converting the real `AttrValue` returned by `get_attribute` into `AttrValueType` //! via `.into()` (`impl From for AttrValueType`) is the safe, pure-read //! conversion described above; this module never needs to go the other direction. //! @@ -63,7 +66,7 @@ //! Unlike the superseded `list-modules`-based design (which had to *assume* a //! shape, since it was never actually queried live), this shape is a confirmed //! fact, captured from a real live tracker dump reached from Rust via -//! `run_command`, using the exact same FFI path this module documents above: +//! `get_attribute`, using the exact same FFI path this module documents above: //! //! - The top-level value is a `List` of rows. //! - Each row is itself a positional `List` of exactly 7 elements (**not** a dict @@ -129,7 +132,7 @@ use std::{ use anyhow::{anyhow, bail, Result}; use intervaltree::Element; use object::File as ObjectFile; -use simics::{free_attribute, get_object, run_command, AttrValueType}; +use simics::{free_attribute, get_attribute, get_object, AttrValueType}; use tracing::{debug, warn}; use walkdir::WalkDir; @@ -454,7 +457,16 @@ pub fn collect_symbols

( where P: AsRef, { - let maps = run_command(format!("{tracker_object}->maps"))?; + // Read `maps` via `SIM_get_attribute` (`get_object` + `get_attribute`), not + // `run_command("{tracker_object}->maps")`. The CLI string-command path was + // confirmed live (a real boot) to return a near-empty/stale + // result (3 elements, all `Invalid`) at a point in boot where a direct + // attribute read of the exact same object/attribute already returns the + // real, fully-populated list (67 real rows) -- the live data is there: + // `run_command`'s string-command round-trip was the actual bug, not a + // tracker-timing issue. + let tracker_conf_object = get_object(tracker_object)?; + let maps = get_attribute(tracker_conf_object, "maps")?; let value = AttrValueType::from(maps); free_attribute(maps)?; From 1905bc4185d75aeff52e913b1b1047c70a6c545c Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:22:07 +0200 Subject: [PATCH 13/18] debug: add temporary diagnostic log for raw/parsed module row counts Investigating why source coverage stays empty even after switching tracker_obj->maps reads from run_command to get_attribute: need to see the real row counts as observed by collect_symbols itself, since an unrelated "AttrValue(Sim_Val_List, 3, ...)" console line (not emitted by this code) turned out to be a red herring -- it didn't change at all after the get_attribute switch, so it isn't actually reporting this function's data. --- src/uefi/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index c0adc9dd..1234ae4f 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -471,6 +471,14 @@ where free_attribute(maps)?; let rows = parse_module_list(&value)?; + if let Ok(o) = get_object("tsffs") { + simics::info!( + o, + "TSFFS_DIAG: raw AttrValueType::List has {} top-level entries; parsed {} real module rows", + if let AttrValueType::List(l) = &value { l.len() } else { 0 }, + rows.len() + ); + } let resolved = UefiOsInfo::resolve(&rows, build_root)?; let mut elements = Vec::new(); From 5ea59f64ec2a382b0f6f4da64699748db3a3a7db Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:33:37 +0200 Subject: [PATCH 14/18] fix: convert tracker_obj->maps rows via as_heterogeneous_list, not plain .into() Root cause of the persistently-empty UEFI source coverage: plain AttrValueType::from(AttrValue) (equivalently `.into()`) recurses into nested lists via AttrValue::as_list::, which requires every element of a list to share the same private_kind before converting any of them -- despite the crate's own doc comment suggesting heterogeneous lists are supported via a *different* method (as_heterogeneous_list). The outer `maps` list is homogeneous (every row is itself a List) so converting it this way works, but each row is [Unsigned, Unsigned, Bool, Unsigned, Unsigned, Bool, String] -- genuinely heterogeneous -- so plain `.into()` silently converted every real row to AttrValueType::Invalid. Confirmed in a live test session: a real, fully-populated 67-row maps parsed as 0 module rows, no errors, no warnings, just silently wrong -- and completely unrelated to the tracker-timing/run_command issues investigated (and fixed) earlier. Now: extract each row as a raw AttrValue first (as_list::, an identity conversion -- fine, since the outer list genuinely is homogeneous), then convert each row with as_heterogeneous_list, which has no such check. --- src/uefi/mod.rs | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 1234ae4f..18f60495 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -132,7 +132,7 @@ use std::{ use anyhow::{anyhow, bail, Result}; use intervaltree::Element; use object::File as ObjectFile; -use simics::{free_attribute, get_attribute, get_object, AttrValueType}; +use simics::{free_attribute, get_attribute, get_object, AttrValue, AttrValueType}; use tracing::{debug, warn}; use walkdir::WalkDir; @@ -467,18 +467,42 @@ where // tracker-timing issue. let tracker_conf_object = get_object(tracker_object)?; let maps = get_attribute(tracker_conf_object, "maps")?; - let value = AttrValueType::from(maps); + + // `AttrValueType::from(AttrValue)` (equivalently, plain `.into()`) recurses + // into nested lists via `AttrValue::as_list::`, which -- despite + // the crate's own "Rust vectors cannot be heterogeneous" comment suggesting + // otherwise -- requires every element of a list to share the same + // `private_kind` before converting any of them, silently returning `None` + // (and thus `AttrValueType::Invalid`) for the whole list otherwise. The outer + // `maps` list is homogeneous (every row is itself a `List`), so converting + // *it* this way works. But each row is `[Unsigned, Unsigned, Bool, Unsigned, + // Unsigned, Bool, String]` -- genuinely heterogeneous -- so plain `.into()` + // silently turned every real row into `AttrValueType::Invalid`, confirmed + // live: a real 67-row `maps` converted this way parsed as 0 module + // rows, no errors, no warnings, just quietly wrong. `as_heterogeneous_list` + // has no such check, so extract each row as a raw `AttrValue` first (via + // `as_list::`, an identity conversion -- fine, since the *outer* + // list is genuinely homogeneous), then convert each row with + // `as_heterogeneous_list`, which is exactly what a 7-element heterogeneous + // row needs. + let raw_rows: Vec = maps + .as_list() + .ok_or_else(|| anyhow!("expected tracker_obj->maps to be an AttrValue list"))?; + + let value = AttrValueType::List( + raw_rows + .iter() + .map(|row| { + row.as_heterogeneous_list() + .map(AttrValueType::List) + .ok_or_else(|| anyhow!("expected each tracker_obj->maps row to be a list")) + }) + .collect::>>()?, + ); + free_attribute(maps)?; let rows = parse_module_list(&value)?; - if let Ok(o) = get_object("tsffs") { - simics::info!( - o, - "TSFFS_DIAG: raw AttrValueType::List has {} top-level entries; parsed {} real module rows", - if let AttrValueType::List(l) = &value { l.len() } else { 0 }, - rows.len() - ); - } let resolved = UefiOsInfo::resolve(&rows, build_root)?; let mut elements = Vec::new(); From 448aad1db5a4fdd8ef7b9d53952cf09b9fa1ecbc Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:39:20 +0200 Subject: [PATCH 15/18] fix: skip unresolvable modules per-module, not fail the whole resolve batch UefiOsInfo::resolve propagated the first per-module resolution error via `?`, aborting the entire batch. Confirmed in a live test session: a real tracker_obj->maps capture always has at least one genuinely pathless "" module (mixed in with dozens of resolvable ones), so that one module was discarding source coverage for every other resolvable module in the same capture -- the exact opposite of "fail gracefully," just failing the whole batch loudly instead of one entry quietly. Now logs a warning and skips just that module, keeping the rest. Updates the existing offline test's asserted semantics (resolving a pathless-only batch is now Ok with zero results, not Err) and adds a new mixed-batch test guarding the actual real-world case: resolvable and unresolvable modules together in one capture. --- src/uefi/mod.rs | 15 ++++++- tests/uefi_module_discovery_fixture.rs | 61 +++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 18f60495..1b5c8dce 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -329,8 +329,19 @@ impl UefiOsInfo { let mut resolved = Vec::with_capacity(modules.len()); for (name, base, _size, embedded_path) in modules { - let local_path = resolve_one(&index, build_root, name, embedded_path)?; - resolved.push((name.clone(), *base, local_path)); + // Skip (with a warning), rather than fail the entire batch over, + // any single module that can't be resolved. Confirmed live + // (a real boot): a real 67-module tracker_obj->maps + // capture always has at least one genuinely pathless "" + // module (see resolve_one's doc comment) -- letting that one + // module's error abort the whole call would discard source + // coverage for the other 66 real, resolvable modules too. + match resolve_one(&index, build_root, name, embedded_path) { + Ok(local_path) => resolved.push((name.clone(), *base, local_path)), + Err(e) => { + warn!("skipping unresolvable module {name:?}: {e}"); + } + } } Ok(Self { modules: resolved }) diff --git a/tests/uefi_module_discovery_fixture.rs b/tests/uefi_module_discovery_fixture.rs index 9567ed6e..5f1ba61c 100644 --- a/tests/uefi_module_discovery_fixture.rs +++ b/tests/uefi_module_discovery_fixture.rs @@ -471,7 +471,7 @@ fn falls_back_to_stem_match_and_warns_on_ambiguity_when_suffix_match_fails() -> } #[test] -fn resolve_fails_gracefully_not_panics_for_pathless_unknown_module() -> Result<()> { +fn resolve_skips_gracefully_not_panics_for_pathless_unknown_module() -> Result<()> { let tmp = tempdir()?; let root = tmp.path(); @@ -482,13 +482,60 @@ fn resolve_fails_gracefully_not_panics_for_pathless_unknown_module() -> Result<( .collect::>(); assert_eq!(modules.len(), 1); - // Resolving a genuinely pathless ("unknown module") row must fail cleanly - // (a `Result::Err`, caught here, not a panic/process abort) -- graceful - // fallback, not a crash or silent misparse. - let result = UefiOsInfo::resolve(&modules, root); + // Resolving a genuinely pathless ("unknown module") row must not panic or + // abort the batch (a real `tracker_obj->maps` capture always has at least + // one such row -- see UefiOsInfo::resolve's doc comment) -- it's skipped + // with a warning, leaving an empty (not missing) result. + let info = UefiOsInfo::resolve(&modules, root)?; assert!( - result.is_err(), - "resolving an unknown/pathless module must return an Err, not silently succeed" + info.modules.is_empty(), + "an unknown/pathless module must be skipped, not resolved to a bogus path" + ); + + Ok(()) +} + +#[test] +fn resolve_skips_only_the_unresolvable_module_in_a_mixed_batch() -> Result<()> { + // The real-world case this guards: a real tracker_obj->maps capture is a + // mix of resolvable and genuinely pathless modules (confirmed live on + // a live test session: 66 resolvable real modules plus 1 pathless "" one, in + // a single 67-row capture). One unresolvable module must not discard + // source coverage for every other resolvable module in the same batch. + let tmp = tempdir()?; + let root = tmp.path(); + + let rows = fixture_rows(); + let modules = parse_module_list(&fixture_attr_value(&rows))?; + let resolvable_count = modules + .iter() + .filter(|(name, ..)| name != UNKNOWN_MODULE_NAME) + .count(); + + for (name, _base, _size, embedded_path) in &modules { + if name == UNKNOWN_MODULE_NAME { + continue; + } + let suffix = embedded_path + .to_string_lossy() + .rsplit_once("DEBUG_GCC/") + .expect("fixture embedded path contains the DEBUG_GCC/ prefix marker") + .1 + .to_string(); + let local_path = root.join(suffix); + create_dir_all( + local_path + .parent() + .expect("local fixture path has a parent directory"), + )?; + write(&local_path, b"contents")?; + } + + let info = UefiOsInfo::resolve(&modules, root)?; + assert_eq!( + info.modules.len(), + resolvable_count, + "every resolvable module in the batch must still resolve despite the one unresolvable module" ); Ok(()) From 4f39b6dee93885d5603093ad3a8c5d07d7f3355a Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:47:21 +0200 Subject: [PATCH 16/18] debug: add temporary diagnostic logs for resolve/element counts --- src/uefi/mod.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 1b5c8dce..6bd7f7b5 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -514,7 +514,20 @@ where free_attribute(maps)?; let rows = parse_module_list(&value)?; + let build_root_display = build_root.as_ref().display().to_string(); let resolved = UefiOsInfo::resolve(&rows, build_root)?; + if let Ok(o) = get_object("tsffs") { + simics::info!( + o, + "TSFFS_DIAG2: parsed {} rows, resolved {} local paths, build_root={}", + rows.len(), + resolved.modules.len(), + build_root_display + ); + if let Some((name, _base, path)) = resolved.modules.first() { + simics::info!(o, "TSFFS_DIAG2: first resolved module {name:?} -> {path:?}"); + } + } let mut elements = Vec::new(); @@ -574,5 +587,9 @@ where } } + if let Ok(o) = get_object("tsffs") { + simics::info!(o, "TSFFS_DIAG2: collected {} interval elements", elements.len()); + } + Ok(elements) } From defaff26b8cccf8269f4c61512ed577dc9f8bf66 Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Wed, 16 Sep 2026 20:56:18 +0200 Subject: [PATCH 17/18] docs: remove temporary diagnostics, document debuginfo_source_directory requirement Removes the TSFFS_DIAG2 logging added to isolate the previous three fixes (get_attribute, as_heterogeneous_list, per-module resolve skip). With all three applied plus debuginfo_source_directory pointed at the real local EDK2 source tree, end-to-end UEFI source coverage is confirmed working in a live test session: a real fuzzing campaign against the compiled-in harness produces a real, non-empty HTML coverage report (e.g. MdePkg/Library/StackCheckLibNull/StackCheckLibNullGcc.c.html) from 66 resolved real UEFI modules and 29 collected DWARF interval elements. Documents on the `uefi` attribute that debuginfo_source_directory must also be set for source *lines* to resolve -- symbols resolve independently of it, but every one has zero lines without it, since DWARF line entries resolve against debuginfo_source_directory, not uefi_debug_info_directory (which only locates each module's own .debug file, not its original source). --- src/lib.rs | 7 +++++++ src/uefi/mod.rs | 17 ----------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bda040cc..3064b29b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -470,6 +470,13 @@ pub(crate) struct Tsffs { /// `symbolic_coverage`, TSFFS collects source coverage for UEFI/SMM modules at /// `HARNESS_START` by querying `uefi_tracker_object`'s loaded module list and /// resolving each module's DWARF debug info under `uefi_debug_info_directory`. + /// `debuginfo_source_directory` must also point at the real local source tree + /// (e.g. the EDK2 checkout) for any source *lines* to be resolved -- symbols + /// resolve independently of it, but every one of them will have zero lines + /// (and thus never contribute to the coverage report) without it, since DWARF + /// line entries are resolved against `debuginfo_source_directory`, not + /// `uefi_debug_info_directory` (which only locates each module's own `.debug` + /// file, not its original source). pub uefi: bool, #[class(attribute(optional, default = String::new()))] /// The Simics object path of the UEFI/SMM module tracker to query for the loaded diff --git a/src/uefi/mod.rs b/src/uefi/mod.rs index 6bd7f7b5..1b5c8dce 100644 --- a/src/uefi/mod.rs +++ b/src/uefi/mod.rs @@ -514,20 +514,7 @@ where free_attribute(maps)?; let rows = parse_module_list(&value)?; - let build_root_display = build_root.as_ref().display().to_string(); let resolved = UefiOsInfo::resolve(&rows, build_root)?; - if let Ok(o) = get_object("tsffs") { - simics::info!( - o, - "TSFFS_DIAG2: parsed {} rows, resolved {} local paths, build_root={}", - rows.len(), - resolved.modules.len(), - build_root_display - ); - if let Some((name, _base, path)) = resolved.modules.first() { - simics::info!(o, "TSFFS_DIAG2: first resolved module {name:?} -> {path:?}"); - } - } let mut elements = Vec::new(); @@ -587,9 +574,5 @@ where } } - if let Ok(o) = get_object("tsffs") { - simics::info!(o, "TSFFS_DIAG2: collected {} interval elements", elements.len()); - } - Ok(elements) } From 7ec6f40c32a74a3155a588d551978c90ba2c4f1f Mon Sep 17 00:00:00 2001 From: Mathieu Tarral Date: Mon, 21 Sep 2026 04:31:16 -0700 Subject: [PATCH 18/18] fix: resolve DWARF subprogram names via DW_AT_abstract_origin/specification Real EDK2 GCC5 debug info universally emits the abstract/concrete instance DWARF split: every concrete DW_TAG_subprogram DIE (100% of 114,310 checked across 914 real .debug files from an internal validation corpus) has no direct DW_AT_name, only DW_AT_abstract_origin pointing at the DIE that carries the real name. The abstract origin is frequently in a different compilation unit (DW_FORM_ref_addr/DebugInfoRef, not a same-unit UnitRef), since GCC emits each abstract instance once and shares it across every CU that inlines/instantiates it. The previous code skipped any DIE without a direct DW_AT_name, silently dropping every real function and leaving symbolic coverage permanently empty with no error anywhere downstream. intervals() now pre-parses all units up front and resolves names across all of them. Validated against all 914 real .debug files from an internal validation run: 0/914 -> 914/914 producing symbols, 114,310 total (matches the exact concrete-DIE count from live investigation), names spot-checked correct. tests/dwarf_fixture.rs (direct DW_AT_name, no indirection) still passes. --- src/dwarf/mod.rs | 90 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 13 deletions(-) diff --git a/src/dwarf/mod.rs b/src/dwarf/mod.rs index b9e02013..97b8a69e 100644 --- a/src/dwarf/mod.rs +++ b/src/dwarf/mod.rs @@ -41,8 +41,8 @@ use std::{borrow::Cow, collections::HashMap, path::PathBuf}; use anyhow::{anyhow, Result}; use gimli::{ - DebuggingInformationEntry, DwarfSections, EndianSlice, LineProgramHeader, Reader, - RunTimeEndian, SectionId, UnitRef, + AttributeValue, DebuggingInformationEntry, DwarfSections, EndianSlice, LineProgramHeader, + Reader, RunTimeEndian, SectionId, Unit, UnitRef, }; use intervaltree::Element; use object::{Object, ObjectSection}; @@ -166,6 +166,7 @@ impl<'data> DwarfModule<'data> { fn unit_symbols( &self, unit_ref: UnitRef, + all_units: &[Unit], source_cache: &SourceCache, ) -> Result> { let Some(incomplete_line_program) = unit_ref.line_program.clone() else { @@ -225,14 +226,14 @@ impl<'data> DwarfModule<'data> { continue; } - let Some(name) = entry.attr_value(gimli::DW_AT_name) else { - // No direct DW_AT_name (e.g. only reachable via DW_AT_specification / - // DW_AT_abstract_origin). Handling that indirection is left for a - // follow-up; skip for now. + let Some((name, name_unit_ref)) = Self::resolve_name(entry, unit_ref, all_units) else { + // No name resolvable via DW_AT_name, DW_AT_abstract_origin, or + // DW_AT_specification -- not a concrete named subprogram we can key + // symbol info on. continue; }; - let name = unit_ref + let name = name_unit_ref .attr_string(name) .map_err(|e| anyhow!("Failed to read DWARF subprogram name: {e}"))? .to_string_lossy() @@ -254,6 +255,58 @@ impl<'data> DwarfModule<'data> { Ok(symbols) } + /// Resolve a DIE's effective `DW_AT_name` attribute for stringification, following + /// `DW_AT_abstract_origin` (falling back to `DW_AT_specification`) to the referenced + /// DIE when `entry` has no direct `DW_AT_name` of its own. Returns the resolved + /// `DW_AT_name` attribute value together with the `UnitRef` of the unit that DIE + /// actually lives in -- needed to correctly stringify forms such as + /// `DW_FORM_strx` (relative to a per-unit string-offsets base); not needed for the + /// common `DW_FORM_strp` (absolute `.debug_str` offset) case seen in practice, but + /// cheap to keep correct either way. + /// + /// GCC5/EDK2 universally emits the standard "abstract instance / concrete + /// instance" DWARF split for every real function: the concrete DIE (the one with + /// `DW_AT_low_pc`/`DW_AT_high_pc`, i.e. `entry` here) has `DW_AT_abstract_origin` + /// pointing at a separate DIE that carries the real `DW_AT_name`, instead of a + /// direct name on itself. Confirmed against 914 real EDK2 GCC5 `.debug` files, + /// that reference is universally `DW_FORM_ref_addr` (a raw `.debug_info`-section + /// offset, decoded by gimli as `AttributeValue::DebugInfoRef`), rather than a same-unit + /// `AttributeValue::UnitRef` -- GCC emits each "abstract instance" DIE once, in + /// whichever compilation unit first defines it, and references it from every other + /// unit that inlines/instantiates it, so the referenced DIE is frequently in a + /// *different* CU than `entry`. Hence resolving it requires searching + /// `all_units` (every unit in this module, pre-parsed by `intervals`), not just + /// `unit_ref`'s own unit -- `AttributeValue::UnitRef` is still handled too, in case + /// some DIEs reference same-unit offsets instead. + fn resolve_name<'u, R: Reader>( + entry: &DebuggingInformationEntry, + unit_ref: UnitRef<'u, R>, + all_units: &'u [Unit], + ) -> Option<(AttributeValue, UnitRef<'u, R>)> { + if let Some(name) = entry.attr_value(gimli::DW_AT_name) { + return Some((name, unit_ref)); + } + + let origin = entry + .attr_value(gimli::DW_AT_abstract_origin) + .or_else(|| entry.attr_value(gimli::DW_AT_specification))?; + + match origin { + AttributeValue::UnitRef(offset) => { + let origin_entry = unit_ref.entry(offset).ok()?; + let name = origin_entry.attr_value(gimli::DW_AT_name)?; + Some((name, unit_ref)) + } + AttributeValue::DebugInfoRef(offset) => all_units.iter().find_map(|candidate| { + let local_offset = offset.to_unit_offset(&candidate.header)?; + let origin_entry = candidate.entry(local_offset).ok()?; + let name = origin_entry.attr_value(gimli::DW_AT_name)?; + Some((name, candidate.unit_ref(unit_ref.dwarf))) + }), + _ => None, + } + } + /// Compute the `[low, high)` link-time address range of a DIE from its /// `DW_AT_low_pc`/`DW_AT_high_pc`/`DW_AT_ranges` attributes, aggregating over all /// ranges if there is more than one (e.g. for a DIE split into disjoint pieces). @@ -334,20 +387,31 @@ impl<'data> DebugInfoModule for DwarfModule<'data> { .map_err(|e| anyhow!("Failed to load DWARF sections: {e}"))?; let dwarf = dwarf_sections.borrow(|section| EndianSlice::new(section, endian)); - let mut symbols = Vec::new(); - + // Pre-parse every compilation unit up front, rather than one at a time as + // they're walked below, so that DW_AT_abstract_origin/DW_AT_specification + // references that cross compilation-unit boundaries (see `resolve_name`) can + // be resolved against *any* unit, not just whichever one is currently being + // walked. + let mut all_units = Vec::new(); let mut unit_headers = dwarf.units(); while let Some(header) = unit_headers .next() .map_err(|e| anyhow!("Failed to read next DWARF unit header: {e}"))? { - let unit = dwarf - .unit(header) - .map_err(|e| anyhow!("Failed to parse DWARF unit: {e}"))?; + all_units.push( + dwarf + .unit(header) + .map_err(|e| anyhow!("Failed to parse DWARF unit: {e}"))?, + ); + } + + let mut symbols = Vec::new(); + + for unit in &all_units { let unit_ref = unit.unit_ref(&dwarf); - symbols.extend(self.unit_symbols(unit_ref, source_cache)?); + symbols.extend(self.unit_symbols(unit_ref, &all_units, source_cache)?); } Ok(symbols