[PATCH 0/1] MR4776: winedbg: Wait for gdb to terminate before exiting (proxy mode).

[eric pouech (＠epo)]

This mainly allows Wine to reset the tty settings upon
termination and not let gdb do it (cf bug report).

Change: user is now required to explicitely terminate gdb
('quit' command) upon debuggee termination.

Wine-Bugs: https://bugs.winehq.org/show_bug.cgi?id=56032

Signed-off-by: Eric Pouech

[PATCH 1/1] winedbg: Wait for gdb to terminate before exiting (proxy mode).

[Eric Pouech]

From: Eric Pouech

This mainly allows Wine to reset the tty settings upon
termination and not let gdb do it (cf bug report).

Change: user is now required to explicitely terminate gdb
('quit' command) upon debuggee termination.

Wine-Bugs: https://bugs.winehq.org/show_bug.cgi?id=56032

Signed-off-by: Eric Pouech 
---
 programs/winedbg/gdbproxy.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 238eecb1538..2b750193a44 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -64,6 +64,7 @@ struct reply_buffer
 struct gdb_context
 {
     /* gdb information */
+    HANDLE                      gdb_ctrl_thread;
     SOCKET                      sock;
     /* incoming buffer */
     char*                       in_buf;
@@ -2391,10 +2392,15 @@ static int fetch_data(struct gdb_context* gdbctx)
     return gdbctx->in_len - in_len;
 }

+static DWORD WINAPI gdb_ctrl_thread(void *pmt)
+{
+    return __wine_unix_spawnvp( (char **)pmt, TRUE );
+}
+
 #define FLAG_NO_START   1
 #define FLAG_WITH_XTERM 2

-static BOOL gdb_exec(unsigned port, unsigned flags)
+static HANDLE gdb_exec(unsigned port, unsigned flags)
 {
     WCHAR tmp[MAX_PATH], buf[MAX_PATH];
     const char *argv[6];
@@ -2405,7 +2411,7 @@ static BOOL gdb_exec(unsigned port, unsigned flags)
     if (!(gdb_path = getenv("WINE_GDB"))) gdb_path = "gdb";
     GetTempPathW( MAX_PATH, buf );
     GetTempFileNameW( buf, L"gdb", 0, tmp );
-    if ((f = _wfopen( tmp, L"w+" )) == NULL) return FALSE;
+    if ((f = _wfopen( tmp, L"w+" )) == NULL) return NULL;
     unix_tmp = wine_get_unix_file_name( tmp );
     fprintf(f, "target remote localhost:%d\n", ntohs(port));
     fprintf(f, "set prompt Wine-gdb>\ \n");
@@ -2428,12 +2434,7 @@ static BOOL gdb_exec(unsigned port, unsigned flags)
     argv[3] = "-x";
     argv[4] = unix_tmp;
     argv[5] = NULL;
-    if (flags & FLAG_WITH_XTERM)
-        __wine_unix_spawnvp( (char **)argv, FALSE );
-    else
-        __wine_unix_spawnvp( (char **)argv + 2, FALSE );
-    HeapFree( GetProcessHeap(), 0, unix_tmp );
-    return TRUE;
+    return CreateThread( NULL, 0, gdb_ctrl_thread, (flags & FLAG_WITH_XTERM) ? argv : argv + 2, 0, NULL );
 }

static BOOL gdb_startup(struct gdb_context* gdbctx, unsigned flags, unsigned port)
@@ -2473,7 +2474,7 @@ static BOOL gdb_startup(struct gdb_context* gdbctx, unsigned flags, unsigned por
     if (flags & FLAG_NO_START)
         fprintf(stderr, "target remote localhost:%d\n", ntohs(s_addrs.sin_port));
     else
-        gdb_exec(s_addrs.sin_port, flags);
+        gdbctx->gdb_ctrl_thread = gdb_exec(s_addrs.sin_port, flags);

/* step 4: wait for gdb to connect actually */
     FD_ZERO( &read_fds );
@@ -2505,6 +2506,7 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne
 {
     int                 i;

+    gdbctx->gdb_ctrl_thread = NULL;
     gdbctx->sock = INVALID_SOCKET;
     gdbctx->in_buf = NULL;
     gdbctx->in_buf_alloc = 0;
@@ -2569,12 +2571,13 @@ static int gdb_remote(unsigned flags, unsigned port)
         }
         if (FD_ISSET( gdbctx.sock, &read_fds ))
         {
-            if (fetch_data(&gdbctx) > 0)
-            {
-                if (extract_packets(&gdbctx)) break;
-            }
+            if (fetch_data(&gdbctx) == 0) /* connection closed */
+                break;
+            extract_packets(&gdbctx);
         }
     }
+    /* connection closed, wait for gdb to terminate */
+    WaitForSingleObject(gdbctx.gdb_ctrl_thread, INFINITE);
     return 0;
 }

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about programs/winedbg/gdbproxy.c:
>  argv[3] = "-x";
>  argv[4] = unix_tmp;
>  argv[5] = NULL;
> - if (flags & FLAG_WITH_XTERM)
> -
>    __wine_unix_spawnvp( (char **)argv, FALSE );
> - else
> -
>    __wine_unix_spawnvp( (char **)argv + 2, FALSE );
> - HeapFree( GetProcessHeap(), 0, unix_tmp );
> - return TRUE;
> - return CreateThread( NULL, 0, gdb_ctrl_thread, (flags & FLAG_WITH_XTERM) ? argv : argv + 2, 0, NULL );

This passes the `argv` variable from the stack to the other thread.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about programs/winedbg/gdbproxy.c:
>      }
>      if (FD_ISSET( gdbctx.sock, &read_fds ))
>      {
> -
>        if (fetch_data(&gdbctx) > 0)
> -
>        {
> -
>            if (extract_packets(&gdbctx)) break;
> -
>        }
> -
>        if (fetch_data(&gdbctx) == 0) /* connection closed */
> -
>            break;
> -
>        extract_packets(&gdbctx);

The `extract_packets` return value is now ignored, it should either be made void, or used to break the loop too (I think the latter is better).

[Rémi Bernon (＠rbernon)]

On Wed Jan  3 17:13:08 2024 +0000, Rémi Bernon wrote:
> The `extract_packets` return value is now ignored, it should either be
> made void, or used to break the loop too (I think the latter is better).

Actually, I think this chunk might not even be needed? Shouldn't `select` fail already (or later) if connection has been closed?

[eric pouech (＠epo)]

On Wed Jan  3 17:14:37 2024 +0000, Rémi Bernon wrote:
> Actually, I think this chunk might not even be needed? Shouldn't
> `select` fail already (or later) if connection has been closed?

actually the select no longer detects the lost connexion. IMO, it's a left over from the unix socket/select => WS2 socket evolution. didn't want to go all the way, but the err_fds should be dropped as well.

and the point is to no longer terminate the gdb stub on debuggee exit (if one day we implement restart or run commands we'll need that), but wait for gdb termination instead.