Fix some build + impl errors with HIO

After rebase there were some API changes, also fix continuation logic in
the main gdbstub.

src/core/gdbstub/gdbstub.cpp

@@ -846,7 +846,7 @@ static void ReadMemory() {
     MemToGdbHex(reply, data.data(), len);
     reply[len * 2] = '\0';
 
-    LOG_DEBUG(Debug_GDBStub, "ReadMemory result: {}", reply);
+    LOG_DEBUG(Debug_GDBStub, "ReadMemory result: {}", (char*)reply);
 
     SendReply(reinterpret_cast<char*>(reply));
 }
@@ -1038,6 +1038,12 @@ void HandlePacket() {
         return;
     }
 
+    if (HasPendingHioRequest()) {
+        const auto request_packet = BuildHioRequestPacket();
+        SendReply(request_packet.data());
+        return;
+    }
+
     if (!IsDataAvailable()) {
         return;
     }
@@ -1047,7 +1053,7 @@ void HandlePacket() {
         return;
     }
 
-    LOG_DEBUG(Debug_GDBStub, "Packet: {}", command_buffer[0]);
+    LOG_DEBUG(Debug_GDBStub, "Packet: {0:d} ('{0:c}')", command_buffer[0]);
 
     switch (command_buffer[0]) {
     case 'q':
@@ -1065,7 +1071,12 @@ void HandlePacket() {
         return;
     case 'F':
         if (HandleHioReply(command_buffer, command_length)) {
-            // TODO figure out how to "resume" the last command
+            // TODO: technically if we were paused when the reply 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
         }
         break;
     case 'g':
@@ -1263,12 +1274,7 @@ void SendTrap(Kernel::Thread* thread, int trap) {
 
     current_thread = thread;
 
-    if (HasPendingHioRequest()) {
-        const auto request_packet = BuildHioRequestPacket();
-        SendReply(request_packet.data());
-    } else {
     SendSignal(thread, trap);
-    }
 
     halt_loop = true;
     send_trap = false;

src/core/gdbstub/hio.cpp

@@ -2,6 +2,8 @@
 // Licensed under GPLv2 or any later version
 // Refer to the license.txt file included.
 
+#include <fmt/ranges.h>
+#include "common/string_util.h"
 #include "core/core.h"
 #include "core/gdbstub/gdbstub.h"
 #include "core/gdbstub/hio.h"
@@ -29,38 +31,41 @@ void SetHioRequest(const VAddr addr) {
         return;
     }
 
-    if (current_hio_request_addr != 0) {
+    if (HasPendingHioRequest() || WaitingForHioReply()) {
         LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress!");
         return;
     }
 
+    auto& memory = Core::System::GetInstance().Memory();
     const auto process = Core::System::GetInstance().Kernel().GetCurrentProcess();
-    if (!Memory::IsValidVirtualAddress(*process, addr)) {
+
+    if (!memory.IsValidVirtualAddress(*process, addr)) {
         LOG_WARNING(Debug_GDBStub, "Invalid address for HIO request");
         return;
     }
 
-    auto& memory = Core::System::GetInstance().Memory();
     memory.ReadBlock(*process, addr, &current_hio_request, sizeof(PackedGdbHioRequest));
 
-    if (std::string_view{current_hio_request.magic} != "GDB") {
+    std::string_view magic{current_hio_request.magic};
-        LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application");
+    if (magic != "GDB") {
+        LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application: magic '{}'", magic);
         current_hio_request_addr = 0;
         current_hio_request = {};
         request_status = Status::NoRequest;
-    } else {
+        return;
+    }
+
     LOG_DEBUG(Debug_GDBStub, "HIO request initiated at 0x{:X}", addr);
     current_hio_request_addr = addr;
     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
+    // is processed by the client. We will 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 (!WaitingForHioReply()) {
@@ -69,6 +74,7 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) {
         return false;
     }
 
+    // Skip 'F' header
     auto* command_pos = command_buffer + 1;
 
     if (*command_pos == 0 || *command_pos == ',') {
@@ -80,57 +86,65 @@ bool HandleHioReply(const u8* const command_buffer, const u32 command_length) {
     if (*command_pos == '-') {
         command_pos++;
         current_hio_request.retval = -1;
-    } else if (*command_pos == '+') {
-        command_pos++;
-        current_hio_request.retval = 1;
     } else {
+        if (*command_pos == '+') {
+            command_pos++;
+        }
+
         current_hio_request.retval = 1;
     }
 
-    const auto retval_end = std::find(command_pos, command_buffer + command_length, ',');
+    const std::string command_str{command_pos, command_buffer + command_length};
-    u64 retval = (u64)HexToInt(command_pos, retval_end - command_pos);
+    std::vector<std::string> command_parts;
-    command_pos = retval_end + 1;
+    Common::SplitString(command_str, ',', command_parts);
 
-    current_hio_request.retval *= retval;
+    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;
+    }
+
+    u64 unsigned_retval = HexToInt((u8*)command_parts[0].data(), command_parts[0].size());
+    current_hio_request.retval *= unsigned_retval;
+
+    if (command_parts.size() > 1) {
+        // Technically the errno could be signed but in practice this doesn't happen
+        current_hio_request.gdb_errno =
+            HexToInt((u8*)command_parts[1].data(), command_parts[1].size());
+    } else {
         current_hio_request.gdb_errno = 0;
-    current_hio_request.ctrl_c = 0;
+    }
 
-    if (*command_pos != 0) {
+    current_hio_request.ctrl_c = false;
-        u32 errno_;
-        // 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;
 
-        current_hio_request.gdb_errno = (int)errno_;
+    if (command_parts.size() > 2 && !command_parts[2].empty()) {
-
+        if (command_parts[2][0] != 'C') {
-        if (*command_pos != 0) {
-            if (*command_pos != 'C') {
             return false;
             // return GDB_ReplyErrno(ctx, EILSEQ);
         }
 
+        // for now we just ignore any trailing ";..." attachments
         current_hio_request.ctrl_c = true;
     }
-    }
 
     std::fill(std::begin(current_hio_request.param_format),
               std::end(current_hio_request.param_format), 0);
 
-    LOG_DEBUG(Debug_GDBStub, "HIO reply: {{retval = {}, errno = {}, ctrl_c = {}}}",
+    LOG_TRACE(Debug_GDBStub, "HIO reply: {{retval = {}, errno = {}, ctrl_c = {}}}",
               current_hio_request.retval, current_hio_request.gdb_errno,
               current_hio_request.ctrl_c);
 
     const auto process = Core::System::GetInstance().Kernel().GetCurrentProcess();
+    auto& memory = Core::System::GetInstance().Memory();
+
     // should have been checked when we first initialized the request,
     // but just double check again before we write to memory
-    if (!Memory::IsValidVirtualAddress(*process, current_hio_request_addr)) {
+    if (!memory.IsValidVirtualAddress(*process, current_hio_request_addr)) {
-        LOG_WARNING(Debug_GDBStub, "Invalid address {:X} to write HIO request",
+        LOG_WARNING(Debug_GDBStub, "Invalid address {:#X} to write HIO reply",
                     current_hio_request_addr);
         return false;
     }
 
-    auto& memory = Core::System::GetInstance().Memory();
     memory.WriteBlock(*process, current_hio_request_addr, &current_hio_request,
                       sizeof(PackedGdbHioRequest));
 
@@ -150,38 +164,45 @@ bool WaitingForHioReply() {
 }
 
 std::string BuildHioRequestPacket() {
-    std::stringstream packet;
+    std::string packet = fmt::format("F{}", current_hio_request.function_name);
-    // TODO:use the IntToGdbHex funcs instead std::hex ?
+    // TODO: should we use the IntToGdbHex funcs instead of fmt::format_to ?
-    packet << "F" << current_hio_request.function_name << std::hex;
 
     u32 nStr = 0;
 
     for (u32 i = 0; i < 8 && current_hio_request.param_format[i] != 0; i++) {
+        u64 param = current_hio_request.parameters[i];
+
         switch (current_hio_request.param_format[i]) {
         case 'i':
         case 'I':
         case 'p':
-            packet << "," << (u32)current_hio_request.parameters[i];
+            // For pointers and 32-bit ints, truncate before sending
-            break;
+            param = static_cast<u32>(param);
+
         case 'l':
         case 'L':
-            packet << "," << current_hio_request.parameters[i];
+            fmt::format_to(std::back_inserter(packet), ",{:x}", param);
             break;
+
         case 's':
-            packet << "," << (u32)current_hio_request.parameters[i] << "/"
+            // strings are written as {pointer}/{length}
-                   << current_hio_request.string_lengths[nStr++];
+            fmt::format_to(std::back_inserter(packet), ",{:x}/{:x}",
+                           (u32)current_hio_request.parameters[i],
+                           current_hio_request.string_lengths[nStr++]);
             break;
+
         default:
-            packet << '\0';
+            // TODO: early return? Or break out of this loop?
             break;
         }
     }
 
-    LOG_DEBUG(Debug_GDBStub, "HIO request packet: {}", packet.str());
+    LOG_TRACE(Debug_GDBStub, "HIO request packet: {}", packet);
 
+    // technically we should set this _after_ the reply is sent...
     request_status = Status::SentWaitingReply;
 
-    return packet.str();
+    return packet;
 }
 
 } // namespace GDBStub

src/core/gdbstub/hio.h

@@ -10,6 +10,10 @@
 
 namespace GDBStub {
 
+/**
+ * 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"
     u32 version;

src/core/hle/kernel/svc.cpp

@@ -1144,7 +1144,7 @@ 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)) {
+    if (!memory.IsValidVirtualAddress(*kernel.GetCurrentProcess(), address)) {
         LOG_WARNING(Kernel_SVC, "OutputDebugString called with invalid address {:X}", address);
         return;
     }