From acaca4188e62820242f1ee902400bde4d8350dcd Mon Sep 17 00:00:00 2001 From: Dimitri A Date: Fri, 8 Mar 2019 06:09:06 +0100 Subject: [PATCH] gdbstub: Fix some bugs in IsMemoryBreak() and ServeBreak. Add workaround to let watchpoints break into GDB. (#4651) * gdbstub: fix IsMemoryBreak() returning false while connected to client As a result, the only existing codepath for a memory watchpoint hit to break into GDB (InterpeterMainLoop, GDB_BP_CHECK, ARMul_State::RecordBreak) is finally taken, which exposes incorrect logic* in both RecordBreak and ServeBreak. * a blank BreakpointAddress structure is passed, which sets r15 (PC) to NULL * gdbstub: DynCom: default-initialize two members/vars used in conditionals * gdbstub: DynCom: don't record memory watchpoint hits via RecordBreak() For now, instead check for GDBStub::IsMemoryBreak() in InterpreterMainLoop and ServeBreak. Fixes PC being set to a stale/unhit breakpoint address (often zero) when a memory watchpoint (rwatch, watch, awatch) is handled in ServeBreak() and generates a GDB trap. Reasons for removing a call to RecordBreak() for memory watchpoints: * The``breakpoint_data`` we pass is typed Execute or None. It describes the predicted next code breakpoint hit relative to PC; * GDBStub::IsMemoryBreak() returns true if a recent Read/Write operation hit a watchpoint. It doesn't specify which in return, nor does it trace it anywhere. Thus, the only data we could give RecordBreak() is a placeholder BreakpointAddress at offset NULL and type Access. I found the idea silly, compared to simply relying on GDBStub::IsMemoryBreak(). There is currently no measure in the code that remembers the addresses (and types) of any watchpoints that were hit by an instruction, in order to send them to GDB as "extended stop information." I'm considering an implementation for this. * gdbstub: Change an ASSERT to DEBUG_ASSERT I have never seen the (Reg[15] == last_bkpt.address) assert fail in practice, even after several weeks of (locally) developping various branches around GDB. Only leave it inside Debug builds. --- src/core/arm/dyncom/arm_dyncom_interpreter.cpp | 8 ++++++-- src/core/arm/skyeye_common/armstate.cpp | 8 +++++--- src/core/arm/skyeye_common/armstate.h | 2 +- src/core/gdbstub/gdbstub.cpp | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/core/arm/dyncom/arm_dyncom_interpreter.cpp b/src/core/arm/dyncom/arm_dyncom_interpreter.cpp index 9de8dddab4..a3a36c43bb 100644 --- a/src/core/arm/dyncom/arm_dyncom_interpreter.cpp +++ b/src/core/arm/dyncom/arm_dyncom_interpreter.cpp @@ -923,7 +923,9 @@ MICROPROFILE_DEFINE(DynCom_Execute, "DynCom", "Execute", MP_RGB(255, 0, 0)); unsigned InterpreterMainLoop(ARMul_State* cpu) { MICROPROFILE_SCOPE(DynCom_Execute); + /// Nearest upcoming GDB code execution breakpoint, relative to the last dispatch's address. GDBStub::BreakpointAddress breakpoint_data; + breakpoint_data.type = GDBStub::BreakpointType::None; #undef RM #undef RS @@ -955,8 +957,10 @@ unsigned InterpreterMainLoop(ARMul_State* cpu) { cpu->Cpsr &= ~(1 << 5); \ cpu->Cpsr |= cpu->TFlag << 5; \ if (GDBStub::IsServerEnabled()) { \ - if (GDBStub::IsMemoryBreak() || (breakpoint_data.type != GDBStub::BreakpointType::None && \ - PC == breakpoint_data.address)) { \ + if (GDBStub::IsMemoryBreak()) { \ + goto END; \ + } else if (breakpoint_data.type != GDBStub::BreakpointType::None && \ + PC == breakpoint_data.address) { \ cpu->RecordBreak(breakpoint_data); \ goto END; \ } \ diff --git a/src/core/arm/skyeye_common/armstate.cpp b/src/core/arm/skyeye_common/armstate.cpp index 3bd4d28f1c..26520da766 100644 --- a/src/core/arm/skyeye_common/armstate.cpp +++ b/src/core/arm/skyeye_common/armstate.cpp @@ -602,13 +602,15 @@ void ARMul_State::ServeBreak() { return; } - if (last_bkpt_hit) { - Reg[15] = last_bkpt.address; + if (last_bkpt_hit && last_bkpt.type == GDBStub::BreakpointType::Execute) { + DEBUG_ASSERT(Reg[15] == last_bkpt.address); } + DEBUG_ASSERT(system != nullptr); Kernel::Thread* thread = system->Kernel().GetThreadManager().GetCurrentThread(); system->CPU().SaveContext(thread->context); - if (last_bkpt_hit || GDBStub::GetCpuStepFlag()) { + + if (last_bkpt_hit || GDBStub::IsMemoryBreak() || GDBStub::GetCpuStepFlag()) { last_bkpt_hit = false; GDBStub::Break(); GDBStub::SendTrap(thread, 5); diff --git a/src/core/arm/skyeye_common/armstate.h b/src/core/arm/skyeye_common/armstate.h index eedc68c695..7a04e929ab 100644 --- a/src/core/arm/skyeye_common/armstate.h +++ b/src/core/arm/skyeye_common/armstate.h @@ -268,5 +268,5 @@ private: bool exclusive_state; GDBStub::BreakpointAddress last_bkpt{}; - bool last_bkpt_hit; + bool last_bkpt_hit = false; }; diff --git a/src/core/gdbstub/gdbstub.cpp b/src/core/gdbstub/gdbstub.cpp index d7649227fb..7f722ab0fb 100644 --- a/src/core/gdbstub/gdbstub.cpp +++ b/src/core/gdbstub/gdbstub.cpp @@ -892,7 +892,7 @@ static void Step() { } bool IsMemoryBreak() { - if (IsConnected()) { + if (!IsConnected()) { return false; }