[PATCH v2 0/8] winedbg: improve winedbg gdb experience (esp. with esoteric module names)
Changelog: v1 -> v2: address review comments so far Jinoh Kang (8): winedbg: crash when allocation fails in packet_realloc(). winedbg: free buffer and return NULL if size==0 in packet_realloc(). winedbg: generalise packet_realloc(), rename to buffer_realloc(). winedbg: Use unsigned int for offset/length in GDB qXfer handler. winedbg: escape special characters in GDB packet reply. winedbg: buffer and escape output of GDB qXfer commands. winedbg: cache GDB qXfer command result for chunked fetching and define handler table. winedbg: Implement GDB qXfer object exec-file. programs/winedbg/gdbproxy.c | 565 ++++++++++++++++++++++++++++-------- 1 file changed, 452 insertions(+), 113 deletions(-) -- 2.31.1
Today, every caller of packet_realloc simply assumes that the allocation always succeeds. *Not* crashing outright on allocation failure makes diagnosing problems more difficult. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 522e4fdb506..deb6b0973f5 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -708,9 +708,8 @@ enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, static char* packet_realloc(char* buf, int size) { if (!buf) - return HeapAlloc(GetProcessHeap(), 0, size); - return HeapReAlloc(GetProcessHeap(), 0, buf, size); - + return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, size); + return HeapReAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, buf, size); } static void packet_reply_grow(struct gdb_context* gdbctx, size_t size) -- 2.31.1
packet_realloc() does not return on error, so we can use NULL for normal circumstances. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index deb6b0973f5..3f5584633e4 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -707,6 +707,12 @@ enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, static char* packet_realloc(char* buf, int size) { + if (!size) + { + if (buf) + HeapFree(GetProcessHeap(), 0, buf); + return NULL; + } if (!buf) return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, size); return HeapReAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, buf, size); -- 2.31.1
Make it return void * and accept size_t as the size. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 3f5584633e4..db0b8d77ba9 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -705,7 +705,7 @@ static int addr_width(struct gdb_context* gdbctx) enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, packet_last_f = 0x80}; -static char* packet_realloc(char* buf, int size) +static void* buffer_realloc(void* buf, size_t size) { if (!size) { @@ -723,7 +723,7 @@ static void packet_reply_grow(struct gdb_context* gdbctx, size_t size) if (gdbctx->out_buf_alloc < gdbctx->out_len + size) { gdbctx->out_buf_alloc = ((gdbctx->out_len + size) / 32 + 1) * 32; - gdbctx->out_buf = packet_realloc(gdbctx->out_buf, gdbctx->out_buf_alloc); + gdbctx->out_buf = buffer_realloc(gdbctx->out_buf, gdbctx->out_buf_alloc); } } @@ -2090,7 +2090,7 @@ static int fetch_data(struct gdb_context* gdbctx) { #define STEP 128 if (gdbctx->in_len + STEP > gdbctx->in_buf_alloc) - gdbctx->in_buf = packet_realloc(gdbctx->in_buf, gdbctx->in_buf_alloc += STEP); + gdbctx->in_buf = buffer_realloc(gdbctx->in_buf, gdbctx->in_buf_alloc += STEP); #undef STEP len = recv(gdbctx->sock, gdbctx->in_buf + gdbctx->in_len, gdbctx->in_buf_alloc - gdbctx->in_len - 1, 0); if (len <= 0) break; -- 2.31.1
packet_query uses sscanf format "%x" to parse out offset and length values. Since %x corresponds to unsigned int in the C standard, adjust the variable types appropriately. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index db0b8d77ba9..e6b1d998f7d 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -785,7 +785,7 @@ static void packet_reply_open_xfer(struct gdb_context* gdbctx) packet_reply_add(gdbctx, "m"); } -static void packet_reply_close_xfer(struct gdb_context* gdbctx, int off, int len) +static void packet_reply_close_xfer(struct gdb_context* gdbctx, unsigned int off, unsigned int len) { int begin = gdbctx->out_curr_packet + 1; int plen; @@ -802,7 +802,7 @@ static void packet_reply_close_xfer(struct gdb_context* gdbctx, int off, int len } plen = gdbctx->out_len - begin; - if (len >= 0 && plen > len) gdbctx->out_len -= (plen - len); + if (plen > len) gdbctx->out_len -= (plen - len); else gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; packet_reply_close(gdbctx); @@ -1764,7 +1764,7 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c static enum packet_return packet_query(struct gdb_context* gdbctx) { - int off, len; + unsigned int off, len; struct backend_cpu *cpu; switch (gdbctx->in_packet[0]) -- 2.31.1
There are four special characters in GDB's remote serial protocol: - '$' (0x24): start of packet - '}' (0x7D): escape - '*' (0x2A): run-length encoding repeat count delimiter - '#' (0x23): end of packet; start of checksum In particular, the '#' and '}' characters are problematic since they are often used in library filenames. A few examples: - %SystemRoot%\assembly\NativeImages_v[.NET ver]\[module+hash]#\*\*.dll - {CLSID or UUID}\*\.dll To make GDB happy with those filenames, we scan for those characters and escape them properly. While we are at it, also remove the assert in the packet_reply function that checks for '$' and '#' in the packet payload. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 68 ++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e6b1d998f7d..bdb73659ade 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -752,18 +752,71 @@ static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len) } } +static const unsigned char gdb_special_chars_lookup_table[4] = { + /* The characters should be sorted by its value modulo table length. */ + + 0x24, /* $: 001001|00 */ + 0x7D, /* }: 011111|01 */ + 0x2A, /* *: 001010|10 */ + 0x23 /* #: 001000|11 */ +}; + +static inline BOOL is_gdb_special_char(unsigned char val) +{ + /* A note on the GDB special character scanning code: + * + * We cannot use strcspn() since we plan to transmit binary data in + * packet reply, which can contain NULL (0x00) bytes. We also don't want + * to slow down memory dump transfers. Therefore, we use a tiny lookup + * table that contains all the four special characters to speed up scanning. + * + * Note: strcspn() uses lookup tables as well, but as of Wine 6.19 + * msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table + * on the stack and populates it on the fly. It would be slower and less + * cache-friendly than a preallocated, tiny static lookup table. + */ + + const size_t length = ARRAY_SIZE(gdb_special_chars_lookup_table); + return gdb_special_chars_lookup_table[val % length] == val; +} + +static void packet_reply_add_raw(struct gdb_context* gdbctx, const void* data, size_t len) +{ + const unsigned char *ptr = data, *sptr, *end = ptr + len; + size_t seg_len; + int is_end; + + while (ptr != end) + { + for (sptr = ptr; sptr != end && !is_gdb_special_char(*sptr); sptr++) + ; + + seg_len = sptr - ptr; + is_end = sptr == end; + + packet_reply_grow(gdbctx, is_end ? seg_len : seg_len + 2); + memcpy(&gdbctx->out_buf[gdbctx->out_len], ptr, seg_len); + gdbctx->out_len += seg_len; + ptr += seg_len; + + if (is_end) + break; + + gdbctx->out_buf[gdbctx->out_len++] = 0x7D; + gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; + } +} + static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str) { - int len = strlen(str); - packet_reply_grow(gdbctx, len); - memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len); - gdbctx->out_len += len; + packet_reply_add_raw(gdbctx, str, strlen(str)); } static void packet_reply_open(struct gdb_context* gdbctx) { assert(gdbctx->out_curr_packet == -1); - packet_reply_add(gdbctx, "$"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '$'; gdbctx->out_curr_packet = gdbctx->out_len; } @@ -773,7 +826,8 @@ static void packet_reply_close(struct gdb_context* gdbctx) int plen; plen = gdbctx->out_len - gdbctx->out_curr_packet; - packet_reply_add(gdbctx, "#"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '#'; cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen); packet_reply_hex_to(gdbctx, &cksum, 1); gdbctx->out_curr_packet = -1; @@ -812,8 +866,6 @@ static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* p { packet_reply_open(gdbctx); - assert(strchr(packet, '$') == NULL && strchr(packet, '#') == NULL); - packet_reply_add(gdbctx, packet); packet_reply_close(gdbctx); -- 2.31.1
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; }; +struct vl_buffer +{ + void *base; + size_t len; + size_t alloc; +}; + struct gdb_context { /* gdb information */ @@ -82,6 +89,7 @@ struct gdb_context /* Unix environment */ ULONG_PTR wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ BOOL no_ack_mode; + struct vl_buffer qxfer_buffer; }; /* assume standard signal and errno values */ @@ -224,6 +232,102 @@ static void hex_to(char* dst, const void* src, size_t len) } } +static void* buffer_realloc(void* buf, size_t size); + +static void vl_resize(struct vl_buffer* vlbuf, size_t alloc) +{ + vlbuf->alloc = alloc; + vlbuf->base = buffer_realloc(vlbuf->base, vlbuf->alloc); +} + +static void vl_empty(struct vl_buffer *vlbuf) +{ + vlbuf->len = 0; + vl_resize(vlbuf, 0); +} + +static void vl_grow(struct vl_buffer* vlbuf, size_t size) +{ + if (vlbuf->alloc < vlbuf->len + size) + vl_resize(vlbuf, ((vlbuf->len + size) / 32 + 1) * 32); +} + +static void vl_append(struct vl_buffer* vlbuf, const void *data, size_t size) +{ + vl_grow(vlbuf, size); + memcpy((void *)((unsigned char *)vlbuf->base + vlbuf->len), data, size); + vlbuf->len += size; +} + +static inline void vl_append_str(struct vl_buffer* vlbuf, const char* str) +{ + vl_append(vlbuf, (const void *)str, strlen(str)); +} + +static inline void vl_append_uinthex(struct vl_buffer* vlbuf, ULONG_PTR val, int len) +{ + char buf[sizeof(ULONG_PTR) * 2], *ptr; + + assert(len <= sizeof(ULONG_PTR)); + + for (ptr = buf + len * 2; ptr != buf; val >>= 4) + *--ptr = hex_to0(val & 0x0F); + + vl_append(vlbuf, ptr, len * 2); +} + +static const unsigned char xml_special_chars_lookup_table[16] = { + /* The characters should be sorted by its value modulo table length. */ + + 0x00, /* NUL */ + 0, + 0x22, /* ": 0010|0010 */ + 0, 0, 0, + 0x26, /* &: 0010|0110 */ + 0x27, /* ': 0010|0111 */ + 0, 0, 0, 0, + 0x3C, /* <: 0011|1100 */ + 0, + 0x3E, /* >: 0011|1110 */ + 0 +}; + +static inline BOOL is_nul_or_xml_special_char(unsigned char val) +{ + /* Note: strcspn() uses lookup tables as well, but as of Wine 6.19 + * msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table + * on the stack and populates it on the fly. It would be slower and less + * cache-friendly than a preallocated, tiny static lookup table. + */ + + const size_t length = ARRAY_SIZE(xml_special_chars_lookup_table); + return xml_special_chars_lookup_table[val % length] == val; +} + +static void vl_append_xmlstr(struct vl_buffer* vlbuf, const char *str) +{ + const char *ptr = str, *sptr; + + for (;;) + { + for (sptr = ptr; !is_nul_or_xml_special_char((unsigned char)*sptr); sptr++) + ; + + vl_append(vlbuf, ptr, sptr - ptr); + ptr = sptr; + + switch (*ptr++) + { + case '"': vl_append_str(vlbuf, """); break; + case '&': vl_append_str(vlbuf, "&"); break; + case '\'': vl_append_str(vlbuf, "'"); break; + case '<': vl_append_str(vlbuf, "<"); break; + case '>': vl_append_str(vlbuf, ">"); break; + default: return; + } + } +} + static unsigned char checksum(const char* ptr, int len) { unsigned cksum = 0; @@ -833,34 +937,6 @@ static void packet_reply_close(struct gdb_context* gdbctx) gdbctx->out_curr_packet = -1; } -static void packet_reply_open_xfer(struct gdb_context* gdbctx) -{ - packet_reply_open(gdbctx); - packet_reply_add(gdbctx, "m"); -} - -static void packet_reply_close_xfer(struct gdb_context* gdbctx, unsigned int off, unsigned int len) -{ - int begin = gdbctx->out_curr_packet + 1; - int plen; - - if (begin + off < gdbctx->out_len) - { - gdbctx->out_len -= off; - memmove(gdbctx->out_buf + begin, gdbctx->out_buf + begin + off, gdbctx->out_len); - } - else - { - gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; - gdbctx->out_len = gdbctx->out_curr_packet + 1; - } - - plen = gdbctx->out_len - begin; - if (plen > len) gdbctx->out_len -= (plen - len); - else gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; - - packet_reply_close(gdbctx); -} static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* packet) { @@ -891,6 +967,38 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, dbg_ packet_reply_hex_to(gdbctx, cpu_register_ptr(gdbctx, ctx, idx), cpu_register_map[idx].length); } +static void packet_reply_xfer( + struct gdb_context* gdbctx, + const void *data, + size_t datalen, + unsigned int off, + unsigned int len, + BOOL *more_p +) +{ + BOOL nonempty, more; + size_t trunc_len; + + packet_reply_open(gdbctx); + + nonempty = (size_t)off < datalen; + more = nonempty && (size_t)off + len < datalen; + if (more) + packet_reply_add(gdbctx, "m"); + else + packet_reply_add(gdbctx, "l"); + + if (nonempty) + { + trunc_len = min((size_t)len, datalen - off); + packet_reply_add_raw(gdbctx, (const unsigned char *)data + off, trunc_len); + } + + packet_reply_close(gdbctx); + + *more_p = more; +} + /* =============================================== * * P A C K E T H A N D L E R S * * =============================================== * @@ -1587,6 +1695,7 @@ static enum packet_return packet_query_remote_command(struct gdb_context* gdbctx static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVOID ctx) { struct gdb_context* gdbctx = ctx; + struct vl_buffer* vlbuf = &gdbctx->qxfer_buffer; MEMORY_BASIC_INFORMATION mbi; IMAGE_SECTION_HEADER *sec; IMAGE_DOS_HEADER *dos = NULL; @@ -1599,11 +1708,11 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO mod.SizeOfStruct = sizeof(mod); SymGetModuleInfo64(gdbctx->process->handle, base, &mod); - packet_reply_add(gdbctx, "<library name=\""); + vl_append_str(vlbuf, "<library name=\""); if (strcmp(mod.LoadedImageName, "[vdso].so") == 0) - packet_reply_add(gdbctx, "linux-vdso.so.1"); + vl_append_xmlstr(vlbuf, "linux-vdso.so.1"); else if (mod.LoadedImageName[0] == '/') - packet_reply_add(gdbctx, mod.LoadedImageName); + vl_append_xmlstr(vlbuf, mod.LoadedImageName); else { UNICODE_STRING nt_name; @@ -1618,15 +1727,15 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO if (IsWow64Process(gdbctx->process->handle, &is_wow64) && is_wow64 && (tmp = strstr(unix_path, "system32"))) memcpy(tmp, "syswow64", 8); - packet_reply_add(gdbctx, unix_path); + vl_append_xmlstr(vlbuf, unix_path); } else - packet_reply_add(gdbctx, mod.LoadedImageName); + vl_append_xmlstr(vlbuf, mod.LoadedImageName); HeapFree(GetProcessHeap(), 0, unix_path); RtlFreeUnicodeString(&nt_name); } - packet_reply_add(gdbctx, "\">"); + vl_append_str(vlbuf, "\">"); size = sizeof(buffer); if (VirtualQueryEx(gdbctx->process->handle, (void *)(UINT_PTR)mod.BaseOfImage, &mbi, sizeof(mbi)) >= sizeof(mbi) && @@ -1657,72 +1766,75 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO for (i = 0; i < max(nth->FileHeader.NumberOfSections, 1); ++i) { if ((char *)(sec + i) >= buffer + size) break; - packet_reply_add(gdbctx, "<segment address=\"0x"); - packet_reply_val(gdbctx, mod.BaseOfImage + sec[i].VirtualAddress, sizeof(ULONG_PTR)); - packet_reply_add(gdbctx, "\"/>"); + vl_append_str(vlbuf, "<segment address=\"0x"); + vl_append_uinthex(vlbuf, mod.BaseOfImage + sec[i].VirtualAddress, sizeof(ULONG_PTR)); + vl_append_str(vlbuf, "\"/>"); } - packet_reply_add(gdbctx, "</library>"); + vl_append_str(vlbuf, "</library>"); return TRUE; } static void packet_query_libraries(struct gdb_context* gdbctx) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; BOOL opt; /* this will resynchronize builtin dbghelp's internal ELF module list */ SymLoadModule(gdbctx->process->handle, 0, 0, 0, 0, 0); - packet_reply_add(gdbctx, "<library-list>"); + vl_append_str(vlbuf, "<library-list>"); opt = SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, TRUE); SymEnumerateModules64(gdbctx->process->handle, packet_query_libraries_cb, gdbctx); SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, opt); - packet_reply_add(gdbctx, "</library-list>"); + vl_append_str(vlbuf, "</library-list>"); } static void packet_query_threads(struct gdb_context* gdbctx) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread; - packet_reply_add(gdbctx, "<threads>"); + vl_append_str(vlbuf, "<threads>"); LIST_FOR_EACH_ENTRY(thread, &process->threads, struct dbg_thread, entry) { - packet_reply_add(gdbctx, "<thread "); - packet_reply_add(gdbctx, "id=\""); - packet_reply_val(gdbctx, thread->tid, 4); - packet_reply_add(gdbctx, "\" name=\""); - packet_reply_add(gdbctx, thread->name); - packet_reply_add(gdbctx, "\"/>"); + vl_append_str(vlbuf, "<thread "); + vl_append_str(vlbuf, "id=\""); + vl_append_uinthex(vlbuf, thread->tid, 4); + vl_append_str(vlbuf, "\" name=\""); + vl_append_str(vlbuf, thread->name); + vl_append_str(vlbuf, "\"/>"); } - packet_reply_add(gdbctx, "</threads>"); + vl_append_str(vlbuf, "</threads>"); } static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_cpu* cpu) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; const char* feature_prefix = NULL; const char* feature = NULL; char buffer[256]; int i; - packet_reply_add(gdbctx, "<target>"); + vl_append_str(vlbuf, "<target>"); switch (cpu->machine) { case IMAGE_FILE_MACHINE_AMD64: - packet_reply_add(gdbctx, "<architecture>i386:x86-64</architecture>"); + vl_append_str(vlbuf, "<architecture>i386:x86-64</architecture>"); feature_prefix = "org.gnu.gdb.i386."; break; case IMAGE_FILE_MACHINE_I386: - packet_reply_add(gdbctx, "<architecture>i386</architecture>"); + vl_append_str(vlbuf, "<architecture>i386</architecture>"); feature_prefix = "org.gnu.gdb.i386."; break; case IMAGE_FILE_MACHINE_ARMNT: - packet_reply_add(gdbctx, "<architecture>arm</architecture>"); + vl_append_str(vlbuf, "<architecture>arm</architecture>"); feature_prefix = "org.gnu.gdb.arm."; break; case IMAGE_FILE_MACHINE_ARM64: - packet_reply_add(gdbctx, "<architecture>aarch64</architecture>"); + vl_append_str(vlbuf, "<architecture>aarch64</architecture>"); feature_prefix = "org.gnu.gdb.aarch64."; break; } @@ -1731,17 +1843,17 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c { if (cpu->gdb_register_map[i].feature) { - if (feature) packet_reply_add(gdbctx, "</feature>"); + if (feature) vl_append_str(vlbuf, "</feature>"); feature = cpu->gdb_register_map[i].feature; - packet_reply_add(gdbctx, "<feature name=\""); - if (feature_prefix) packet_reply_add(gdbctx, feature_prefix); - packet_reply_add(gdbctx, feature); - packet_reply_add(gdbctx, "\">"); + vl_append_str(vlbuf, "<feature name=\""); + if (feature_prefix) vl_append_str(vlbuf, feature_prefix); + vl_append_str(vlbuf, feature); + vl_append_str(vlbuf, "\">"); if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 && strcmp(feature, "core") == 0) - packet_reply_add(gdbctx, "<flags id=\"i386_eflags\" size=\"4\">" + vl_append_str(vlbuf, "<flags id=\"i386_eflags\" size=\"4\">" "<field name=\"CF\" start=\"0\" end=\"0\"/>" "<field name=\"\" start=\"1\" end=\"1\"/>" "<field name=\"PF\" start=\"2\" end=\"2\"/>" @@ -1763,7 +1875,7 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 && strcmp(feature, "sse") == 0) - packet_reply_add(gdbctx, "<vector id=\"v4f\" type=\"ieee_single\" count=\"4\"/>" + vl_append_str(vlbuf, "<vector id=\"v4f\" type=\"ieee_single\" count=\"4\"/>" "<vector id=\"v2d\" type=\"ieee_double\" count=\"2\"/>" "<vector id=\"v16i8\" type=\"int8\" count=\"16\"/>" "<vector id=\"v8i16\" type=\"int16\" count=\"8\"/>" @@ -1798,20 +1910,20 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c snprintf(buffer, ARRAY_SIZE(buffer), "<reg name=\"%s\" bitsize=\"%Iu\"", cpu->gdb_register_map[i].name, 8 * cpu->gdb_register_map[i].length); - packet_reply_add(gdbctx, buffer); + vl_append_str(vlbuf, buffer); if (cpu->gdb_register_map[i].type) { - packet_reply_add(gdbctx, " type=\""); - packet_reply_add(gdbctx, cpu->gdb_register_map[i].type); - packet_reply_add(gdbctx, "\""); + vl_append_str(vlbuf, " type=\""); + vl_append_str(vlbuf, cpu->gdb_register_map[i].type); + vl_append_str(vlbuf, "\""); } - packet_reply_add(gdbctx, "/>"); + vl_append_str(vlbuf, "/>"); } - if (feature) packet_reply_add(gdbctx, "</feature>"); - packet_reply_add(gdbctx, "</target>"); + if (feature) vl_append_str(vlbuf, "</feature>"); + vl_append_str(vlbuf, "</target>"); } static enum packet_return packet_query(struct gdb_context* gdbctx) @@ -1941,32 +2053,56 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) case 'X': if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error; - packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_libraries(gdbctx); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; } if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error; - packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_threads(gdbctx); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; } if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error; if (!(cpu = gdbctx->process->be_cpu)) return packet_error; - packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_target_xml(gdbctx, cpu); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; } break; @@ -2283,6 +2419,8 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0; + memset(&gdbctx->qxfer_buffer, 0, sizeof(gdbctx->qxfer_buffer)); + /* wait for first trap */ while (WaitForDebugEvent(&gdbctx->de, INFINITE)) { -- 2.31.1
GDB does not retrieve the result of a qXfer command at once; instead, it issues a series of requests to obtain the result one "chunk" at a time, and concatenates those chunks internally. Each request contains offset and length variables that specify which portion of the result shall be retrieved. Today, Winedbg handles this by generating the entire result data each time a request is received and slicing out the requested range for the response. This is not only inefficient due to repeated computation, but also prone to race condition since the result may change between successive chunk requests due to the dynamic nature of some commands such as "libraries" and "threads." Fix this by cacheing the result into a buffer at the first request, and use the buffer to serve successive chunk requests. The cache is invalidated when the remote requests a different object, or the debugger reaches the end of the result cache buffer. Also replace the series of sscanf in packet_query with a dedicated qXfer request parser function and a lookup into a predefined handler table. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 235 ++++++++++++++++++++++++++---------- 1 file changed, 174 insertions(+), 61 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index caf64983c49..19ad76910af 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -61,6 +61,8 @@ struct vl_buffer size_t alloc; }; +#define MAX_QX_ANNEX_LEN 32 + struct gdb_context { /* gdb information */ @@ -89,6 +91,8 @@ struct gdb_context /* Unix environment */ ULONG_PTR wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ BOOL no_ack_mode; + int qxfer_object_idx; + char qxfer_object_annex[MAX_QX_ANNEX_LEN]; struct vl_buffer qxfer_buffer; }; @@ -807,7 +811,7 @@ static int addr_width(struct gdb_context* gdbctx) } enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, - packet_last_f = 0x80}; + packet_send_buffer = 0x03, packet_last_f = 0x80}; static void* buffer_realloc(void* buf, size_t size) { @@ -1776,11 +1780,16 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO return TRUE; } -static void packet_query_libraries(struct gdb_context* gdbctx) +static enum packet_return packet_query_libraries(struct gdb_context* gdbctx) { struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; BOOL opt; + if (!gdbctx->process) return packet_error; + + if (gdbctx->qxfer_object_annex[0]) + return packet_reply_error(gdbctx, 0); + /* this will resynchronize builtin dbghelp's internal ELF module list */ SymLoadModule(gdbctx->process->handle, 0, 0, 0, 0, 0); @@ -1789,14 +1798,21 @@ static void packet_query_libraries(struct gdb_context* gdbctx) SymEnumerateModules64(gdbctx->process->handle, packet_query_libraries_cb, gdbctx); SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, opt); vl_append_str(vlbuf, "</library-list>"); + + return packet_send_buffer; } -static void packet_query_threads(struct gdb_context* gdbctx) +static enum packet_return packet_query_threads(struct gdb_context* gdbctx) { struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread; + if (!process) return packet_error; + + if (gdbctx->qxfer_object_annex[0]) + return packet_reply_error(gdbctx, 0); + vl_append_str(vlbuf, "<threads>"); LIST_FOR_EACH_ENTRY(thread, &process->threads, struct dbg_thread, entry) { @@ -1808,11 +1824,12 @@ static void packet_query_threads(struct gdb_context* gdbctx) vl_append_str(vlbuf, "\"/>"); } vl_append_str(vlbuf, "</threads>"); + + return packet_send_buffer; } -static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_cpu* cpu) +static void packet_query_target_xml(struct gdb_context* gdbctx, struct vl_buffer *vlbuf, struct backend_cpu* cpu) { - struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; const char* feature_prefix = NULL; const char* feature = NULL; char buffer[256]; @@ -1926,10 +1943,89 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c vl_append_str(vlbuf, "</target>"); } +static enum packet_return packet_query_features(struct gdb_context* gdbctx) +{ + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; + struct dbg_process* process = gdbctx->process; + + if (!process) return packet_error; + + if (strncmp(gdbctx->qxfer_object_annex, "target.xml", MAX_QX_ANNEX_LEN) == 0) + { + struct backend_cpu *cpu = process->be_cpu; + if (!cpu) return packet_error; + + packet_query_target_xml(gdbctx, vlbuf, cpu); + + return packet_send_buffer; + } + + return packet_reply_error(gdbctx, 0); +} + +struct qxfer +{ + const char* name; + enum packet_return (*handler)(struct gdb_context* gdbctx); +} qxfer_handlers[] = +{ + {"libraries", packet_query_libraries}, + {"threads" , packet_query_threads }, + {"features" , packet_query_features }, +}; + +static BOOL parse_xfer_read(const char *ptr, char *name_buf, size_t name_buf_size, char *annex_buf, size_t annex_buf_size, unsigned int *offp, unsigned int *lenp) +{ + const char *name, *annex; + char *endptr; + size_t name_size, annex_size; + unsigned long off, len; + + if (strncmp(ptr, "Xfer:", 5) != 0) + return FALSE; + ptr += 5; + + name = ptr; + ptr = strchr(ptr, ':'); + if (!ptr || (name_size = ptr - name) >= name_buf_size) + return FALSE; + ptr++; + + if (strncmp(ptr, "read:", 5) != 0) + return FALSE; + ptr += 5; + + annex = ptr; + ptr = strchr(ptr, ':'); + if (!ptr || (annex_size = ptr - annex) >= annex_buf_size) + return FALSE; + ptr++; + + off = strtoul(ptr, &endptr, 16); + if (endptr == ptr || *endptr != ',' || off > (unsigned int)-1) + return FALSE; + ptr = endptr + 1; + + len = strtoul(ptr, &endptr, 16); + if (endptr == ptr || *endptr != '\0' || len > (unsigned int)-1) + return FALSE; + + memcpy(name_buf, name, name_size); + memset(name_buf + name_size, '\0', name_buf_size - name_size); + + memcpy(annex_buf, annex, annex_size); + memset(annex_buf + annex_size, '\0', annex_buf_size - annex_size); + + *offp = off; + *lenp = len; + + return TRUE; +} + static enum packet_return packet_query(struct gdb_context* gdbctx) { + char object_name[32], annex[MAX_QX_ANNEX_LEN]; unsigned int off, len; - struct backend_cpu *cpu; switch (gdbctx->in_packet[0]) { @@ -2016,11 +2112,16 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_ok; if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) { + size_t i; + packet_reply_open(gdbctx); packet_reply_add(gdbctx, "QStartNoAckMode+;"); - packet_reply_add(gdbctx, "qXfer:libraries:read+;"); - packet_reply_add(gdbctx, "qXfer:threads:read+;"); - packet_reply_add(gdbctx, "qXfer:features:read+;"); + for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++) + { + packet_reply_add(gdbctx, "qXfer:"); + packet_reply_add(gdbctx, qxfer_handlers[i].name); + packet_reply_add(gdbctx, ":read+;"); + } packet_reply_close(gdbctx); return packet_done; } @@ -2051,59 +2152,69 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) } break; case 'X': - if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2) + if (parse_xfer_read(gdbctx->in_packet, + object_name, sizeof(object_name), + annex, sizeof(annex), + &off, &len)) { - BOOL more; - - if (!gdbctx->process) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_libraries(gdbctx); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); - vl_empty(&gdbctx->qxfer_buffer); - return packet_done; - } - - if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2) - { - BOOL more; - - if (!gdbctx->process) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_threads(gdbctx); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); - vl_empty(&gdbctx->qxfer_buffer); - return packet_done; - } - - if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2) - { - BOOL more; - - if (!gdbctx->process) return packet_error; - if (!(cpu = gdbctx->process->be_cpu)) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_target_xml(gdbctx, cpu); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); - vl_empty(&gdbctx->qxfer_buffer); - return packet_done; + enum packet_return result; + int i; + + for (i = 0; ; i++) + { + if (i >= ARRAY_SIZE(qxfer_handlers)) + { + ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len); + return packet_error; + } + if (strcmp(qxfer_handlers[i].name, object_name) == 0) + break; + } + + TRACE("qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len); + + if (off > 0 && + gdbctx->qxfer_buffer.len > 0 && + gdbctx->qxfer_object_idx == i && + strncmp(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN) == 0) + { + result = packet_send_buffer; + TRACE("qXfer read result = %d (cached)\n", result); + } + else + { + vl_empty(&gdbctx->qxfer_buffer); + + gdbctx->qxfer_object_idx = i; + memcpy(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN); + + result = (*qxfer_handlers[i].handler)(gdbctx); + TRACE("qXfer read result = %d\n", result); + } + + if ((result & ~packet_last_f) == packet_send_buffer) + { + BOOL more; + + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + if (!more) + vl_empty(&gdbctx->qxfer_buffer); + + result = (result & packet_last_f) | packet_done; + } + else + { + gdbctx->qxfer_object_idx = -1; + memset(gdbctx->qxfer_object_annex, 0, MAX_QX_ANNEX_LEN); + vl_empty(&gdbctx->qxfer_buffer); + } + + return result; } break; } @@ -2419,6 +2530,8 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0; + gdbctx->qxfer_object_idx = -1; + memset(gdbctx->qxfer_object_annex, 0, sizeof(gdbctx->qxfer_object_annex)); memset(&gdbctx->qxfer_buffer, 0, sizeof(gdbctx->qxfer_buffer)); /* wait for first trap */ -- 2.31.1
Remove the necessity of using the `file` command to specify the executable. Otherwise, RHEL's downstream GDB complains with the following message: Remote gdbserver does not support determining executable automatically. RHEL <=6.8 and <=7.2 versions of gdbserver do not support such automatic executable detection. The following versions of gdbserver support it: - Upstream version of gdbserver (unsupported) 7.10 or later - Red Hat Developer Toolset (DTS) version of gdbserver from DTS 4.0 or later (only on x86_64) - RHEL-7.3 versions of gdbserver (on any architecture) Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- programs/winedbg/gdbproxy.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 19ad76910af..27f59aac259 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -99,7 +99,10 @@ struct gdb_context /* assume standard signal and errno values */ enum host_error { + HOST_EPERM = 1, + HOST_ENOENT = 2, HOST_ESRCH = 3, + HOST_ENOMEM = 12, HOST_EFAULT = 14, HOST_EINVAL = 22, }; @@ -1963,6 +1966,33 @@ static enum packet_return packet_query_features(struct gdb_context* gdbctx) return packet_reply_error(gdbctx, 0); } +static enum packet_return packet_query_exec_file(struct gdb_context* gdbctx) +{ + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; + struct dbg_process* process = gdbctx->process; + char *unix_path; + BOOL is_wow64; + char *tmp; + + if (!process) return packet_error; + + if (gdbctx->qxfer_object_annex[0] || !process->imageName) + return packet_reply_error(gdbctx, HOST_EPERM); + + if (!(unix_path = wine_get_unix_file_name(process->imageName))) + return packet_reply_error(gdbctx, GetLastError() == ERROR_NOT_ENOUGH_MEMORY ? HOST_ENOMEM : HOST_ENOENT); + + if (IsWow64Process(process->handle, &is_wow64) && + is_wow64 && (tmp = strstr(unix_path, "system32"))) + memcpy(tmp, "syswow64", 8); + + vl_append_str(vlbuf, unix_path); + + HeapFree(GetProcessHeap(), 0, unix_path); + + return packet_send_buffer; +} + struct qxfer { const char* name; @@ -1972,6 +2002,7 @@ struct qxfer {"libraries", packet_query_libraries}, {"threads" , packet_query_threads }, {"features" , packet_query_features }, + {"exec-file", packet_query_exec_file}, }; static BOOL parse_xfer_read(const char *ptr, char *name_buf, size_t name_buf_size, char *annex_buf, size_t annex_buf_size, unsigned int *offp, unsigned int *lenp) -- 2.31.1
participants (1)
-
Jinoh Kang