[PATCH 1/4] winedbg: Set thread->name from MSVC exception when in GDB proxy mode.

[Brendan Shanks]

Signed-off-by: Brendan Shanks 
---
 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

[PATCH 2/4] winedbg: Enlarge thread->name to 64 bytes.

[Brendan Shanks]

Signed-off-by: Brendan Shanks 
---

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

[PATCH 3/4] winedbg: Prefer thread name from GetThreadDescription() in GDB proxy mode.

[Brendan Shanks]

Signed-off-by: Brendan Shanks 
---
 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);
         }

[PATCH 3/4] winedbg: Prefer thread name from GetThreadDescription() in GDB proxy mode.

[Eric Pouech]

Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
> Signed-off-by: Brendan Shanks
>
> 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+

[PATCH 4/4] winedbg: Prefer thread name from GetThreadDescription() in 'info thread' listing.

[Brendan Shanks]

Signed-off-by: Brendan Shanks 
---
 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);
         }

[Eric Pouech]

Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
> Signed-off-by: Brendan Shanks
>
> 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+

[Brendan Shanks]

> On Mar 16, 2022, at 12:20 AM, Eric Pouech  wrote:
>
> Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
> > Signed-off-by: Brendan Shanks
> >
> > 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

[Eric Pouech]

Le 16/03/2022 à 21:13, Brendan Shanks a écrit :
> > On Mar 16, 2022, at 12:20 AM, Eric Pouech  wrote:
> >
> > Le 15/03/2022 à 20:05, Brendan Shanks a écrit :
> > > Signed-off-by: Brendan Shanks
> > >
> > > 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+