From 6e45de760eb8ea75d68f003138156e0d1a2ad52e Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Tue, 4 Apr 2023 13:09:34 -0400 Subject: [PATCH] Cleanup new code, add docs and error handling --- src/core/gdbstub/gdbstub.cpp | 28 +++-------- src/core/gdbstub/gdbstub.h | 13 ++++- src/core/gdbstub/hio.cpp | 92 ++++++++++++++++++++++++++---------- src/core/gdbstub/hio.h | 29 ++++++++---- src/core/hle/kernel/svc.cpp | 9 ++-- 5 files changed, 112 insertions(+), 59 deletions(-) diff --git a/src/core/gdbstub/gdbstub.cpp b/src/core/gdbstub/gdbstub.cpp index c8012dda2..0a4f8769c 100644 --- a/src/core/gdbstub/gdbstub.cpp +++ b/src/core/gdbstub/gdbstub.cpp @@ -487,12 +487,7 @@ static void SendPacket(const char packet) { } } -/** - * Send reply to gdb client. - * - * @param reply Reply to be sent to client. - */ -static void SendReply(const char* reply) { +void SendReply(const char* reply) { if (!IsConnected()) { return; } @@ -1048,9 +1043,8 @@ void HandlePacket() { return; } - if (HasPendingHioRequest()) { - const auto request_packet = BuildHioRequestPacket(); - SendReply(request_packet.data()); + if (HandlePendingHioRequestPacket()) { + // Don't do anything else while we wait for the client to respond return; } @@ -1076,21 +1070,13 @@ void HandlePacket() { SendSignal(current_thread, latest_signal); break; case 'k': - ToggleServer(false); - // Continue execution so we don't hang forever after shutting down the - // server - Continue(); LOG_INFO(Debug_GDBStub, "killed by gdb"); + ToggleServer(false); + // Continue execution so we don't hang forever after shutting down the server + Continue(); return; case 'F': - if (HandleHioReply(command_buffer, command_length)) { - // TODO: technically if we were paused when the request came in, we - // shouldn't continue here. Could recurse back into HandlePacket() maybe?? - Continue(); - } else { - // TODO reply with errno if relevant. Maybe that code should live in - // HandleHioReply - } + HandleHioReply(command_buffer, command_length); break; case 'g': ReadRegisters(); diff --git a/src/core/gdbstub/gdbstub.h b/src/core/gdbstub/gdbstub.h index 70aa0e2db..33d6bd8f1 100644 --- a/src/core/gdbstub/gdbstub.h +++ b/src/core/gdbstub/gdbstub.h @@ -90,6 +90,11 @@ 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(); +/** + * If set to true, the CPU will halt at the beginning of the next CPU loop. + * + * @param halt whether to halt on the next loop + */ void SetCpuHaltFlag(bool halt); // If set to true and the CPU is halted, the CPU will step one instruction. @@ -111,7 +116,13 @@ void SetCpuStepFlag(bool is_step); void SendTrap(Kernel::Thread* thread, int trap); /** -u32 HexToInt(const u8* src, std::size_t len) { + * Send reply to gdb client. + * + * @param reply Reply to be sent to client. + */ +void SendReply(const char* reply); + +/** * Converts input hex string characters into an array of equivalent of u8 bytes. * * @param src Pointer to array of output hex string characters. diff --git a/src/core/gdbstub/hio.cpp b/src/core/gdbstub/hio.cpp index 06eb3eeaa..50fd1a0fb 100644 --- a/src/core/gdbstub/hio.cpp +++ b/src/core/gdbstub/hio.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Citra Emulator Project +// Copyright 2023 Citra Emulator Project // Licensed under GPLv2 or any later version // Refer to the license.txt file included. @@ -23,19 +23,52 @@ enum class Status { static std::atomic request_status{Status::NoRequest}; +static std::atomic was_halted = false; +static std::atomic was_stepping = false; + } // namespace +/** + * @return Whether the application has requested I/O, and it has not been sent. + */ +static bool HasPendingHioRequest() { + return current_hio_request_addr != 0 && request_status == Status::NotSent; +} + +/** + * @return Whether the GDB stub is awaiting a reply from the client. + */ +static bool WaitingForHioReply() { + return current_hio_request_addr != 0 && request_status == Status::SentWaitingReply; +} + +/** + * Send a response indicating an error back to the application. + * + * @param error_code The error code to respond back to the app. This typically corresponds to errno. + * + * @param retval The return value of the syscall the app requested. + */ +static void SendErrorReply(int error_code, int retval = -1) { + auto packet = fmt::format("F{:x},{:x}", retval, error_code); + SendReply(packet.data()); +} + void SetHioRequest(const VAddr addr) { if (!IsServerEnabled()) { LOG_WARNING(Debug_GDBStub, "HIO requested but GDB stub is not running"); return; } - if (HasPendingHioRequest() || WaitingForHioReply()) { + if (WaitingForHioReply()) { LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress!"); return; } + if (HasPendingHioRequest()) { + LOG_INFO(Debug_GDBStub, "overwriting existing HIO request that was not sent yet"); + } + auto& memory = Core::System::GetInstance().Memory(); const auto process = Core::System::GetInstance().Kernel().GetCurrentProcess(); @@ -59,6 +92,9 @@ void SetHioRequest(const VAddr addr) { current_hio_request_addr = addr; request_status = Status::NotSent; + was_halted = GetCpuHaltFlag(); + was_stepping = GetCpuStepFlag(); + // Now halt, so that no further instructions are executed until the request // is processed by the client. We will continue after the reply comes back Break(); @@ -67,19 +103,19 @@ void SetHioRequest(const VAddr addr) { Core::GetRunningCore().ClearInstructionCache(); } -bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { +void HandleHioReply(const u8* const command_buffer, const u32 command_length) { if (!WaitingForHioReply()) { LOG_WARNING(Debug_GDBStub, "Got HIO reply but never sent a request"); - // TODO send error reply packet? - return false; + return; } // Skip 'F' header auto* command_pos = command_buffer + 1; if (*command_pos == 0 || *command_pos == ',') { - // return GDB_ReplyErrno(ctx, EILSEQ); - return false; + LOG_WARNING(Debug_GDBStub, "bad HIO packet format position 0: {}", *command_pos); + SendErrorReply(EILSEQ); + return; } // Set the sign of the retval @@ -100,8 +136,8 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { if (command_parts.empty() || command_parts.size() > 3) { LOG_WARNING(Debug_GDBStub, "unexpected reply packet size: {}", command_parts); - // return GDB_ReplyErrno(ctx, EILSEQ); - return false; + SendErrorReply(EILSEQ); + return; } u64 unsigned_retval = HexToInt((u8*)command_parts[0].data(), command_parts[0].size()); @@ -119,8 +155,9 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { if (command_parts.size() > 2 && !command_parts[2].empty()) { if (command_parts[2][0] != 'C') { - return false; - // return GDB_ReplyErrno(ctx, EILSEQ); + LOG_WARNING(Debug_GDBStub, "expected ctrl-c flag got '{}'", command_parts[2][0]); + SendErrorReply(EILSEQ); + return; } // for now we just ignore any trailing ";..." attachments @@ -142,7 +179,7 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { if (!memory.IsValidVirtualAddress(*process, current_hio_request_addr)) { LOG_WARNING(Debug_GDBStub, "Invalid address {:#X} to write HIO reply", current_hio_request_addr); - return false; + return; } memory.WriteBlock(*process, current_hio_request_addr, ¤t_hio_request, @@ -152,18 +189,22 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) { current_hio_request_addr = 0; request_status = Status::NoRequest; - return true; + // Restore state from before the request came in + SetCpuStepFlag(was_stepping); + SetCpuHaltFlag(was_halted); + Core::GetRunningCore().ClearInstructionCache(); } -bool HasPendingHioRequest() { - return current_hio_request_addr != 0 && request_status == Status::NotSent; -} +bool HandlePendingHioRequestPacket() { + if (!HasPendingHioRequest()) { + return false; + } -bool WaitingForHioReply() { - return current_hio_request_addr != 0 && request_status == Status::SentWaitingReply; -} + if (WaitingForHioReply()) { + // We already sent it, continue waiting for a reply + return true; + } -std::string BuildHioRequestPacket() { std::string packet = fmt::format("F{}", current_hio_request.function_name); // TODO: should we use the IntToGdbHex funcs instead of fmt::format_to ? @@ -192,17 +233,18 @@ std::string BuildHioRequestPacket() { break; default: - // TODO: early return? Or break out of this loop? - break; + LOG_WARNING(Debug_GDBStub, "unexpected hio request param format '{}'", + current_hio_request.param_format[i]); + SendErrorReply(EILSEQ); + return false; } } LOG_TRACE(Debug_GDBStub, "HIO request packet: {}", packet); - // technically we should set this _after_ the reply is sent... + SendReply(packet.data()); request_status = Status::SentWaitingReply; - - return packet; + return true; } } // namespace GDBStub diff --git a/src/core/gdbstub/hio.h b/src/core/gdbstub/hio.h index ec6804df0..dda4e8ef1 100644 --- a/src/core/gdbstub/hio.h +++ b/src/core/gdbstub/hio.h @@ -1,4 +1,4 @@ -// Copyright 2022 Citra Emulator Project +// Copyright 2023 Citra Emulator Project // Licensed under GPLv2 or any later version // Refer to the license.txt file included. @@ -11,11 +11,14 @@ namespace GDBStub { /** + * A request from a debugged application to perform some I/O with the GDB client. + * This structure is also used to encode the reply back to the application. + * * Based on the Rosalina implementation: * https://github.com/LumaTeam/Luma3DS/blob/master/sysmodules/rosalina/include/gdb.h#L46C27-L62 */ struct PackedGdbHioRequest { - char magic[4]; // "GDB\x00" + char magic[4]; // "GDB\0" u32 version; // Request @@ -31,14 +34,24 @@ struct PackedGdbHioRequest { bool ctrl_c; }; +/** + * Set the current HIO request to the given address. This is how the debugged + * app indicates to the gdbstub that it wishes to perform a request. + * + * @param address The memory address of the \ref PackedGdbHioRequest. + */ void SetHioRequest(const VAddr address); -bool HandleHioReply(const u8* const command_buffer, const u32 command_length); +/** + * If there is a pending HIO request, send it to the client. + * + * @returns whethere any request was sent to the client. + */ +bool HandlePendingHioRequestPacket(); -bool HasPendingHioRequest(); - -bool WaitingForHioReply(); - -std::string BuildHioRequestPacket(); +/** + * Process an HIO reply from the client. + */ +void HandleHioReply(const u8* const command_buffer, const u32 command_length); } // namespace GDBStub diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index e2ddb1e91..6bdc72535 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -1141,8 +1141,8 @@ void SVC::Break(u8 break_reason) { system.SetStatus(Core::System::ResultStatus::ErrorUnknown); } -/// Used to output a message on a debug hardware unit, or for the GDB HIO -// protocol - does nothing on a retail unit. +/// Used to output a message on a debug hardware unit, or for the GDB file I/O +/// (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); @@ -1154,7 +1154,8 @@ void SVC::OutputDebugString(VAddr address, s32 len) { return; } - if (len <= 0) { + if (len < 0) { + LOG_WARNING(Kernel_SVC, "OutputDebugString called with invalid length {}", len); return; } @@ -2224,7 +2225,7 @@ const std::array SVC::SVC_Table{{ {0x60, nullptr, "DebugActiveProcess"}, {0x61, nullptr, "BreakDebugProcess"}, {0x62, nullptr, "TerminateDebugProcess"}, - {0x63, nullptr, "GetProcessDebugEvent"}, // TODO: do we need this for HIO to work? + {0x63, nullptr, "GetProcessDebugEvent"}, {0x64, nullptr, "ContinueDebugEvent"}, {0x65, &SVC::Wrap<&SVC::GetProcessList>, "GetProcessList"}, {0x66, nullptr, "GetThreadList"},