Cleanup new code, add docs and error handling

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();

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.

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<Status> request_status{Status::NoRequest};
 
+static std::atomic<bool> was_halted = false;
+static std::atomic<bool> 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, &current_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

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

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::FunctionDef, 180> 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"},