[3/3] Windows: Fix run/attach hang after bad run/attach

  On Cygwin, gdb.base/attach.exp exposes that a attach after a
previously failed attach hangs:

 (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited
 attach 0
 Can't attach to process 0 (error 2: The system cannot find the file specified.)
 (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited
 attach 10644
 FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout)

The problem is that windows_nat_target::attach always returns success
even if the attach fails.  When we return success, the helper thread
begins waiting for events (which will never come), and thus the next
attach deadlocks on the do_synchronously call within
windows_nat_target::attach.

"run" has the same problem, which is exposed by the new
gdb.base/run-fail-twice.exp testcase:

 (gdb) run
 Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
 Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.)
 (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1
 run
 Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
 FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout)

The problem here is the same, except that this time it is
windows_nat_target::create_inferior that returns the incorrect result.

This commit fixes both the "attach" and "run" paths.  The tests
mentioned above now pass on Cygwin.

Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239
---
 gdb/windows-nat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
  

Am Montag, 12. Februar 2024 um 21:02:54 MEZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> On Cygwin, gdb.base/attach.exp exposes that a attach after a
> previously failed attach hangs:
>
> (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited
> attach 0
> Can't attach to process 0 (error 2: The system cannot find the file specified.)
> (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited
> attach 10644
> FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout)
>
> The problem is that windows_nat_target::attach always returns success
> even if the attach fails.  When we return success, the helper thread
> begins waiting for events (which will never come), and thus the next
> attach deadlocks on the do_synchronously call within
> windows_nat_target::attach.
>
> "run" has the same problem, which is exposed by the new
> gdb.base/run-fail-twice.exp testcase:
>
> (gdb) run
> Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
> Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.)
> (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1
> run
> Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
> FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout)
>
> The problem here is the same, except that this time it is
> windows_nat_target::create_inferior that returns the incorrect result.
>
> This commit fixes both the "attach" and "run" paths.  The tests
> mentioned above now pass on Cygwin.
>
> Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239
> ---
> gdb/windows-nat.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index b446afd72d8..5d2e23600e3 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -2043,7 +2043,7 @@ windows_nat_target::attach (const char *args, int from_tty)
>       if (!ok)
>     err = (unsigned) GetLastError ();
>
> -      return true;
> +      return ok;
>     });
>
>   if (err.has_value ())
> @@ -2642,12 +2642,15 @@ windows_nat_target::create_inferior (const char *exec_file,
>   windows_init_thread_list ();
>   do_synchronously ([&] ()
>     {
> -      if (!create_process (nullptr, args, flags, w32_env,
> -              inferior_cwd != nullptr ? infcwd : nullptr,
> -              disable_randomization,
> -              &si, &pi))
> +      BOOL ok = create_process (nullptr, args, flags, w32_env,
> +                inferior_cwd != nullptr ? infcwd : nullptr,
> +                disable_randomization,
> +                &si, &pi);
> +
> +      if (!ok)
>     ret = (unsigned) GetLastError ();
> -      return true;
> +
> +      return ok;
>     });
>
>   if (w32_env)
>
> --
> 2.43.0

The same problem also exists in the !__CYGWIN__ branch, I ran into this
multiple times already.


Hannes
  

On 2024-02-12 20:14, Hannes Domani wrote:

> The same problem also exists in the !__CYGWIN__ branch, I ran into this
> multiple times already.

Wow, the #ifdef region is so long that I didn't notice this particular code was Cygwin-only.

I had put "Windows" in the subject as I thought I was fixing native Windows too...

I'll go fix the !__CYGWIN__ branches too.

Thanks!

Pedro Alves
  

On 2024-02-13 12:20, Pedro Alves wrote:
> On 2024-02-12 20:14, Hannes Domani wrote:
> 
>> The same problem also exists in the !__CYGWIN__ branch, I ran into this
>> multiple times already.
> 
> Wow, the #ifdef region is so long that I didn't notice this particular code was Cygwin-only.
> 
> I had put "Windows" in the subject as I thought I was fixing native Windows too...
> 
> I'll go fix the !__CYGWIN__ branches too.
> 

Here's an updated version that now handles the !__CYGWIN__ branch too.  I confirmed manually
that I saw the hangs on MinGW gdb without the fix, and that both the "attach" and "run" hangs go away
with this version patch (the previous version still hanged with "run").


---- 8< ----
From af5cae1e2c0bf2cac2c5178d45f70eae5795cf8f Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Subject: [PATCH] Windows: Fix run/attach hang after bad run/attach

On Cygwin, gdb.base/attach.exp exposes that a attach after a
previously failed attach hangs:

 (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to digits-starting nonsense is prohibited
 attach 0
 Can't attach to process 0 (error 2: The system cannot find the file specified.)
 (gdb) PASS: gdb.base/attach.exp: do_attach_failure_tests: attach to nonexistent process is prohibited
 attach 10644
 FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach (timeout)

The problem is that windows_nat_target::attach always returns success
even if the attach fails.  When we return success, the helper thread
begins waiting for events (which will never come), and thus the next
attach deadlocks on the do_synchronously call within
windows_nat_target::attach.

"run" has the same problem, which is exposed by the new
gdb.base/run-fail-twice.exp testcase:

 (gdb) run
 Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
 Error creating process .../gdb.base/run-fail-twice/run-fail-twice.nox, (error 6: The handle is invalid.)
 (gdb) PASS: gdb.base/run-fail-twice.exp: test: bad run 1
 run
 Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
 FAIL: gdb.base/run-fail-twice.exp: test: bad run 2 (timeout)

The problem here is the same, except that this time it is
windows_nat_target::create_inferior that returns the incorrect result.

This commit fixes both the "attach" and "run" paths, and the latter
both the Cygwin and MinGW paths.  The tests mentioned above now pass
on Cygwin.  Confirmed the fixes manually for MinGW GDB.

Change-Id: I15ec9fa279aff269d4982b00f4ea7c25ae917239
---
 gdb/windows-nat.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 7f3044fc61d..5c47dd40738 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2059,7 +2059,7 @@ windows_nat_target::attach (const char *args, int from_tty)
       if (!ok)
 	err = (unsigned) GetLastError ();
 
-      return true;
+      return ok;
     });
 
   if (err.has_value ())
@@ -2664,12 +2664,15 @@ windows_nat_target::create_inferior (const char *exec_file,
   windows_init_thread_list ();
   do_synchronously ([&] ()
     {
-      if (!create_process (nullptr, args, flags, w32_env,
-			   inferior_cwd != nullptr ? infcwd : nullptr,
-			   disable_randomization,
-			   &si, &pi))
+      BOOL ok = create_process (nullptr, args, flags, w32_env,
+				inferior_cwd != nullptr ? infcwd : nullptr,
+				disable_randomization,
+				&si, &pi);
+
+      if (!ok)
 	ret = (unsigned) GetLastError ();
-      return true;
+
+      return ok;
     });
 
   if (w32_env)
@@ -2790,16 +2793,18 @@ windows_nat_target::create_inferior (const char *exec_file,
   windows_init_thread_list ();
   do_synchronously ([&] ()
     {
-      if (!create_process (nullptr, /* image */
-			   args,	/* command line */
-			   flags,	/* start flags */
-			   w32env,	/* environment */
-			   inferior_cwd, /* current directory */
-			   disable_randomization,
-			   &si,
-			   &pi))
+      BOOL ok = create_process (nullptr, /* image */
+				args,	/* command line */
+				flags,	/* start flags */
+				w32env,	/* environment */
+				inferior_cwd, /* current directory */
+				disable_randomization,
+				&si,
+				&pi);
+      if (!ok)
 	ret = (unsigned) GetLastError ();
-      return true;
+
+      return ok;
     });
   if (tty != INVALID_HANDLE_VALUE)
     CloseHandle (tty);

base-commit: a16034bf6417dc2259fef43fd5bcc2dd1dac562f