gdb/Windows: Fix detach while running

  While testing a WIP Cygwin GDB that supports non-stop, I noticed that
gdb.threads/attach-non-stop.exp exposes that this:

 (gdb) attach PID&
 ...
 (gdb) detach

... hangs.

And it turns out that it hangs in all-stop as well.  This commits
fixes that.

After "attach &", the target is set running, we've called
ContinueDebugEvent and the process_thread thread is waiting for
WaitForDebugEvent events.  It is the equivalent of "attach; c&".

In windows_nat_target::detach, the first thing we do is
unconditionally call windows_continue (for ContinueDebugEvent), which
blocks in do_synchronously, until the process_thread sees an event out
of WaitForDebugEvent.  Unless the inferior happens to run into a
breakpoint, etc., then this hangs indefinitely.

If we've already called ContinueDebugEvent earlier, then we shouldn't
be calling it again in ::detach.

Still in windows_nat_target::detach, we have an interesting issue that
ends up being the bulk of the patch -- only the process_thread thread
can call DebugActiveProcessStop, but if it is blocked in
WaitForDebugEvent, we need to somehow force it to break out of it.
The only way to do that, is to force the inferior to do something that
causes WaitForDebugEvent to return some event.

This patch uses CreateRemoteThread to do it, which results in
WaitForDebugEvent reporting CREATE_THREAD_DEBUG_EVENT.  We then
terminate the injected thread before it has a chance to run any
userspace code.

Note that Win32 functions like DebugBreakProcess and
GenerateConsoleCtrlEvent would also inject a new thread in the
inferior.  I first used DebugBreakProcess, but that is actually more
complicated to use, because we'd have to be sure to consume the
breakpoint event before detaching, otherwise the inferior would likely
die due a breakpoint exception being raised with no debugger around to
intercept it.

See the new break_out_process_thread method.

So the fix has two parts:

 - Keep track of whether we've called ContinueDebugEvent and the
   process_thread thread is waiting for events, or whether
   WaitForDebugEvent already returned an event.

 - In windows_nat_target::detach, if the process_thread thread is
   waiting for events, unblock out of its WaitForDebugEvent, before
   proceeding with the actual detach.

New test included.  Passes cleanly on GNU/Linux native and gdbserver,
and also passes cleanly on Cygwin, with the fix.  Before the fix, it
would hang and fail with a timeout.

Change-Id: Ifb91c58c08af1a9bcbafecedc93dfce001040905
---
 .../gdb.threads/detach-while-running.c        |  30 ++++
 .../gdb.threads/detach-while-running.exp      |  77 ++++++++++
 gdb/windows-nat.c                             | 131 ++++++++++++++++--
 3 files changed, 230 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/detach-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/detach-while-running.exp


base-commit: 02d02fc7924992ddd98073b95810b957efdc421a
  

Am Donnerstag, 11. April 2024 um 22:04:34 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> While testing a WIP Cygwin GDB that supports non-stop, I noticed that
> gdb.threads/attach-non-stop.exp exposes that this:
>
> (gdb) attach PID&
> ...
> (gdb) detach
>
> ... hangs.
>
> And it turns out that it hangs in all-stop as well.  This commits
> fixes that.
>
> After "attach &", the target is set running, we've called
> ContinueDebugEvent and the process_thread thread is waiting for
> WaitForDebugEvent events.  It is the equivalent of "attach; c&".
>
> In windows_nat_target::detach, the first thing we do is
> unconditionally call windows_continue (for ContinueDebugEvent), which
> blocks in do_synchronously, until the process_thread sees an event out
> of WaitForDebugEvent.  Unless the inferior happens to run into a
> breakpoint, etc., then this hangs indefinitely.
>
> If we've already called ContinueDebugEvent earlier, then we shouldn't
> be calling it again in ::detach.
>
> Still in windows_nat_target::detach, we have an interesting issue that
> ends up being the bulk of the patch -- only the process_thread thread
> can call DebugActiveProcessStop, but if it is blocked in
> WaitForDebugEvent, we need to somehow force it to break out of it.
> The only way to do that, is to force the inferior to do something that
> causes WaitForDebugEvent to return some event.
>
> This patch uses CreateRemoteThread to do it, which results in
> WaitForDebugEvent reporting CREATE_THREAD_DEBUG_EVENT.  We then
> terminate the injected thread before it has a chance to run any
> userspace code.
>
> Note that Win32 functions like DebugBreakProcess and
> GenerateConsoleCtrlEvent would also inject a new thread in the
> inferior.  I first used DebugBreakProcess, but that is actually more
> complicated to use, because we'd have to be sure to consume the
> breakpoint event before detaching, otherwise the inferior would likely
> die due a breakpoint exception being raised with no debugger around to
> intercept it.
>
> See the new break_out_process_thread method.
>
> So the fix has two parts:
>
> - Keep track of whether we've called ContinueDebugEvent and the
>   process_thread thread is waiting for events, or whether
>   WaitForDebugEvent already returned an event.
>
> - In windows_nat_target::detach, if the process_thread thread is
>   waiting for events, unblock out of its WaitForDebugEvent, before
>   proceeding with the actual detach.
>
> New test included.  Passes cleanly on GNU/Linux native and gdbserver,
> and also passes cleanly on Cygwin, with the fix.  Before the fix, it
> would hang and fail with a timeout.

I tried this, and it works for me too for Windows native.

Tested-By: Hannes Domani <ssbssa@yahoo.de>

I have 2 comments below.


> Change-Id: Ifb91c58c08af1a9bcbafecedc93dfce001040905
> ---
> .../gdb.threads/detach-while-running.c        |  30 ++++
> .../gdb.threads/detach-while-running.exp      |  77 ++++++++++
> gdb/windows-nat.c                            | 131 ++++++++++++++++--
> 3 files changed, 230 insertions(+), 8 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/detach-while-running.c
> create mode 100644 gdb/testsuite/gdb.threads/detach-while-running.exp
>
> diff --git a/gdb/testsuite/gdb.threads/detach-while-running.c b/gdb/testsuite/gdb.threads/detach-while-running.c
> new file mode 100644
> index 00000000000..19cc3b5761c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/detach-while-running.c
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +  Copyright 2024 Free Software Foundation, Inc.
> +
> +  This program is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU General Public License as published by
> +  the Free Software Foundation; either version 3 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <assert.h>
> +#include <unistd.h>
> +
> +int
> +main (int argc, char **argv)
> +{
> +  alarm (30);
> +
> +  while (1)
> +    sleep (1);
> +
> +  return 0;
> +}

The test passes for me, but I had to #ifdef the alarm() function out,
since it doesn't exist on Windows.


> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index b123a66ef0f..7e571f281cb 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -2072,20 +2089,118 @@ windows_nat_target::attach (const char *args, int from_tty)
>   target_terminal::ours ();
> }
>
> +void
> +windows_nat_target::break_out_process_thread (bool &process_alive)
> +{
> +  /* This is called when the process_thread thread is blocked in
> +    WaitForDebugEvent (unless it already returned some event we
> +    haven't consumed yet), and we need to unblock it so that we can
> +    have it call DebugActiveProcessStop.
> +
> +    To make WaitForDebugEvent return, we need to force some event in
> +    the inferior.  Any method that lets us do that (without
> +    disturbing the other threads), injects a new thread in the
> +    inferior.
> +
> +    We don't use DebugBreakProcess for this, because that injects a
> +    thread that ends up executing a breakpoint instruction.  We can't
> +    let the injected thread hit that breakpoint _after_ we've
> +    detached.  Consuming events until we see a breakpoint trap isn't
> +    100% reliable, because we can't distinguish it from some other
> +    thread itself deciding to call int3 while we're detaching, unless
> +    we temporarily suspend all threads.  It's just a lot of
> +    complication, and there's an easier way.
> +
> +    Important observation: the thread creation event for the newly
> +    injected thread is sufficient to unblock WaitForDebugEvent.
> +
> +    Instead of DebugBreakProcess, we can instead use
> +    CreateRemoteThread to control the code that the injected thread
> +    runs ourselves.  We could consider pointing the injected thread
> +    at some side-effect-free Win32 function as entry point.  However,
> +    finding the address of such a function requires having at least
> +    minimal symbols loaded for ntdll.dll.  Having a way that avoids
> +    that is better, so that detach always works correctly even when
> +    we don't have any symbols loaded.
> +
> +    So what we do is inject a thread that doesn't actually run ANY
> +    userspace code, because we force-terminate it as soon as we see
> +    its corresponding thread creation event.  CreateRemoteThread
> +    gives us the new thread's ID, which we can match with the thread
> +    associated with the CREATE_THREAD_DEBUG_EVENT event.  */
> +
> +  DWORD injected_thread_id = 0;
> +  HANDLE injected_thread_handle
> +    = CreateRemoteThread (windows_process.handle, NULL,
> +              0, (LPTHREAD_START_ROUTINE) 0,
> +              NULL, 0, &injected_thread_id);
> +
> +  process_alive = true;
> +
> +  /* At this point, the user has declared that they want to detach, so
> +    any event that happens from this point on should be forwarded to
> +    the inferior.  */
> +
> +  for (;;)
> +    {
> +      DEBUG_EVENT current_event;
> +      wait_for_debug_event_main_thread (&current_event);
> +
> +      if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
> +    {
> +      DEBUG_EVENTS ("got EXIT_PROCESS_DEBUG_EVENT");
> +      process_alive = false;
> +      break;

Not sure how likely it is to get here, but injected_thread_handle
leaks in this case.


> +    }
> +
> +      if (current_event.dwDebugEventCode == CREATE_THREAD_DEBUG_EVENT
> +      && current_event.dwThreadId == injected_thread_id)
> +    {
> +      DEBUG_EVENTS ("got CREATE_THREAD_DEBUG_EVENT for injected thread");
> +
> +      /* Terminate the injected thread, so it doesn't run any code
> +        at all.  All we wanted was some event, and
> +        CREATE_THREAD_DEBUG_EVENT is sufficient.  */
> +      CHECK (TerminateThread (injected_thread_handle, 0));
> +      CHECK (CloseHandle (injected_thread_handle));
> +      break;
> +    }
> +
> +      DEBUG_EVENTS ("got unrelated event, code %u",
> +            current_event.dwDebugEventCode);
> +      windows_continue (DBG_CONTINUE, -1, 0);
> +    }
> +}


Hannes