From dc917f7463d4f9e5f4d9e6958cce45f9b434a1c6 Mon Sep 17 00:00:00 2001 From: Gerd Zellweger Date: Wed, 3 Nov 2021 23:54:37 -0700 Subject: [PATCH 1/5] Extend GDB test to include Ctrl+C. --- kernel/src/arch/x86_64/gdb/mod.rs | 15 ++++++++------- .../src/arch/x86_64/gdb/single_thread_ops.rs | 8 ++++++-- kernel/src/integration_tests.rs | 18 ++++++++++-------- kernel/tests/integration-test.rs | 16 ++++++++++++++++ 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/kernel/src/arch/x86_64/gdb/mod.rs b/kernel/src/arch/x86_64/gdb/mod.rs index 5b6745291..e2196b746 100644 --- a/kernel/src/arch/x86_64/gdb/mod.rs +++ b/kernel/src/arch/x86_64/gdb/mod.rs @@ -98,8 +98,7 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { loop { gdb_stm = match gdb_stm { GdbStubStateMachine::Idle(mut gdb_stm_inner) => { - //trace!("GdbStubStateMachine::Idle"); - + trace!("GdbStubStateMachine::Idle"); // This means we expect stuff on the serial line (from GDB) // Let's read and react to it: let conn = gdb_stm_inner.borrow_conn(); @@ -119,8 +118,10 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { } } GdbStubStateMachine::CtrlCInterrupt(gdb_stm_inner) => { - match gdb_stm_inner.interrupt_handled(&mut target, Some(ThreadStopReason::DoneStep)) - { + trace!("GdbStubStateMachine::CtrlCInterrupt"); + let reason = stop_reason.take(); + assert_eq!(reason, Some(ThreadStopReason::Signal(Signal::SIGINT))); + match gdb_stm_inner.interrupt_handled(&mut target, reason) { Ok(gdb) => gdb, Err(e) => { error!("gdbstub error {:?}", e); @@ -129,11 +130,11 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { } } GdbStubStateMachine::Disconnected(_gdb_stm_inner) => { - error!("GdbStubStateMachine::Disconnected byebye"); + error!("GdbStubStateMachine::Disconnected"); break; } GdbStubStateMachine::Running(mut gdb_stm_inner) => { - //trace!("GdbStubStateMachine::DeferredStopReason"); + trace!("GdbStubStateMachine::Running"); // If we're here we were running but have stopped now (either // because we hit Ctrl+c in gdb and hence got a serial interrupt @@ -245,7 +246,7 @@ impl KernelDebugger { // Also does some additional stuff like re-enabling the breakpoints. fn determine_stop_reason(&mut self, reason: KCoreStopReason) -> Option> { match reason { - KCoreStopReason::ConnectionInterrupt => Some(ThreadStopReason::Signal(Signal::SIGTRAP)), + KCoreStopReason::ConnectionInterrupt => Some(ThreadStopReason::Signal(Signal::SIGINT)), KCoreStopReason::BreakpointInterrupt => { unimplemented!("Breakpoint interrupt not implemented"); //Some(ThreadStopReason::SwBreak(NonZeroUsize::new(1).unwrap())) diff --git a/kernel/src/arch/x86_64/gdb/single_thread_ops.rs b/kernel/src/arch/x86_64/gdb/single_thread_ops.rs index 4bf5aa1dc..fb2c26072 100644 --- a/kernel/src/arch/x86_64/gdb/single_thread_ops.rs +++ b/kernel/src/arch/x86_64/gdb/single_thread_ops.rs @@ -24,10 +24,14 @@ impl SingleThreadOps for KernelDebugger { self._signal = signal; self.resume_with = Some(ExecMode::Continue); trace!( - "resume: signal = {:?} resume_with = {:?}", + "SingleThreadOps::resume: signal = {:?} resume_with = {:?}", signal, self.resume_with ); + info!( + "SingleThreadOps::resume: signal = {:?} resume_with = {:?}", + signal, self.resume_with + ); // If the target is running under the more advanced GdbStubStateMachine // API, it is possible to "defer" reporting a stop reason to some point @@ -287,7 +291,7 @@ impl SingleThreadSingleStep for KernelDebugger { self._signal = signal; self.resume_with = Some(ExecMode::SingleStep); info!( - "set signal = {:?} resume_with = {:?}", + "SingleThreadSingleStep::step: set signal = {:?} resume_with = {:?}", signal, self.resume_with ); diff --git a/kernel/src/integration_tests.rs b/kernel/src/integration_tests.rs index 49eb59ffa..3ddadea40 100644 --- a/kernel/src/integration_tests.rs +++ b/kernel/src/integration_tests.rs @@ -579,23 +579,25 @@ fn vspace_debug() { fn gdb() { use log::info; - //arch::irq::ioapic_establish_route(0x0, 0x0); - // watchpoint test: let mut watchpoint_trigger: usize = 0; info!("watchpoint_trigger is {}", watchpoint_trigger); watchpoint_trigger = 0xdeadbeef; info!("watchpoint_trigger is {}", watchpoint_trigger); - // step through all of info: + // step through all of info: info!("step"); info!("step"); - //arch::irq::enable(); - //let mut cond = true; - //while cond {} - //cond = false; - //info!("cond is {}", cond); + // Test ctrl+c + arch::irq::ioapic_establish_route(0x0, 0x0); // TODO(api): enables serial irq + arch::irq::enable(); + let mut cond = true; + while cond {} + cond = false; // make sure cond is mut + // (I figured having cond mut is a little less shady + // if we overwrite it with gdb) + info!("cond is {}", cond); // continue until exit: shutdown(ExitReason::Ok); diff --git a/kernel/tests/integration-test.rs b/kernel/tests/integration-test.rs index a2979899f..dc002e525 100644 --- a/kernel/tests/integration-test.rs +++ b/kernel/tests/integration-test.rs @@ -1327,6 +1327,22 @@ fn s02_gdb() { // Test `continue` output += gdb.wait_for_prompt()?.as_str(); gdb.send_line("continue")?; + // We're in the "infinite" loop now, we won't get a prompt... + std::thread::sleep(std::time::Duration::from_millis(300)); + + // Interrupt execution with a Ctrl+C + gdb.send_control('c')?; + output += gdb + .exp_string("Program received signal SIGINT, Interrupt.")? + .as_str(); + output += gdb.wait_for_prompt()?.as_str(); + + // Get out of the loop by overwriting var (test mem/register write) + gdb.send_line("set cond = false")?; + output += gdb.wait_for_prompt()?.as_str(); + + // Should go to the end now... + gdb.send_line("continue")?; output += gdb.exp_string("Remote connection closed")?.as_str(); output += p.exp_eof()?.as_str(); From 15c1d818c8d767f35d89b8a677b1be9879633e0c Mon Sep 17 00:00:00 2001 From: Gerd Zellweger Date: Thu, 4 Nov 2021 00:21:03 -0700 Subject: [PATCH 2/5] Clarify comment. --- kernel/src/arch/x86_64/gdb/mod.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/kernel/src/arch/x86_64/gdb/mod.rs b/kernel/src/arch/x86_64/gdb/mod.rs index e2196b746..31f5dce8a 100644 --- a/kernel/src/arch/x86_64/gdb/mod.rs +++ b/kernel/src/arch/x86_64/gdb/mod.rs @@ -119,9 +119,18 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { } GdbStubStateMachine::CtrlCInterrupt(gdb_stm_inner) => { trace!("GdbStubStateMachine::CtrlCInterrupt"); - let reason = stop_reason.take(); - assert_eq!(reason, Some(ThreadStopReason::Signal(Signal::SIGINT))); - match gdb_stm_inner.interrupt_handled(&mut target, reason) { + let _reason = stop_reason.take(); + // Ideally, this would hold: assert_eq!(reason, + // Some(ThreadStopReason::Signal(Signal::SIGINT))); + // + // But, turns out `reason` can be `StepDone` too if we are + // single-stepping through things and the ctrl+c packet arrives + // while we're in this file. + // + // So we just consume `reason` and report SIGINT unconditionally. + match gdb_stm_inner + .interrupt_handled(&mut target, Some(ThreadStopReason::Signal(Signal::SIGINT))) + { Ok(gdb) => gdb, Err(e) => { error!("gdbstub error {:?}", e); @@ -144,6 +153,8 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { let data_to_read = conn.peek().unwrap().is_some(); if data_to_read { + trace!("GdbStubStateMachine::Running data_to_read"); + let byte = gdb_stm_inner.borrow_conn().read().unwrap(); match gdb_stm_inner.incoming_data(&mut target, byte) { Ok(pumped_stm) => pumped_stm, @@ -157,6 +168,8 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { } } } else if let Some(reason) = stop_reason.take() { + trace!("GdbStubStateMachine::Running stop_reason"); + match gdb_stm_inner.report_stop(&mut target, reason) { Ok(gdb_stm_new) => gdb_stm_new, Err(GdbStubError::TargetError(e)) => { @@ -169,6 +182,8 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { } } } else if target.resume_with.is_some() { + trace!("GdbStubStateMachine::Running target.resume_with.is_some"); + // We don't have a `stop_reason` and we don't have something // to read on the line. This probably means we're done and // we should run again. From a113b0137f6452ec43d22e62be24369ff5ab600c Mon Sep 17 00:00:00 2001 From: Gerd Zellweger Date: Thu, 4 Nov 2021 16:16:58 -0700 Subject: [PATCH 3/5] Enable single register read/write. --- kernel/src/arch/x86_64/gdb/mod.rs | 2 +- kernel/src/arch/x86_64/gdb/single_thread_ops.rs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/kernel/src/arch/x86_64/gdb/mod.rs b/kernel/src/arch/x86_64/gdb/mod.rs index 31f5dce8a..e0f18e80e 100644 --- a/kernel/src/arch/x86_64/gdb/mod.rs +++ b/kernel/src/arch/x86_64/gdb/mod.rs @@ -98,7 +98,7 @@ pub(crate) fn event_loop(reason: KCoreStopReason) -> Result<(), KError> { loop { gdb_stm = match gdb_stm { GdbStubStateMachine::Idle(mut gdb_stm_inner) => { - trace!("GdbStubStateMachine::Idle"); + //trace!("GdbStubStateMachine::Idle"); // This means we expect stuff on the serial line (from GDB) // Let's read and react to it: let conn = gdb_stm_inner.borrow_conn(); diff --git a/kernel/src/arch/x86_64/gdb/single_thread_ops.rs b/kernel/src/arch/x86_64/gdb/single_thread_ops.rs index fb2c26072..cbe5877b4 100644 --- a/kernel/src/arch/x86_64/gdb/single_thread_ops.rs +++ b/kernel/src/arch/x86_64/gdb/single_thread_ops.rs @@ -43,6 +43,7 @@ impl SingleThreadOps for KernelDebugger { &mut self, regs: &mut gdbstub_arch::x86::reg::X86_64CoreRegs, ) -> TargetResult<(), Self> { + trace!("read_registers"); let kcb = super::super::kcb::get_kcb(); if let Some(saved) = &kcb.save_area { // RAX, RBX, RCX, RDX, RSI, RDI, RBP, RSP, r8-r15 @@ -273,13 +274,11 @@ impl SingleThreadOps for KernelDebugger { } fn support_single_register_access(&mut self) -> Option> { - //Some(self) - None + Some(self) } fn support_single_step(&mut self) -> Option> { Some(self) - //None } } From bf936d5c617050c21d2181921fdf341b177706bc Mon Sep 17 00:00:00 2001 From: Gerd Zellweger Date: Thu, 4 Nov 2021 17:14:49 -0700 Subject: [PATCH 4/5] Inital stub for multi-threading support. --- kernel/src/arch/x86_64/gdb/mod.rs | 7 +- .../src/arch/x86_64/gdb/multi_thread_ops.rs | 163 ++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 kernel/src/arch/x86_64/gdb/multi_thread_ops.rs diff --git a/kernel/src/arch/x86_64/gdb/mod.rs b/kernel/src/arch/x86_64/gdb/mod.rs index e0f18e80e..075684e48 100644 --- a/kernel/src/arch/x86_64/gdb/mod.rs +++ b/kernel/src/arch/x86_64/gdb/mod.rs @@ -22,6 +22,7 @@ use super::debug::GDB_REMOTE_PORT; use crate::error::KError; mod breakpoints; +mod multi_thread_ops; mod section_offsets; mod serial; mod single_register; @@ -354,7 +355,11 @@ impl Target for KernelDebugger { type Arch = X86_64_SSE; fn base_ops(&mut self) -> BaseOps { - BaseOps::SingleThread(self) + if cfg!(feature = "bsp-only") { + BaseOps::SingleThread(self) + } else { + BaseOps::MultiThread(self) + } } fn support_section_offsets(&mut self) -> Option> { diff --git a/kernel/src/arch/x86_64/gdb/multi_thread_ops.rs b/kernel/src/arch/x86_64/gdb/multi_thread_ops.rs new file mode 100644 index 000000000..c7bcb417c --- /dev/null +++ b/kernel/src/arch/x86_64/gdb/multi_thread_ops.rs @@ -0,0 +1,163 @@ +#![allow(warnings)] + +use core::convert::TryInto; + +use log::{debug, error, info, trace, warn}; + +use gdbstub::common::{Signal, Tid}; +use gdbstub::target; +use gdbstub::target::ext::base::multithread::{MultiThreadOps, MultiThreadSingleStep}; +use gdbstub::target::ext::breakpoints::WatchKind; +use gdbstub::target::{Target, TargetError, TargetResult}; + +use super::{ExecMode, KernelDebugger}; +use crate::error::KError; +use crate::memory::vspace::AddressSpace; +use crate::memory::{VAddr, BASE_PAGE_SIZE}; + +impl MultiThreadOps for KernelDebugger { + fn resume(&mut self) -> Result<(), Self::Error> { + // Upon returning from the `resume` method, the target being debugged should be + // configured to run according to whatever resume actions the GDB client has + // specified (as specified by `set_resume_action`, `set_resume_range_step`, + // `set_reverse_{step, continue}`, etc...) + // + // In this basic `armv4t_multicore` example, the `resume` method is actually a + // no-op, as the execution mode of the emulator's interpreter loop has already + // been modified via the various `set_X` methods. + // + // In more complex implementations, it's likely that the target being debugged + // will be running in another thread / process, and will require some kind of + // external "orchestration" to set it's execution mode (e.g: modifying the + // target's process state via platform specific debugging syscalls). + + Ok(()) + } + + fn clear_resume_actions(&mut self) -> Result<(), Self::Error> { + //self.exec_mode.clear(); + Ok(()) + } + + #[inline(always)] + fn support_single_step( + &mut self, + ) -> Option> { + Some(self) + } + + fn set_resume_action_continue( + &mut self, + tid: Tid, + signal: Option, + ) -> Result<(), Self::Error> { + /*if signal.is_some() { + return Err(KError::Unknown); + } + + self.exec_mode + .insert(tid_to_cpuid(tid)?, ExecMode::Continue);*/ + + Ok(()) + } + + fn read_registers( + &mut self, + regs: &mut gdbstub_arch::x86::reg::X86_64CoreRegs, + tid: Tid, + ) -> TargetResult<(), Self> { + /* + let cpu = match tid_to_cpuid(tid).map_err(TargetError::Fatal)? { + CpuId::Cpu => &mut self.cpu, + CpuId::Cop => &mut self.cop, + }; + + let mode = cpu.mode(); + + for i in 0..13 { + regs.r[i] = cpu.reg_get(mode, i as u8); + } + regs.sp = cpu.reg_get(mode, reg::SP); + regs.lr = cpu.reg_get(mode, reg::LR); + regs.pc = cpu.reg_get(mode, reg::PC); + regs.cpsr = cpu.reg_get(mode, reg::CPSR);*/ + + Ok(()) + } + + fn write_registers( + &mut self, + regs: &gdbstub_arch::x86::reg::X86_64CoreRegs, + tid: Tid, + ) -> TargetResult<(), Self> { + /*let cpu = match tid_to_cpuid(tid).map_err(TargetError::Fatal)? { + CpuId::Cpu => &mut self.cpu, + CpuId::Cop => &mut self.cop, + }; + + let mode = cpu.mode(); + + for i in 0..13 { + cpu.reg_set(mode, i, regs.r[i as usize]); + } + cpu.reg_set(mode, reg::SP, regs.sp); + cpu.reg_set(mode, reg::LR, regs.lr); + cpu.reg_set(mode, reg::PC, regs.pc); + cpu.reg_set(mode, reg::CPSR, regs.cpsr); + */ + Ok(()) + } + + fn read_addrs( + &mut self, + start_addr: u64, + data: &mut [u8], + _tid: Tid, // same address space for each core + ) -> TargetResult<(), Self> { + /*for (addr, val) in (start_addr..).zip(data.iter_mut()) { + *val = self.mem.r8(addr) + }*/ + Ok(()) + } + + fn write_addrs( + &mut self, + start_addr: u64, + data: &[u8], + _tid: Tid, // same address space for each core + ) -> TargetResult<(), Self> { + /*for (addr, val) in (start_addr..).zip(data.iter().copied()) { + self.mem.w8(addr, val) + }*/ + Ok(()) + } + + fn list_active_threads( + &mut self, + register_thread: &mut dyn FnMut(Tid), + ) -> Result<(), Self::Error> { + //register_thread(cpuid_to_tid(CpuId::Cpu)); + //register_thread(cpuid_to_tid(CpuId::Cop)); + Ok(()) + } +} + +/// Adds `gdbstub` support for single-stepping. +impl MultiThreadSingleStep for KernelDebugger { + fn set_resume_action_step( + &mut self, + _tid: Tid, + signal: Option, + ) -> Result<(), Self::Error> { + assert!(signal.is_none(), "Not supported at the moment."); + + self._signal = signal; + self.resume_with = Some(ExecMode::SingleStep); + info!( + "SingleThreadSingleStep::step: set signal = {:?} resume_with = {:?}", + signal, self.resume_with + ); + + Ok(()) + } +} From 4e49c9fafae4e79c1cb2737a8210202aa9ff4aa1 Mon Sep 17 00:00:00 2001 From: Gerd Zellweger Date: Sat, 4 Jun 2022 21:14:35 -0700 Subject: [PATCH 5/5] Fix compilation. --- kernel/src/integration_tests.rs | 4 ++-- kernel/tests/integration-test.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/src/integration_tests.rs b/kernel/src/integration_tests.rs index 3ddadea40..52fedfc42 100644 --- a/kernel/src/integration_tests.rs +++ b/kernel/src/integration_tests.rs @@ -590,8 +590,8 @@ fn gdb() { info!("step"); // Test ctrl+c - arch::irq::ioapic_establish_route(0x0, 0x0); // TODO(api): enables serial irq - arch::irq::enable(); + crate::arch::irq::ioapic_establish_route(0x0, 0x0); // TODO(api): enables serial irq + crate::arch::irq::enable(); let mut cond = true; while cond {} cond = false; // make sure cond is mut diff --git a/kernel/tests/integration-test.rs b/kernel/tests/integration-test.rs index dc002e525..eae86cc2b 100644 --- a/kernel/tests/integration-test.rs +++ b/kernel/tests/integration-test.rs @@ -1246,7 +1246,7 @@ fn s02_gdb() { /// Spawn the gdb debugger pub fn spawn_gdb(binary: &str) -> Result { // The `-nx` ignores any potential .gdbinit files that may mess with the test - spawn(format!("gdb -nx {}", binary).as_str(), Some(3_000)).and_then(|p| { + spawn(format!("gdb -nx {}", binary).as_str(), Some(5_000)).and_then(|p| { Ok(PtyReplSession { prompt: "(gdb) ".to_string(), pty_session: p,