diff mbox series

Windows gdb: Avoid hang second attach/run

Message ID 20260505121823.1442331-1-pedro@palves.net
State New
Headers
Series Windows gdb: Avoid hang second attach/run |

Commit Message

Pedro Alves May 5, 2026, 12:18 p.m. UTC 
  gdb.base/attach.exp starts a second inferior and tries to attach the
second inferior to the same process that inferior 1 is already
debugging.  The point is to make sure that the backend errors out when
it tries to attach to a process that is already being debugged.

windows_nat_target::attach and windows_nat_target::create_inferior
both hang in this situation, because they call into do_synchronously,
which hangs because the 'process_thread' thread is blocked in
WaitForDebugEvent.

E.g.:

  attach 4420
  FAIL: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again (timeout)

Until the Windows backend is taught to debug multiple processes, which
will probably require having one process_thread thread per inferior,
detect the situation and error out before GDB hangs.

This results in the following progression in gdb.base/attach.exp:

 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again (timeout)
 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: set confirm off (timeout)
 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: switch to inferior 1 (timeout)
 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: exit after attach failures (timeout)
 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: gdb_breakpoint: set breakpoint at main (timeout)
 -FAIL: gdb.base/attach.exp: do_attach_failure_tests: stop at main (timeout)
 +PASS: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again
 +PASS: gdb.base/attach.exp: do_attach_failure_tests: set confirm off
 +PASS: gdb.base/attach.exp: do_attach_failure_tests: switch to inferior 1
 +PASS: gdb.base/attach.exp: do_attach_failure_tests: exit after attach failures
 +PASS: gdb.base/attach.exp: do_attach_failure_tests: stop at main

There are still other failures not addressed by this patch.

Tests that launch a second program with "run" exist, but are normally
gated by allow_multi_inferior_tests.

Change-Id: I55b4438795439673c49fa55f55ddf0191f4f0ea8
commit-id: 1caa5be0
---
 gdb/testsuite/gdb.base/attach.exp |  4 ++++
 gdb/windows-nat.c                 | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)


base-commit: 8c0ac471835ec86a67c5b42713d9f138f31e4014

Comments

Tom Tromey May 5, 2026, 2:05 p.m. UTC | #1 
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> windows_nat_target::attach and windows_nat_target::create_inferior
Pedro> both hang in this situation, because they call into do_synchronously,
Pedro> which hangs because the 'process_thread' thread is blocked in
Pedro> WaitForDebugEvent.

Do we need something similar for gdbserver?

Pedro> Until the Windows backend is taught to debug multiple processes, which
Pedro> will probably require having one process_thread thread per inferior,
Pedro> detect the situation and error out before GDB hangs.

Yeah.  I've never understood why MS did things this way instead of the
seemingly obvious approach of having debug events integrated into
WaitForMultipleObjects.

Pedro> There are still other failures not addressed by this patch.

FWIW the internal AdaCore automated testing shows a number of failures
after a merge on 20260427.  I guess when I did my testing I happened to
pick the one particular OS instance that had no problems :(

Tom
Tom Tromey May 5, 2026, 2:05 p.m. UTC | #2 
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> gdb.base/attach.exp starts a second inferior and tries to attach the
Pedro> second inferior to the same process that inferior 1 is already
Pedro> debugging.  The point is to make sure that the backend errors out when
Pedro> it tries to attach to a process that is already being debugged.

Pedro> windows_nat_target::attach and windows_nat_target::create_inferior
Pedro> both hang in this situation, because they call into do_synchronously,
Pedro> which hangs because the 'process_thread' thread is blocked in
Pedro> WaitForDebugEvent.

Forgot the tag.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
Pedro Alves May 7, 2026, 6:24 p.m. UTC | #3 
On 2026-05-05 15:05, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> windows_nat_target::attach and windows_nat_target::create_inferior
> Pedro> both hang in this situation, because they call into do_synchronously,
> Pedro> which hangs because the 'process_thread' thread is blocked in
> Pedro> WaitForDebugEvent.
> 
> Do we need something similar for gdbserver?

We don't, because gdbserver does not have the do_synchronously machinery for async, and, also,
because core gdbserver rejects the attach earlier:

  if (startswith (own_buf, "vAttach;"))
    {
      if ((!extended_protocol || !cs.multi_process) && target_running ())
	{
	  fprintf (stderr, "Already debugging a process\n");
	  write_enn (own_buf);
	  return;
	}

We get:

 attach 22600
 Attaching to Remote target
 Attaching to Remote target failed: 01
 (gdb) FAIL: gdb.base/attach.exp: do_attach_failure_tests: fail to attach again

BTW, I'm been thinking that the slow piecemeal sharing of code between gdb and gdbserver's Windows
backends is a lost cause by now.  The backends started diverging a lot with the async support,
and more so now with the non-stop support.  They used to be very similar before.

I've been thinking that it'll be easier to completely dump gdbserver/win32-low.c, make
gdbserver build gdb/windows-nat.c (and friends), start with some #ifdefs, and then work on
eliminating the #ifdefs incrementally with some abstractions and/or normalizing gdb and
gdbserver core<=>backend interfaces more.

> 
> Pedro> Until the Windows backend is taught to debug multiple processes, which
> Pedro> will probably require having one process_thread thread per inferior,
> Pedro> detect the situation and error out before GDB hangs.
> 
> Yeah.  I've never understood why MS did things this way instead of the
> seemingly obvious approach of having debug events integrated into
> WaitForMultipleObjects.
> 
> Pedro> There are still other failures not addressed by this patch.
> 
> FWIW the internal AdaCore automated testing shows a number of failures
> after a merge on 20260427.  I guess when I did my testing I happened to
> pick the one particular OS instance that had no problems :(
Ouch, I was indeed surprised that you saw no problems.  :-/

(To be clear, the failures I mean above are pre-existing failures in gdb.base/attach.exp.)

Pedro Alves
Tom Tromey May 8, 2026, 12:24 p.m. UTC | #4 
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> I've been thinking that it'll be easier to completely dump
Pedro> gdbserver/win32-low.c, make gdbserver build gdb/windows-nat.c
Pedro> (and friends), start with some #ifdefs, and then work on
Pedro> eliminating the #ifdefs incrementally with some abstractions
Pedro> and/or normalizing gdb and gdbserver core<=>backend interfaces
Pedro> more.

The main thing for me is that we share code and don't let things diverge
too much.

We should probably also revisit the idea of just always using a
gdbserver.

>> FWIW the internal AdaCore automated testing shows a number of failures
>> after a merge on 20260427.  I guess when I did my testing I happened to
>> pick the one particular OS instance that had no problems :(

Pedro> Ouch, I was indeed surprised that you saw no problems.  :-/

I'll send a note about this later.

Tom
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
index bffdfd9b1aa..a4e7d72ef94 100644
--- a/gdb/testsuite/gdb.base/attach.exp
+++ b/gdb/testsuite/gdb.base/attach.exp
@@ -175,6 +175,10 @@  proc_with_prefix do_attach_failure_tests {} {
 	    # Response expected when using gdbserver.
 	    pass "$test"
 	}
+	-re -wrap "Can only debug one process at a time\\." {
+	    # Response expected on Windows.
+	    pass "$test"
+	}
     }
 
     # To ensure the target is still alive and working after this, try to run
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a9647e90bb8..6b2a5d30132 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2010,14 +2010,24 @@  set_process_privilege (const char *privilege, BOOL enable)
   return ret;
 }
 
+/* Throw an error if we're already debugging a Windows process.  We
+   can only debug one at a time currently.  */
+
+static void
+ensure_only_one_process ()
+{
+  if (windows_process->process_id != 0)
+    error (_("Can only debug one process at a time."));
+}
+
 /* Attach to process PID, then initialize for debugging it.  */
 
 void
 windows_nat_target::attach (const char *args, int from_tty)
 {
-  DWORD pid;
+  ensure_only_one_process ();
 
-  pid = parse_pid_to_attach (args);
+  DWORD pid = parse_pid_to_attach (args);
 
   if (set_process_privilege (SE_DEBUG_NAME, TRUE) < 0)
     warning ("Failed to get SE_DEBUG_NAME privilege\n"
@@ -2369,6 +2379,8 @@  windows_nat_target::detach (inferior *inf, int from_tty)
   switch_to_no_thread ();
   detach_inferior (inf);
 
+  windows_process->process_id = 0;
+
   maybe_unpush_target ();
 }
 
@@ -2811,6 +2823,8 @@  windows_nat_target::create_inferior (const char *exec_file,
   DWORD flags = 0;
   const std::string &inferior_tty = current_inferior ()->tty ();
 
+  ensure_only_one_process ();
+
   if (!exec_file)
     error (_("No executable specified, use `target exec'."));
 
@@ -3120,6 +3134,7 @@  windows_nat_target::mourn_inferior ()
       CHECK (CloseHandle (windows_process->handle));
       windows_process->open_process_used = 0;
     }
+  windows_process->process_id = 0;
   inf_child_target::mourn_inferior ();
 }