From f92f494cab96856b45d6e4f3fe94e84b46489a76 Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Sun, 4 Dec 2022 15:34:06 -0500 Subject: [PATCH] Stop execution when performing HIO request Since the HIO request is basically treated like a syscall, we need to stop emulation while waiting for the GDB client to reply with the result. This ensures any memory queries etc. that GDB makes to fulfill the HIO request are accessing memory as it was at the time of the request, instead of afterwards. --- src/core/gdbstub/gdbstub.cpp | 38 +++++++++++++++---------------- src/core/gdbstub/gdbstub.h | 2 ++ src/core/gdbstub/hio.cpp | 44 ++++++++++++++++++++++++------------ src/core/gdbstub/hio.h | 6 ++++- src/core/hle/kernel/svc.cpp | 5 ++++ 5 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/core/gdbstub/gdbstub.cpp b/src/core/gdbstub/gdbstub.cpp index f6f5c4154..d700309a5 100644 --- a/src/core/gdbstub/gdbstub.cpp +++ b/src/core/gdbstub/gdbstub.cpp @@ -638,7 +638,7 @@ static void ReadCommand() { memset(command_buffer, 0, sizeof(command_buffer)); u8 c = ReadByte(); - if (c == '+') { + if (c == GDB_STUB_ACK) { // ignore ack return; } else if (c == 0x03) { @@ -828,7 +828,7 @@ static void ReadMemory() { u32 len = HexToInt(start_offset, static_cast((command_buffer + command_length) - start_offset)); - LOG_DEBUG(Debug_GDBStub, "gdb: addr: {:08x} len: {:08x}\n", addr, len); + LOG_DEBUG(Debug_GDBStub, "ReadMemory addr: {:08x} len: {:08x}", addr, len); if (len * 2 > sizeof(reply)) { SendReply("E01"); @@ -845,6 +845,9 @@ static void ReadMemory() { MemToGdbHex(reply, data.data(), len); reply[len * 2] = '\0'; + + LOG_DEBUG(Debug_GDBStub, "ReadMemory result: {}", reply); + SendReply(reinterpret_cast(reply)); } @@ -1046,21 +1049,6 @@ void HandlePacket() { LOG_DEBUG(Debug_GDBStub, "Packet: {}", command_buffer[0]); - // HACK: instead of polling DebugEvents properly via SVC, just check for - // whether there's a pending a request, and send it if so. - // ...This doesn't seem good enough for the general case - switch (command_buffer[0]) { - case 'c': - case 'C': - case 's': - if (HasHioRequest()) { - // Really, this request needs to get sent _after_ the step or continue - // began, but not sure how to schedule that... - const auto request_packet = BuildHioRequestPacket(); - SendReply(request_packet.data()); - } - } - switch (command_buffer[0]) { case 'q': HandleQuery(); @@ -1077,8 +1065,8 @@ void HandlePacket() { return; case 'F': if (HandleHioReply(command_buffer, command_length)) { - Continue(); - }; + // TODO figure out how to "resume" the last command + } break; case 'g': ReadRegisters(); @@ -1256,6 +1244,10 @@ bool GetCpuHaltFlag() { return halt_loop; } +void SetCpuHaltFlag(bool halt) { + halt_loop = halt; +} + bool GetCpuStepFlag() { return step_loop; } @@ -1270,7 +1262,13 @@ void SendTrap(Kernel::Thread* thread, int trap) { } current_thread = thread; - SendSignal(thread, trap); + + if (HasPendingHioRequest()) { + const auto request_packet = BuildHioRequestPacket(); + SendReply(request_packet.data()); + } else { + SendSignal(thread, trap); + } halt_loop = true; send_trap = false; diff --git a/src/core/gdbstub/gdbstub.h b/src/core/gdbstub/gdbstub.h index 2eb092758..e11d31324 100644 --- a/src/core/gdbstub/gdbstub.h +++ b/src/core/gdbstub/gdbstub.h @@ -90,6 +90,8 @@ bool CheckBreakpoint(VAddr addr, GDBStub::BreakpointType type); // If set to true, the CPU will halt at the beginning of the next CPU loop. bool GetCpuHaltFlag(); +void SetCpuHaltFlag(bool halt); + // If set to true and the CPU is halted, the CPU will step one instruction. bool GetCpuStepFlag(); diff --git a/src/core/gdbstub/hio.cpp b/src/core/gdbstub/hio.cpp index 7e3c6d61e..6be94b7e5 100644 --- a/src/core/gdbstub/hio.cpp +++ b/src/core/gdbstub/hio.cpp @@ -12,7 +12,14 @@ namespace { static VAddr current_hio_request_addr; static PackedGdbHioRequest current_hio_request; -static std::atomic sent_request{false}; + +enum class Status { + NoRequest, + NotSent, + SentWaitingReply, +}; + +static std::atomic request_status{Status::NoRequest}; } // namespace @@ -40,15 +47,23 @@ void SetHioRequest(const VAddr addr) { LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application"); current_hio_request_addr = 0; current_hio_request = {}; + request_status = Status::NoRequest; } else { + LOG_DEBUG(Debug_GDBStub, "HIO request initiated at 0x{:X}", addr); current_hio_request_addr = addr; - sent_request = false; - LOG_DEBUG(Debug_GDBStub, "HIO request initiated"); + request_status = Status::NotSent; + + // Now halt, so that no further instructions are executed until the request + // is processed by the client. We auto-continue after the reply comes back + Break(); + SetCpuHaltFlag(true); + SetCpuStepFlag(false); + Core::GetRunningCore().ClearInstructionCache(); } } bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { - if (current_hio_request_addr == 0 || !sent_request) { + if (!WaitingForHioReply()) { LOG_WARNING(Debug_GDBStub, "Got HIO reply but never sent a request"); // TODO send error reply packet? return false; @@ -73,10 +88,6 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { } const auto retval_end = std::find(command_pos, command_buffer + command_length, ','); - if (retval_end >= command_buffer + command_length) { - // return GDB_ReplyErrno(ctx, EILSEQ); - return false; - } u64 retval = (u64)HexToInt(command_pos, retval_end - command_pos); command_pos = retval_end + 1; @@ -86,7 +97,7 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { if (*command_pos != 0) { u32 errno_; - // GDB protocol technically allows errno to have a +/- prefix but this will never happen. + // GDB protocol technically allows errno to have a +/- prefix but this should never happen. const auto errno_end = std::find(command_pos, command_buffer + command_length, ','); errno_ = HexToInt(command_pos, errno_end - command_pos); command_pos = errno_end + 1; @@ -123,15 +134,19 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { memory.WriteBlock(*process, current_hio_request_addr, ¤t_hio_request, sizeof(PackedGdbHioRequest)); - current_hio_request = PackedGdbHioRequest{}; + current_hio_request = {}; current_hio_request_addr = 0; - sent_request = false; + request_status = Status::NoRequest; return true; } -bool HasHioRequest() { - return current_hio_request_addr != 0 && !sent_request; +bool HasPendingHioRequest() { + return current_hio_request_addr != 0 && request_status == Status::NotSent; +} + +bool WaitingForHioReply() { + return current_hio_request_addr != 0 && request_status == Status::SentWaitingReply; } std::string BuildHioRequestPacket() { @@ -163,7 +178,8 @@ std::string BuildHioRequestPacket() { } LOG_DEBUG(Debug_GDBStub, "HIO request packet: {}", packet.str()); - sent_request = true; + + request_status = Status::SentWaitingReply; return packet.str(); } diff --git a/src/core/gdbstub/hio.h b/src/core/gdbstub/hio.h index 5fbf45d93..58c7d1e66 100644 --- a/src/core/gdbstub/hio.h +++ b/src/core/gdbstub/hio.h @@ -4,6 +4,8 @@ #pragma once +#include + #include "common/common_types.h" namespace GDBStub { @@ -29,7 +31,9 @@ void SetHioRequest(const VAddr address); bool HandleHioReply(const u8* const command_buffer, const u32 command_length); -bool HasHioRequest(); +bool HasPendingHioRequest(); + +bool WaitingForHioReply(); std::string BuildHioRequestPacket(); diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index 8157d92ea..f3f00a8b7 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -1144,6 +1144,11 @@ void SVC::Break(u8 break_reason) { /// Used to output a message on a debug hardware unit, or for the GDB HIO // protocol - does nothing on a retail unit. void SVC::OutputDebugString(VAddr address, s32 len) { + if (!Memory::IsValidVirtualAddress(*kernel.GetCurrentProcess(), address)) { + LOG_WARNING(Kernel_SVC, "OutputDebugString called with invalid address {:X}", address); + return; + } + if (len == 0) { GDBStub::SetHioRequest(address); return;