Report source file:line (or symbol+offset) in diagnostics (issue #74)

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>) -> 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<String>, 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

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<M68kCpu>) {
+/// 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<M68kCpu>,
+    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<i32, String> {
     // 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<loader::LoadResult> = 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<i32, String> {
         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<i32, String> {
     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
 }
 

crates/volamos/tests/hello_cli.rs

@@ -138,6 +138,29 @@ 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

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.

fixtures/linetest.s

@@ -0,0 +1,37 @@
+; linetest.s -- fixture for volamos's HUNK_DEBUG "LINE" block parser
+; (issue #74). PhxAss syntax, NOT vasm mot syntax (see "Regenerating"
+; in fixtures/README.md for why this one is special).
+;
+; The program itself is deliberately trivial, in the same shape as
+; fixtures/hello.s: it PutStr's a short message via dos.library's
+; PutStr (LVO -948, i.e. `jsr -948(a6)`), then sets D0 = 0 and RTS's
+; back to the runtime's exit stub, exactly like hello.s. There is no
+; OpenLibrary call -- see hello.s's own header comment for the full
+; calling-convention rationale (A6 is pre-seeded by the runtime with
+; a fake dos.library base for the one LVO call below).
+;
+; What actually matters here is *not* the program's behaviour but its
+; HUNK_DEBUG LINE block: the three instructions below sit on lines
+; 20, 25, 30. See fixtures/README.md's "linetest" section for the
+; exact expected (line, offset) pairs.
+    section code,code
+
+start:
+    move.l  #msg,d1         ; line 20: D1 = pointer to the message
+
+; padding
+; padding
+
+    jsr     -948(a6)        ; line 25: call dos.library/PutStr
+
+; padding
+; padding
+
+    moveq   #0,d0            ; line 30: D0 = process exit code (0)
+    rts                       ; return to the runtime's exit stub
+
+    section data,data
+
+msg:
+    dc.b    "Hello from linetest\n",0
+    even

userdocs/CLI-Reference.md

@@ -411,6 +411,56 @@ noise, but a single violation's detail (for a corrupted return address,
 the expected and actual addresses) is the whole diagnostic value and is
 never collapsed away.
 
+## Source locations in diagnostics
+
+When a program carries debug information, volamos annotates sanitizer
+violations with where in the source they happened, instead of only an
+address:
+
+```console
+$ volamos --sanitize memtest overrun
+sanitizer: 1 site(s), 1 violation(s):
+  invalid 1-byte write at 0x000032a0 (heap redzone) from PC 0x00002b38 (at work:memtest.s:281)
+```
+
+Nothing needs enabling — it is used automatically when present. Two
+sources are consulted, best first:
+
+1. **`file:line`**, from a `HUNK_DEBUG` `LINE` block. Emitted by SAS/C
+   when asked (`sc DEBUG=LINE`) and by PhxAss (`LINEDEBUG`).
+2. **`symbol+offset`**, from the binary's `HUNK_SYMBOL` table, when
+   there is no line coverage:
+
+   ```
+   return address corrupted at stack slot 0x00ffffe0: expected 0x00002b40,
+     found 0x41414141 from PC 0x00002b10 (at ___main+0x3c)
+   ```
+
+The raw PC is always kept alongside, because that is what you need to
+find the instruction in a disassembly.
+
+!!! note "What each toolchain gives you"
+    **m68k-amigaos-gcc** emits *stabs* debug info, which volamos does
+    not decode, so `-g` alone does not produce `file:line` here. gcc
+    binaries do carry a symbol table, though, so they get
+    `symbol+offset` — the example above is a real gcc-built program.
+
+    **SAS/C** and **PhxAss** produce `LINE` blocks when asked, and those
+    give true `file:line`.
+
+!!! warning "Two honest limits on attribution"
+    **Line numbers point at statements, not instructions.** `LINE` data
+    is sparse — one entry per source line — so a PC between two entries
+    is reported against the earlier one. That is the right answer for
+    "which statement", not "which instruction".
+
+    **Symbol attribution is only as good as the symbol table.** `static`
+    functions are not exported and so do not appear in `HUNK_SYMBOL` at
+    all; a violation inside one is reported against the nearest
+    preceding *exported* symbol, which can be a surprising name with a
+    large offset. Prefer `file:line` where your toolchain can produce
+    it.
+
 ## `--dirty-heap`
 
 Fills every `AllocMem`/`AllocVec`/`AllocPooled` block allocated