[REBASE & FF] patina_debugger: Fix use of crate without "alloc" and fix associated issues

core/patina_debugger/src/arch/x64.rs

@@ -197,10 +197,7 @@ impl DebuggerArch for X64Arch {
                     let addr = match u64::from_str_radix(val.trim_start_matches("0x"), 16) {
                         Ok(a) => a,
                         Err(_) => {
-                            let _ = out.write_str(
-                                alloc::format!("Invalid address format: '{val}'. Expected hex address (e.g. 0x1000).")
-                                    .as_str(),
-                            );
+                            let _ = write!(out, "Invalid address format: '{val}'. Expected hex address (e.g. 0x1000).");
                             return;
                         }
                     };

core/patina_debugger/src/dbg_target/monitor.rs

@@ -13,7 +13,10 @@
 use core::{fmt::Write, str::SplitWhitespace};
 use gdbstub::target::ext::{self, monitor_cmd::ConsoleOutput};
 
-use crate::arch::{DebuggerArch, SystemArch};
+use crate::{
+    arch::{DebuggerArch, SystemArch},
+    system::SystemStateTrait,
+};
 
 use super::PatinaTarget;
 
@@ -59,9 +62,7 @@ impl ext::monitor_cmd::MonitorCmd for PatinaTarget {
                 let _ = buf.write_str(MONITOR_HELP);
                 let _ = buf.write_str("External commands:\n");
                 if let Some(state) = self.system_state.try_lock() {
-                    for cmd in state.monitor_commands.iter() {
-                        let _ = writeln!(buf, "    {} - {}", cmd.command, cmd.description);
-                    }
+                    state.dump_monitor_commands(&mut buf);
                 };
             }
             Some("mod") => {
@@ -119,38 +120,24 @@ impl PatinaTarget {
 
         match tokens.next() {
             Some("breakall") => {
-                state.modules.break_on_all();
+                state.set_module_breakpoint_all();
                 let _ = out.write_str("Will break for all module loads.");
             }
-            #[cfg(feature = "alloc")]
             Some("break") => {
                 for module in tokens.by_ref() {
-                    state.modules.add_module_breakpoint(module);
+                    state.add_module_breakpoint(module);
                 }
                 let _ = out.write_str("Module breakpoints:\n");
-                for module in state.modules.get_module_breakpoints().iter() {
-                    let _ = writeln!(out, "\t{module}");
-                }
-            }
-            #[cfg(not(feature = "alloc"))]
-            Some("break") => {
-                let _ = out.write_str("Specific Module breakpoints only supported with 'alloc' feature.");
+                state.dump_module_breakpoints(out);
             }
             Some("clear") => {
-                state.modules.clear_module_breakpoints();
+                state.clear_module_breakpoints();
                 let _ = out.write_str("Cleared module breaks!");
             }
             Some("list") => {
                 let count: usize = tokens.next().and_then(|token| token.parse().ok()).unwrap_or(usize::MAX);
                 let start: usize = tokens.next().and_then(|token| token.parse().ok()).unwrap_or(0);
-                let mut printed = 0;
-                for module in state.modules.get_modules().iter().skip(start) {
-                    let _ = writeln!(out, "\t{}: {:#x} : {:#x}", module.name, module.base, module.size);
-                    printed += 1;
-                    if printed >= count {
-                        break;
-                    }
-                }
+                let printed = state.dump_modules(out, start, count);
 
                 if printed == 0 {
                     let _ = out.write_str("No modules.");

core/patina_debugger/src/debugger.rs

@@ -11,7 +11,10 @@
 //! SPDX-License-Identifier: Apache-2.0
 //!
 
-use core::sync::atomic::{AtomicBool, Ordering};
+use core::{
+    ptr::NonNull,
+    sync::atomic::{AtomicBool, Ordering},
+};
 
 #[cfg(feature = "alloc")]
 use alloc::boxed::Box;
@@ -27,7 +30,7 @@ use crate::{
     DebugError, Debugger, DebuggerLoggingPolicy, ExceptionInfo,
     arch::{DebuggerArch, SystemArch},
     dbg_target::PatinaTarget,
-    system::SystemState,
+    system::{SystemState, SystemStateTrait},
     transport::{LoggingSuspender, SerialConnection},
 };
 
@@ -38,8 +41,18 @@ const GDB_BUFF_LEN: usize = 0x2000;
 const GDB_STOP_PACKET: &[u8] = b"$T05thread:01;#07";
 const GDB_NACK_PACKET: &[u8] = b"-";
 
-#[cfg(not(feature = "alloc"))]
-static GDB_BUFFER: [u8; GDB_BUFF_LEN] = [0; GDB_BUFF_LEN];
+cfg_if::cfg_if! {
+    if #[cfg(not(feature = "alloc"))] {
+        /// Static buffer for GDB communication when the `alloc` feature is not enabled.
+        static GDB_BUFFER: StaticGdbBuffer = StaticGdbBuffer(core::cell::UnsafeCell::new([0; GDB_BUFF_LEN]));
+
+        /// Wrapper for the static struct for mutability.
+        struct StaticGdbBuffer(core::cell::UnsafeCell<[u8; GDB_BUFF_LEN]>);
+
+        /// Safety: The buffer is just memory, the use of which is controlled by the debugger internal state lock.
+        unsafe impl Sync for StaticGdbBuffer {}
+    }
+}
 
 // SAFETY: The exception info is not actually stored globally, but this is needed to satisfy
 // the compiler as it will be a contained within the target struct which the GdbStub
@@ -80,6 +93,9 @@ where
     connection_timed_out: AtomicBool,
 }
 
+/// Safety: Send is safe by default for all but the gdb_buffer, but this is just a raw buffer and is safe to send between threads.
+unsafe impl<T: SerialIO> Send for DebuggerInternal<'static, T> {}
+
 /// Internal Debugger State
 ///
 /// contains the internal configuration and state for the debugger. This will
@@ -90,7 +106,7 @@ where
     T: SerialIO,
 {
     gdb: Option<GdbStubStateMachine<'a, PatinaTarget, SerialConnection<'a, T>>>,
-    gdb_buffer: Option<&'a [u8; GDB_BUFF_LEN]>,
+    gdb_buffer: Option<NonNull<[u8; GDB_BUFF_LEN]>>,
     timer: Option<&'a dyn ArchTimerFunctionality>,
     initial_breakpoint: bool,
 }
@@ -194,22 +210,19 @@ impl<T: SerialIO> PatinaDebugger<T> {
         let mut gdb = match debug.gdb {
             Some(_) => debug.gdb.take().unwrap(),
             None => {
-                let const_buffer = debug.gdb_buffer.ok_or(DebugError::NotInitialized)?;
-
                 // Flush any stale data from the transport.
                 while self.transport.try_read().is_some() {}
 
                 // SAFETY: The buffer will only ever be used by the paired GDB stub
                 // within the internal state lock. Because there is no GDB stub at
                 // this point, there is no other references to the buffer. This
                 // ensures a single locked mutable reference to the buffer.
-                let mut_buffer =
-                    unsafe { core::slice::from_raw_parts_mut(const_buffer.as_ptr() as *mut u8, const_buffer.len()) };
+                let gdb_buffer = unsafe { debug.gdb_buffer.ok_or(DebugError::NotInitialized)?.as_mut() };
 
                 let conn = SerialConnection::new(&self.transport);
 
                 let builder = GdbStubBuilder::new(conn)
-                    .with_packet_buffer(mut_buffer)
+                    .with_packet_buffer(gdb_buffer)
                     .build()
                     .map_err(|_| DebugError::GdbStubInit)?;
 
@@ -343,11 +356,11 @@ impl<T: SerialIO> Debugger for PatinaDebugger<T> {
             cfg_if::cfg_if! {
                 if #[cfg(feature = "alloc")] {
                     if internal.gdb_buffer.is_none() {
-                        internal.gdb_buffer = Some(Box::leak(Box::new([0u8; GDB_BUFF_LEN])));
+                        internal.gdb_buffer = Some(NonNull::new(Box::leak(Box::new([0u8; GDB_BUFF_LEN]))).unwrap());
                     }
                 }
                 else {
-                    internal.gdb_buffer = unsafe { Some(&*(GDB_BUFFER.as_ptr() as *mut [u8; GDB_BUFF_LEN])) };
+                    internal.gdb_buffer = Some(NonNull::new(GDB_BUFFER.0.get()).unwrap());
                 }
             }
 
@@ -397,8 +410,8 @@ impl<T: SerialIO> Debugger for PatinaDebugger<T> {
 
         let breakpoint = {
             let mut state = self.system_state.lock();
-            state.modules.add_module(module_name, address, length);
-            state.modules.check_module_breakpoints(module_name)
+            state.add_module(module_name, address, length);
+            state.check_module_breakpoints(module_name)
         };
 
         if breakpoint {
@@ -422,6 +435,7 @@ impl<T: SerialIO> Debugger for PatinaDebugger<T> {
         }
     }
 
+    #[cfg(feature = "alloc")]
     fn add_monitor_command(
         &'static self,
         command: &'static str,

core/patina_debugger/src/lib.rs

@@ -115,6 +115,7 @@ mod memory;
 mod system;
 mod transport;
 
+#[cfg(any(feature = "alloc", test))]
 extern crate alloc;
 
 pub use debugger::PatinaDebugger;
@@ -141,9 +142,10 @@ static DEBUGGER: spin::Once<&dyn Debugger> = spin::Once::new();
 /// For example, if the command is `my_command arg1 arg2`, then `arg1` and `arg2` will
 /// be the first and second elements of the iterator respectively.
 ///
-/// the second argument is a writer that should be used to write the output of the
+/// The second argument is a writer that should be used to write the output of the
 /// command. This can be done by directly invoking the [core::fmt::Write] trait methods
-/// or using the `write!` macro.
+/// or using the `write!` macro. `format!` should be avoided as it will allocate memory
+/// which shouldn't be done in debugger when possible.
 pub type MonitorCommandFn = dyn Fn(&mut core::str::SplitWhitespace<'_>, &mut dyn core::fmt::Write) + Send + Sync;
 
 /// Trait for debugger interaction. This is required to allow for a global to the
@@ -172,6 +174,7 @@ trait Debugger: Sync {
     /// Polls the debugger for any pending interrupts.
     fn poll_debugger(&'static self);
 
+    #[cfg(feature = "alloc")]
     fn add_monitor_command(
         &'static self,
         command: &'static str,
@@ -187,7 +190,7 @@ enum DebugError {
     Reentry,
     /// The debugger configuration is locked. This indicates a failure during debugger configuration.
     ConfigLocked,
-    /// The debugger was invoked without being fuly initialized.
+    /// The debugger was invoked without being fully initialized.
     NotInitialized,
     /// Failure from the GDB stub initialization.
     GdbStubInit,
@@ -325,14 +328,12 @@ where
 /// ```
 ///
 #[cfg(not(feature = "alloc"))]
-pub fn add_monitor_command<F>(cmd: &'static str, description: &'static str, function: F)
+pub fn add_monitor_command<F>(cmd: &'static str, _description: &'static str, _function: F)
 where
     F: Fn(&mut core::str::SplitWhitespace<'_>, &mut dyn core::fmt::Write) + Send + Sync + 'static,
 {
-    if let Some(debugger) = DEBUGGER.get() {
-        log::warn!(
-            "Monitor commands are only supported with the 'alloc' feature enabled. Will not add command: {command}"
-        );
+    if let Some(_) = DEBUGGER.get() {
+        log::warn!("Dynamic monitor commands require the 'alloc' feature. Will not add command: {cmd}");
     }
 }