[PATCH 1/4] winedbg: Set thread->name from MSVC exception when in GDB proxy mode.
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 0268a288481..bcde120adeb 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -482,7 +482,6 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e { const THREADNAME_INFO *threadname = (const THREADNAME_INFO *)rec->ExceptionInformation; struct dbg_thread *thread; - char name[9]; SIZE_T read; if (threadname->dwType != 0x1000) @@ -494,10 +493,12 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e if (thread) { if (gdbctx->process->process_io->read( gdbctx->process->handle, - threadname->szName, name, sizeof(name), &read) && read == sizeof(name)) + threadname->szName, thread->name, sizeof(thread->name), &read) && + read == sizeof(thread->name)) { - fprintf(stderr, "Thread ID=%04lx renamed to \"%.9s\"\n", - threadname->dwThreadID, name); + thread->name[sizeof(thread->name) - 1] = '\0'; + fprintf(stderr, "Thread ID=%04lx renamed to \"%s\"\n", + threadname->dwThreadID, thread->name); } } else -- 2.34.1
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- I don't believe the 9 character limit has been relevant since MSVC 6. Current applications use much longer thread names (like 'ThreadPoolSingleThreadCOMSTASharedForegroundBlocking1' in Chromium). If memory usage is a concern, thread->name could be dynamically allocated instead. programs/winedbg/debugger.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/winedbg/debugger.h b/programs/winedbg/debugger.h index 0cc2f8b7a6c..c528dd97674 100644 --- a/programs/winedbg/debugger.h +++ b/programs/winedbg/debugger.h @@ -182,7 +182,7 @@ struct dbg_breakpoint typedef struct tagTHREADNAME_INFO { DWORD dwType; /* Must be 0x1000 */ - LPCSTR szName; /* Pointer to name - limited to 9 bytes (8 characters + terminator) */ + LPCSTR szName; /* Pointer to name (in user addr space). */ DWORD dwThreadID; /* Thread ID (-1 = caller thread) */ DWORD dwFlags; /* Reserved for future use. Must be zero. */ } THREADNAME_INFO; @@ -205,7 +205,7 @@ struct dbg_thread ADDRESS_MODE addr_mode; /* mode */ int stopped_xpoint; /* xpoint on which the thread has stopped (-1 if none) */ struct dbg_breakpoint step_over_bp; - char name[9]; + char name[64]; BOOL in_exception; /* TRUE if thread stopped with an exception */ BOOL first_chance; /* TRUE if thread stopped with a first chance exception * - only valid when in_exception is TRUE -- 2.34.1
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bcde120adeb..60c69f81d23 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1773,11 +1773,40 @@ static enum packet_return packet_query_libraries(struct gdb_context* gdbctx) return packet_send_buffer; } +static char *get_thread_description(DWORD tid) +{ + HANDLE h; + char *desc = NULL; + WCHAR *descW = NULL; + int len; + + h = OpenThread(THREAD_QUERY_LIMITED_INFORMATION, FALSE, tid); + if (!h) + return NULL; + + GetThreadDescription(h, &descW); + if (!descW) + goto cleanup; + + len = WideCharToMultiByte(CP_ACP, 0, descW, -1, NULL, 0, NULL, NULL); + if (len <= 1) /* failure or empty string */ + goto cleanup; + + desc = malloc(len); + WideCharToMultiByte(CP_ACP, 0, descW, -1, desc, len, NULL, NULL); + +cleanup: + LocalFree(descW); + CloseHandle(h); + return desc; +} + static enum packet_return packet_query_threads(struct gdb_context* gdbctx) { struct reply_buffer* reply = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread; + char *name; if (!process) return packet_error; @@ -1791,7 +1820,12 @@ static enum packet_return packet_query_threads(struct gdb_context* gdbctx) reply_buffer_append_str(reply, "id=\""); reply_buffer_append_uinthex(reply, thread->tid, 4); reply_buffer_append_str(reply, "\" name=\""); - if (strlen(thread->name)) + if ((name = get_thread_description(thread->tid))) + { + reply_buffer_append_str(reply, name); + free(name); + } + else if (strlen(thread->name)) { reply_buffer_append_str(reply, thread->name); } -- 2.34.1
Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
Signed-off-by: Brendan Shanks<bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bcde120adeb..60c69f81d23 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1773,11 +1773,40 @@ static enum packet_return packet_query_libraries(struct gdb_context* gdbctx) return packet_send_buffer; }
+static char *get_thread_description(DWORD tid) +{ + HANDLE h; + char *desc = NULL; + WCHAR *descW = NULL; + int len; + + h = OpenThread(THREAD_QUERY_LIMITED_INFORMATION, FALSE, tid); + if (!h) + return NULL; + + GetThreadDescription(h, &descW); + if (!descW) + goto cleanup; + + len = WideCharToMultiByte(CP_ACP, 0, descW, -1, NULL, 0, NULL, NULL); + if (len <= 1) /* failure or empty string */ + goto cleanup; + + desc = malloc(len); + WideCharToMultiByte(CP_ACP, 0, descW, -1, desc, len, NULL, NULL); + +cleanup: + LocalFree(descW); + CloseHandle(h); + return desc; +} + static enum packet_return packet_query_threads(struct gdb_context* gdbctx) { struct reply_buffer* reply = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread; + char *name;
if (!process) return packet_error;
@@ -1791,7 +1820,12 @@ static enum packet_return packet_query_threads(struct gdb_context* gdbctx) reply_buffer_append_str(reply, "id=\""); reply_buffer_append_uinthex(reply, thread->tid, 4); reply_buffer_append_str(reply, "\" name=\""); - if (strlen(thread->name)) + if ((name = get_thread_description(thread->tid))) + { + reply_buffer_append_str(reply, name); + free(name); + } + else if (strlen(thread->name)) { reply_buffer_append_str(reply, thread->name); }
it's a bit ackward to add two implementations of get_thread_description ; moreover differing in ansi/unicode but also memory allocation strategy it would be better to only have one (esp when considering that GetThreadDescription is only avail in W10) IMO a single helper would be preferable A+
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/info.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/programs/winedbg/info.c b/programs/winedbg/info.c index 2b8e377e6f4..1c75280aeec 100644 --- a/programs/winedbg/info.c +++ b/programs/winedbg/info.c @@ -581,6 +581,26 @@ static BOOL get_process_name(DWORD pid, PROCESSENTRY32* entry) return ret; } +static WCHAR *get_thread_description(DWORD tid) +{ + HANDLE h; + WCHAR *desc = NULL; + + h = OpenThread(THREAD_QUERY_LIMITED_INFORMATION, FALSE, tid); + if (!h) + return NULL; + + GetThreadDescription(h, &desc); + CloseHandle(h); + + if (desc && desc[0] == '\0') + { + LocalFree(desc); + return NULL; + } + return desc; +} + void info_win32_threads(void) { HANDLE snap = CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0); @@ -591,6 +611,7 @@ void info_win32_threads(void) DWORD lastProcessId = 0; struct dbg_process* p = NULL; struct dbg_thread* t = NULL; + WCHAR *description; entry.dwSize = sizeof(entry); ok = Thread32First(snap, &entry); @@ -622,12 +643,20 @@ void info_win32_threads(void) entry.th32OwnerProcessID, p ? " (D)" : "", exename); lastProcessId = entry.th32OwnerProcessID; } - t = dbg_get_thread(p, entry.th32ThreadID); - dbg_printf("\t%08lx %4ld%s %s\n", + dbg_printf("\t%08lx %4ld%s ", entry.th32ThreadID, entry.tpBasePri, - (entry.th32ThreadID == dbg_curr_tid) ? " <==" : " ", - t ? t->name : ""); + (entry.th32ThreadID == dbg_curr_tid) ? " <==" : " "); + if ((description = get_thread_description(entry.th32ThreadID))) + { + dbg_printf("%ls\n", description); + LocalFree(description); + } + else + { + t = dbg_get_thread(p, entry.th32ThreadID); + dbg_printf("%s\n", t ? t->name : ""); + } } ok = Thread32Next(snap, &entry); } -- 2.34.1
Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 0268a288481..bcde120adeb 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -482,7 +482,6 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e { const THREADNAME_INFO *threadname = (const THREADNAME_INFO *)rec->ExceptionInformation; struct dbg_thread *thread; - char name[9]; SIZE_T read;
if (threadname->dwType != 0x1000) @@ -494,10 +493,12 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e if (thread) { if (gdbctx->process->process_io->read( gdbctx->process->handle, - threadname->szName, name, sizeof(name), &read) && read == sizeof(name)) + threadname->szName, thread->name, sizeof(thread->name), &read) && + read == sizeof(thread->name))
it looks a bit strange to me that we can always expect being able to read sizeof(thread->name) here... using existing memory_get_string helper might be a better idea A+
On Mar 16, 2022, at 12:20 AM, Eric Pouech <eric.pouech(a)orange.fr> wrote:
Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 0268a288481..bcde120adeb 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -482,7 +482,6 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e { const THREADNAME_INFO *threadname = (const THREADNAME_INFO *)rec->ExceptionInformation; struct dbg_thread *thread; - char name[9]; SIZE_T read; if (threadname->dwType != 0x1000) @@ -494,10 +493,12 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e if (thread) { if (gdbctx->process->process_io->read( gdbctx->process->handle, - threadname->szName, name, sizeof(name), &read) && read == sizeof(name)) + threadname->szName, thread->name, sizeof(thread->name), &read) && + read == sizeof(thread->name))
it looks a bit strange to me that we can always expect being able to read sizeof(thread->name) here...
using existing memory_get_string helper might be a better idea
Thanks, I hadn’t seen that function before, I’ll use it. I think the end result will be the same though, since memory_get_string() uses the same read() that's implemented with ReadProcessMemory(), which doesn’t do partial reads. ReadProcessMemory() could fail if sizeof(thread->name) would overflow into an inaccessible page, but for a convenience feature like thread names (and this is the old/deprecated way of setting them) I’m not sure it’s worth handling that rare case. Brendan
Le 16/03/2022 à 21:13, Brendan Shanks a écrit :
On Mar 16, 2022, at 12:20 AM, Eric Pouech <eric.pouech(a)orange.fr> wrote:
Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
Signed-off-by: Brendan Shanks <bshanks(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 0268a288481..bcde120adeb 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -482,7 +482,6 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e { const THREADNAME_INFO *threadname = (const THREADNAME_INFO *)rec->ExceptionInformation; struct dbg_thread *thread; - char name[9]; SIZE_T read; if (threadname->dwType != 0x1000) @@ -494,10 +493,12 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e if (thread) { if (gdbctx->process->process_io->read( gdbctx->process->handle, - threadname->szName, name, sizeof(name), &read) && read == sizeof(name)) + threadname->szName, thread->name, sizeof(thread->name), &read) && + read == sizeof(thread->name))
it looks a bit strange to me that we can always expect being able to read sizeof(thread->name) here...
using existing memory_get_string helper might be a better idea
Thanks, I hadn’t seen that function before, I’ll use it. I think the end result will be the same though, since memory_get_string() uses the same read() that's implemented with ReadProcessMemory(), which doesn’t do partial reads. ReadProcessMemory() could fail if sizeof(thread->name) would overflow into an inaccessible page, but for a convenience feature like thread names (and this is the old/deprecated way of setting them) I’m not sure it’s worth handling that rare case.
sounds reasonable (we could evolve memory_get_string if that ever happens for any kind of string) A+
participants (2)
-
Brendan Shanks -
Eric Pouech