Apply suggestions from code review.

Mostly comment fixes.

Signed-off-by: Diego Asanza <[email protected]>

Author: Diego Asanza

README.md

@@ -29,6 +29,10 @@ zmu supports Linux and Windows operating systems.
 - DWT
     - Cycle counter
 - Instruction trace
+- GDB Server
+    - single stepping
+    - range stepping
+    - breakpoints
 
 ## Missing / Planned features
 - Time simulation / sync to real time
@@ -43,6 +47,10 @@ zmu supports Linux and Windows operating systems.
 - System Simulation:
     - device profiles, eg stm32 device support
     - board profiles, external peripheral simulation
+- GDB Server:
+    - Reading/Writting memory
+    - Writting Registers
+    - Pass the port on the command line
 
 ## Depedencies
 
@@ -178,3 +186,43 @@ Run the emulator:
 $zmu run tests/hello_world/hello_world-cm0.elf
 hello, world
 ```
+
+Run the GDB Server:
+```
+$zmu run --gdb tests/hello_world/hello_world-cm0.elf
+Starting GDB Server on port 9001 ...
+```
+
+On a separate terminal start the gdb client:
+```
+$ gdb-multiarch tests/hello_world/hello_world-cm0.elf
+GNU gdb (Debian 13.1-3) 13.1
+Copyright (C) 2023 Free Software Foundation, Inc.
+License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
+This is free software: you are free to change and redistribute it.
+There is NO WARRANTY, to the extent permitted by law.
+Type "show copying" and "show warranty" for details.
+This GDB was configured as "x86_64-linux-gnu".
+Type "show configuration" for configuration details.
+For bug reporting instructions, please see:
+<https://www.gnu.org/software/gdb/bugs/>.
+Find the GDB manual and other documentation resources online at:
+    <http://www.gnu.org/software/gdb/documentation/>.
+
+For help, type "help".
+Type "apropos word" to search for commands related to "word"...
+Reading symbols from tests/hello_world/hello_world-cm0.elf...
+(gdb) target remote localhost:9001
+Remote debugging using localhost:9001
+Reset_Handler ()
+    at /usr/share/doc/gcc-arm-none-eabi/examples/startup/startup_ARMCM0.S:150
+150             ldr     r1, =__etext
+(gdb) b main
+Breakpoint 1 at 0x5c: file main.c, line 6.
+(gdb) c
+Continuing.
+
+Breakpoint 1, main () at main.c:6
+6           printf("hello, world\n");
+(gdb)
+```

zmu_cortex_m/Cargo.toml

@@ -9,6 +9,7 @@ byteorder = "1"
 enum-set = "0.0.8"
 gdbstub = "0.7"
 gdbstub_arch = "0.3"
+log = "0.4"
 
 
 [features]

zmu_cortex_m/src/gdb/conn.rs

@@ -12,10 +12,9 @@ impl TcpConnection {
         let listener = TcpListener::bind(("127.0.0.1", port)).unwrap();
 
         for stream in listener.incoming() {
-            let stream = stream.unwrap();
+            let stream = stream.map_err(|_| "Error accepting socket connection")?;
             stream.set_read_timeout(Some(std::time::Duration::from_millis(1)))
-                .expect("set_read_timeout call failed");
-            // stream.set_nonblocking(true).expect("set_nonblocking call failed");
+                .map_err(|_| "Error setting timeout")?;
             return Ok(TcpConnection { stream });
         };
 
@@ -79,7 +78,6 @@ impl ConnectionExt for TcpConnection {
                     #[cfg(unix)]
                     std::io::ErrorKind::WouldBlock => return Ok(None),
                     _ => {
-                        println!("peek error: {:?}", e);
                         return Err("socket peek failed")
                     }
                 }

zmu_cortex_m/src/gdb/server.rs

@@ -1,5 +1,5 @@
 //!
-//! Flash Memory simulation
+//! GDB Server implementation
 //!
 //!
 
@@ -27,14 +27,19 @@ use crate::semihosting::SemihostingResponse;
 /// The gdb Server
 /// 
 pub struct GdbServer {
-    // number: i32,
     target: ZmuTarget,
 }
 
-/// 
 impl GdbServer {
 
-    ///
+    /// Create a new GDB server. 
+    /// 
+    /// # Arguments
+    /// 
+    /// * `code` - The binary code to run in the emulator
+    /// * `semihost_func` - A function that will be called when a semihosting command is issued
+    /// * `map` - The memory map configuration
+    /// * `flash_size` - The size of the flash memory
     pub fn new(
         code: &[u8],
         semihost_func: Box<dyn FnMut(&SemihostingCommand) -> SemihostingResponse + 'static>,
@@ -47,7 +52,8 @@ impl GdbServer {
         Ok(GdbServer {target})
     }
 
-    ///
+    /// Start the GDB Server. This function will block until the GDB client disconnects.
+    /// or program execution is complete..
     pub fn start(&mut self) -> Result<u32, &'static str> {
         println!("Starting GDB server");
         let mut exit_code = 0;

zmu_cortex_m/src/gdb/simulation.rs

@@ -14,48 +14,48 @@ use crate::semihosting::SemihostingCommand;
 use crate::semihosting::SemihostingResponse;
 
 ///
-/// Cortex ystem simulation framework
+/// Cortex System simulation framework
 /// 
 pub struct Simulation {
     /// processor state
     pub processor: Processor,
-    ///
+    /// Simulation Execution mode
     pub exec_mode: SimulationExecMode,
-    ///
+    /// Watchpoints
     pub watchpoints: Vec<u32>,
-    ///
+    /// Breakpoints
     pub breakpoints: Vec<u32>,
 }
 
 ///
-/// 
+/// A simulation event.
 /// 
 pub enum SimulationEvent {
-    ///
+    /// Step done
     DoneStep,
-    ///
+    /// Processor is halted
     #[allow(dead_code)]
     Halted,
-    ///
+    /// A breakpoint was hit
     Break,
-    ///
+    /// A write watchpoint was hit
     #[allow(dead_code)]
     WatchWrite(u32),
-    ///
+    /// A read watchpoint was hit
     WatchRead(u32),
-    ///
+    /// Simulation is finalized
     Finalized(u32)
 }
 
 ///
-/// 
+/// Mode of execution for the simulation
 /// 
 pub enum SimulationExecMode {
-    ///
+    /// Step by Step execution (Single Step)
     Step,
-    ///
+    /// Continuous execution
     Continue,
-    ///
+    /// Range Step execution (from start to end)
     RangeStep(u32, u32),
 }
 
@@ -65,18 +65,15 @@ impl Simulation {
     /// 
     pub fn new(code: &[u8],
         semihost_func: Box<dyn FnMut(&SemihostingCommand) -> SemihostingResponse + 'static>,
-        // itm_file: Option<Box<dyn io::Write + 'static>>,
         map: Option<MemoryMapConfig>,
         flash_size: usize,    
     ) -> Result<Simulation, &'static str> {
         let mut processor = Processor::new();
-        // processor.itm(itm_file);
         processor.semihost(Some(semihost_func));
         processor.memory_map(map);
         processor.flash_memory(flash_size, code);
-        //processor.ram_memory(ram_size);
         processor.cache_instructions();
-        processor.running = true; // running
+        processor.running = true; 
         match processor.reset() {
             Ok(_) => {},
             Err(_) => return Err("Error resetting processor"),
@@ -90,17 +87,27 @@ impl Simulation {
         })  
     }
 
-    ///
-    pub fn run(&mut self, mut poll_incomming_data: impl FnMut() -> bool ) -> SimulationRunEvent {
+    /// Run the simulation. This function will block until the simulation is complete.
+    pub fn run(&mut self, mut poll_incoming_data: impl FnMut() -> bool ) -> SimulationRunEvent {
         match self.exec_mode {
             SimulationExecMode::Step => {
                 return SimulationRunEvent::Event(self.step());
             },
             SimulationExecMode::Continue => {
                 let mut cycles = 0;
                 loop {
+                    // The poll_incoming_data function checks if there is any data available 
+                    // to be read from the connection to the GDB server.
+                    // Currently, each call to poll_incoming_data blocks until either data 
+                    // is available or a timeout occurs. This provides a simple mechanism 
+                    // to detect if the GDB client has sent any commands to the server.
+                    // However, this approach is inefficient because the simulation is 
+                    // effectively stalled until either the GDB client sends data or the 
+                    // timeout expires.
+                    // To reduce the performance impact, we only poll for incoming data 
+                    // every 1024 cycles.
                     if cycles % 1024 == 0 {
-                        if poll_incomming_data() {
+                        if poll_incoming_data() {
                             return SimulationRunEvent::IncomingData;
                         }
                     }
@@ -116,7 +123,7 @@ impl Simulation {
                 let mut cycles = 0;
                 loop {
                     if cycles % 1024 == 0 {
-                        if poll_incomming_data() {
+                        if poll_incoming_data() {
                             return SimulationRunEvent::IncomingData;
                         }
                     }
@@ -138,7 +145,7 @@ impl Simulation {
     }
 
     ///
-    /// 
+    /// Reset the simulation
     /// 
     pub fn reset(&mut self) -> Result<(), SimulationError> {
         match self.processor.reset() {
@@ -148,7 +155,7 @@ impl Simulation {
     }
 
     ///
-    /// 
+    /// Single step the simulation
     /// 
     pub fn step(&mut self) -> SimulationEvent {
         if self.processor.running {
@@ -168,12 +175,12 @@ impl Simulation {
 }
 
 ///
-/// 
+/// A simulation run event.
 /// 
 
 pub enum SimulationRunEvent {
-    ///
+    /// Incoming data from the GDB server
     IncomingData,
-    ///
+    /// A simulation event
     Event(SimulationEvent),
 }

zmu_cortex_m/src/gdb/target.rs

@@ -5,6 +5,8 @@ use gdbstub::target::ext::monitor_cmd::MonitorCmd;
 use gdbstub::common::Signal;
 use gdbstub::outputln;
 
+use log::debug;
+
 use crate::MemoryMapConfig;
 use crate::gdb::simulation;
 
@@ -46,11 +48,6 @@ impl ZmuTarget {
         self.simulation.run(poll_incomming_data)
     }
 
-    // pub fn reset(&mut self) -> Result<(), &'static str> {
-    //     self.simulation.reset();
-
-    // }
-
     pub fn step(&mut self) -> SimulationEvent {
         self.simulation.step()
     }
@@ -93,7 +90,7 @@ impl SingleThreadBase for ZmuTarget {
         &mut self,
         regs: &mut gdbstub_arch::arm::reg::ArmCoreRegs,
     ) -> TargetResult<(), Self> {
-        print!("> read_registers");
+        debug!("> read_registers");
         regs.r = self.simulation.processor.r0_12;
         regs.sp = self.simulation.processor.get_r(Reg::SP);
         regs.lr = self.simulation.processor.lr;
@@ -107,7 +104,7 @@ impl SingleThreadBase for ZmuTarget {
         &mut self,
         _regs: &gdbstub_arch::arm::reg::ArmCoreRegs
     ) -> TargetResult<(), Self> {
-        print!("> write_registers");
+        debug!("> write_registers");
         Ok(())
     }
 
@@ -117,7 +114,7 @@ impl SingleThreadBase for ZmuTarget {
         _start_addr: u32,
         data: &mut [u8],
     ) -> TargetResult<usize, Self> {
-        print!("> read_addrs");
+        debug!("> read_addrs");
         data.iter_mut().for_each(|b| *b = 0x55);
         Ok(data.len())
     }
@@ -128,7 +125,7 @@ impl SingleThreadBase for ZmuTarget {
         _start_addr: u32,
         _data: &[u8],
     ) -> TargetResult<(), Self> {
-        print!("> write_addrs");
+        debug!("> write_addrs");
         Ok(())
     }
 
@@ -221,7 +218,7 @@ impl MonitorCmd for ZmuTarget {
         cmd: &[u8],
         mut out: gdbstub::target::ext::monitor_cmd::ConsoleOutput<'_>,
     ) -> Result<(), Self::Error> {
-        print!("> handle_monitor_cmd {:?}", cmd);
+        debug!("> handle_monitor_cmd {:?}", cmd);
         let cmd = core::str::from_utf8(cmd).map_err(|_| "Invalid UTF-8")?;
         match cmd  {
             "reset" => {