Return correct thread for cached event

  Commit 2e1aacf15a9 ("Windows gdb+gdbserver: Make current_event
per-thread state") introduced a regression.  This was detected by the
AdaCore internal test suite in a somewhat unusual configuration: when
using "attach" with a 32-bit Windows process, an extra stop would be
generated, like:

    (gdb) break break_me
    Breakpoint 1 at 0x1211a04: file pck.adb, line 18.
    (gdb) continue
    Continuing.

    Thread 4 received signal SIGINT, Interrupt.
    [Switching to thread 4 (Thread 6652)]
    0x77034210 in ntdll!RtlUserThreadStart () from C:/Windows/SysWOW64/ntdll.dll

Here, we expect to stop in break_me, but instead stop in some Windows
DLL.

I tracked this down to this hunk in the aforementioned commit:

-      return debug_event_ptid (&windows_process.current_event);
+      return ptid_t (windows_process.process_id,
+		     windows_process.main_thread_id, 0);

What happens here is that the "cached" stop ends up being reported in
the main thread, rather than whatever thread actually caused this
stop.

This patch fixes the problem by arranging to also cache the thread
ptid.

I am not sure whether the call to switch_to_thread here is really
needed; but since other returns seem to switch the thread, I thought
this one ought to as well.

I tested this using the AdaCore internal test suite.

As this is a regression, when it lands I also plan to apply it to the
gdb 18 branch.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34583
---
 gdbserver/win32-low.cc | 5 +++--
 gdbserver/win32-low.h  | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)


base-commit: 49c379f8c2f8ff888b621b4a070dc1976b942577
  

Am Mittwoch, 2. September 2026 um 20:10:06 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> Commit 2e1aacf15a9 ("Windows gdb+gdbserver: Make current_event
> per-thread state") introduced a regression.  This was detected by the
> AdaCore internal test suite in a somewhat unusual configuration: when
> using "attach" with a 32-bit Windows process, an extra stop would be
> generated, like:
> 
>     (gdb) break break_me
>     Breakpoint 1 at 0x1211a04: file pck.adb, line 18.
>     (gdb) continue
>     Continuing.
> 
>     Thread 4 received signal SIGINT, Interrupt.
>     [Switching to thread 4 (Thread 6652)]
>     0x77034210 in ntdll!RtlUserThreadStart () from C:/Windows/SysWOW64/ntdll.dll
> 
> Here, we expect to stop in break_me, but instead stop in some Windows
> DLL.
> 
> I tracked this down to this hunk in the aforementioned commit:
> 
> -      return debug_event_ptid (&windows_process.current_event);
> +      return ptid_t (windows_process.process_id,
> +            windows_process.main_thread_id, 0);
> 
> What happens here is that the "cached" stop ends up being reported in
> the main thread, rather than whatever thread actually caused this
> stop.
> 
> This patch fixes the problem by arranging to also cache the thread
> ptid.
> 
> I am not sure whether the call to switch_to_thread here is really
> needed; but since other returns seem to switch the thread, I thought
> this one ought to as well.
> 
> I tested this using the AdaCore internal test suite.
> 
> As this is a regression, when it lands I also plan to apply it to the
> gdb 18 branch.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34583
> ---
> gdbserver/win32-low.cc | 5 +++--
> gdbserver/win32-low.h  | 2 ++
> 2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index 13c14a7c69f..55e4b2e2fe3 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -337,6 +337,7 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
>       || status.kind () == TARGET_WAITKIND_STOPPED)
>     {
>       windows_process.cached_status = status;
> +      windows_process.cached_ptid = current_thread->id;
>       break;
>     }
> 
> @@ -1145,8 +1146,8 @@ win32_process_target::wait (ptid_t ptid, target_waitstatus *ourstatus,
>     fails).  Report it now.  */
>       *ourstatus = windows_process.cached_status;
>       windows_process.cached_status.set_ignore ();
> -      return ptid_t (windows_process.process_id,
> -            windows_process.main_thread_id, 0);
> +      switch_to_thread (find_thread_ptid (windows_process.cached_ptid));
> +      return windows_process.cached_ptid;
>     }
> 
>   while (1)
> diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
> index 439adb84bc2..f4f02851734 100644
> --- a/gdbserver/win32-low.h
> +++ b/gdbserver/win32-low.h
> @@ -193,6 +193,8 @@ struct gdbserver_windows_process : public windows_nat::windows_process_info
>       win32_wait should return it next, instead of fetching the next
>       debug event off the win32 API.  */
>   struct target_waitstatus cached_status;
> +  /* The ptid corresponding to the above status.  */
> +  ptid_t cached_ptid;
> 
>   /* True if current_process_handle needs to be closed.  */
>   bool open_process_used = false;
> 
> base-commit: 49c379f8c2f8ff888b621b4a070dc1976b942577
> -- 
> 2.55.0

This fix works for me, and the change itself LGTM as well.
With this windows_process_info::main_thread_id can be removed I think.

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


Hannes
  

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> This fix works for me, and the change itself LGTM as well.

Thanks for trying it.

Hannes> With this windows_process_info::main_thread_id can be removed I think.

Nice find.  I can do that separately.

Tom
  

On 2026-09-02 19:09, Tom Tromey wrote:
> Commit 2e1aacf15a9 ("Windows gdb+gdbserver: Make current_event
> per-thread state") introduced a regression.  This was detected by the
> AdaCore internal test suite in a somewhat unusual configuration: when
> using "attach" with a 32-bit Windows process, an extra stop would be
> generated, like:
> 
>     (gdb) break break_me
>     Breakpoint 1 at 0x1211a04: file pck.adb, line 18.
>     (gdb) continue
>     Continuing.
> 
>     Thread 4 received signal SIGINT, Interrupt.
>     [Switching to thread 4 (Thread 6652)]
>     0x77034210 in ntdll!RtlUserThreadStart () from C:/Windows/SysWOW64/ntdll.dll
> 
> Here, we expect to stop in break_me, but instead stop in some Windows
> DLL.
> 
> I tracked this down to this hunk in the aforementioned commit:
> 
> -      return debug_event_ptid (&windows_process.current_event);
> +      return ptid_t (windows_process.process_id,
> +		     windows_process.main_thread_id, 0);
> 
> What happens here is that the "cached" stop ends up being reported in
> the main thread, rather than whatever thread actually caused this
> stop.

Ouch, sorry about this.  And thanks for all the investigation.

I must have very early on in the non-stop work assumed that the initial events
all come from the main thread, but later on I learned that they don't (I even
ran into that very nasty bug that led to the state => internal_state split), and
missed this assumption here.

> 
> This patch fixes the problem by arranging to also cache the thread
> ptid.
> 
> I am not sure whether the call to switch_to_thread here is really
> needed; but since other returns seem to switch the thread, I thought
> this one ought to as well.
> 
> I tested this using the AdaCore internal test suite.
> 
> As this is a regression, when it lands I also plan to apply it to the
> gdb 18 branch.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34583

Approved-by: Pedro Alves <pedro@palves.net>