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

@@ -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<u32>((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<char*>(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;
+
+    if (HasPendingHioRequest()) {
+        const auto request_packet = BuildHioRequestPacket();
+        SendReply(request_packet.data());
+    } else {
         SendSignal(thread, trap);
+    }
 
     halt_loop = true;
     send_trap = false;

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

src/core/gdbstub/hio.cpp

@@ -12,7 +12,14 @@ namespace {
 
 static VAddr current_hio_request_addr;
 static PackedGdbHioRequest current_hio_request;
-static std::atomic<bool> sent_request{false};
+
+enum class Status {
+    NoRequest,
+    NotSent,
+    SentWaitingReply,
+};
+
+static std::atomic<Status> 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, &current_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();
 }

src/core/gdbstub/hio.h

@@ -4,6 +4,8 @@
 
 #pragma once
 
+#include <string>
+
 #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();
 

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;