From df40b8670932dc651de5f7fb9015e3d16066081f Mon Sep 17 00:00:00 2001 From: Simon Dick Date: Wed, 16 Sep 2026 17:26:59 +0100 Subject: [PATCH 1/2] Report source file:line (or symbol+offset) in diagnostics (issue #74) Sanitizer violations now say where in the source they happened: invalid 1-byte write at 0x000032a0 (heap redzone) from PC 0x00002b38 (at work:memtest.s:281) and line 281 of that source is exactly the offending instruction. Two sources, best first: a HUNK_DEBUG `LINE` block's (line, offset) pairs, else the binary's HUNK_SYMBOL table as symbol+offset. The raw PC is always kept alongside rather than replaced -- it is what a disassembly needs, and sparse line data points at a statement rather than a specific instruction. Parsing is lazy: `parse`/`load` capture the raw HUNK_DEBUG and HUNK_SYMBOL payloads cheaply and never interpret them, with structured decoding happening on first query. That meant no signature changes and no existing caller touched, and it keeps the common case from paying for debug info that can dominate a file (LawBreaker is 776 bytes of which 456 is debug). `ShadowMap::report_with` takes a resolver *closure* rather than reaching for the information: a shadow map knows PCs and nothing about hunks, load addresses or debug blocks, so the lookup stays on the CLI's side and `crate::sanitize` gains no dependency on the loader. `report()` still exists, delegating with a no-op resolver. Investigating what real toolchains actually emit reshaped the scope twice, and both findings are documented rather than glossed: - SAS/C's `LINE` block (DEBUG=LINE) is byte-compatible with the one PhxAss emits (LINEDEBUG) and with LawBreaker's, so one parser serves three producers. SAS/C also emits OPTS and SRC6 blocks alongside. - m68k-amigaos-gcc emits *untagged stabs* -- its debug payload begins `00 00 00 4c 00 00 00 10 ff ff ff ff`, so where LINE/OPTS/SRC6 carry ASCII it has none. gcc therefore gets no file:line from this, which matters because gcc users are the audience that motivated the whole sanitizer effort. They do get symbol+offset: a real gcc-built stack smash now reports `found 0x41414141 ... (at ___main+0x3c)`. Stabs is filed as follow-up work; an untagged block has no magic to dispatch on, so identifying it safely is its own problem, and a wrong line number is worse than none. The magic dispatch is an explicit enum with a documented unrecognised-and-skipped arm, so adding stabs is a match arm rather than unpicking an `if`. A test uses gcc's real untagged byte sequence to prove it is skipped cleanly rather than mistaken for LINE. New fixtures/linetest carries real LINE data, built by PhxAss with LINEDEBUG. It replaces tests pointing at a scratch file and at LawBreaker (unvendorable under Enforcer's licence), both of which would have silently skipped forever once those files vanished. It is deliberately PhxAss-only with no generator, since amiga_asm.py cannot emit debug hunks and a generator producing a debug-free binary would look authoritative while lacking the entire point. It also proved a real requirement: PhxAss's data-hunk LINE pairs are *not* offset-monotonic (line 33 -> 0x0e, 36 -> 0x00, 37 -> 0x15), so the parser must sort rather than trust file order. Assuming order there would have produced wrong line numbers, not missing ones. Documented limits: line data is sparse, so a PC between entries is attributed to the earlier statement; and `static` functions never appear in HUNK_SYMBOL, so a violation inside one falls to the nearest exported symbol, which can be a surprising name with a large offset. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23 --- crates/volamos-core/src/loader.rs | 1495 ++++++++++++++++++++++++++- crates/volamos-core/src/sanitize.rs | 37 +- crates/volamos/src/main.rs | 52 +- crates/volamos/tests/hello_cli.rs | 19 + fixtures/README.md | 106 ++ fixtures/linetest | Bin 0 -> 240 bytes fixtures/linetest.s | 37 + userdocs/CLI-Reference.md | 50 + userdocs/Changelog.md | 20 + 9 files changed, 1762 insertions(+), 54 deletions(-) create mode 100644 fixtures/linetest create mode 100644 fixtures/linetest.s diff --git a/crates/volamos-core/src/loader.rs b/crates/volamos-core/src/loader.rs index 3f3aa09..34b5aac 100644 --- a/crates/volamos-core/src/loader.rs +++ b/crates/volamos-core/src/loader.rs @@ -28,22 +28,129 @@ //! read position mid-longword). //! //! `HUNK_NAME` (0x3E8), `HUNK_SYMBOL` (0x3F0) and `HUNK_DEBUG` (0x3F1) -//! blocks are recognized and skipped (their contents are discarded) -//! wherever a hunk boundary allows one to appear -- immediately before a -//! hunk's body (a real assembler-produced binary, e.g. one built by -//! `vasm`/`PhxAss` with source-line debug info left in, can open a hunk -//! with one or more `HUNK_DEBUG` blocks before its `HUNK_CODE`), and in -//! their traditional position after a body's relocations. Any number of -//! them can appear back-to-back in either spot. This is so binaries built -//! with `-nosym` *or* with symbol/debug info left in still load. -//! `HUNK_SYMBOL`'s on-disk shape is not the simple longword-count-prefixed -//! payload the other two use -- it's a list of `{ name_length_longwords, -//! name, value }` entries terminated by a zero `name_length`, so it needs -//! its own parsing loop (see [`skip_metadata_block`]) rather than being -//! treated like `HUNK_NAME`/`HUNK_DEBUG`. `HUNK_LIB` (link library -//! archives) is still not supported -- that's a different format -//! entirely (an indexed collection of object modules for a linker to pull -//! from, not something `LoadSeg` ever sees). +//! blocks are recognized wherever a hunk boundary allows one to appear -- +//! immediately before a hunk's body (a real assembler-produced binary, +//! e.g. one built by `vasm`/`PhxAss` with source-line debug info left in, +//! can open a hunk with one or more `HUNK_DEBUG` blocks before its +//! `HUNK_CODE`), and in their traditional position after a body's +//! relocations. Any number of them can appear back-to-back in either +//! spot. This is so binaries built with `-nosym` *or* with symbol/debug +//! info left in still load. `HUNK_LIB` (link library archives) is still +//! not supported -- that's a different format entirely (an indexed +//! collection of object modules for a linker to pull from, not something +//! `LoadSeg` ever sees). +//! +//! Only `HUNK_NAME` is discarded outright (see [`skip_metadata_block`]). +//! `HUNK_DEBUG` and `HUNK_SYMBOL` are *captured*, not discarded -- their +//! raw bytes go into [`Hunk::debug_blocks`]/[`Hunk::symbol_blocks`] +//! (bounds-checked copies, same precedent as everything else here -- see +//! [`read_debug_block_payload`]/[`read_symbol_block_raw`]) but aren't +//! *interpreted* at parse time; see "Source-line info" and "Symbol +//! attribution" below. `HUNK_SYMBOL`'s on-disk shape is not the simple +//! longword-count-prefixed payload `HUNK_NAME` uses -- it's a list of +//! `{ name_length_longwords, name, value }` entries terminated by a zero +//! `name_length`, so capturing it means walking that list to find where +//! it ends (see [`read_symbol_block_raw`]), not just copying `n` +//! longwords like `HUNK_NAME`/`HUNK_DEBUG`. +//! +//! # Source-line info (`HUNK_DEBUG` / `LINE` blocks, issue #74) +//! +//! A `HUNK_DEBUG` block's payload can hold several different things, +//! identified by a 4-byte magic after a leading base-offset longword +//! (or, for one family below, by *no* magic at all). The one this +//! loader decodes is `LINE`: a source filename plus a list of `(source +//! line number, hunk-relative byte offset)` pairs. Confirmed +//! byte-for-byte against two real producers: a real `LawBreaker` binary +//! (magic `LINE`, filename `LawBreaker.asm`, built by an assembler) and +//! a real SAS/C 6.58 `sc DEBUG=LINE` object file (magic `LINE`, +//! filename `work:hello.c`, alongside sibling `OPTS`/`SRC6` blocks this +//! loader doesn't interpret). `OPTS`/`SRC6` (SAS/C) and `HEAD` (the +//! first four bytes of a `HEADDBGV01` directory block some assemblers +//! emit, indexing the file offsets of their own `HUNK_DEBUG` blocks -- +//! redundant with the blocks already appearing in the hunk stream, so +//! not needed here) are recognized magics that this loader deliberately +//! does *not* act on. +//! +//! **`m68k-amigaos-gcc` is not covered.** Checked empirically against a +//! real local install (`-g -O0 -noixemul`): its `HUNK_DEBUG` payload is +//! stabs-format debug info, not `LINE` -- and per a reference decode +//! (alfishe/amiga-bootcamp's `hunk_debug_info.md`), the stabs family is +//! tagged `=APS` (SAS/C 6.x) or `=GCC`, "or no tag at all" for +//! `m68k-amigaos-gcc` specifically, which is exactly what was observed: +//! the four bytes where `LINE`/`OPTS`/`SRC6` carry an ASCII magic are +//! `00 00 00 10` here, not a magic at all. Stabs entries encode line +//! info as `N_SLINE` records against a separate string table -- a +//! different, more involved format this loader does not attempt to +//! parse (no magic to safely dispatch on, and it needs string-table +//! handling this format doesn't). An untagged/stabs block is simply +//! unrecognized here and skipped, same as any other unknown magic -- +//! see [`DebugMagic`]. This matters because gcc-built programs are a +//! large share of what this loader ultimately serves diagnostics for; +//! they get no `file:line` from this code today, only assembler/SAS/C +//! binaries built with `LINE`-format debug info do. +//! +//! Magic dispatch is an explicit match on [`DebugMagic`] (see +//! [`parse_line_debug_block`]) rather than an ad hoc `if tag == "LINE"`, +//! specifically so a future `=APS`/`=GCC`/untagged-stabs decoder has an +//! obvious arm to add rather than a buried comparison to unpick. +//! +//! Decoding a hunk's captured `HUNK_DEBUG` blocks into a queryable +//! [`HunkLineInfo`] is lazy -- see [`Hunk::line_info`]'s doc for why -- +//! and the offset-to-line lookup within one is a greatest-offset-<=-target +//! binary search, not an exact match, because the pairs are sparse (one +//! per source line, not per instruction). See [`HunkLineInfo::lookup`] +//! and [`LoadResult::lookup_line`] (which also subtracts a hunk's load +//! address to turn a guest PC into the hunk-relative offset this all +//! operates on). +//! +//! **The pairs aren't guaranteed to be offset-monotonic, or even in +//! line-number order.** A real `PhxAss` `LINEDEBUG` build +//! (`fixtures/linetest`, a repo-owned fixture built specifically to +//! exercise this) emits a data-hunk `LINE` block whose pairs, verbatim +//! in file order, are `(33, 0x0e), (36, 0x00), (37, 0x15)` -- offset +//! *decreasing* from the first pair to the second. Per PhxAss's own +//! author this is because line 33 is the `section data,data` directive +//! itself, and the offset it records lands inside the following +//! message string rather than at a clean boundary; nothing about *why* +//! needs to be understood here, only that it happens on a real, +//! unmodified assembler build, not just a theoretical malformed-input +//! case. [`Hunk::line_info`] always (re-)sorts by offset rather than +//! trusting file order for exactly this reason -- see +//! `real_linetest_fixture_matches_known_pairs` and +//! `non_monotonic_pairs_are_sorted_before_lookup` in this module's +//! tests. A lookup at an offset that several out-of-order-by-line-number +//! entries could plausibly "claim" (e.g. `0x10` for the pairs above) +//! resolves purely by offset, per the documented "greatest offset `<=` +//! target" rule -- never by line number and never by which pair +//! happened to appear first or last in the file. +//! +//! # Symbol attribution (`HUNK_SYMBOL`, issue #74 follow-up) +//! +//! `m68k-amigaos-gcc` builds carry no `LINE` data this loader can decode +//! (see above), but they do carry real `HUNK_SYMBOL` data -- a real +//! local `-g -O0 -noixemul` build has 47/15/19 named symbols across its +//! code/data/bss hunks, e.g. `_free` at `0x226e`. That's real, useful +//! attribution (`_free+0x5`) for a large share of the binaries this +//! loader otherwise has nothing but a bare hex address for. +//! +//! [`Hunk::symbol_table`] decodes a hunk's captured +//! [`Hunk::symbol_blocks`] into a [`SymbolTable`], lazily, the same +//! rationale as `HunkLineInfo`. [`SymbolTable::lookup`] is the same +//! greatest-offset-<=-target search as `LINE`'s (a symbol's value marks +//! where a routine *starts*, not every address inside it) -- but with +//! one addition: the span attributed to a table's *last* symbol is +//! capped at the owning hunk's own size, so a small symbol table can't +//! "match" an offset arbitrarily far past its final entry (a 3-symbol +//! table matching 40 KB past the last one and reporting a nonsense delta +//! was the concrete failure mode that prompted this cap). See +//! [`SymbolTable::lookup`]'s doc for the full reasoning. +//! +//! [`Hunk::locate`] and [`LoadResult::lookup_location`] combine the two +//! into a single [`Location`] lookup with a fixed precedence -- `LINE` +//! (`file:line`) first, `HUNK_SYMBOL` (`symbol+offset`) as a fallback, +//! nothing if neither has coverage -- so callers annotating diagnostics +//! have one entry point rather than reimplementing that ordering (or +//! calling two separate lookups) at every site. //! //! # Overlay files (`HUNK_OVERLAY` / `HUNK_BREAK`) //! @@ -249,6 +356,429 @@ pub struct Hunk { pub reserved_size: usize, /// 32-bit relocations that apply within this hunk. pub relocs: Vec, + /// Raw payload bytes of every `HUNK_DEBUG` block that appeared for + /// this hunk (in file order; a hunk can have more than one -- see + /// the module docs), captured verbatim but *not* interpreted -- + /// see [`Hunk::line_info`], which decodes them lazily on demand. + pub debug_blocks: Vec>, + /// Raw bytes of every `HUNK_SYMBOL` block that appeared for this + /// hunk (in file order; there can be more than one, same as + /// `debug_blocks`), captured verbatim but *not* decoded into names + /// -- see [`Hunk::symbol_table`], which does that lazily on demand. + /// This is the `symbol+offset` fallback for binaries with no usable + /// `LINE` data (most `m68k-amigaos-gcc` output -- see the module + /// docs), added in issue #74's follow-up. + pub symbol_blocks: Vec>, +} + +impl Hunk { + /// Decodes this hunk's captured [`Hunk::debug_blocks`] into + /// source-line info, on demand. + /// + /// This is deliberately not done during [`parse`]: debug info can + /// dominate a small file -- the real `LawBreaker` fixture used to + /// develop this feature (issue #74) is 776 bytes total, 456 of + /// which is debug data -- and the common case (no diagnostic ever + /// needs to be annotated) shouldn't pay to decode line tables it + /// will never query. `parse`/`load` always capture the raw block + /// bytes (cheap: a bounds-checked copy, no interpretation -- see + /// [`read_debug_block_payload`]); this method is where the actual + /// per-block parsing happens. A caller that wants file:line for many + /// diagnostics against the same binary should call this once per + /// hunk and cache the resulting [`HunkLineInfo`], rather than + /// re-decoding on every lookup (see [`LoadResult::lookup_line`], + /// which does exactly that re-decoding and documents the tradeoff). + pub fn line_info(&self) -> HunkLineInfo { + let mut entries: Vec<(String, LineEntry)> = Vec::new(); + for block in &self.debug_blocks { + if let Some((filename, block_entries)) = parse_line_debug_block(block) { + entries.extend(block_entries.into_iter().map(|e| (filename.clone(), e))); + } + } + // Always (re-)sort rather than trust file order: verified sorted + // in both real producers checked for issue #74, but nothing in + // the format guarantees it, and merging more than one LINE block + // (see HunkLineInfo's docs) needs a sort across blocks regardless. + entries.sort_by_key(|(_, e)| e.offset); + HunkLineInfo { entries } + } + + /// Decodes this hunk's captured [`Hunk::symbol_blocks`] into a + /// queryable [`SymbolTable`], on demand -- same lazy-decode + /// rationale as [`Hunk::line_info`] (see its doc): `parse`/`load` + /// always capture the raw bytes cheaply (see + /// [`read_symbol_block_raw`]), and turning them into `String`s and + /// sorting by value only happens when a caller actually asks. + pub fn symbol_table(&self) -> SymbolTable { + let mut entries = Vec::new(); + for block in &self.symbol_blocks { + entries.extend(parse_symbol_block(block)); + } + entries.sort_by_key(|(_, value)| *value); + SymbolTable { entries } + } + + /// Best available [`Location`] for a hunk-relative `offset`: tries + /// [`Hunk::line_info`] first (`file:line` is always more precise + /// than a symbol when it's available), falls back to + /// [`Hunk::symbol_table`] (`symbol+offset`, bounded -- see + /// [`SymbolTable::lookup`]), and returns `None` if neither has + /// anything to say, in which case the caller's existing "print the + /// raw address" fallback applies. This is the one place the + /// LINE-then-symbol precedence lives, per issue #74's coordinator + /// follow-up, specifically so callers annotating diagnostics don't + /// each reimplement the ordering (or call two lookups) themselves. + pub fn locate(&self, offset: u32) -> Option { + if let Some((file, line)) = self.line_info().lookup(offset) { + return Some(Location::Line { + file: file.to_string(), + line, + }); + } + let table = self.symbol_table(); + let (name, delta) = table.lookup(offset, self.reserved_size as u32)?; + Some(Location::Symbol { + name: name.to_string(), + offset: delta, + }) + } +} + +/// Best available source attribution for a hunk-relative offset (or, +/// via [`LoadResult::lookup_location`], a guest address): which of +/// `LINE` debug info or `HUNK_SYMBOL` data -- if either -- covers it. +/// See [`Hunk::locate`] for the precedence between the two variants. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Location { + /// A `LINE` block's entry covers this offset exactly (greatest + /// offset `<=` target -- see [`HunkLineInfo::lookup`]). + Line { file: String, line: u32 }, + /// No `LINE` coverage, but a preceding `HUNK_SYMBOL` entry does, + /// within [`SymbolTable::lookup`]'s bounded span. `offset` here is + /// the delta from the symbol's own value, e.g. `Do_Law+0x12`. + Symbol { name: String, offset: u32 }, +} + +/// A `HUNK_DEBUG` block's 4-byte payload magic (the four bytes right +/// after the leading base-offset longword -- see the module docs), +/// dispatched on explicitly so adding a decoder for a currently-ignored +/// family (`=APS`/`=GCC` stabs, or gcc's untagged stabs, per the +/// coordinator's finding on issue #74) is a matter of adding a match arm +/// here, not unpicking an `if` buried in a parse function. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum DebugMagic { + /// Source-line table: filename + `(line, offset)` pairs. The only + /// family this loader decodes -- see [`parse_line_debug_block`]. + Line, + /// SAS/C 6.58 compiler-options block, emitted alongside `LINE`. + /// Recognized so it doesn't fall through as "unknown", but its + /// payload isn't interpreted. + Opts, + /// SAS/C 6.58 source-file-list block, emitted alongside `LINE`. + /// Recognized but not interpreted, same as `Opts`. + Src6, + /// The first four bytes of a `HEADDBGV01` debug-block directory some + /// assemblers emit (the LawBreaker binary's producer among them), + /// indexing the file offsets of the `HUNK_DEBUG` blocks already + /// present in the hunk stream. Redundant with those blocks, so not + /// interpreted. + Head, + /// Anything else, including no recognizable magic at all -- e.g. + /// `m68k-amigaos-gcc`'s stabs-format `HUNK_DEBUG` payload, which (per + /// a reference decode of the format, cross-checked against a real + /// local gcc build) carries no tag in this position at all. Not + /// decoded by this loader; see the module docs' "gcc is not covered" + /// note. + Unrecognized, +} + +impl DebugMagic { + /// Classifies a 4-byte magic slice (already bounds-checked by the + /// caller). Unknown bytes -- including gcc's untagged stabs, which + /// simply don't spell any of the known magics -- classify as + /// [`DebugMagic::Unrecognized`] rather than erroring: this loader + /// only ever *skips* what it doesn't understand here. + fn classify(magic: &[u8]) -> DebugMagic { + match magic { + b"LINE" => DebugMagic::Line, + b"OPTS" => DebugMagic::Opts, + b"SRC6" => DebugMagic::Src6, + b"HEAD" => DebugMagic::Head, + _ => DebugMagic::Unrecognized, + } + } +} + +/// One `(source line number, hunk-relative byte offset)` pair decoded +/// from a `LINE` debug block. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct LineEntry { + pub offset: u32, + pub line: u32, +} + +/// Source-line info for one hunk, decoded on demand by [`Hunk::line_info`] +/// from that hunk's captured `HUNK_DEBUG` blocks. See the module docs +/// and issue #74 for the on-disk format (verified against a real +/// assembler's output for `LawBreaker` and SAS/C 6.58's `DEBUG=LINE`; +/// **not** produced for `m68k-amigaos-gcc` builds -- see the module +/// docs' "gcc is not covered" note). +/// +/// Flattens every recognized `LINE` block into one offset-sorted table, +/// since a hunk can carry more than one -- e.g. one per `#include`d +/// source file. +/// +/// # Overlapping offsets +/// If two `LINE` blocks both record an entry at the exact same +/// hunk-relative offset, the one from the block that was captured +/// *later* (i.e. appears later in the hunk's `HUNK_DEBUG` sequence) +/// wins: the sort in [`Hunk::line_info`] is stable, so of two +/// equal-offset entries the later one sorts second, and +/// [`HunkLineInfo::lookup`]'s "greatest offset <= target" search returns +/// the last of any tied group. This is an arbitrary but deterministic +/// tie-break -- the format doesn't specify what overlapping blocks are +/// supposed to mean. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct HunkLineInfo { + /// `(source filename, entry)`, sorted ascending by `entry.offset`. + entries: Vec<(String, LineEntry)>, +} + +impl HunkLineInfo { + /// Looks up the source location for a hunk-relative byte `offset`: + /// the entry with the greatest recorded offset that is `<= offset`. + /// The pairs are sparse (one per source line, not per instruction), + /// so an exact match usually doesn't exist -- this is why the search + /// can't be a plain equality lookup. Returns `None` if `offset` + /// precedes every recorded entry, or if there's no line info at all. + pub fn lookup(&self, offset: u32) -> Option<(&str, u32)> { + let idx = self.entries.partition_point(|(_, e)| e.offset <= offset); + if idx == 0 { + return None; + } + let (filename, entry) = &self.entries[idx - 1]; + Some((filename.as_str(), entry.line)) + } + + /// True if no `LINE` blocks (recognized or otherwise) contributed + /// any entries. + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } +} + +/// Reads a big-endian `u32` from `bytes` at `pos`, or `None` if that +/// would run past the end -- the bounds-checked primitive +/// [`parse_line_debug_block`] builds on, since a `HUNK_DEBUG` payload is +/// untrusted file data that must never be indexed unchecked. +fn read_u32_be(bytes: &[u8], pos: usize) -> Option { + let end = pos.checked_add(4)?; + let slice = bytes.get(pos..end)?; + Some(u32::from_be_bytes([slice[0], slice[1], slice[2], slice[3]])) +} + +/// Trims trailing NUL padding (the on-disk convention shared by `LINE` +/// filenames and `HUNK_SYMBOL` names alike) and decodes what's left as +/// UTF-8, lossily. Amiga strings aren't guaranteed valid UTF-8 (the +/// native charset isn't UTF-8), so this decodes rather than rejects the +/// whole entry over a handful of unusual bytes -- a garbled-but-present +/// name is still more useful than none, and lossy decoding can't turn a +/// *correct* entry into a *wrong* one (only a cosmetically imperfect +/// name), which is the property that matters for untrusted file input. +fn decode_nul_padded_lossy(raw: &[u8]) -> String { + let trimmed = match raw.iter().rposition(|&b| b != 0) { + Some(last) => &raw[..=last], + None => &raw[..0], + }; + String::from_utf8_lossy(trimmed).into_owned() +} + +/// Decodes one `HUNK_DEBUG` block's raw payload (as captured into +/// [`Hunk::debug_blocks`]) if -- and only if -- [`DebugMagic::classify`] +/// says it's `LINE`; any other magic (recognized-but-uninterpreted, or +/// genuinely unrecognized -- including a payload too short to even carry +/// one) returns `None` and contributes nothing, per issue #74's +/// "dispatch on the magic, skip anything unrecognized". `LINE` payloads +/// that are merely *malformed* (truncated, an absurd filename length, a +/// dangling trailing pair) also degrade to `None` or a truncated entry +/// list rather than erroring -- see [`HunkLineInfo`]'s and +/// [`Hunk::line_info`]'s docs: this is untrusted file input, and +/// reporting nothing is always safer than reporting the wrong line. +/// +/// Payload layout (big-endian throughout, offsets relative to the start +/// of the payload, i.e. right after the block's own length longword): +/// ```text +/// 0 u32 base offset within the hunk (added to every pair's offset) +/// 4 4 magic, e.g. "LINE" +/// 8 u32 source filename length, in LONGWORDS (LINE blocks only) +/// 12 N*4 filename, NUL-padded to that longword count +/// .. then (line number: u32, hunk offset: u32) pairs to the end +/// ``` +fn parse_line_debug_block(payload: &[u8]) -> Option<(String, Vec)> { + let base_offset = read_u32_be(payload, 0)?; + let magic = payload.get(4..8)?; + if DebugMagic::classify(magic) != DebugMagic::Line { + return None; + } + + let name_longwords = read_u32_be(payload, 8)? as usize; + let name_bytes = name_longwords.checked_mul(4)?; + let name_start = 12usize; + let name_end = name_start.checked_add(name_bytes)?; + let raw_name = payload.get(name_start..name_end)?; + let filename = decode_nul_padded_lossy(raw_name); + + let mut entries = Vec::new(); + let mut pos = name_end; + while let Some(entry) = read_line_entry_pair(payload, pos, base_offset) { + entries.push(entry); + pos += 8; + } + + Some((filename, entries)) +} + +/// Reads one `(line number, hunk offset)` pair at payload byte `pos` +/// (`line` at `pos`, `offset` at `pos + 4`), or `None` if either half +/// runs past the end of `payload` -- covers both a fully truncated pair +/// and a dangling partial one (a line number present with no offset to +/// follow it), which [`parse_line_debug_block`]'s loop both treat the +/// same way: stop, don't error, keep whatever full pairs were already +/// found. `offset` is `base_offset`-adjusted here (wrapping, so a +/// pathological base offset can't panic) since every caller wants the +/// final hunk-relative offset, never the raw on-disk value. +fn read_line_entry_pair(payload: &[u8], pos: usize, base_offset: u32) -> Option { + let line = read_u32_be(payload, pos)?; + let entry_offset = read_u32_be(payload, pos.checked_add(4)?)?; + Some(LineEntry { + offset: base_offset.wrapping_add(entry_offset), + line, + }) +} + +/// Decoded `HUNK_SYMBOL` data for one hunk: `(name, hunk-relative +/// value)` pairs, sorted ascending by value. Built by +/// [`Hunk::symbol_table`] from that hunk's captured +/// [`Hunk::symbol_blocks`]; see that method's doc for why decoding is +/// lazy. Added in issue #74's follow-up, as a `symbol+offset` fallback +/// for binaries (mostly `m68k-amigaos-gcc` output) that carry no usable +/// `LINE` data -- see [`Hunk::locate`] for how the two combine. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct SymbolTable { + entries: Vec<(String, u32)>, +} + +impl SymbolTable { + /// Looks up the symbol whose value is the greatest one `<= offset` + /// -- the same "greatest offset `<=` target" search as + /// [`HunkLineInfo::lookup`], and for the same underlying reason: a + /// `HUNK_SYMBOL` value marks where a routine *starts*, not every + /// address inside it. Returns `(name, offset - symbol_value)` on a + /// hit. + /// + /// `hunk_size` bounds the span attributed to the table's *last* + /// symbol. Every earlier symbol's span is already implicitly bounded + /// by the next symbol's value (the binary search below guarantees + /// `offset` is `<` it whenever a later entry exists) -- but nothing + /// bounds the final entry without `hunk_size`, and an otherwise-tiny + /// symbol table would happily "match" an offset arbitrarily far past + /// its last entry, e.g. a 3-symbol table matching 40 KB past the + /// last one and reporting a delta that's not remotely useful (the + /// concrete case that prompted this cap, from issue #74's + /// coordinator follow-up). Passing the owning hunk's own + /// `Hunk::reserved_size` here (as [`Hunk::locate`] does) makes the + /// cap exactly "the rest of this hunk" rather than an arbitrary + /// constant -- there's nothing past a hunk's own end to attribute to + /// anything. + /// + /// Returns `None` when the match would fall at or past that bound, + /// when `offset` precedes every symbol, or when there are no symbols + /// at all. + pub fn lookup(&self, offset: u32, hunk_size: u32) -> Option<(&str, u32)> { + let idx = self.entries.partition_point(|(_, value)| *value <= offset); + if idx == 0 { + return None; + } + let (name, value) = &self.entries[idx - 1]; + let bound = self.entries.get(idx).map_or(hunk_size, |(_, v)| *v); + if offset >= bound { + return None; + } + Some((name.as_str(), offset - value)) + } + + /// True if no symbols were captured/decoded. + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } +} + +/// Reads one `HUNK_SYMBOL` block's raw bytes, given that the +/// `HUNK_SYMBOL` type word has already been consumed. Walks the +/// `{ name_length_longwords, name, value }` list (terminated by a zero +/// name length) purely to find where the block ends -- the same shape +/// [`skip_metadata_block`] used to walk for every metadata block type +/// before `HUNK_DEBUG`/`HUNK_SYMBOL` grew their own capturing paths -- +/// then returns the exact bytes spanned (terminator included) verbatim. +/// Captured, not decoded: the same "read now during `parse`, interpret +/// later on demand" split [`read_debug_block_payload`] uses for +/// `HUNK_DEBUG`. [`parse_symbol_block`] (via [`Hunk::symbol_table`]) is +/// what actually turns this into `(name, value)` pairs. +fn read_symbol_block_raw(r: &mut Reader<'_>) -> Result, LoadError> { + let start = r.pos; + loop { + let name_longwords = r.read_u32()?; + if name_longwords == 0 { + break; + } + r.skip_longwords(name_longwords as usize)?; // symbol name + r.read_u32()?; // symbol value (hunk-relative offset) + } + Ok(r.bytes[start..r.pos].to_vec()) +} + +/// Decodes a raw `HUNK_SYMBOL` block's bytes (as captured by +/// [`read_symbol_block_raw`] into [`Hunk::symbol_blocks`]) into `(name, +/// value)` pairs: repeating `{ name_length_longwords: u32, name: N*4 +/// bytes NUL-padded, value: u32 }` entries, terminated by a zero name +/// length. +/// +/// Degrades to however many entries parsed cleanly before hitting +/// something malformed, rather than panicking -- consistent with +/// [`parse_line_debug_block`]'s philosophy, even though in practice a +/// block captured straight out of a successful [`parse`] call is always +/// well-formed (the capture walk in [`read_symbol_block_raw`] already +/// validated it): this function doesn't lean on that guarantee, since a +/// [`Hunk`] can also be constructed directly with arbitrary bytes in +/// `symbol_blocks` (tests do exactly this). +fn parse_symbol_block(bytes: &[u8]) -> Vec<(String, u32)> { + let mut entries = Vec::new(); + let mut pos = 0usize; + while let Some((entry, next_pos)) = read_symbol_entry(bytes, pos) { + entries.push(entry); + pos = next_pos; + } + entries +} + +/// Reads one `{ name_length_longwords, name, value }` symbol entry at +/// payload byte `pos`, returning the decoded `(name, value)` pair and +/// the byte position right after it. Returns `None` both for a proper +/// terminator (a zero name length -- not malformed, just "no more +/// entries") and for anything that runs past the end of `bytes` -- +/// [`parse_symbol_block`]'s loop treats both the same way: stop, don't +/// error, keep whatever entries were already decoded. +fn read_symbol_entry(bytes: &[u8], pos: usize) -> Option<((String, u32), usize)> { + let name_longwords = read_u32_be(bytes, pos)?; + if name_longwords == 0 { + return None; // proper terminator + } + let name_byte_len = (name_longwords as usize).checked_mul(4)?; + let name_start = pos.checked_add(4)?; + let name_end = name_start.checked_add(name_byte_len)?; + let raw_name = bytes.get(name_start..name_end)?; + let value = read_u32_be(bytes, name_end)?; + Some(((decode_nul_padded_lossy(raw_name), value), name_end + 4)) } /// The `HUNK_OVERLAY` block's payload, verbatim -- exactly the longwords @@ -323,6 +853,75 @@ pub struct LoadResult { pub end: u32, } +impl LoadResult { + /// Translates a guest address into `(source filename, line number)`, + /// using `file`'s per-hunk line info (see [`Hunk::line_info`]) and + /// this result's per-hunk load addresses: finds which hunk `addr` + /// falls inside, converts to a hunk-relative offset by subtracting + /// that hunk's load address, and looks the offset up with + /// [`HunkLineInfo::lookup`]'s greatest-offset-<=-target search. + /// + /// `file` must be the [`HunkFile`] this [`LoadResult`] was produced + /// from by [`load`] -- `load` borrows rather than owns it, so the + /// caller already has it on hand; mismatched hunk counts between + /// `self` and `file` just make this return `None` rather than panic. + /// Returns `None` if `addr` isn't inside any loaded hunk, or the + /// owning hunk has no line info covering it (no debug data, or the + /// address precedes every recorded entry). + /// + /// This decodes the owning hunk's debug blocks fresh on every call + /// (see [`Hunk::line_info`]'s doc on why that's lazy rather than + /// precomputed); a caller doing this repeatedly for the same binary + /// should cache per-hunk [`HunkLineInfo`] itself instead of calling + /// this in a hot loop. + pub fn lookup_line(&self, file: &HunkFile, addr: u32) -> Option<(String, u32)> { + for (hunk, &hunk_addr) in file.hunks.iter().zip(&self.hunk_addrs) { + if addr < hunk_addr { + continue; + } + let offset = addr - hunk_addr; + if offset as usize >= hunk.reserved_size { + continue; + } + let info = hunk.line_info(); + return info + .lookup(offset) + .map(|(filename, line)| (filename.to_string(), line)); + } + None + } + + /// Best available [`Location`] for a guest address: `file:line` when + /// covered by `LINE` debug info, else `symbol+offset` when covered + /// by `HUNK_SYMBOL` data, else `None` (the caller's existing "print + /// the raw address" fallback covers that case). This is + /// [`lookup_line`](Self::lookup_line) generalized with the + /// `HUNK_SYMBOL` fallback added in issue #74's follow-up -- keep + /// using `lookup_line` directly at a site that only ever wants + /// `file:line` specifically (e.g. something that formats source + /// listings); use this one wherever "best available attribution" + /// is what's wanted, which is every diagnostic site. + /// + /// Finds which hunk `addr` falls inside the same way `lookup_line` + /// does, then delegates the LINE-vs-symbol precedence to + /// [`Hunk::locate`] -- see that method's doc. Same laziness caveat + /// as `lookup_line`: this decodes the owning hunk's debug/symbol + /// blocks fresh on every call. + pub fn lookup_location(&self, file: &HunkFile, addr: u32) -> Option { + for (hunk, &hunk_addr) in file.hunks.iter().zip(&self.hunk_addrs) { + if addr < hunk_addr { + continue; + } + let offset = addr - hunk_addr; + if offset as usize >= hunk.reserved_size { + continue; + } + return hunk.locate(offset); + } + None + } +} + /// A tiny cursor over a byte slice that reads big-endian 32-bit words and /// turns short reads into [`LoadError::UnexpectedEof`] instead of /// panicking. @@ -409,43 +1008,50 @@ impl<'a> Reader<'a> { /// traditional position) after one's relocations, and in any number back /// to back. See the module docs and [`skip_metadata_block`], which does /// the actual parsing. +/// +/// Neither `HUNK_DEBUG` nor `HUNK_SYMBOL` is included here even though +/// both can appear in the same positions: unlike `HUNK_NAME`, neither is +/// discarded -- both are captured (see [`read_debug_block_payload`] and +/// [`read_symbol_block_raw`], and the module docs' "Source-line info" +/// section), so [`parse_node`] checks for them explicitly, ahead of this +/// check, rather than folding them into the generic skip path. fn is_metadata_block(block_type: u32) -> bool { - matches!(block_type, HUNK_NAME | HUNK_SYMBOL | HUNK_DEBUG) + block_type == HUNK_NAME } -/// Skips one metadata block's payload, given that its type word (one of -/// `HUNK_NAME`/`HUNK_SYMBOL`/`HUNK_DEBUG` -- see [`is_metadata_block`]) -/// has already been consumed. Shared by the leading-position skip (in -/// [`parse_node`]'s per-hunk loop, before the body-type match) and the -/// trailing-position skip (in the same function's post-body block loop), -/// so the two positions can't drift apart on what a metadata block looks -/// like. -/// -/// `HUNK_NAME` and `HUNK_DEBUG` share the simple shape: a longword count -/// `n`, then `n * 4` bytes of payload we don't interpret. `HUNK_SYMBOL` -/// is *not* that shape -- it's a list of `{ name_length_longwords, name, -/// value }` entries terminated by a zero `name_length`, and mistakenly -/// treating it as a single count-prefixed block misparses the rest of -/// the file (confirmed the hard way against a real binary). +/// Skips a `HUNK_NAME` block's payload, given that its type word has +/// already been consumed: a longword count `n`, then `n * 4` bytes of +/// payload we don't interpret (the same on-disk shape `HUNK_DEBUG` uses +/// -- see [`read_debug_block_payload`], which shares that much but +/// returns the bytes instead of discarding them). Shared by the +/// leading-position skip (in [`parse_node`]'s per-hunk loop, before the +/// body-type match) and the trailing-position skip (in the same +/// function's post-body block loop), so the two positions can't drift +/// apart on what a `HUNK_NAME` block looks like. fn skip_metadata_block(r: &mut Reader<'_>, block_type: u32) -> Result<(), LoadError> { match block_type { - HUNK_NAME | HUNK_DEBUG => { + HUNK_NAME => { let n_longwords = r.read_u32()?; r.skip_longwords(n_longwords as usize)?; } - HUNK_SYMBOL => loop { - let name_longwords = r.read_u32()?; - if name_longwords == 0 { - break; - } - r.skip_longwords(name_longwords as usize)?; // symbol name - r.read_u32()?; // symbol value (offset within hunk) - }, other => unreachable!("skip_metadata_block called with non-metadata block type {other:#x}"), } Ok(()) } +/// Reads one `HUNK_DEBUG` block's raw payload and returns it verbatim, +/// given that the `HUNK_DEBUG` type word has already been consumed. Same +/// on-disk shape as `HUNK_NAME` (a longword count `n`, then `n * 4` +/// payload bytes -- see [`skip_metadata_block`]) and the same +/// `checked_mul`/`checked_add` bounds-checking precedent as +/// [`Reader::skip_longwords`], but the bytes are kept rather than +/// discarded: they're what [`Hunk::line_info`] decodes lazily later. +fn read_debug_block_payload(r: &mut Reader<'_>) -> Result, LoadError> { + let n_longwords = r.read_u32()? as usize; + let n_bytes = n_longwords.checked_mul(4).ok_or(LoadError::UnexpectedEof)?; + r.read_bytes(n_bytes) +} + /// A parsed `HUNK_HEADER`'s own fields, returned alongside the hunks /// [`parse_node`] reads for its declared range. struct HeaderInfo { @@ -515,15 +1121,25 @@ fn parse_node(r: &mut Reader<'_>) -> Result<(HeaderInfo, Vec), LoadError> let mut hunks = Vec::with_capacity(n_sizes); for (i, &reserved_size) in declared_sizes.iter().enumerate() { let hunk_index = first_hunk + i; - // Skip any number of leading metadata blocks (HUNK_NAME/ - // HUNK_SYMBOL/HUNK_DEBUG) before the hunk's real body -- a real - // assembler build with source-line debug info left in can open a - // hunk with one or more HUNK_DEBUG blocks ahead of its HUNK_CODE - // (see the module docs). Memory-flag bits never apply to these - // metadata type words (only to CODE/DATA/BSS), so check the raw - // word directly. + // Skip (HUNK_NAME) or capture (HUNK_DEBUG/HUNK_SYMBOL) any number + // of leading metadata blocks before the hunk's real body -- a + // real assembler build with source-line debug info left in can + // open a hunk with one or more HUNK_DEBUG blocks ahead of its + // HUNK_CODE (see the module docs). Memory-flag bits never apply + // to these metadata type words (only to CODE/DATA/BSS), so check + // the raw word directly. + let mut debug_blocks: Vec> = Vec::new(); + let mut symbol_blocks: Vec> = Vec::new(); let raw_body_type = loop { let candidate = r.read_u32()?; + if candidate == HUNK_DEBUG { + debug_blocks.push(read_debug_block_payload(r)?); + continue; + } + if candidate == HUNK_SYMBOL { + symbol_blocks.push(read_symbol_block_raw(r)?); + continue; + } if is_metadata_block(candidate) { skip_metadata_block(r, candidate)?; continue; @@ -622,7 +1238,13 @@ fn parse_node(r: &mut Reader<'_>) -> Result<(HeaderInfo, Vec), LoadError> } r.align_to_longword(); } - HUNK_NAME | HUNK_SYMBOL | HUNK_DEBUG => { + HUNK_DEBUG => { + debug_blocks.push(read_debug_block_payload(r)?); + } + HUNK_SYMBOL => { + symbol_blocks.push(read_symbol_block_raw(r)?); + } + HUNK_NAME => { skip_metadata_block(r, block_type)?; } HUNK_END => break, @@ -669,6 +1291,8 @@ fn parse_node(r: &mut Reader<'_>) -> Result<(HeaderInfo, Vec), LoadError> data, reserved_size, relocs, + debug_blocks, + symbol_blocks, }); } @@ -1663,4 +2287,779 @@ mod tests { } ); } + + // --- HUNK_DEBUG / LINE source-line info (issue #74) --- + + /// A `Hunk` with the given raw `HUNK_DEBUG` payloads and nothing + /// else -- for tests that exercise [`Hunk::line_info`] directly + /// without going through a full file parse. + fn make_hunk(debug_blocks: Vec>) -> Hunk { + make_hunk_full(debug_blocks, Vec::new(), 0x1000) + } + + /// Same as `make_hunk` but with `HUNK_SYMBOL` blocks too, for tests + /// that exercise [`Hunk::symbol_table`]/[`Hunk::locate`]. + fn make_hunk_with_symbols(symbol_blocks: Vec>) -> Hunk { + make_hunk_full(Vec::new(), symbol_blocks, 0x1000) + } + + /// Fully-parameterized `Hunk` builder for tests, including + /// `reserved_size` -- needed by tests that exercise + /// [`SymbolTable::lookup`]'s hunk-end distance cap, which only bites + /// when the hunk isn't the default helper's generous 0x1000 bytes. + fn make_hunk_full( + debug_blocks: Vec>, + symbol_blocks: Vec>, + reserved_size: usize, + ) -> Hunk { + Hunk { + kind: HunkKind::Code, + data: Vec::new(), + reserved_size, + relocs: Vec::new(), + debug_blocks, + symbol_blocks, + } + } + + /// Builds a `LINE`-format `HUNK_DEBUG` *payload* (the bytes that end + /// up in `Hunk::debug_blocks`, i.e. after the block's own type and + /// length longwords): base offset, `"LINE"` magic, longword-counted + /// NUL-padded filename, then `(line, offset)` pairs -- the layout + /// documented in issue #74 and verified against LawBreaker/SAS/C. + fn build_line_payload(base_offset: u32, filename: &str, pairs: &[(u32, u32)]) -> Vec { + let mut buf = Vec::new(); + push_u32(&mut buf, base_offset); + buf.extend_from_slice(b"LINE"); + let mut name_bytes = filename.as_bytes().to_vec(); + while !name_bytes.len().is_multiple_of(4) { + name_bytes.push(0); + } + push_u32(&mut buf, (name_bytes.len() / 4) as u32); + buf.extend_from_slice(&name_bytes); + for &(line, offset) in pairs { + push_u32(&mut buf, line); + push_u32(&mut buf, offset); + } + buf + } + + /// Appends a full on-disk `HUNK_DEBUG` block (type longword + length + /// longword + payload) to `buf`, for tests that exercise the full + /// [`parse`] path rather than constructing a [`Hunk`] directly. + fn push_debug_block_raw(buf: &mut Vec, payload: &[u8]) { + assert_eq!( + payload.len() % 4, + 0, + "test helper requires a longword-aligned debug payload" + ); + push_u32(buf, HUNK_DEBUG); + push_u32(buf, (payload.len() / 4) as u32); + buf.extend_from_slice(payload); + } + + #[test] + fn line_debug_block_parses_into_line_info() { + // The LawBreaker pairs from issue #74, verified against real + // instruction boundaries in that binary. + let payload = build_line_payload( + 0, + "LawBreaker.asm", + &[ + (133, 0x0000), + (134, 0x0004), + (135, 0x0006), + (136, 0x000a), + (137, 0x000e), + (141, 0x0010), + ], + ); + let hunk = make_hunk(vec![payload]); + let info = hunk.line_info(); + assert!(!info.is_empty()); + assert_eq!(info.lookup(0x0000), Some(("LawBreaker.asm", 133))); + assert_eq!(info.lookup(0x0004), Some(("LawBreaker.asm", 134))); + assert_eq!(info.lookup(0x000a), Some(("LawBreaker.asm", 136))); + assert_eq!(info.lookup(0x0010), Some(("LawBreaker.asm", 141))); + } + + /// The pairs are sparse (one per source line, not per instruction), + /// so a lookup for an offset strictly between two recorded offsets + /// must return the *preceding* entry, not `None` and not the next + /// one -- an exact-match lookup would report nothing for most + /// addresses, which is the whole reason this is a "greatest offset + /// <= target" search. + #[test] + fn lookup_returns_preceding_entry_for_offset_between_pairs() { + let payload = build_line_payload(0, "x.asm", &[(10, 0x00), (20, 0x10), (30, 0x20)]); + let info = make_hunk(vec![payload]).line_info(); + + assert_eq!(info.lookup(0x08), Some(("x.asm", 10))); + assert_eq!(info.lookup(0x0f), Some(("x.asm", 10))); + assert_eq!(info.lookup(0x1f), Some(("x.asm", 20))); + } + + #[test] + fn lookup_before_first_entry_is_none() { + let payload = build_line_payload(0, "x.asm", &[(10, 0x10), (20, 0x20)]); + let info = make_hunk(vec![payload]).line_info(); + + assert_eq!(info.lookup(0x00), None); + assert_eq!(info.lookup(0x0f), None); + } + + #[test] + fn lookup_past_last_entry_returns_the_last_entry() { + let payload = build_line_payload(0, "x.asm", &[(10, 0x00), (20, 0x10)]); + let info = make_hunk(vec![payload]).line_info(); + + assert_eq!(info.lookup(0x10), Some(("x.asm", 20))); + assert_eq!(info.lookup(0xFFFF_FFFF), Some(("x.asm", 20))); + } + + #[test] + fn empty_line_info_has_no_entries_and_no_lookups() { + let info = make_hunk(Vec::new()).line_info(); + assert!(info.is_empty()); + assert_eq!(info.lookup(0), None); + } + + /// `OPTS`/`SRC6` (SAS/C 6.58) and a `HEAD`-magic block (the leading + /// four bytes of a `HEADDBGV01` directory) must be recognized as + /// *not* `LINE` and contribute nothing, without disturbing the + /// parse of a real `LINE` block alongside them -- both the leading + /// and trailing `HUNK_DEBUG` positions are exercised. + #[test] + fn skips_unrecognized_debug_magics_without_disturbing_line_info() { + let mut opts_payload = Vec::new(); + push_u32(&mut opts_payload, 0); + opts_payload.extend_from_slice(b"OPTS"); + push_u32(&mut opts_payload, 0xAAAA_AAAA); + + let mut src6_payload = Vec::new(); + push_u32(&mut src6_payload, 0); + src6_payload.extend_from_slice(b"SRC6"); + + let mut head_payload = Vec::new(); + push_u32(&mut head_payload, 0); + head_payload.extend_from_slice(b"HEAD"); + push_u32(&mut head_payload, 0xBBBB_BBBB); + + let line_payload = build_line_payload(0, "hello.c", &[(2, 0x0000), (3, 0x000c)]); + + let mut buf = Vec::new(); + push_u32(&mut buf, HUNK_HEADER); + push_u32(&mut buf, 0); + push_u32(&mut buf, 1); + push_u32(&mut buf, 0); + push_u32(&mut buf, 0); + push_u32(&mut buf, 1); // hunk 0: 1 longword + + // Leading position: OPTS then HEAD. + push_debug_block_raw(&mut buf, &opts_payload); + push_debug_block_raw(&mut buf, &head_payload); + + push_u32(&mut buf, HUNK_CODE); + push_u32(&mut buf, 1); + push_u32(&mut buf, 0x4E71_4E71); + + // Trailing position: SRC6 then the real LINE block. + push_debug_block_raw(&mut buf, &src6_payload); + push_debug_block_raw(&mut buf, &line_payload); + + push_u32(&mut buf, HUNK_END); + + let file = parse(&buf).expect("OPTS/SRC6/HEAD debug blocks should be skipped, not error"); + assert_eq!(file.hunks.len(), 1); + assert_eq!(file.hunks[0].debug_blocks.len(), 4, "all four are captured"); + + let info = file.hunks[0].line_info(); + assert_eq!(info.lookup(0x0000), Some(("hello.c", 2))); + assert_eq!(info.lookup(0x000c), Some(("hello.c", 3))); + } + + /// A hunk can carry more than one `LINE` block for different source + /// files (e.g. one per `#include`d header); [`Hunk::line_info`] + /// merges them into a single lookup table. + #[test] + fn merges_two_line_blocks_with_different_filenames() { + let main_c = build_line_payload(0, "main.c", &[(5, 0x00), (6, 0x08)]); + let included_h = build_line_payload(0, "included.h", &[(1, 0x04), (2, 0x0c)]); + let info = make_hunk(vec![main_c, included_h]).line_info(); + + assert_eq!(info.lookup(0x00), Some(("main.c", 5))); + assert_eq!(info.lookup(0x04), Some(("included.h", 1))); + assert_eq!(info.lookup(0x08), Some(("main.c", 6))); + assert_eq!(info.lookup(0x0c), Some(("included.h", 2))); + } + + /// When two blocks record an entry at the *exact* same offset, the + /// later-encountered block wins (see [`HunkLineInfo`]'s doc on the + /// tie-break) -- not a panic, not an arbitrary pick. + #[test] + fn overlapping_offset_across_two_blocks_prefers_the_later_block() { + let first = build_line_payload(0, "old.c", &[(1, 0x00)]); + let second = build_line_payload(0, "new.c", &[(99, 0x00)]); + let info = make_hunk(vec![first, second]).line_info(); + + assert_eq!(info.lookup(0x00), Some(("new.c", 99))); + } + + // --- Real-world non-monotonic LINE pairs (PhxAss LINEDEBUG) --- + + /// A real PhxAss `LINEDEBUG` build (`fixtures/linetest`, see that + /// test below) emits a `LINE` block whose `(line, offset)` pairs are + /// **not** offset-monotonic and **not** in line-number order either + /// -- e.g. `(33, 0x0e), (36, 0x00), (37, 0x15)` verbatim, in that + /// file order. This is a synthetic reproduction of exactly that + /// shape, so the "always re-sort, never assume file order" property + /// documented on [`Hunk::line_info`] has a regression test that + /// doesn't depend on the fixture file: if sorting were ever dropped + /// (or replaced with an "already sorted, trust it" fast path), this + /// fails with a *wrong* line number, not a missing one -- lookups at + /// offsets between out-of-order entries would silently pick up + /// whichever pair happened to be read last rather than the one with + /// the truly greatest offset `<=` target. + #[test] + fn non_monotonic_pairs_are_sorted_before_lookup() { + let payload = build_line_payload(0, "x.asm", &[(33, 0x0e), (36, 0x00), (37, 0x15)]); + let info = make_hunk(vec![payload]).line_info(); + + // After sorting by offset: (0x00, 36), (0x0e, 33), (0x15, 37). + assert_eq!(info.lookup(0x00), Some(("x.asm", 36))); + assert_eq!(info.lookup(0x0e), Some(("x.asm", 33))); + assert_eq!(info.lookup(0x15), Some(("x.asm", 37))); + // An offset that several out-of-order-by-line-number entries + // could plausibly "claim" (it's numerically between the 0x0e + // and 0x15 entries) resolves purely by offset, per the + // documented rule -- not by line number, and not by which + // pair appeared first/last in the file. The answer is "line + // 33's code is still the most recent thing before this + // address", even though line 33 is numerically lower than the + // line (36) whose offset precedes it. + assert_eq!(info.lookup(0x10), Some(("x.asm", 33))); + } + + /// End-to-end, real-artifact confirmation of the same property, + /// against a real PhxAss-built binary committed to the repo (not a + /// scratch file or an unvendorable third-party fixture -- see + /// [`warn_real_artifact_missing`]'s doc on why the LawBreaker/SAS-C + /// tests below are corroboration only, and this one is the durable + /// coverage). `fixtures/linetest.s` is a trivial two-section (code + + /// data) program assembled with PhxAss's `LINEDEBUG` option, which + /// emits one `LINE` block per section/hunk. + /// + /// The data hunk's real pairs, verified against a raw hex dump of + /// the file (not just this parser's own output): `(33, 0x0e), (36, + /// 0x00), (37, 0x15)`. Per the assembler's own author (not + /// established further, and not something this loader needs to + /// understand): line 33 is the `section data,data` directive itself, + /// and PhxAss records its offset as `0x0e` -- inside the section's + /// message string, not at a boundary -- rather than `0x00`, which + /// line 36 gets instead. The code hunk's pairs are ordinary + /// (offset-monotonic); the data hunk's are the real counter-example + /// this loader must not assume away. + #[test] + fn real_linetest_fixture_matches_known_pairs() { + const LINETEST: &[u8] = include_bytes!("../../../fixtures/linetest"); + + let file = parse(LINETEST).expect("fixtures/linetest should be a well-formed hunk file"); + assert_eq!(file.hunks.len(), 2, "one code hunk, one data hunk"); + + let code_info = file.hunks[0].line_info(); + assert_eq!(code_info.lookup(0x00), Some(("work:linetest.s", 20))); + assert_eq!(code_info.lookup(0x06), Some(("work:linetest.s", 25))); + assert_eq!(code_info.lookup(0x0a), Some(("work:linetest.s", 30))); + assert_eq!(code_info.lookup(0x0c), Some(("work:linetest.s", 31))); + + let data_info = file.hunks[1].line_info(); + // Real, non-monotonic pairs as emitted (see this test's doc): + // (33, 0x0e), (36, 0x00), (37, 0x15) -- sorted by offset that's + // (0x00, 36), (0x0e, 33), (0x15, 37). + assert_eq!(data_info.lookup(0x00), Some(("work:linetest.s", 36))); + assert_eq!(data_info.lookup(0x0e), Some(("work:linetest.s", 33))); + assert_eq!(data_info.lookup(0x15), Some(("work:linetest.s", 37))); + // Between the line-33 and line-37 entries: still resolves by + // offset alone, landing on line 33 despite line 36's entry + // having a *lower* offset than line 33's -- exactly the + // property `non_monotonic_pairs_are_sorted_before_lookup` + // isolates synthetically. + assert_eq!(data_info.lookup(0x10), Some(("work:linetest.s", 33))); + } + + // --- HUNK_SYMBOL / symbol+offset attribution (issue #74 follow-up) --- + + /// Builds a `HUNK_SYMBOL` block's raw bytes (the on-disk shape -- + /// see [`read_symbol_block_raw`]/[`parse_symbol_block`]): repeating + /// `{ name_length_longwords, name (NUL-padded), value }` entries, + /// terminated by a zero name length. + fn build_symbol_block_payload(entries: &[(&str, u32)]) -> Vec { + let mut buf = Vec::new(); + for &(name, value) in entries { + let mut name_bytes = name.as_bytes().to_vec(); + while !name_bytes.len().is_multiple_of(4) { + name_bytes.push(0); + } + push_u32(&mut buf, (name_bytes.len() / 4) as u32); + buf.extend_from_slice(&name_bytes); + push_u32(&mut buf, value); + } + push_u32(&mut buf, 0); // terminator + buf + } + + #[test] + fn symbol_lookup_returns_preceding_symbol_for_offset_between_two() { + let payload = build_symbol_block_payload(&[("foo", 0x00), ("bar", 0x20)]); + let table = make_hunk_with_symbols(vec![payload]).symbol_table(); + + assert_eq!(table.lookup(0x10, 0x100), Some(("foo", 0x10))); + assert_eq!(table.lookup(0x1f, 0x100), Some(("foo", 0x1f))); + assert_eq!(table.lookup(0x20, 0x100), Some(("bar", 0x00))); + } + + #[test] + fn symbol_lookup_before_first_symbol_is_none() { + let payload = build_symbol_block_payload(&[("foo", 0x10)]); + let table = make_hunk_with_symbols(vec![payload]).symbol_table(); + + assert_eq!(table.lookup(0x00, 0x100), None); + assert_eq!(table.lookup(0x0f, 0x100), None); + } + + /// A small symbol table must not "match" an offset arbitrarily far + /// past its last entry -- the concrete failure mode this guards + /// against (`Done+0x9c40`, from a three-symbol table matching 40 KB + /// past its last entry) is exactly what prompted issue #74's + /// coordinator follow-up. The bound is the owning hunk's own size: + /// an offset within it but far past the last symbol reports + /// nothing; one right at the boundary or beyond does too (there's + /// nothing past a hunk's end to attribute to anything). + #[test] + fn symbol_lookup_caps_distance_at_the_hunk_end() { + let payload = build_symbol_block_payload(&[("start", 0x00)]); + let table = make_hunk_with_symbols(vec![payload]).symbol_table(); + + // hunk_size = 0x20: offset 0x1f is the last in-bounds byte. + assert_eq!(table.lookup(0x1f, 0x20), Some(("start", 0x1f))); + assert_eq!(table.lookup(0x20, 0x20), None, "at the hunk's own end"); + assert_eq!( + table.lookup(0x9c40, 0x20), + None, + "40 KB past the last symbol must not report Done+0x9c40-style nonsense" + ); + } + + /// Symbol names aren't guaranteed valid UTF-8 (same caveat as `LINE` + /// filenames); a non-UTF-8 name must decode lossily rather than + /// drop the entry or panic. + #[test] + fn symbol_name_non_utf8_decodes_lossily_without_panicking() { + let mut payload = Vec::new(); + push_u32(&mut payload, 1); // name: 1 longword (4 bytes) + payload.extend_from_slice(&[0xFF, 0xFE, b'z', 0]); // invalid UTF-8 + 'z' + NUL pad + push_u32(&mut payload, 0x40); + push_u32(&mut payload, 0); // terminator + + let table = make_hunk_with_symbols(vec![payload]).symbol_table(); + let (name, delta) = table + .lookup(0x40, 0x1000) + .expect("entry should be recovered"); + assert_eq!(delta, 0); + assert!( + name.contains('z'), + "the one valid byte should survive lossy decoding, got {name:?}" + ); + } + + /// [`Hunk::locate`]'s precedence: `file:line` wins over + /// `symbol+offset` whenever `LINE` covers the offset, even though a + /// symbol covers the same offset too. + #[test] + fn locate_prefers_line_over_symbol_when_both_cover_the_offset() { + let line_payload = build_line_payload(0, "main.c", &[(10, 0x00), (11, 0x10)]); + let symbol_payload = build_symbol_block_payload(&[("_main", 0x00)]); + let hunk = make_hunk_full(vec![line_payload], vec![symbol_payload], 0x100); + + // Offset 0x08 is covered by both the LINE entry at 0x00 and the + // _main symbol at 0x00 -- LINE must win. + assert_eq!( + hunk.locate(0x08), + Some(Location::Line { + file: "main.c".to_string(), + line: 10, + }) + ); + } + + /// With no `LINE` coverage at all, [`Hunk::locate`] falls back to + /// the symbol table. + #[test] + fn locate_falls_back_to_symbol_when_there_is_no_line_info() { + let symbol_payload = build_symbol_block_payload(&[("_main", 0x00), ("_helper", 0x20)]); + let hunk = make_hunk_full(Vec::new(), vec![symbol_payload], 0x100); + + assert_eq!( + hunk.locate(0x05), + Some(Location::Symbol { + name: "_main".to_string(), + offset: 5, + }) + ); + assert_eq!( + hunk.locate(0x00), + Some(Location::Symbol { + name: "_main".to_string(), + offset: 0, + }), + "exactly at the first symbol's own value" + ); + } + + /// End-to-end evidence against a real `m68k-amigaos-gcc -g -O0 + /// -noixemul` executable: it carries real, per-hunk `HUNK_SYMBOL` + /// data (47/15/19 symbols across its code/data/bss hunks) but no + /// `LINE` data this loader attaches to any hunk -- its one + /// `HUNK_DEBUG` block (the untagged stabs blob covered by + /// `real_gcc_untagged_stabs_block_is_skipped_cleanly`) sits in the + /// file *after* the last hunk the header declares, so it's outside + /// any hunk's boundary and this loader (matching real `LoadSeg`, + /// which also never reads past the declared hunk range) never + /// captures it into any `Hunk::debug_blocks` at all. So `_free` gets + /// no `file:line`, but does get `_free+0x5` via the symbol fallback + /// -- exactly the case this whole follow-up exists for. Skips + /// loudly if the fixture isn't present (see + /// [`warn_real_artifact_missing`]); corroboration only, same + /// reasoning as the other two real-artifact tests. + #[test] + fn real_gcc_binary_falls_back_to_symbols_with_no_line_info() { + let path = "/private/tmp/claude-501/-Users-simond-src-volamos/25505440-09a1-4588-b085-ae3c886e6132/scratchpad/gccd/t"; + let Ok(bytes) = std::fs::read(path) else { + warn_real_artifact_missing( + "real_gcc_binary_falls_back_to_symbols_with_no_line_info", + path, + ); + return; + }; + + let file = parse(&bytes).expect("real gcc binary should parse"); + assert_eq!(file.hunks.len(), 3, "code, data, bss"); + let code = &file.hunks[0]; + assert!( + code.line_info().is_empty(), + "no LINE data is attached to any hunk in this binary" + ); + assert!(!code.symbol_table().is_empty()); + + // _free is at 0x226e in the real binary, with no closer symbol + // until 0x2318 -- 0x2273 (0x226e + 5) should resolve to + // _free+0x5 via the symbol fallback. + match code.locate(0x2273) { + Some(Location::Symbol { name, offset }) => { + assert_eq!(name, "_free"); + assert_eq!(offset, 5); + } + other => panic!("expected a symbol fallback for _free+5, got {other:?}"), + } + } + + // --- Malformed HUNK_DEBUG payloads: must degrade to "no info", never panic --- + + #[test] + fn malformed_too_short_for_a_magic_yields_no_entries() { + // Only 6 bytes: not even a full base-offset-plus-magic header. + let payload = vec![0, 0, 0, 0, b'L', b'I']; + let info = make_hunk(vec![payload]).line_info(); + assert!(info.is_empty()); + } + + #[test] + fn malformed_absurd_filename_length_yields_no_entries() { + let mut payload = Vec::new(); + push_u32(&mut payload, 0); + payload.extend_from_slice(b"LINE"); + push_u32(&mut payload, u32::MAX); // absurd: claims ~16GB of filename + let info = make_hunk(vec![payload]).line_info(); + assert!(info.is_empty()); + } + + #[test] + fn malformed_filename_length_overruns_block_yields_no_entries() { + let mut payload = Vec::new(); + push_u32(&mut payload, 0); + payload.extend_from_slice(b"LINE"); + push_u32(&mut payload, 100); // claims 400 bytes; payload has none + let info = make_hunk(vec![payload]).line_info(); + assert!(info.is_empty()); + } + + /// A non-UTF-8 filename must decode (lossily) rather than drop the + /// whole block or panic -- the entries themselves are still valid + /// and must still be reachable. + #[test] + fn malformed_non_utf8_filename_decodes_lossily_without_panicking() { + let mut payload = Vec::new(); + push_u32(&mut payload, 0); + payload.extend_from_slice(b"LINE"); + push_u32(&mut payload, 1); // filename: 1 longword (4 bytes) + payload.extend_from_slice(&[0xFF, 0xFE, b'a', 0]); // invalid UTF-8 + 'a' + NUL pad + push_u32(&mut payload, 42); + push_u32(&mut payload, 0x10); + + let info = make_hunk(vec![payload]).line_info(); + let (filename, line) = info.lookup(0x10).expect("entry should still be recovered"); + assert_eq!(line, 42); + assert!( + filename.contains('a'), + "the one valid byte should survive lossy decoding, got {filename:?}" + ); + } + + /// A dangling partial pair (a line number with no following offset) + /// at the end of a block must be dropped silently, not turned into + /// a bogus entry and not treated as an error for the whole block. + #[test] + fn malformed_dangling_partial_pair_is_ignored() { + let mut payload = build_line_payload(0, "x.asm", &[(10, 0x00)]); + push_u32(&mut payload, 99); // a line number with no offset to follow it + let hunk = make_hunk(vec![payload]); + let info = hunk.line_info(); + + assert_eq!(info.lookup(0x00), Some(("x.asm", 10))); + assert_eq!( + info.entries.len(), + 1, + "the dangling partial pair must not have contributed an entry" + ); + } + + /// The exact untagged bytes reported from a real local + /// `m68k-amigaos-gcc -g -O0 -noixemul` build's single `HUNK_DEBUG` + /// block (stabs-format, no ASCII magic in the position `LINE`/ + /// `OPTS`/`SRC6`/`HEAD` use -- see the module docs' "gcc is not + /// covered" note). Must classify as unrecognized and contribute no + /// entries, without panicking or being mistaken for `LINE`. + #[test] + fn real_gcc_untagged_stabs_block_is_skipped_cleanly() { + let payload: Vec = vec![ + 0x00, 0x00, 0x00, 0x4c, 0x00, 0x00, 0x00, 0x10, 0xff, 0xff, 0xff, 0xff, 0x03, 0x00, + 0x01, 0x7e, + ]; + let info = make_hunk(vec![payload]).line_info(); + assert!(info.is_empty()); + } + + /// The same real gcc payload, but exercised through a full leading- + /// position [`parse`] (rather than a direct [`Hunk::line_info`] + /// call), confirming it doesn't derail parsing of the hunk it + /// precedes -- matching the untagged-stabs shape found in a real + /// `m68k-amigaos-gcc` build. + #[test] + fn real_gcc_untagged_stabs_block_does_not_disturb_parse() { + let gcc_payload: Vec = vec![ + 0x00, 0x00, 0x00, 0x4c, 0x00, 0x00, 0x00, 0x10, 0xff, 0xff, 0xff, 0xff, 0x03, 0x00, + 0x01, 0x7e, + ]; + + let mut buf = Vec::new(); + push_u32(&mut buf, HUNK_HEADER); + push_u32(&mut buf, 0); + push_u32(&mut buf, 1); + push_u32(&mut buf, 0); + push_u32(&mut buf, 0); + push_u32(&mut buf, 1); + + push_debug_block_raw(&mut buf, &gcc_payload); + + push_u32(&mut buf, HUNK_CODE); + push_u32(&mut buf, 1); + push_u32(&mut buf, 0x4E71_4E71); + push_u32(&mut buf, HUNK_END); + + let file = parse(&buf).expect("the untagged gcc debug block should be skipped, not error"); + assert_eq!(file.hunks.len(), 1); + assert_eq!(file.hunks[0].data, 0x4E71_4E71u32.to_be_bytes()); + assert!(file.hunks[0].line_info().is_empty()); + } + + #[test] + fn lookup_line_translates_a_loaded_address_via_the_owning_hunks_load_offset() { + let line_payload = build_line_payload(0, "prog.asm", &[(1, 0x00), (2, 0x04)]); + let mut buf = Vec::new(); + push_u32(&mut buf, HUNK_HEADER); + push_u32(&mut buf, 0); + push_u32(&mut buf, 1); + push_u32(&mut buf, 0); + push_u32(&mut buf, 0); + push_u32(&mut buf, 2); // hunk 0: 2 longwords + + push_u32(&mut buf, HUNK_CODE); + push_u32(&mut buf, 2); + push_u32(&mut buf, 0x4E71_4E71); + push_u32(&mut buf, 0x4E71_4E71); + push_debug_block_raw(&mut buf, &line_payload); + push_u32(&mut buf, HUNK_END); + + let file = parse(&buf).unwrap(); + let mut mem = FlatMemory::new(0x1000); + let result = load(&file, &mut mem, 0x400).unwrap(); + + assert_eq!( + result.lookup_line(&file, 0x404), + Some(("prog.asm".to_string(), 2)) + ); + assert_eq!( + result.lookup_line(&file, 0x406), + Some(("prog.asm".to_string(), 2)), + "sparse lookup: still the line-2 entry, the greatest offset <= target" + ); + // Before the hunk's load address entirely. + assert_eq!(result.lookup_line(&file, 0x100), None); + // Past the end of the hunk's reserved size. + assert_eq!(result.lookup_line(&file, 0x500), None); + } + + /// Prints a hard-to-miss banner (not a quiet one-liner) when a + /// real-artifact test can't find its fixture and is about to return + /// early without running its assertions. `cargo test`'s default + /// output capture swallows `eprintln!` on a *passing* test, so this + /// can't force visibility on every run -- but under `--nocapture`, + /// or if the artifact vanishing ever coincides with an unrelated + /// failure that flips capture on, this makes it unmistakable that + /// "test passed" here means "test didn't run", not "assertions + /// held". These two tests are corroboration for + /// `fixtures/linetest`'s committed, always-present coverage (see + /// [`real_linetest_fixture_matches_known_pairs`]) -- not something + /// this loader's correctness depends on, since neither artifact can + /// be vendored into the repo (LawBreaker ships under Enforcer's + /// non-commercial/no-modification terms; the SAS/C object lives in a + /// session-scoped scratch directory). + fn warn_real_artifact_missing(test_name: &str, path: &str) { + eprintln!( + "\n\ + ============================================================\n\ + SKIPPED (not a failure, but NOT a pass either): {test_name}\n\ + Real-artifact fixture not found at: {path}\n\ + This test's assertions did NOT run. It is local corroboration\n\ + only -- fixtures/linetest's committed-fixture tests are the\n\ + durable coverage this loader's correctness actually relies on.\n\ + ============================================================\n" + ); + } + + /// End-to-end evidence against the real `LawBreaker` binary (issue + /// #74's primary source): three real `HUNK_DEBUG` blocks (two + /// `HEADDBGV01` directory blocks this loader doesn't interpret, and + /// one real `LINE` block for `LawBreaker.asm`), parsed through the + /// full [`parse`] entry point exactly as a caller would use it. + /// Skips (loudly -- see [`warn_real_artifact_missing`]) rather than + /// fails if the fixture isn't present on this machine: it lives + /// outside the repo (LawBreaker can't be vendored in -- Enforcer's + /// non-commercial, no-modification distribution terms), so this is + /// corroboration, not something CI (or any other machine) can rely + /// on. `fixtures/linetest` (see + /// [`real_linetest_fixture_matches_known_pairs`]) is the committed, + /// always-present equivalent this loader's tested correctness + /// actually depends on. + #[test] + fn real_lawbreaker_binary_line_info_matches_known_pairs() { + let path = "/Users/simond/.claude/uploads/25505440-09a1-4588-b085-ae3c886e6132/9d77395a-LawBreaker"; + let Ok(bytes) = std::fs::read(path) else { + warn_real_artifact_missing( + "real_lawbreaker_binary_line_info_matches_known_pairs", + path, + ); + return; + }; + + let file = parse(&bytes).expect("real LawBreaker binary should parse"); + assert_eq!(file.hunks.len(), 1); + // Two HEADDBGV01 directory blocks plus the one real LINE block. + assert_eq!(file.hunks[0].debug_blocks.len(), 3); + + let info = file.hunks[0].line_info(); + assert_eq!(info.lookup(0x0000), Some(("LawBreaker.asm", 133))); + assert_eq!(info.lookup(0x0004), Some(("LawBreaker.asm", 134))); + assert_eq!(info.lookup(0x0006), Some(("LawBreaker.asm", 135))); + assert_eq!(info.lookup(0x000a), Some(("LawBreaker.asm", 136))); + assert_eq!(info.lookup(0x000e), Some(("LawBreaker.asm", 137))); + assert_eq!(info.lookup(0x0010), Some(("LawBreaker.asm", 141))); + // Sparse: an offset strictly between two recorded entries + // (0x0006 and 0x000a) must return the preceding one. + assert_eq!(info.lookup(0x0008), Some(("LawBreaker.asm", 135))); + + // LawBreaker also carries a real HUNK_SYMBOL block: 3 named + // entries (LawBreaker@0x0, Do_Law@0x22, Done@0xa6). Offset 0 is + // covered by both the LawBreaker@0x0 symbol *and* the LINE + // entry for line 133 -- Hunk::locate's precedence (issue #74's + // coordinator follow-up) must prefer file:line. + let symbols = file.hunks[0].symbol_table(); + assert_eq!(symbols.lookup(0x00, 0xcc), Some(("LawBreaker", 0))); + assert_eq!(symbols.lookup(0x25, 0xcc), Some(("Do_Law", 3))); + assert_eq!(symbols.lookup(0xa8, 0xcc), Some(("Done", 2))); + assert_eq!( + file.hunks[0].locate(0x0000), + Some(Location::Line { + file: "LawBreaker.asm".to_string(), + line: 133, + }), + "file:line must win over the LawBreaker@0x0 symbol at the same offset" + ); + } + + /// End-to-end evidence against a real SAS/C 6.58 object file + /// (`sc DEBUG=LINE hello.c`), which is *not* loadable through + /// [`parse`] (it's a `HUNK_UNIT` object module, not a `HUNK_HEADER` + /// executable) -- so this tests the block-level decoder + /// (`Hunk::line_info` via a directly-constructed `Hunk`) against the + /// object file's real, unmodified `HUNK_DEBUG` payload bytes + /// instead, sliced out at the byte offsets its real `LINE` block + /// occupies (found by inspection: type/length longwords at file + /// offset 212, 56-byte payload immediately after). Skips loudly if + /// the fixture isn't present (see [`warn_real_artifact_missing`]), + /// same reasoning as the LawBreaker test: this session's scratch + /// directory doesn't survive, so this is corroboration, not durable + /// coverage -- see [`real_linetest_fixture_matches_known_pairs`] for + /// that. + #[test] + fn real_sasc_object_line_block_parses() { + let path = "/private/tmp/claude-501/-Users-simond-src-volamos/25505440-09a1-4588-b085-ae3c886e6132/scratchpad/dbgd/hello.o"; + let Ok(bytes) = std::fs::read(path) else { + warn_real_artifact_missing("real_sasc_object_line_block_parses", path); + return; + }; + + // The real file's HUNK_DEBUG/LINE block: type longword (0x3F1) + // and length longword (0xe = 14 longwords) at file offset 212, + // payload (56 bytes) immediately after. + assert_eq!( + u32::from_be_bytes(bytes[212..216].try_into().unwrap()), + HUNK_DEBUG, + "fixture layout assumption: HUNK_DEBUG type word at offset 212" + ); + let n_longwords = u32::from_be_bytes(bytes[216..220].try_into().unwrap()) as usize; + let payload = &bytes[220..220 + n_longwords * 4]; + + let (filename, entries) = + parse_line_debug_block(payload).expect("real SAS/C LINE block should parse"); + assert_eq!(filename, "work:hello.c"); + assert_eq!( + entries, + vec![ + LineEntry { line: 2, offset: 0 }, + LineEntry { + line: 3, + offset: 0xc + }, + LineEntry { + line: 4, + offset: 22 + }, + LineEntry { + line: 5, + offset: 24 + }, + ] + ); + } } diff --git a/crates/volamos-core/src/sanitize.rs b/crates/volamos-core/src/sanitize.rs index a24ca68..ac7aaf8 100644 --- a/crates/volamos-core/src/sanitize.rs +++ b/crates/volamos-core/src/sanitize.rs @@ -1591,6 +1591,22 @@ impl ShadowMap { /// work (bounded by [`MAX_VIOLATIONS`] regardless) costs nothing /// that matters. pub fn report(&self) -> String { + self.report_with(|_| None) + } + + /// As [`Self::report`], but annotating each site with a + /// human-readable source location when `resolve` can supply one for + /// its PC -- `"file.c:42"` or `"Do_Law+0x12"` (see + /// `crate::loader`'s debug-info lookup). + /// + /// Taking a closure rather than reaching for the information keeps + /// this module free of any dependency on the loader or on how a + /// program was loaded: a shadow map knows PCs, and nothing about + /// hunks, load addresses or debug hunks. The CLI, which has all of + /// that, supplies the mapping. `resolve` returning `None` (the + /// default via [`Self::report`]) simply prints the address alone, + /// exactly as before this existed. + pub fn report_with(&self, resolve: impl Fn(u32) -> Option) -> String { use std::fmt::Write as _; let violations = self.violations.borrow(); @@ -1642,7 +1658,8 @@ impl ShadowMap { let max_addr = cluster.iter().map(|v| v.addr).max().unwrap_or(0); let _ = writeln!( out, - " PC {pc:#010x}: {}", + " PC {pc:#010x}{}: {}", + location_suffix(&resolve, *pc), describe_cluster( kind, cluster[0].size, @@ -1654,7 +1671,7 @@ impl ShadowMap { ); } else { for v in cluster { - let _ = writeln!(out, " {v}"); + let _ = writeln!(out, " {v}{}", location_suffix(&resolve, v.pc)); } } } @@ -1671,6 +1688,22 @@ impl ShadowMap { } } +/// Renders a resolved source location as the ` (at ...)` suffix +/// [`ShadowMap::report_with`] appends to a site's line, or an empty +/// string when the resolver has nothing for that PC. +/// +/// Appended rather than substituted: the raw PC stays in the message +/// even when a location is known, because the address is what you need +/// to find the instruction in a disassembly, and a `file:line` from +/// sparse debug info points at the *statement*, not necessarily the +/// exact instruction within it. +fn location_suffix(resolve: &impl Fn(u32) -> Option, pc: u32) -> String { + match resolve(pc) { + Some(loc) => format!(" (at {loc})"), + None => String::new(), + } +} + /// The minimum number of distinct (already-deduplicated) violations a /// single `(pc, kind, size, reason)` cluster must have before /// [`ShadowMap::report`] collapses it into one aggregate summary line diff --git a/crates/volamos/src/main.rs b/crates/volamos/src/main.rs index 78768af..2175b98 100644 --- a/crates/volamos/src/main.rs +++ b/crates/volamos/src/main.rs @@ -81,6 +81,7 @@ use std::process::ExitCode; use volamos_core::backend::{CpuType, M68kCpu, TRAP_TABLE_END}; use volamos_core::dispatch::{Runtime, StartConfig, TraceEvent}; use volamos_core::exectask::install_host_break_handler; +use volamos_core::loader::Location; use volamos_core::memory::FlatMemory; use volamos_core::vfs::{Vfs, VfsConfig}; use volamos_core::{DEFAULT_STACK_SIZE, LoadError, loader}; @@ -801,11 +802,42 @@ fn program_name_from_path(path: &std::path::Path) -> String { /// per top-level or nested run, right after it finishes -- so a /// `System()`/`Execute()`-spawned nested program's own violations are /// reported too, not just the top-level program's. -fn report_sanitizer_violations(runtime: &Runtime) { +/// Formats a resolved [`Location`] for a diagnostic message: +/// `"hello.c:42"` for source-line info, `"Do_Law+0x12"` for a symbol. +fn format_location(loc: &Location) -> String { + match loc { + Location::Line { file, line } => format!("{file}:{line}"), + Location::Symbol { name, offset } if *offset == 0 => name.clone(), + Location::Symbol { name, offset } => format!("{name}+{offset:#x}"), + } +} + +/// Prints a `--sanitize` run's violations, annotated with source +/// locations where the program's debug info (or failing that, its +/// symbol table) can supply them -- see `crate::loader`'s +/// `lookup_location` and issue #74. +/// +/// `program` is the parsed executable and `load` where its hunks +/// landed; both are needed because the debug info records +/// *hunk-relative* offsets, so a PC has to have its hunk's load address +/// subtracted before it means anything. Passing `None` (the overlay +/// loading path, which doesn't produce a `LoadResult`) just prints +/// addresses alone, exactly as before. +fn report_sanitizer_violations( + runtime: &Runtime, + program: Option<(&loader::HunkFile, &loader::LoadResult)>, +) { if let Some(shadow) = runtime.memory().shadow() && shadow.violation_count() > 0 { - eprint!("{}", shadow.report()); + match program { + Some((file, load)) => eprint!( + "{}", + shadow + .report_with(|pc| load.lookup_location(file, pc).as_ref().map(format_location)) + ), + None => eprint!("{}", shadow.report()), + } } } @@ -914,7 +946,10 @@ fn run_nested_program( let stdout = io::stdout(); let mut out = stdout.lock(); let result = runtime.run(&mut out, None).unwrap_or(-1); - report_sanitizer_violations(&runtime); + // Nested runs parse their own executable locally and don't keep the + // result around; source-location lookup is a top-level nicety, so + // these report plain addresses. + report_sanitizer_violations(&runtime, None); result } @@ -981,6 +1016,10 @@ fn run(opts: &Options) -> Result { // program built that way runs straight into a wild-PC crash -- // found running a real overlay-linked binary. See // Runtime::load_top_level_program's doc for the full story. + // `None` on the overlay path, which loads via + // `load_top_level_program` and produces no `LoadResult` -- those + // runs simply report addresses without source locations. + let mut loaded: Option = None; let mut runtime = if hunk_file.overlay.is_some() { let mut mem = FlatMemory::new(opts.ram_size as usize); // Right after construction, before anything is loaded into it @@ -1014,6 +1053,11 @@ fn run(opts: &Options) -> Result { let load_result = loader::load(&hunk_file, &mut mem, TRAP_TABLE_END) .map_err(|e| format!("couldn't load '{}': {e}", opts.program))?; check_ram_fits(load_result.end, opts.stack_size, opts.ram_size)?; + // Kept for the sanitizer report's source-location lookup (issue + // #74): the debug info records hunk-relative offsets, so a PC + // needs its hunk's load address subtracted, which only this + // result knows. + loaded = Some(load_result.clone()); let config = StartConfig { entry: load_result.entry, load_end: load_result.end, @@ -1073,7 +1117,7 @@ fn run(opts: &Options) -> Result { let result = runtime .run(&mut out, Some(&mut trace)) .map_err(|e| format!("{}: {e}", opts.program)); - report_sanitizer_violations(&runtime); + report_sanitizer_violations(&runtime, loaded.as_ref().map(|load| (&hunk_file, load))); result } diff --git a/crates/volamos/tests/hello_cli.rs b/crates/volamos/tests/hello_cli.rs index 0afeaeb..a0cbbf0 100644 --- a/crates/volamos/tests/hello_cli.rs +++ b/crates/volamos/tests/hello_cli.rs @@ -138,6 +138,25 @@ fn memtest_stdout(flags: &[&str], mode: &str) -> String { String::from_utf8(output.stdout).unwrap() } +/// Path to `fixtures/linetest`, the fixture built with real `LINE` +/// debug info (PhxAss `LINEDEBUG`) -- see `fixtures/README.md`. +const LINETEST_PATH: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/../../fixtures/linetest"); + +#[test] +fn a_binary_with_line_debug_info_still_runs_normally() { + // The debug hunks must not disturb loading or execution -- issue + // #70 was precisely a leading debug block being rejected outright. + let output = Command::new(env!("CARGO_BIN_EXE_volamos")) + .arg(LINETEST_PATH) + .output() + .expect("failed to run the volamos binary"); + assert!(output.status.success(), "linetest exited {:?}", output.status); + assert!( + String::from_utf8_lossy(&output.stdout).contains("linetest"), + "expected linetest's own message" + ); +} + #[test] fn dirty_heap_changes_which_branch_a_zero_dependent_guest_takes() { // The whole point of --dirty-heap (issue #80): `zerodep` reads an diff --git a/fixtures/README.md b/fixtures/README.md index 34002fe..87a99d4 100644 --- a/fixtures/README.md +++ b/fixtures/README.md @@ -1131,3 +1131,109 @@ emits PhxAss's short 8-bit-displacement forms) account for the size difference. If you change `matchflags.s`, update `gen_matchflags.py` to match (or vice versa), and re-run both builds plus the tree/flat/usage/ bad-path sweep above before trusting the result. + +## `linetest`: `HUNK_DEBUG` `LINE` fixture (issue #74) + +Source: `linetest.s`. Built binary: `linetest`. This fixture exists +purely to carry real `HUNK_DEBUG` `LINE` blocks (source-line-to-code- +offset tables) so the loader's line-info parser has something durable +to test against — the two artefacts that motivated it (a scratch- +directory file that gets cleaned up, and the LawBreaker binary, which +can't be vendored here under Enforcer's non-commercial/no-modification +licence) don't survive as fixtures. + +The program is trivial by design — same shape as `hello.s` (fake +pre-seeded `A6`, no `OpenLibrary`): it `PutStr`s a short message via +`dos.library`'s `PutStr` (`-948(a6)`), sets `D0 = 0`, and `rts`s. What +matters is *where* those three instructions sit in the source: they're +placed on deliberately spread-out, memorable lines (20, 25, 30) with +comment/blank padding between them, so the `(line, offset)` pairs +PhxAss emits are small and easy to check a test against. + +### This fixture is PhxAss-only + +Unlike every other fixture in this directory, `linetest` has **no** +`gen_linetest.py` and none is planned. `fixtures/amiga_asm.py` (the +toolchain-free assembler the other `gen_*.py` scripts share) has no +notion of source lines at all and cannot emit `HUNK_DEBUG` blocks — +there is nothing for it to build here that would demonstrate the thing +this fixture exists to test. A generator that produced a debug-info- +free binary would look like this repo's usual authoritative, +toolchain-free build while silently missing the entire point of the +fixture, so it's deliberately not written. `fixtures/linetest` is +committed as assembled by real **PhxAss 4.40** (Aminet freeware, +living outside this repo, not relied on in CI), same "assemble under +`volamos` itself" convention as `matchflags.s`/`memtest.s`: + +```sh +mkdir -p /tmp/linetest && cp fixtures/linetest.s /tmp/linetest/ +./target/release/volamos -V work:/tmp/linetest ~/amiga/PhxAss/PhxAss work:linetest.s LINEDEBUG +cp /tmp/linetest/linetest fixtures/linetest +``` + +PhxAss's `LINEDEBUG` command-line option is what makes it emit a +`HUNK_DEBUG` `LINE` block per section (two sections here — CODE and +DATA — so two `LINE` blocks, confirmed below). Without `LINEDEBUG` it +assembles the same program with no debug hunks at all. + +### Verified binary contents + +Running `./target/debug/volamos fixtures/linetest` prints `Hello from +linetest` and exits 0 — it's still a working program, not just a +debug-info carrier. + +The binary is two hunks (`HUNK_CODE` then `HUNK_DATA`), each followed +by its own `HUNK_DEBUG`/`LINE` block before the hunk's `HUNK_END`, +confirmed by walking the raw hunk stream. Both `LINE` blocks record the +filename PhxAss itself saw while assembling — the **Amiga path passed +on its command line**, `work:linetest.s` — not any host path; that's +worth remembering since it won't match `fixtures/linetest.s`'s host +location. + +**Code-hunk `LINE` block** (`tag=b'LINE'`, `base_offset=0`, +`name="work:linetest.s"`): + +| source line | code-hunk offset | instruction | +|---|---|---| +| 20 | `0x00` | `move.l #msg,d1` | +| 25 | `0x06` | `jsr -948(a6)` | +| 30 | `0x0a` | `moveq #0,d0` | +| 31 | `0x0c` | `rts` | + +**Data-hunk `LINE` block** (`tag=b'LINE'`, `base_offset=0`, +`name="work:linetest.s"`): + +| source line | data-hunk offset | corresponds to | +|---|---|---| +| 33 | `0x0e` | `section data,data` (see note below) | +| 36 | `0x00` | `msg: dc.b "Hello from linetest\n",0` | +| 37 | `0x15` | `even` (alignment padding byte) | + +Both blocks were read straight off the committed binary with a short +Python script walking the hunk stream for `HUNK_DEBUG` (`0x3F1`) and +decoding its payload (`base_offset:u32`, `tag:4 bytes`, +`namelen:u32` longwords, the name itself, then `(line:u32, +offset:u32)` pairs to the end of the payload) — not hand-transcribed. + +**Note on the data-hunk block's first pair**: line 33 is the `section +data,data` directive itself, yet its recorded offset (`0x0e`, i.e. 14) +falls *inside* the message string rather than at the start or end of +the data hunk, and the three pairs are not offset-monotonic in line +order (33→0x0e, 36→0x00, 37→0x15). This is exactly what real PhxAss +4.40 emitted — verified via a raw hex dump of the payload bytes, not a +parsing artefact of the script above — so it's recorded here as an +observed real-assembler quirk for the `LINE`-block parser to tolerate +(don't assume offsets are sorted or that every pair maps cleanly onto +"the instruction that starts there"), not something to "fix" in this +fixture. + +### Regenerating + +There is no toolchain-free path for this one — see "This fixture is +PhxAss-only" above. Re-run the `LINEDEBUG` command shown there with +real PhxAss under `volamos` and re-commit `fixtures/linetest`. If you +change `linetest.s`, keep the three instructions' source line numbers +documented above in sync with the actual file (they're deliberately +memorable, not incidental), and re-verify the `(line, offset)` table +by re-dumping the rebuilt binary's `HUNK_DEBUG` blocks rather than +assuming the old table still applies. diff --git a/fixtures/linetest b/fixtures/linetest new file mode 100644 index 0000000000000000000000000000000000000000..3de44e1ff4b88d36ffc3d0f6a2a41abcce37e708 GIT binary patch literal 240 zcmZQzVE)Vi0ZdSu5lFKDF&hH|^Ggt0$p$3jx9*Qm0fS$uUm;NZ4Nw524uqllm_Gv9 z{6Ng$ Date: Wed, 16 Sep 2026 17:33:07 +0100 Subject: [PATCH 2/2] Format the linetest integration test cargo fmt --all --check was failing on an assert! that rustfmt wants split across lines. Tests and clippy were green; this was purely formatting, added after the gate run that formatted everything else. The pre-push check missed it because it read $? through a pipe, which reports the exit status of the last command in the pipeline (head), not cargo's -- so a genuine exit 1 read as a pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23 --- crates/volamos/tests/hello_cli.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/volamos/tests/hello_cli.rs b/crates/volamos/tests/hello_cli.rs index a0cbbf0..e4928df 100644 --- a/crates/volamos/tests/hello_cli.rs +++ b/crates/volamos/tests/hello_cli.rs @@ -150,7 +150,11 @@ fn a_binary_with_line_debug_info_still_runs_normally() { .arg(LINETEST_PATH) .output() .expect("failed to run the volamos binary"); - assert!(output.status.success(), "linetest exited {:?}", output.status); + assert!( + output.status.success(), + "linetest exited {:?}", + output.status + ); assert!( String::from_utf8_lossy(&output.stdout).contains("linetest"), "expected linetest's own message"