Address review comments

Use more modern C++ style casts + std::array, clean up some log
messages, and rename some functions.
This commit is contained in:
Ian Chamberlain 2023-04-05 12:41:17 -04:00
parent 696c0904f8
commit 4cc181e5b8
No known key found for this signature in database
GPG Key ID: AE5484D09405AA60
3 changed files with 42 additions and 23 deletions

View File

@ -322,7 +322,7 @@ camera_inner_flip =
[Miscellaneous] [Miscellaneous]
# A filter which removes logs below a certain logging level. # 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 log_filter = *:Info
[Debugging] [Debugging]

View File

@ -38,7 +38,7 @@ static bool HasPendingHioRequest() {
/** /**
* @return Whether the GDB stub is awaiting a reply from the client. * @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; return current_hio_request_addr != 0 && request_status == Status::SentWaitingReply;
} }
@ -60,8 +60,8 @@ void SetHioRequest(const VAddr addr) {
return; return;
} }
if (WaitingForHioReply()) { if (IsWaitingForHioReply()) {
LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress!"); LOG_WARNING(Debug_GDBStub, "HIO requested while already in progress");
return; return;
} }
@ -79,9 +79,14 @@ void SetHioRequest(const VAddr addr) {
memory.ReadBlock(*process, addr, &current_hio_request, sizeof(PackedGdbHioRequest)); memory.ReadBlock(*process, addr, &current_hio_request, sizeof(PackedGdbHioRequest));
std::string_view magic{current_hio_request.magic}; if (current_hio_request.magic != std::array{'G', 'D', 'B', '\0'}) {
if (magic != "GDB") { std::string_view bad_magic{
LOG_WARNING(Debug_GDBStub, "Invalid HIO request sent by application: magic '{}'", 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_addr = 0;
current_hio_request = {}; current_hio_request = {};
request_status = Status::NoRequest; request_status = Status::NoRequest;
@ -104,7 +109,7 @@ void SetHioRequest(const VAddr addr) {
} }
void HandleHioReply(const u8* const command_buffer, const u32 command_length) { 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"); LOG_WARNING(Debug_GDBStub, "Got HIO reply but never sent a request");
return; return;
} }
@ -135,7 +140,7 @@ void HandleHioReply(const u8* const command_buffer, const u32 command_length) {
Common::SplitString(command_str, ',', command_parts); Common::SplitString(command_str, ',', command_parts);
if (command_parts.empty() || command_parts.size() > 3) { 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); SendErrorReply(EILSEQ);
return; return;
} }
@ -201,25 +206,30 @@ bool HandlePendingHioRequestPacket() {
return false; return false;
} }
if (WaitingForHioReply()) { if (IsWaitingForHioReply()) {
// We already sent it, continue waiting for a reply // We already sent it, continue waiting for a reply
return true; return true;
} }
std::string packet = fmt::format("F{}", current_hio_request.function_name); // We need a null-terminated string from char* instead of using
// TODO: should we use the IntToGdbHex funcs instead of fmt::format_to ? // 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++) { for (u32 i = 0; i < 8 && current_hio_request.param_format[i] != 0; i++) {
u64 param = current_hio_request.parameters[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]) { switch (current_hio_request.param_format[i]) {
case 'i': case 'i':
case 'I': case 'I':
case 'p': 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<u32>(param); param = static_cast<u32>(param);
[[fallthrough]];
case 'l': case 'l':
case 'L': case 'L':
@ -230,7 +240,7 @@ bool HandlePendingHioRequestPacket() {
// strings are written as {pointer}/{length} // strings are written as {pointer}/{length}
fmt::format_to(std::back_inserter(packet), ",{:x}/{:x}", fmt::format_to(std::back_inserter(packet), ",{:x}/{:x}",
static_cast<u32>(current_hio_request.parameters[i]), static_cast<u32>(current_hio_request.parameters[i]),
current_hio_request.string_lengths[nStr++]); current_hio_request.string_lengths[str_length_idx++]);
break; break;
default: 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()); SendReply(packet.data());
request_status = Status::SentWaitingReply; request_status = Status::SentWaitingReply;

View File

@ -14,19 +14,25 @@ namespace GDBStub {
* A request from a debugged application to perform some I/O with the GDB client. * 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. * 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/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 { struct PackedGdbHioRequest {
char magic[4]; // "GDB\0" std::array<char, 4> magic; // "GDB\0"
u32 version; u32 version;
// Request private:
char function_name[16 + 1]; static inline constexpr std::size_t MAX_FUNCNAME_LEN = 16;
char param_format[8 + 1]; static inline constexpr std::size_t PARAM_COUNT = 8;
u64 parameters[8]; public:
u32 string_lengths[8]; // Request. Char arrays have +1 entry for null terminator
std::array<char, MAX_FUNCNAME_LEN + 1> function_name;
std::array<char, PARAM_COUNT + 1> param_format;
std::array<u64, PARAM_COUNT> parameters;
std::array<u32, PARAM_COUNT> string_lengths;
// Return // Return
s64 retval; s64 retval;
@ -34,6 +40,9 @@ struct PackedGdbHioRequest {
bool ctrl_c; 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 * 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. * app indicates to the gdbstub that it wishes to perform a request.