Re: [PATCH] Improve attach on Windows

[Pedro Alves <pedro@palves.net>]

Hi!

I'm looking at the windows non-stop series again, trying to figure out where we were.

There was one problem exposed by the AdaCore testsuite that I wasn't able to reproduce locally, so I don't know what to do about it.

More below.

I'll look at the rest of the series, rebase, and see what can be merged already.

On 2025-06-11 23:06, Pedro Alves wrote:

> On 2025-06-05 18:57, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
>>
>> Pedro> I've pushed the series to the users/palves/windows-non-stop-v2 branch
>> Pedro> on sourceware.org, for your convenience.
>>
>> I merged this into the local tree and ran the internal AdaCore test
>> suite on Windows 2016.
> 
> Thank you very much for doing this.  Much appreciated.
> 
>>
>> There were three "failures".
>>
>> One of them is actually an improvement, where the test works around a
>> current issue.
>>
> 
> One step forward, two steps back, I guess.
> 
> Tackling this incrementally.  I've looked at the first one:
> 
>> One of them seems to be a problem with the test.  This test "attach"es
>> to a running process without a "file", and the test doesn't seem to
>> match the new output:
>>
>>     (gdb) attach 1228
>>     Attaching to process 1228
>>     [Switching to Thread 1228.0x580]
>>     0x00007ffe3c756714 in ntdll!ZwDelayExecution ()
>>        from C:\Windows\SYSTEM32\ntdll.dll
>>
> 
> I guess your test wasn't expecting that the current frame is printed?
> 
> See the patch below, on top of current master.  You are now seeing the current
> frame being printed, unlike what I claim in the commit log of the patch below,
> because with target-non-stop on, the Windows backend is a target_wait_no_wait==false
> target, like Linux.
> 
> But even with the non-stop series, with "maint set target-non-stop off",
> (or with Windows older than Windows 10,) then target_wait_no_wait==true, and
> GDB doesn't print the frame.
> 
> IMO, this is just a preexisting bug that the non-stop series happens to hide.
> 
> Let me know what you think of the patch.
> 
> And yes, the fact that run control communicates with normal_stop via
> globals is ugly and fragile to say the least.
> 
> From 0cb83571c91ba42b3de519b034509092a526dc65 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 11 Jun 2025 22:05:23 +0100
> Subject: [PATCH] Improve attach on Windows

I think this one is a clear and easy improvement.
I've retested it, and I see the same progression in gdb.base/attach.exp, so I've now pushed it.

Thanks,
Pedro Alves

> 
> Unlike most targets, on Windows, when you attach, GDB doesn't print
> the current stack frame.  Vis:
> 
> On GNU/Linux:
> 
>  attach 3340347
>  Attaching to program: /home/pedro/gdb/build/gdb/testsuite/outputs/gdb.base/attach/attach, process 3340347
>  Reading symbols from /lib/x86_64-linux-gnu/libc.so.6...
>  Reading symbols from /usr/lib/debug/.build-id/d5/197096f709801829b118af1b7cf6631efa2dcd.debug...
>  Reading symbols from /lib64/ld-linux-x86-64.so.2...
>  Reading symbols from /usr/lib/debug/.build-id/9c/b53985768bb99f138f48655f7b8bf7e420d13d.debug...
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>  0x00005b3bf29be174 in main () at /home/pedro/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/attach.c:19
>  19        while (! should_exit)
>  (gdb) PASS: gdb.base/attach.exp: do_attach_tests: attach1, after setting file
> 
> On Cygwin:
> 
>  (gdb) attach 6692
>  Attaching to program: /home/alves/gdb/build-cygwin-testsuite/outputs/gdb.base/attach/attach, process 6692
>  [New Thread 6692.0x2e60]
>  [New Thread 6692.0x2e9c]
>  [New Thread 6692.0xd6c]
>  [New Thread 6692.0x137c]
>  [New Thread 6692.0x1270]
>  (gdb) FAIL: gdb.base/attach.exp: do_attach_tests: attach1, after setting file
> 
> On Linux, GDB prints the frame because after the target_attach, GDB
> goes back to the event loop, to wait for an initial stop event.  The
> stop event arrives, and we process it, which sets the stop_print_frame
> global, and then we get to normal_stop, which prints the frame iff
> stop_print_frame is set, which it is.
> 
> Windows OTOH, is a target_attach_no_wait target, so after
> target_attach, there is no going back to event loop.  In
> infcmd.c:attach_command, we go straight to attach_post_wait which
> takes us to normal_stop.  But this time, nothing set
> stop_print_frame to true, so no frame is printed.  Actually, if the
> global happened to be true due to an earlier event from debugging a
> previous inferior, then we will print the frame.
> 
> This patch makes GDB's behavior consistent, by making sure the globals
> normal_stop looks at are in a good state in the target_attach_no_wait
> path.
> 
> With that alone, GDB now prints the frame:
> 
>  (gdb) attach 2915
>  Attaching to program: /usr/bin/sleep.exe, process 2832
>  [New Thread 2832.0x2a68]
>  [New Thread 2832.0xb1c]
>  [New Thread 2832.0x8ac]
>  [Switching to Thread 2832.0x8ac]
>  0x00007ffec51d4a71 in ntdll!DbgBreakPoint () from C:/Windows/SYSTEM32/ntdll.dll
> 
> This is still not ideal, IMHO, as the current thread is the thread
> that Windows injects to attach:
> 
>  (gdb) info threads
>    Id   Target Id                  Frame
>    1    Thread 2832.0x2100 "sleep" 0x00007ffec51d18d7 in ntdll!ZwWaitForMultipleObjects () from C:/Windows/SYSTEM32/ntdll.dll
>    2    Thread 2832.0x2a68 "sig"   0x00007ffec51d0e47 in ntdll!ZwReadFile () from C:/Windows/SYSTEM32/ntdll.dll
>    3    Thread 2832.0xb1c          0x00007ffec51d49d7 in ntdll!ZwWaitForWorkViaWorkerFactory () from C:/Windows/SYSTEM32/ntdll.dll
>  * 4    Thread 2832.0x8ac          0x00007ffec51d4a71 in ntdll!DbgBreakPoint () from C:/Windows/SYSTEM32/ntdll.dll
> 
> Automatically switching to main thread is IMHO more useful.  That
> results in very similar output than what we see on Linux:
> 
>  attach 5164
>  Attaching to program: /home/alves/gdb/build-cygwin-testsuite/outputs/gdb.base/attach/attach, process 5164
>  [New Thread 5164.0x87c]
>  [New Thread 5164.0x28f0]
>  [New Thread 5164.0x376c]
>  [New Thread 5164.0x2db4]
>  [New Thread 5164.0xce4]
>  main () at /home/alves/gdb/src/gdb/testsuite/gdb.base/attach.c:19
>  19        while (! should_exit)
>  (gdb)
> 
> If we do this, then we can simplify gdb.base/attach.exp a bit by
> removing a couple Cygwin special cases.
> 
> The patch does all that, which results in the following
> gdb.base/attach.exp progressions:
> 
>  -FAIL: gdb.base/attach.exp: do_attach_tests: attach1, after setting file
>  -FAIL: gdb.base/attach.exp: do_attach_tests: attach2, with no file
>  -FAIL: gdb.base/attach.exp: do_attach_tests: load file manually, after attach2 (re-read) (got interactive prompt)
>  -FAIL: gdb.base/attach.exp: do_attach_tests: attach when process' a.out not in cwd
>  -FAIL: gdb.base/attach.exp: do_attach_failure_tests: first attach
>  +PASS: gdb.base/attach.exp: do_attach_tests: attach1, after setting file
>  +PASS: gdb.base/attach.exp: do_attach_tests: attach2, with no file
>  +PASS: gdb.base/attach.exp: do_attach_tests: attach when process' a.out not in cwd
>  +PASS: gdb.base/attach.exp: do_attach_failure_tests: first attach
> 
> Change-Id: I359bdb25660c9a4d5d873e8771cfd1cd2a54c97b
> ---
>  gdb/infcmd.c                      |  5 ++++-
>  gdb/infrun.c                      | 13 +++++++++++++
>  gdb/infrun.h                      |  3 +++
>  gdb/testsuite/gdb.base/attach.exp | 26 ++++++--------------------
>  gdb/windows-nat.c                 |  7 +++++++
>  5 files changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index e9b58ce5521..cbf2373193c 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2722,7 +2722,10 @@ attach_command (const char *args, int from_tty)
>        return;
>      }
>    else
> -    attach_post_wait (from_tty, mode);
> +    {
> +      set_normal_stop_state_just_attached ();
> +      attach_post_wait (from_tty, mode);
> +    }
>  
>    disable_commit_resumed.reset_and_commit ();
>  }
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e02642c52a..41ab3342ee3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -407,6 +407,19 @@ static process_stratum_target *target_last_proc_target;
>  static ptid_t target_last_wait_ptid;
>  static struct target_waitstatus target_last_waitstatus;
>  
> +/* See infrun.h.  */
> +
> +void
> +set_normal_stop_state_just_attached ()
> +{
> +  stop_print_frame = true;
> +  stopped_by_random_signal = 0;
> +
> +  target_waitstatus status;
> +  status.set_ignore ();
> +  set_last_target_status (nullptr, minus_one_ptid, status);
> +}
> +
>  void init_thread_stepping_state (struct thread_info *tss);
>  
>  static const char follow_fork_mode_child[] = "child";
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index b9b64aca45a..b707314fba6 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -418,5 +418,8 @@ struct scoped_enable_commit_resumed
>    bool m_prev_enable_commit_resumed;
>  };
>  
> +/* Set up state for normal_stop after we just attached, on
> +   target_attach_no_wait targets.  */
> +extern void set_normal_stop_state_just_attached ();
>  
>  #endif /* GDB_INFRUN_H */
> diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
> index 0d1550a0541..484fa776ae4 100644
> --- a/gdb/testsuite/gdb.base/attach.exp
> +++ b/gdb/testsuite/gdb.base/attach.exp
> @@ -159,16 +159,9 @@ proc_with_prefix do_attach_failure_tests {} {
>      
>      # Verify that we can't double attach to the process.
>  
> -    set test "first attach"
> -    gdb_test_multiple "attach $testpid" "$test" {
> -	-re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*main.*at .*$srcfile:.*$gdb_prompt $" {
> -	    pass "$test"
> -	}
> -	-re "Attaching to program.*`?$escapedbinfile\.exe'?, process $testpid.*\[Switching to thread $testpid\..*\].*$gdb_prompt $" {
> -	    # Response expected on Cygwin.
> -	    pass "$test"
> -	}
> -    }
> +    gdb_test "attach $testpid" \
> +	"Attaching to program.*`?${escapedbinfile}(\.exe)?'?, process $testpid.*main.*at .*$srcfile:.*" \
> +	"first attach"
>  
>      gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
>      gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
> @@ -252,16 +245,9 @@ proc_with_prefix do_attach_tests {} {
>  	}
>      }
>  
> -    set test "attach1, after setting file"
> -    gdb_test_multiple "attach $testpid" "$test" {
> -	-re "Attaching to program.*`?$escapedbinfile'?, process $testpid.*main.*at .*$srcfile:.*$gdb_prompt $" {
> -	    pass "$test"
> -	}
> -	-re "Attaching to program.*`?$escapedbinfile\.exe'?, process $testpid.*\[Switching to thread $testpid\..*\].*$gdb_prompt $" {
> -	    # Response expected on Cygwin
> -	    pass "$test"
> -	}
> -    }
> +    gdb_test "attach $testpid" \
> +	"Attaching to program.*`?${escapedbinfile}(\.exe)?'?, process $testpid.*main.*at .*$srcfile:.*" \
> +	"attach1, after setting file"
>  
>      # Verify that we can "see" the variable "should_exit" in the
>      # program, and that it is zero.
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 939c5813aa7..0477731d40c 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -1980,6 +1980,13 @@ windows_nat_target::attach (const char *args, int from_tty)
>  #endif
>  
>    do_initial_windows_stuff (pid, 1);
> +
> +  /* The thread that reports the initial breakpoint, and thus ends up
> +     as the selected thread when we get here, was injected into the
> +     inferior by DebugActiveProcess.  Switch to the main thread, which
> +     is normally more useful to the user than the injected thread.  */
> +  switch_to_thread (first_thread_of_inferior (current_inferior ()));
> +
>    target_terminal::ours ();
>  }
>  
> 
> base-commit: eb6c9310ee4d6cbde509d251fafb54ae45f5a5bf