From 4cc181e5b8382d1351dbb047dcd1186d1c5ce40c Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Wed, 5 Apr 2023 12:41:17 -0400 Subject: [PATCH] Address review comments Use more modern C++ style casts + std::array, clean up some log messages, and rename some functions. --- src/citra/default_ini.h | 2 +- src/core/gdbstub/hio.cpp | 40 +++++++++++++++++++++++++--------------- src/core/gdbstub/hio.h | 23 ++++++++++++++++------- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/citra/default_ini.h b/src/citra/default_ini.h index afb59a375..692134590 100644 --- a/src/citra/default_ini.h +++ b/src/citra/default_ini.h @@ -322,7 +322,7 @@ camera_inner_flip = [Miscellaneous] # A filter which removes logs below a certain logging level. -# Examples: *:Debug Kernel.SVC:Trace Service:Critical +# Examples: *:Debug Kernel.SVC:Trace Service.*:Critical log_filter = *:Info [Debugging] diff --git a/src/core/gdbstub/hio.cpp b/src/core/gdbstub/hio.cpp index b19535c21..02d149b0f 100644 --- a/src/core/gdbstub/hio.cpp +++ b/src/core/gdbstub/hio.cpp @@ -38,7 +38,7 @@ static bool HasPendingHioRequest() { /** * @return Whether the GDB stub is awaiting a reply from the client. */ -static bool WaitingForHioReply() { +static bool IsWaitingForHioReply() { return current_hio_request_addr != 0 && request_status == Status::SentWaitingReply; } @@ -60,8 +60,8 @@ void SetHioRequest(const VAddr addr) { return; } - if (WaitingForHioReply()) { - LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress!"); + if (IsWaitingForHioReply()) { + LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress"); return; } @@ -79,9 +79,14 @@ void SetHioRequest(const VAddr addr) { memory.ReadBlock(*process, addr, ¤t_hio_request, sizeof(PackedGdbHioRequest)); - std::string_view magic{current_hio_request.magic}; - if (magic != "GDB") { - LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application: magic '{}'", magic); + if (current_hio_request.magic != std::array{'G', 'D', 'B', '\0'}) { + std::string_view bad_magic{ + std::begin(current_hio_request.magic), + std::end(current_hio_request.magic), + }; + LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application: bad magic '{}'", + bad_magic); + current_hio_request_addr = 0; current_hio_request = {}; request_status = Status::NoRequest; @@ -104,7 +109,7 @@ void SetHioRequest(const VAddr addr) { } void HandleHioReply(const u8* const command_buffer, const u32 command_length) { - if (!WaitingForHioReply()) { + if (!IsWaitingForHioReply()) { LOG_WARNING(Debug_GDBStub, "Got HIO reply but never sent a request"); return; } @@ -135,7 +140,7 @@ void HandleHioReply(const u8* const command_buffer, const u32 command_length) { Common::SplitString(command_str, ',', command_parts); if (command_parts.empty() || command_parts.size() > 3) { - LOG_WARNING(Debug_GDBStub, "unexpected reply packet size: {}", command_parts); + LOG_WARNING(Debug_GDBStub, "Unexpected reply packet size: {}", command_parts); SendErrorReply(EILSEQ); return; } @@ -201,25 +206,30 @@ bool HandlePendingHioRequestPacket() { return false; } - if (WaitingForHioReply()) { + if (IsWaitingForHioReply()) { // We already sent it, continue waiting for a reply return true; } - std::string packet = fmt::format("F{}", current_hio_request.function_name); - // TODO: should we use the IntToGdbHex funcs instead of fmt::format_to ? + // We need a null-terminated string from char* instead of using + // the full length of the array (like {.begin(), .end()} constructor would) + std::string_view function_name{current_hio_request.function_name.data()}; - u32 nStr = 0; + std::string packet = fmt::format("F{}", function_name); + + u32 str_length_idx = 0; for (u32 i = 0; i < 8 && current_hio_request.param_format[i] != 0; i++) { u64 param = current_hio_request.parameters[i]; + // TODO: should we use the IntToGdbHex funcs instead of fmt::format_to ? switch (current_hio_request.param_format[i]) { case 'i': case 'I': case 'p': - // For pointers and 32-bit ints, truncate before sending + // For pointers and 32-bit ints, truncate down to size before sending param = static_cast(param); + [[fallthrough]]; case 'l': case 'L': @@ -230,7 +240,7 @@ bool HandlePendingHioRequestPacket() { // strings are written as {pointer}/{length} fmt::format_to(std::back_inserter(packet), ",{:x}/{:x}", static_cast(current_hio_request.parameters[i]), - current_hio_request.string_lengths[nStr++]); + current_hio_request.string_lengths[str_length_idx++]); break; default: @@ -241,7 +251,7 @@ bool HandlePendingHioRequestPacket() { } } - LOG_TRACE(Debug_GDBStub, "HIO request packet: {}", packet); + LOG_TRACE(Debug_GDBStub, "HIO request packet: '{}'", packet); SendReply(packet.data()); request_status = Status::SentWaitingReply; diff --git a/src/core/gdbstub/hio.h b/src/core/gdbstub/hio.h index dda4e8ef1..f85f1e7dc 100644 --- a/src/core/gdbstub/hio.h +++ b/src/core/gdbstub/hio.h @@ -14,19 +14,25 @@ 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: + * Based on the Rosalina + libctru implementations: * https://github.com/LumaTeam/Luma3DS/blob/master/sysmodules/rosalina/include/gdb.h#L46C27-L62 + * https://github.com/devkitPro/libctru/blob/master/libctru/source/gdbhio.c#L71-L87 */ struct PackedGdbHioRequest { - char magic[4]; // "GDB\0" + std::array magic; // "GDB\0" u32 version; - // Request - char function_name[16 + 1]; - char param_format[8 + 1]; +private: + static inline constexpr std::size_t MAX_FUNCNAME_LEN = 16; + static inline constexpr std::size_t PARAM_COUNT = 8; - u64 parameters[8]; - u32 string_lengths[8]; +public: + // Request. Char arrays have +1 entry for null terminator + std::array function_name; + std::array param_format; + + std::array parameters; + std::array string_lengths; // Return s64 retval; @@ -34,6 +40,9 @@ struct PackedGdbHioRequest { bool ctrl_c; }; +static_assert(sizeof(PackedGdbHioRequest) == 152, + "HIO request size must match libctru implementation"); + /** * 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.