From cd694c01d8148f899f5328bc9d44dd6a3653c3b1 Mon Sep 17 00:00:00 2001 From: Stenzek Date: Tue, 11 Mar 2025 20:30:46 +1000 Subject: [PATCH] GDBServer: Improve ack handling Treat acks as complete packets, that way it doesn't spam an error when only an ack is sent. --- src/common/log_channels.h | 2 +- src/core/gdb_server.cpp | 285 ++++++++++++++++++++------------------ 2 files changed, 152 insertions(+), 135 deletions(-) diff --git a/src/common/log_channels.h b/src/common/log_channels.h index 5d06d2398..c5c45771a 100644 --- a/src/common/log_channels.h +++ b/src/common/log_channels.h @@ -23,7 +23,7 @@ X(FileLoader) \ X(FileSystem) \ X(FullscreenUI) \ - X(GDBProtocol) \ + X(GDBServer) \ X(GPU) \ X(GPUDevice) \ X(GPUDump) \ diff --git a/src/core/gdb_server.cpp b/src/core/gdb_server.cpp index 84c52c16b..d732f9f3a 100644 --- a/src/core/gdb_server.cpp +++ b/src/core/gdb_server.cpp @@ -21,19 +21,121 @@ #include #include -LOG_CHANNEL(GDBProtocol); +LOG_CHANNEL(GDBServer); -namespace GDBProtocol { +namespace GDBServer { + +static u8* GetMemoryPointer(PhysicalMemoryAddress address, u32 length); +static u8 ComputeChecksum(std::string_view str); +static std::optional DeserializePacket(std::string_view in); +static std::string SerializePacket(std::string_view in); + +static std::optional Cmd$_questionMark(std::string_view data); +static std::optional Cmd$g(std::string_view data); +static std::optional Cmd$G(std::string_view data); +static std::optional Cmd$m(std::string_view data); +static std::optional Cmd$M(std::string_view data); +static std::optional Cmd$z1(std::string_view data); +static std::optional Cmd$Z1(std::string_view data); +static std::optional Cmd$vMustReplyEmpty(std::string_view data); +static std::optional Cmd$qSupported(std::string_view data); + +static bool IsPacketAck(std::string_view data); static bool IsPacketInterrupt(std::string_view data); static bool IsPacketContinue(std::string_view data); static bool IsPacketComplete(std::string_view data); static std::string ProcessPacket(std::string_view data); -} // namespace GDBProtocol -namespace GDBProtocol { +/// Number of registers in GDB remote protocol for MIPS III. +constexpr int NUM_GDB_REGISTERS = 73; + +/// List of GDB remote protocol registers for MIPS III (excluding FP). +static const std::array REGISTERS{ + &CPU::g_state.regs.r[0], + &CPU::g_state.regs.r[1], + &CPU::g_state.regs.r[2], + &CPU::g_state.regs.r[3], + &CPU::g_state.regs.r[4], + &CPU::g_state.regs.r[5], + &CPU::g_state.regs.r[6], + &CPU::g_state.regs.r[7], + &CPU::g_state.regs.r[8], + &CPU::g_state.regs.r[9], + &CPU::g_state.regs.r[10], + &CPU::g_state.regs.r[11], + &CPU::g_state.regs.r[12], + &CPU::g_state.regs.r[13], + &CPU::g_state.regs.r[14], + &CPU::g_state.regs.r[15], + &CPU::g_state.regs.r[16], + &CPU::g_state.regs.r[17], + &CPU::g_state.regs.r[18], + &CPU::g_state.regs.r[19], + &CPU::g_state.regs.r[20], + &CPU::g_state.regs.r[21], + &CPU::g_state.regs.r[22], + &CPU::g_state.regs.r[23], + &CPU::g_state.regs.r[24], + &CPU::g_state.regs.r[25], + &CPU::g_state.regs.r[26], + &CPU::g_state.regs.r[27], + &CPU::g_state.regs.r[28], + &CPU::g_state.regs.r[29], + &CPU::g_state.regs.r[30], + &CPU::g_state.regs.r[31], + + &CPU::g_state.cop0_regs.sr.bits, + &CPU::g_state.regs.lo, + &CPU::g_state.regs.hi, + &CPU::g_state.cop0_regs.BadVaddr, + &CPU::g_state.cop0_regs.cause.bits, + &CPU::g_state.pc, +}; + +/// List of all GDB remote protocol packets supported by us. +static const std::map(std::string_view)>> COMMANDS{ + {"?", Cmd$_questionMark}, + {"g", Cmd$g}, + {"G", Cmd$G}, + {"m", Cmd$m}, + {"M", Cmd$M}, + {"z0,", Cmd$z1}, + {"Z0,", Cmd$Z1}, + {"z1,", Cmd$z1}, + {"Z1,", Cmd$Z1}, + {"vMustReplyEmpty", Cmd$vMustReplyEmpty}, + {"qSupported", Cmd$qSupported}, +}; + +namespace { +class ClientSocket final : public BufferedStreamSocket +{ +public: + ClientSocket(SocketMultiplexer& multiplexer, SocketDescriptor descriptor); + ~ClientSocket() override; + + void OnSystemPaused(); + void OnSystemResumed(); -static u8* GetMemoryPointer(PhysicalMemoryAddress address, u32 length) +protected: + void OnConnected() override; + void OnDisconnected(const Error& error) override; + void OnRead() override; + +private: + void SendPacket(std::string_view sv); + + bool m_seen_resume = false; +}; +} // namespace + +static std::shared_ptr s_gdb_listen_socket; +static std::vector> s_gdb_clients; + +} // namespace GDBServer + +u8* GDBServer::GetMemoryPointer(PhysicalMemoryAddress address, u32 length) { auto region = Bus::GetMemoryRegionForAddress(address); if (region) @@ -48,7 +150,7 @@ static u8* GetMemoryPointer(PhysicalMemoryAddress address, u32 length) return nullptr; } -static u8 ComputeChecksum(std::string_view str) +u8 GDBServer::ComputeChecksum(std::string_view str) { u8 checksum = 0; for (char c : str) @@ -58,7 +160,7 @@ static u8 ComputeChecksum(std::string_view str) return checksum; } -static std::optional DeserializePacket(std::string_view in) +std::optional GDBServer::DeserializePacket(std::string_view in) { if ((in.size() < 4) || (in[0] != '$') || (in[in.size() - 3] != '#')) { @@ -79,67 +181,21 @@ static std::optional DeserializePacket(std::string_view in) } } -static std::string SerializePacket(std::string_view in) +std::string GDBServer::SerializePacket(std::string_view in) { std::stringstream ss; ss << '$' << in << '#' << TinyString::from_format("{:02x}", ComputeChecksum(in)); return ss.str(); } -/// List of GDB remote protocol registers for MIPS III (excluding FP). -static const std::array REGISTERS{ - &CPU::g_state.regs.r[0], - &CPU::g_state.regs.r[1], - &CPU::g_state.regs.r[2], - &CPU::g_state.regs.r[3], - &CPU::g_state.regs.r[4], - &CPU::g_state.regs.r[5], - &CPU::g_state.regs.r[6], - &CPU::g_state.regs.r[7], - &CPU::g_state.regs.r[8], - &CPU::g_state.regs.r[9], - &CPU::g_state.regs.r[10], - &CPU::g_state.regs.r[11], - &CPU::g_state.regs.r[12], - &CPU::g_state.regs.r[13], - &CPU::g_state.regs.r[14], - &CPU::g_state.regs.r[15], - &CPU::g_state.regs.r[16], - &CPU::g_state.regs.r[17], - &CPU::g_state.regs.r[18], - &CPU::g_state.regs.r[19], - &CPU::g_state.regs.r[20], - &CPU::g_state.regs.r[21], - &CPU::g_state.regs.r[22], - &CPU::g_state.regs.r[23], - &CPU::g_state.regs.r[24], - &CPU::g_state.regs.r[25], - &CPU::g_state.regs.r[26], - &CPU::g_state.regs.r[27], - &CPU::g_state.regs.r[28], - &CPU::g_state.regs.r[29], - &CPU::g_state.regs.r[30], - &CPU::g_state.regs.r[31], - - &CPU::g_state.cop0_regs.sr.bits, - &CPU::g_state.regs.lo, - &CPU::g_state.regs.hi, - &CPU::g_state.cop0_regs.BadVaddr, - &CPU::g_state.cop0_regs.cause.bits, - &CPU::g_state.pc, -}; - -/// Number of registers in GDB remote protocol for MIPS III. -constexpr int NUM_GDB_REGISTERS = 73; - /// Get stop reason. -static std::optional Cmd$_questionMark(std::string_view data) +std::optional GDBServer::Cmd$_questionMark(std::string_view data) { return {"S02"}; } /// Get general registers. -static std::optional Cmd$g(std::string_view data) +std::optional GDBServer::Cmd$g(std::string_view data) { std::stringstream ss; @@ -159,7 +215,7 @@ static std::optional Cmd$g(std::string_view data) } /// Set general registers. -static std::optional Cmd$G(std::string_view data) +std::optional GDBServer::Cmd$G(std::string_view data) { if (data.size() == NUM_GDB_REGISTERS * 8) { @@ -185,7 +241,7 @@ static std::optional Cmd$G(std::string_view data) } /// Get memory. -static std::optional Cmd$m(std::string_view data) +std::optional GDBServer::Cmd$m(std::string_view data) { std::stringstream ss{std::string{data}}; std::string dataAddress, dataLength; @@ -211,7 +267,7 @@ static std::optional Cmd$m(std::string_view data) } /// Set memory. -static std::optional Cmd$M(std::string_view data) +std::optional GDBServer::Cmd$M(std::string_view data) { std::stringstream ss{std::string{data}}; std::string dataAddress, dataLength, dataPayload; @@ -241,7 +297,7 @@ static std::optional Cmd$M(std::string_view data) } /// Remove hardware breakpoint. -static std::optional Cmd$z1(std::string_view data) +std::optional GDBServer::Cmd$z1(std::string_view data) { auto address = StringUtil::FromChars(data, 16); if (address) @@ -256,7 +312,7 @@ static std::optional Cmd$z1(std::string_view data) } /// Insert hardware breakpoint. -static std::optional Cmd$Z1(std::string_view data) +std::optional GDBServer::Cmd$Z1(std::string_view data) { auto address = StringUtil::FromChars(data, 16); if (address) @@ -270,65 +326,45 @@ static std::optional Cmd$Z1(std::string_view data) } } -static std::optional Cmd$vMustReplyEmpty(std::string_view data) +std::optional GDBServer::Cmd$vMustReplyEmpty(std::string_view data) { return {""}; } -static std::optional Cmd$qSupported(std::string_view data) +std::optional GDBServer::Cmd$qSupported(std::string_view data) { return {""}; } -/// List of all GDB remote protocol packets supported by us. -static const std::map(std::string_view)>> COMMANDS{ - {"?", Cmd$_questionMark}, - {"g", Cmd$g}, - {"G", Cmd$G}, - {"m", Cmd$m}, - {"M", Cmd$M}, - {"z0,", Cmd$z1}, - {"Z0,", Cmd$Z1}, - {"z1,", Cmd$z1}, - {"Z1,", Cmd$Z1}, - {"vMustReplyEmpty", Cmd$vMustReplyEmpty}, - {"qSupported", Cmd$qSupported}, -}; +bool GDBServer::IsPacketAck(std::string_view data) +{ + DebugAssert(data.size() >= 1); + return (data[0] == '+' || data[0] == '-'); +} -bool IsPacketInterrupt(std::string_view data) +bool GDBServer::IsPacketInterrupt(std::string_view data) { - return (data.size() >= 1) && (data[data.size() - 1] == '\003'); + DebugAssert(data.size() >= 1); + return (data[data.size() - 1] == '\003'); } -bool IsPacketContinue(std::string_view data) +bool GDBServer::IsPacketContinue(std::string_view data) { return (data.size() >= 5) && (data.substr(data.size() - 5) == "$c#63"); } -bool IsPacketComplete(std::string_view data) +bool GDBServer::IsPacketComplete(std::string_view data) { return ((data.size() == 1) && (data[0] == '\003')) || ((data.size() > 3) && (*(data.end() - 3) == '#')); } -std::string ProcessPacket(std::string_view data) +std::string GDBServer::ProcessPacket(std::string_view data) { - std::string_view trimmedData = data; - - // Eat ACKs. - while (!trimmedData.empty() && (trimmedData[0] == '+' || trimmedData[0] == '-')) - { - if (trimmedData[0] == '-') - { - ERROR_LOG("Received negative ack"); - } - trimmedData = trimmedData.substr(1); - } - // Validate packet. - auto packet = DeserializePacket(trimmedData); + auto packet = DeserializePacket(data); if (!packet) { - ERROR_LOG("Malformed packet '{}'", trimmedData); + ERROR_LOG("Malformed packet '{}'", data); return "-"; } @@ -340,7 +376,7 @@ std::string ProcessPacket(std::string_view data) { if (packet->starts_with(command.first)) { - DEBUG_LOG("Processing command '{}'", command.first); + DEV_LOG("Processing command '{}'", command.first); // Invoke command, remove command name from payload. reply = command.second(packet->substr(strlen(command.first))); @@ -350,41 +386,11 @@ std::string ProcessPacket(std::string_view data) } if (!processed) - WARNING_LOG("Failed to process packet '{}'", trimmedData); + WARNING_LOG("Failed to process packet '{}'", data); return reply ? "+" + SerializePacket(*reply) : "+"; } -} // namespace GDBProtocol - -namespace GDBServer { - -namespace { -class ClientSocket final : public BufferedStreamSocket -{ -public: - ClientSocket(SocketMultiplexer& multiplexer, SocketDescriptor descriptor); - ~ClientSocket() override; - - void OnSystemPaused(); - void OnSystemResumed(); - -protected: - void OnConnected() override; - void OnDisconnected(const Error& error) override; - void OnRead() override; - -private: - void SendPacket(std::string_view sv); - - bool m_seen_resume = false; -}; -} // namespace - -static std::shared_ptr s_gdb_listen_socket; -static std::vector> s_gdb_clients; -} // namespace GDBServer - GDBServer::ClientSocket::ClientSocket(SocketMultiplexer& multiplexer, SocketDescriptor descriptor) : BufferedStreamSocket(multiplexer, descriptor, 65536, 65536) { @@ -433,25 +439,34 @@ void GDBServer::ClientSocket::OnRead() const std::string_view current_packet(reinterpret_cast(buffer.data() + buffer_offset), current_packet_size); - if (GDBProtocol::IsPacketInterrupt(current_packet)) + if (GDBServer::IsPacketAck(current_packet)) + { + // Eat ACKs. + if (current_packet[0] == '-') + ERROR_LOG("Received negative ack"); + + packet_complete = true; + break; + } + else if (GDBServer::IsPacketInterrupt(current_packet)) { DEV_LOG("{} > Interrupt request", GetRemoteAddress().ToString()); System::PauseSystem(true); packet_complete = true; break; } - else if (GDBProtocol::IsPacketContinue(current_packet)) + else if (GDBServer::IsPacketContinue(current_packet)) { DEV_LOG("{} > Continue request", GetRemoteAddress().ToString()); System::PauseSystem(false); packet_complete = true; break; } - else if (GDBProtocol::IsPacketComplete(current_packet)) + else if (GDBServer::IsPacketComplete(current_packet)) { // TODO: Make this not copy. DEV_LOG("{} > {}", GetRemoteAddress().ToString(), current_packet); - SendPacket(GDBProtocol::ProcessPacket(current_packet)); + SendPacket(GDBServer::ProcessPacket(current_packet)); packet_complete = true; break; } @@ -459,7 +474,9 @@ void GDBServer::ClientSocket::OnRead() if (!packet_complete) { - WARNING_LOG("Incomplete packet, got {} bytes.", buffer.size() - buffer_offset); + WARNING_LOG( + "Incomplete packet, got {} bytes: {}", buffer.size() - buffer_offset, + std::string_view(reinterpret_cast(buffer.data() + buffer_offset), buffer.size() - buffer_offset)); break; } else @@ -489,7 +506,7 @@ void GDBServer::ClientSocket::OnSystemPaused() m_seen_resume = false; // Generate a stop reply packet, insert '?' command to generate it. - SendPacket(GDBProtocol::ProcessPacket("$?#3f")); + SendPacket(GDBServer::ProcessPacket("$?#3f")); } void GDBServer::ClientSocket::OnSystemResumed()