Gdb stepping fixes

src/gdb_stub.rs

@@ -356,9 +356,12 @@ impl SingleThreadBase for IrisTarget {
     }
 
     fn read_addrs(&mut self, start_addr: u64, data: &mut [u8]) -> TargetResult<usize, Self> {
-        if self.cpu.read_mem(mips_sign_extend(start_addr), data).is_err() {
-            data.fill(0);
-        }
+        // Fail the read instead of returning zeros with an OK: GDB decodes
+        // zeroed bytes as a valid `nop` and would plan its software
+        // single-step from that fabricated code instead of reporting a fault.
+        self.cpu
+            .read_mem(mips_sign_extend(start_addr), data)
+            .map_err(|_| TargetError::NonFatal)?;
         Ok(data.len())
     }
 

src/mips_exec.rs

@@ -1085,6 +1085,21 @@ pub(crate) const HW_READ_FIXUP_ADDRS: &[u64] = &[
     0x1fbd98bc, 0x1fbd98bd, 0x1fbd98be, 0x1fbd98bf, // IOC_TIMER_CTL
 ];
 
+/// Comparison key for breakpoint addresses (see `check_breakpoint`): word-aligns,
+/// and folds the unmapped kseg0/kseg1 windows onto the physical address they alias
+/// so either window hits. Every other address keeps all of its bits.
+#[inline]
+fn bp_match_key(addr: u64) -> u64 {
+    let hi = addr >> 32;
+    if hi == 0 || hi == 0xFFFF_FFFF {
+        let lo = addr & 0xFFFF_FFFF;
+        if (0x8000_0000..0xC000_0000).contains(&lo) {
+            return (lo & 0x1FFF_FFFF) & !3;
+        }
+    }
+    addr & !3
+}
+
 // ---- translate_fn slow-path wrappers (one per privilege × addressing-mode combination) ------
 // These are free functions so they can be stored as bare fn pointers in MipsExecutor.
 // They are only called on a nanotlb miss — the nanotlb probe happens before the fn-pointer call.
@@ -2184,7 +2199,15 @@ impl<T: Tlb, C: MipsCache> MipsExecutor<T, C> {
 
         #[cfg(not(feature = "lightning"))]
         if self.bp_enabled() && self.check_breakpoint::<{ BpType::Pc as u8 }>(pc) {
-            return EXEC_BREAKPOINT;
+            // This runs before the fetch, so only honor the breakpoint once the
+            // instruction is fetchable: an instruction whose fetch takes a TLB
+            // miss never executes, and a debugger can't read that page either.
+            if !self.debug_translate(pc).is_exception() {
+                return EXEC_BREAKPOINT;
+            }
+            // Not taken: drop the id check_breakpoint() just recorded, or the
+            // next stop is misreported as this breakpoint.
+            self.last_bp_hit = None;
         }
 
         // No per-instruction CP0 Count work: Count is virtual (materialized
@@ -2365,12 +2388,10 @@ impl<T: Tlb, C: MipsCache> MipsExecutor<T, C> {
         let mut hit = false;
         for bp in &self.breakpoints {
             if bp.enabled && bp.kind as u8 == KIND {
-                // Normalize to physical: strip sign-extension and kseg bits (top 3 of
-                // low 32), then mask bottom 2 for word alignment.  This makes a bp set
-                // on physical 0x1fbb0010 hit whether the CPU access came through kseg0
-                // (0x9fbb0010) or kseg1 (0xbfbb0010).
-                const PHYS_MASK: u64 = 0x1FFF_FFF8;
-                if (bp.addr & PHYS_MASK) == (addr & PHYS_MASK) {
+                // Normalize kseg0/kseg1 to physical so a bp on physical
+                // 0x1fbb0010 hits through kseg0 (0x9fbb0010) or kseg1
+                // (0xbfbb0010), then word-align. Never mask below bit 2.
+                if bp_match_key(bp.addr) == bp_match_key(addr) {
                     // Check optional register condition
                     if let Some(expr) = &bp.condition {
                         let symbols = self.symbols.lock();
@@ -7797,6 +7818,11 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> MipsCpu<T, C> {
         let task = move || {
             let mut exec = executor.lock();
 
+            // Clear any breakpoint id left from a previous run/step, or a stop
+            // caused by something else (retry, exception_mask) is reported by
+            // run_blocking() — and to GDB — as a phantom breakpoint hit.
+            exec.last_bp_hit = None;
+
             // Same reasoning as MipsCpu::start(): this closure runs on its own
             // fresh OS thread ("MIPS-Debug"), so the host FPU rounding mode
             // needs to be re-synced from the guest's tracked FCSR.RM rather
@@ -7818,6 +7844,10 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> MipsCpu<T, C> {
 
             let mut first_step = true;
             let mut steps_since_yield = 0;
+            // Consecutive EXEC_RETRY count for the instruction currently being
+            // attempted (see the EXEC_RETRY arm); reset once anything retires.
+            let mut retry_count: u32 = 0;
+            const MAX_RUN_RETRIES: u32 = 100_000;
 
             loop {
                 if !running.load(Ordering::Relaxed) {
@@ -7901,8 +7931,19 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> MipsCpu<T, C> {
                 let pc = exec.core.pc;
                 match status {
                     EXEC_RETRY => {
-                        writeln!(writer, "PC={:016x}: Retry (Bus Busy)", pc).unwrap();
-                        break;
+                        // Nothing retired, so the same PC must be attempted
+                        // again (validate.rs documents the same contract);
+                        // breaking out made a transient bus-busy a real stop.
+                        retry_count += 1;
+                        if retry_count > MAX_RUN_RETRIES {
+                            writeln!(writer, "PC={:016x}: Retry (Bus Busy) — still busy after {} attempts, giving up", pc, MAX_RUN_RETRIES).unwrap();
+                            break;
+                        }
+                        // Refund the step budget: a retry retires no
+                        // instruction, so `step N` must still retire N.
+                        if let Some(c) = count { count = Some(c + 1); }
+                        std::hint::spin_loop();
+                        continue;
                     }
                     s if s & EXEC_IS_EXCEPTION != 0 && s & EXEC_IS_TLB_REFILL == 0 => {
                         let code = (s >> crate::mips_core::CAUSE_EXCCODE_SHIFT) & 0x1F;
@@ -7933,6 +7974,10 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> MipsCpu<T, C> {
                     _ => {}
                 }
 
+                // Something retired: the retry budget is per instruction
+                // attempt, not cumulative over a long run.
+                retry_count = 0;
+
                 steps_since_yield += 1;
                 if steps_since_yield >= 500000 {
                     steps_since_yield = 0;
@@ -10763,11 +10808,16 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> CpuDebug
     }
 
     fn step_one(&self) -> StopReason {
-        use crate::mips_exec::EXEC_BREAKPOINT;
+        use crate::mips_exec::{EXEC_BREAKPOINT, EXEC_RETRY};
         self.cpu.stop(); // ensure no thread is running
         let mut exec = self.cpu.executor.lock();
         exec.last_bp_hit = None;
 
+        // EXEC_RETRY means the instruction did not retire and PC is unmoved, so
+        // the same PC must be stepped again — bounded like validate.rs's
+        // MAX_RETRIES_PER_INSTRUCTION so a stuck device can't spin here.
+        const MAX_STEP_RETRIES: u32 = 100_000;
+
         // Execute one instruction. If it's a branch, also execute the delay
         // slot, so a single GDB-visible step lands past it rather than
         // stopping mid-delay-slot. `core.in_delay_slot` (set by
@@ -10776,7 +10826,27 @@ impl<T: Tlb + Send + 'static, C: MipsCache + Send + 'static> CpuDebug
         // yet — status codes carry no PC-related information of their own
         // (every handler sets core.pc itself before returning).
         //eprintln!("GDB: step_one: PC={:#018x}", exec.core.pc);
-        let status = exec.step();
+        let mut status = exec.step();
+        let mut retries = 0u32;
+        while status == EXEC_RETRY && retries < MAX_STEP_RETRIES {
+            retries += 1;
+            std::hint::spin_loop();
+            status = exec.step();
+        }
+        if status == EXEC_RETRY {
+            // Still busy after the cap: a device is stuck, not transiently
+            // busy. Report a stop rather than a phantom "step completed".
+            drop(exec);
+            self.stop_state.set(StopReason::Interrupted);
+            return StopReason::Interrupted;
+        }
+        if status == EXEC_BREAKPOINT {
+            // Sitting on an active breakpoint: step() matches before it
+            // dispatches, so without a one-shot skip every later single-step
+            // re-detects it (as run_debug_loop's first_step does for `cont`).
+            exec.skip_breakpoints = true;
+            status = exec.step();
+        }
         //eprintln!("GDB: step_one: after step status={:#010x} PC={:#018x}", status, exec.core.pc);
         let reason = if status == EXEC_BREAKPOINT {
             drop(exec);