Repository: probe-rs/probe-rs
Pull request #3304 (2025-05-10 22:10:53)

Fix a bunch of breakpoint-related issues
First commit is #3302


Patch:
diff --git a/changelog/fixed-breakpoints.md b/changelog/fixed-breakpoints.md
new file mode 100644
index 00000000..ea566c43
--- /dev/null
+++ b/changelog/fixed-breakpoints.md
@@ -0,0 +1,1 @@
+Fixed various breakpoint-related issues
\ No newline at end of file
diff --git a/changelog/fixed-xtensa-step-crash.md b/changelog/fixed-xtensa-step-crash.md
new file mode 100644
index 00000000..422e3721
--- /dev/null
+++ b/changelog/fixed-xtensa-step-crash.md
@@ -0,0 +1,1 @@
+Fixed a crash during step operations while debugging Xtensa CPUs.
\ No newline at end of file
diff --git a/probe-rs-debug/src/debug_step.rs b/probe-rs-debug/src/debug_step.rs
index bb9d42e6..a91cb3e2 100644
--- a/probe-rs-debug/src/debug_step.rs
+++ b/probe-rs-debug/src/debug_step.rs
@@ -327,31 +327,43 @@ fn run_to_address(
     target_address: u64,
     core: &mut impl CoreInterface,
 ) -> Result<(CoreStatus, u64), DebugError> {
-    Ok(if target_address == program_counter {
+    if target_address == program_counter {
         // No need to step further. e.g. For inline functions we have already stepped to the best available target address..
-        (
+        return Ok((
             core.status()?,
             core.read_core_reg(core.program_counter().id())?
                 .try_into()?,
-        )
-    } else if core.set_hw_breakpoint(0, target_address).is_ok() {
+        ));
+    }
+
+    let breakpoints = core.hw_breakpoints()?;
+    let bp_to_use = breakpoints.iter().position(|bp| bp.is_none()).unwrap_or(0);
+
+    if core.set_hw_breakpoint(bp_to_use, target_address).is_ok() {
         core.run()?;
         // It is possible that we are stepping over long running instructions.
-        match core.wait_for_core_halted(Duration::from_millis(1000)) {
+        let status = core.wait_for_core_halted(Duration::from_millis(1000));
+
+        // Restore the original breakpoint.
+        if let Some(Some(bp)) = breakpoints.get(bp_to_use) {
+            core.set_hw_breakpoint(bp_to_use, *bp)?;
+        } else {
+            core.clear_hw_breakpoint(bp_to_use)?;
+        }
+
+        match status {
             Ok(()) => {
                 // We have hit the target address, so all is good.
                 // NOTE: It is conceivable that the core has halted, but we have not yet stepped to the target address. (e.g. the user tries to step out of a function, but there is another breakpoint active before the end of the function.)
                 //       This is a legitimate situation, so we clear the breakpoint at the target address, and pass control back to the user
-                core.clear_hw_breakpoint(0)?;
-                (
+                Ok((
                     core.status()?,
                     core.read_core_reg(core.program_counter().id())?
                         .try_into()?,
-                )
+                ))
             }
             Err(error) => {
                 program_counter = core.halt(Duration::from_millis(500))?.pc;
-                core.clear_hw_breakpoint(0)?;
                 if matches!(
                     error,
                     probe_rs::Error::Arm(ArmError::Timeout)
@@ -364,21 +376,22 @@ fn run_to_address(
                         target_address,
                         program_counter
                     );
-                    (core.status()?, program_counter)
+                    Ok((core.status()?, program_counter))
                 } else {
                     // Something else is wrong.
-                    return Err(DebugError::Other(format!(
+                    Err(DebugError::Other(format!(
                         "Unexpected error while waiting for the core to halt after stepping to {:#010X}. Forced a halt at {:#010X}. {:?}.",
                         program_counter, target_address, error
-                    )));
+                    )))
                 }
             }
         }
     } else {
         // If we don't have breakpoints to use, we have to rely on single stepping.
         // TODO: In theory, this could go on for a long time. Should we consider NOT allowing this kind of stepping if there are no breakpoints available?
-        step_to_address(target_address..=u64::MAX, core)?
-    })
+
+        Ok(step_to_address(target_address..=u64::MAX, core)?)
+    }
 }
 
 /// In some cases, we need to single-step the core, until ONE of the following conditions are met:
diff --git a/probe-rs/src/architecture/xtensa/communication_interface.rs b/probe-rs/src/architecture/xtensa/communication_interface.rs
index f6aedd26..3fca5584 100644
--- a/probe-rs/src/architecture/xtensa/communication_interface.rs
+++ b/probe-rs/src/architecture/xtensa/communication_interface.rs
@@ -422,10 +422,18 @@ impl<'probe> XtensaCommunicationInterface<'probe> {
         self.schedule_write_register(ICount(-((1 + by) as i32) as u32))?;
 
         self.resume_core()?;
-        self.wait_for_core_halted(Duration::from_millis(100))?;
+        // TODO: instructions like WAITI should be emulated as they are not single steppable.
+        // For now it's good enough to force a halt on timeout (instead of crashing) although it can
+        // stop in a long-running interrupt handler which isn't necessarily what the user wants.
+        // Even then, WAITI should be detected and emulated.
+        match self.wait_for_core_halted(Duration::from_millis(100)) {
+            Ok(()) => {}
+            Err(XtensaError::Timeout) => self.halt(Duration::from_millis(100))?,
+            Err(e) => return Err(e),
+        }
 
         // Avoid stopping again
-        self.schedule_write_register(ICountLevel(self.core_properties.debug_level as u32 + 1))?;
+        self.schedule_write_register(ICountLevel(0))?;
 
         Ok(())
     }
diff --git a/probe-rs/src/architecture/xtensa/mod.rs b/probe-rs/src/architecture/xtensa/mod.rs
index 3adbfb0d..b6ca7d7b 100644
--- a/probe-rs/src/architecture/xtensa/mod.rs
+++ b/probe-rs/src/architecture/xtensa/mod.rs
@@ -168,7 +168,7 @@ impl<'probe> Xtensa<'probe> {
         Ok(CoreInformation { pc: pc.try_into()? })
     }
 
-    fn skip_breakpoint_instruction(&mut self) -> Result<(), Error> {
+    fn skip_breakpoint(&mut self) -> Result<(), Error> {
         self.state.semihosting_command = None;
         if !self.state.pc_written {
             let debug_cause = self.debug_cause()?;
@@ -183,10 +183,22 @@ impl<'probe> Xtensa<'probe> {
 
             if pc_increment > 0 {
                 // Step through the breakpoint
-                let pc = self.program_counter().id;
-                let mut pc_value = self.read_core_reg(pc)?;
-                pc_value.increment_address(pc_increment)?;
-                self.write_core_reg(pc, pc_value)?;
+                let mut pc_value = self.interface.read_register_untyped(Register::CurrentPc)?;
+                pc_value += pc_increment;
+                self.interface
+                    .write_register_untyped(Register::CurrentPc, pc_value)?;
+            } else if debug_cause.ibreak_exception() {
+                let pc_value = self.interface.read_register_untyped(Register::CurrentPc)?;
+                let bps = self.hw_breakpoints()?;
+                if let Some(bp_unit) = bps.iter().position(|bp| *bp == Some(pc_value as u64)) {
+                    // Disable the breakpoint
+                    self.clear_hw_breakpoint(bp_unit)?;
+                    // Single step
+                    let ps = self.current_ps()?;
+                    self.interface.step(1, ps.intlevel())?;
+                    // Re-enable the breakpoint
+                    self.set_hw_breakpoint(bp_unit, pc_value as u64)?;
+                }
             }
         }
 
@@ -476,7 +488,7 @@ impl CoreInterface for Xtensa<'_> {
     }
 
     fn run(&mut self) -> Result<(), Error> {
-        self.skip_breakpoint_instruction()?;
+        self.skip_breakpoint()?;
         Ok(self.interface.resume_core()?)
     }
 
@@ -499,10 +511,12 @@ impl CoreInterface for Xtensa<'_> {
     }
 
     fn step(&mut self) -> Result<CoreInformation, Error> {
-        self.skip_breakpoint_instruction()?;
-        let ps = self.current_ps()?;
+        self.skip_breakpoint()?;
 
+        // Only count instructions in the current context.
+        let ps = self.current_ps()?;
         self.interface.step(1, ps.intlevel())?;
+
         self.on_halted()?;
 
         self.core_info()
diff --git a/probe-rs/src/core.rs b/probe-rs/src/core.rs
index a4f8b368..7c4bd456 100644
--- a/probe-rs/src/core.rs
+++ b/probe-rs/src/core.rs
@@ -405,21 +405,6 @@ impl<'probe> Core<'probe> {
         self.inner.return_address()
     }
 
-    /// Find the index of the next available HW breakpoint comparator.
-    fn find_free_breakpoint_comparator_index(&mut self) -> Result<usize, Error> {
-        let mut next_available_hw_breakpoint = 0;
-        for breakpoint in self.inner.hw_breakpoints()? {
-            if breakpoint.is_none() {
-                return Ok(next_available_hw_breakpoint);
-            } else {
-                next_available_hw_breakpoint += 1;
-            }
-        }
-        Err(Error::Other(
-            "No available hardware breakpoints".to_string(),
-        ))
-    }
-
     /// Set a hardware breakpoint
     ///
     /// This function will try to set a hardware breakpoint att `address`.
@@ -433,15 +418,15 @@ impl<'probe> Core<'probe> {
         }
 
         // If there is a breakpoint set already, return its bp_unit_index, else find the next free index.
-        let breakpoint_comparator_index = match self
-            .inner
-            .hw_breakpoints()?
-            .iter()
-            .position(|&bp| bp == Some(address))
-        {
-            Some(breakpoint_comparator_index) => breakpoint_comparator_index,
-            None => self.find_free_breakpoint_comparator_index()?,
-        };
+        let breakpoints = self.inner.hw_breakpoints()?;
+        let breakpoint_comparator_index =
+            match breakpoints.iter().position(|&bp| bp == Some(address)) {
+                Some(breakpoint_comparator_index) => breakpoint_comparator_index,
+                None => breakpoints
+                    .iter()
+                    .position(|bp| bp.is_none())
+                    .ok_or_else(|| Error::Other("No available hardware breakpoints".to_string()))?,
+            };
 
         tracing::debug!(
             "Trying to set HW breakpoint #{} with comparator address  {:#08x}",
@@ -464,7 +449,7 @@ impl<'probe> Core<'probe> {
             .inner
             .hw_breakpoints()?
             .iter()
-            .position(|bp| bp.is_some() && bp.unwrap() == address);
+            .position(|bp| *bp == Some(address));
 
         tracing::debug!(
             "Will clear HW breakpoint    #{} with comparator address    {:#08x}",
@@ -619,8 +604,9 @@ impl CoreInterface for Core<'_> {
         self.set_hw_breakpoint(addr)
     }
 
-    fn clear_hw_breakpoint(&mut self, _unit_index: usize) -> Result<(), Error> {
-        self.clear_all_hw_breakpoints()
+    fn clear_hw_breakpoint(&mut self, unit_index: usize) -> Result<(), Error> {
+        self.inner.clear_hw_breakpoint(unit_index)?;
+        Ok(())
     }
 
     fn registers(&self) -> &'static registers::CoreRegisters {
@@ -644,7 +630,7 @@ impl CoreInterface for Core<'_> {
     }
 
     fn hw_breakpoints_enabled(&self) -> bool {
-        todo!()
+        self.inner.hw_breakpoints_enabled()
     }
 
     fn architecture(&self) -> Architecture {


Test patch:
diff --git a/smoke-tester/src/tests.rs b/smoke-tester/src/tests.rs
index d476fc50..d3ce7cf7 100644
--- a/smoke-tester/src/tests.rs
+++ b/smoke-tester/src/tests.rs
@@ -3,7 +3,7 @@ use std::time::Instant;
 use colored::Colorize;
 use linkme::distributed_slice;
 use probe_rs::{
-    Architecture, Core, MemoryInterface, Session,
+    Architecture, Core, CoreInterface, MemoryInterface, Session,
     config::MemoryRegion,
     flashing::{DownloadOptions, FlashProgress, FormatKind, download_file_with_options},
 };
@@ -247,6 +247,24 @@ fn test_hw_breakpoints(tracker: &TestTracker, core: &mut Core) -> TestResult {
 
         println_test_status!(tracker, blue, "{} breakpoints supported", num_breakpoints);
 
+        // Test CoreInterface
+        for i in 0..num_breakpoints {
+            CoreInterface::set_hw_breakpoint(
+                core,
+                i as usize,
+                initial_breakpoint_addr + 4 * i as u64,
+            )
+            .into_diagnostic()?;
+        }
+
+        // TODO: check if a breakpoint can be overwritten - the position argument is currently ignored
+
+        // Clear all breakpoints again
+        for i in 0..num_breakpoints {
+            CoreInterface::clear_hw_breakpoint(core, i as usize).into_diagnostic()?;
+        }
+
+        // Test inherent methods
         for i in 0..num_breakpoints {
             core.set_hw_breakpoint(initial_breakpoint_addr + 4 * i as u64)
                 .into_diagnostic()?;