[PATCH 0/3] [gdb/testsuite] Fix gdb.base/quit-live.exp for remote host

[PATCH 0/3] [gdb/testsuite] Fix gdb.base/quit-live.exp for remote host

[Tom de Vries]

A few patches that fix test-case gdb.base/quit-live.exp for remote host.

Tom de Vries (3):
  [gdb/testsuite] Add gdb_exit_cleanup
  [gdb/testsuite] Use gdb_exit_cleanup a bit more
  [gdb/testsuite] Fix gdb.base/quit-live.exp for remote host

 gdb/testsuite/gdb.base/quit-live.exp          | 14 +++--
 .../gdb.server/monitor-exit-quit.exp          |  7 +--
 gdb/testsuite/gdb.threads/killed.exp          |  1 +
 gdb/testsuite/lib/gdb.exp                     | 52 ++++++++++++++++---
 4 files changed, 57 insertions(+), 17 deletions(-)


base-commit: dd41575147933228c3996204b68c6c6e6ed3d9e8
-- 
2.51.0

[PATCH 1/3] [gdb/testsuite] Add gdb_exit_cleanup

[Tom de Vries]

Proc default_gdb_exit does two things:
- it tries to make gdb exit, and
- it does cleanup that needs doing after gdb exits.

Factor out the second part as new proc gdb_exit_cleanup.
---
 .../gdb.server/monitor-exit-quit.exp          |  7 +-----
 gdb/testsuite/lib/gdb.exp                     | 23 ++++++++++++-------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/gdb/testsuite/gdb.server/monitor-exit-quit.exp b/gdb/testsuite/gdb.server/monitor-exit-quit.exp
index cb90169ef0c..75c3e6e2221 100644
--- a/gdb/testsuite/gdb.server/monitor-exit-quit.exp
+++ b/gdb/testsuite/gdb.server/monitor-exit-quit.exp
@@ -70,10 +70,5 @@ gdb_test_multiple "quit" "" {
 
 # Cleanup, as in default_gdb_exit.
 if { $do_cleanup } {
-    if { ![is_remote host] } {
-	    remote_close host
-    }
-    unset gdb_spawn_id
-    unset ::gdb_tty_name
-    unset inferior_spawn_id
+    gdb_exit_cleanup
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 1ebdaf6ba10..7bdc5dad0a7 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2505,6 +2505,20 @@ proc gdb_reinitialize_dir { subdir } {
     }
 }
 
+# Clean up after the current gdb instance that has exited.
+
+proc gdb_exit_cleanup {} {
+    if {![is_remote host]} {
+	if {[catch { remote_close host } message]} {
+	    warning "closing gdb failed with: $message"
+	}
+    }
+
+    unset ::gdb_spawn_id
+    unset ::gdb_tty_name
+    unset ::inferior_spawn_id
+}
+
 #
 # gdb_exit -- exit the GDB, killing the target program if necessary
 #
@@ -2547,14 +2561,7 @@ proc default_gdb_exit {} {
 	}
     }
 
-    if {![is_remote host]} {
-	if {[catch { remote_close host } message]} {
-	    warning "closing gdb failed with: $message"
-	}
-    }
-    unset gdb_spawn_id
-    unset ::gdb_tty_name
-    unset inferior_spawn_id
+    gdb_exit_cleanup
 }
 
 # Load a file into the debugger.
-- 
2.51.0

[PATCH 2/3] [gdb/testsuite] Use gdb_exit_cleanup a bit more

[Tom de Vries]

I ran into trouble running test-case gdb.base/quit-live.exp using a remote
host configuration:
- host board local-remote-host
- target board remote-gdbserver-on-localhost.

The problem is that the test-case makes gdb quit without updating
gdb_spawn_id.  Consequently, default_gdb_exit tries to exit gdb.
It does so by sending quit to gdb and waiting for it to exit, which get us:
...
ERROR: : spawn id exp9 not open
...

Fix this using gdb_exit_cleanup.  Likewise in gdb.threads/killed.exp.
---
 gdb/testsuite/gdb.base/quit-live.exp | 1 +
 gdb/testsuite/gdb.threads/killed.exp | 1 +
 2 files changed, 2 insertions(+)

diff --git a/gdb/testsuite/gdb.base/quit-live.exp b/gdb/testsuite/gdb.base/quit-live.exp
index 14f87f9b7f8..8dae4fe2aca 100644
--- a/gdb/testsuite/gdb.base/quit-live.exp
+++ b/gdb/testsuite/gdb.base/quit-live.exp
@@ -152,6 +152,7 @@ proc quit_with_live_inferior {appear_how extra_inferior quit_how} {
 		gdb_test_multiple "" $test {
 		    eof {
 			pass $test
+			gdb_exit_cleanup
 		    }
 		}
 	    }
diff --git a/gdb/testsuite/gdb.threads/killed.exp b/gdb/testsuite/gdb.threads/killed.exp
index 39b60dff8dd..a52ae552a2c 100644
--- a/gdb/testsuite/gdb.threads/killed.exp
+++ b/gdb/testsuite/gdb.threads/killed.exp
@@ -76,6 +76,7 @@ gdb_expect {
     }
     eof {
 	pass "GDB exits after multi-threaded program exits messily"
+	gdb_exit_cleanup
     }
     -re "Cannot find thread ${decimal}: generic error\[\r\n\]*$gdb_prompt $" {
 	kfail "gdb/568" "GDB exits after multi-threaded program exits messily"
-- 
2.51.0

[PATCH 3/3] [gdb/testsuite] Fix gdb.base/quit-live.exp for remote host

[Tom de Vries]

I ran into trouble running test-case gdb.base/quit-live.exp using a remote
host configuration:
- host board local-remote-host
- target board remote-gdbserver-on-localhost.

The test-case uses kill to send a signal to gdb_spawn_id, but with remote host
the gdb_spawn_id points to an ssh session, not gdb.

Detect this more clearly by factoring out a proc kill_gdb (and
remote_kill_spawn_id and remote_kill_spawn_id_p), and erroring out for remote
host/target.

Then fix this by skipping the relevant tests using remote_kill_spawn_id_p.
---
 gdb/testsuite/gdb.base/quit-live.exp | 13 ++++++++++---
 gdb/testsuite/lib/gdb.exp            | 29 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/quit-live.exp b/gdb/testsuite/gdb.base/quit-live.exp
index 8dae4fe2aca..8e1b4a428c7 100644
--- a/gdb/testsuite/gdb.base/quit-live.exp
+++ b/gdb/testsuite/gdb.base/quit-live.exp
@@ -43,8 +43,7 @@ if {[build_executable "failed to build" $testfile $srcfile debug]} {
 # Send signal SIG to GDB, and expect GDB to exit.
 
 proc test_quit_with_sig {sig} {
-    set gdb_pid [exp_pid -i [board_info host fileid]]
-    remote_exec host "kill -$sig ${gdb_pid}"
+    kill_gdb $sig
 
     set test "quit with SIG$sig"
     # If GDB mishandles the signal and doesn't exit, this should FAIL
@@ -176,7 +175,15 @@ foreach_with_prefix appear_how {"run" "attach" "attach-nofile"} {
     }
 
     foreach_with_prefix extra_inferior {0 1} {
-	foreach_with_prefix quit_how {"quit" "sigterm" "sighup"} {
+	set qhs {}
+	lappend qhs "quit"
+	foreach sig {"sigterm" "sighup"} {
+	    if {[remote_kill_spawn_id_p host $sig]} {
+		lappend qhs $sig
+	    }
+	}
+
+	foreach_with_prefix quit_how $qhs {
 	    quit_with_live_inferior $appear_how $extra_inferior $quit_how
 	}
     }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7bdc5dad0a7..b249425528d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -12473,6 +12473,35 @@ proc unprintable_to_octal { input_string } {
 # Ignore args and don't do anything.  Can be used with proc with_override.
 proc nop {args} {}
 
+# Return 1 if SIG can be delivered to a spawn_id on board.
+proc remote_kill_spawn_id_p { board sig } {
+    if {$board == "host" || $board == "target"} {
+	if {[isremote $board]} {
+	    # For remote host/target, the spawn_id holds the pid of the ssh
+	    # session, so we'd end up sending signals to ssh instead.
+	    return 0
+	}
+    }
+
+    return 1
+}
+
+# Send SIG to PID on BOARD.
+proc remote_kill_spawn_id { board spawn_id sig } {
+    set pid [exp_pid -i $spawn_id]
+
+    if {![remote_kill_spawn_id_p $board $sig]} {
+	error "Can't send signal $sig to $pid on remote $board"
+    }
+
+    return [remote_exec $board "kill -$sig $pid"]
+}
+
+# Send SIG to GDB.
+proc kill_gdb { sig } {
+    return [remote_kill_spawn_id host $::gdb_spawn_id $sig]
+}
+
 require {tcl_version_at_least 8 6 2}
 
 # Always load compatibility stuff.
-- 
2.51.0

Re: [PATCH 2/3] [gdb/testsuite] Use gdb_exit_cleanup a bit more

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> I ran into trouble running test-case gdb.base/quit-live.exp using a remote
> host configuration:
> - host board local-remote-host
> - target board remote-gdbserver-on-localhost.
>
> The problem is that the test-case makes gdb quit without updating
> gdb_spawn_id.  Consequently, default_gdb_exit tries to exit gdb.
> It does so by sending quit to gdb and waiting for it to exit, which get us:
> ...
> ERROR: : spawn id exp9 not open
> ...
>
> Fix this using gdb_exit_cleanup.  Likewise in gdb.threads/killed.exp.
> ---
>  gdb/testsuite/gdb.base/quit-live.exp | 1 +
>  gdb/testsuite/gdb.threads/killed.exp | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/quit-live.exp b/gdb/testsuite/gdb.base/quit-live.exp
> index 14f87f9b7f8..8dae4fe2aca 100644
> --- a/gdb/testsuite/gdb.base/quit-live.exp
> +++ b/gdb/testsuite/gdb.base/quit-live.exp
> @@ -152,6 +152,7 @@ proc quit_with_live_inferior {appear_how extra_inferior quit_how} {
>  		gdb_test_multiple "" $test {
>  		    eof {
>  			pass $test
> +			gdb_exit_cleanup

There's another place in this test script which detects eof from GDB,
but you've not added the gdb_exit_cleanup call there.

Should we also be patching that location?  If not why not?  And if the
answer is there's a good reason why not, then I think it is worth
mentioning in the commit message, and as a comment at that location in
the code.

Thanks,
Andrew

Re: [PATCH 1/3] [gdb/testsuite] Add gdb_exit_cleanup

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> Proc default_gdb_exit does two things:
> - it tries to make gdb exit, and
> - it does cleanup that needs doing after gdb exits.
>
> Factor out the second part as new proc gdb_exit_cleanup.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> ---
>  .../gdb.server/monitor-exit-quit.exp          |  7 +-----
>  gdb/testsuite/lib/gdb.exp                     | 23 ++++++++++++-------
>  2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.server/monitor-exit-quit.exp b/gdb/testsuite/gdb.server/monitor-exit-quit.exp
> index cb90169ef0c..75c3e6e2221 100644
> --- a/gdb/testsuite/gdb.server/monitor-exit-quit.exp
> +++ b/gdb/testsuite/gdb.server/monitor-exit-quit.exp
> @@ -70,10 +70,5 @@ gdb_test_multiple "quit" "" {
>  
>  # Cleanup, as in default_gdb_exit.
>  if { $do_cleanup } {
> -    if { ![is_remote host] } {
> -	    remote_close host
> -    }
> -    unset gdb_spawn_id
> -    unset ::gdb_tty_name
> -    unset inferior_spawn_id
> +    gdb_exit_cleanup
>  }
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 1ebdaf6ba10..7bdc5dad0a7 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2505,6 +2505,20 @@ proc gdb_reinitialize_dir { subdir } {
>      }
>  }
>  
> +# Clean up after the current gdb instance that has exited.
> +
> +proc gdb_exit_cleanup {} {
> +    if {![is_remote host]} {
> +	if {[catch { remote_close host } message]} {
> +	    warning "closing gdb failed with: $message"
> +	}
> +    }
> +
> +    unset ::gdb_spawn_id
> +    unset ::gdb_tty_name
> +    unset ::inferior_spawn_id
> +}
> +
>  #
>  # gdb_exit -- exit the GDB, killing the target program if necessary
>  #
> @@ -2547,14 +2561,7 @@ proc default_gdb_exit {} {
>  	}
>      }
>  
> -    if {![is_remote host]} {
> -	if {[catch { remote_close host } message]} {
> -	    warning "closing gdb failed with: $message"
> -	}
> -    }
> -    unset gdb_spawn_id
> -    unset ::gdb_tty_name
> -    unset inferior_spawn_id
> +    gdb_exit_cleanup
>  }
>  
>  # Load a file into the debugger.
> -- 
> 2.51.0

Re: [PATCH 2/3] [gdb/testsuite] Use gdb_exit_cleanup a bit more

[Tom de Vries]

On 9/27/26 10:52 AM, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> I ran into trouble running test-case gdb.base/quit-live.exp using a remote
>> host configuration:
>> - host board local-remote-host
>> - target board remote-gdbserver-on-localhost.
>>
>> The problem is that the test-case makes gdb quit without updating
>> gdb_spawn_id.  Consequently, default_gdb_exit tries to exit gdb.
>> It does so by sending quit to gdb and waiting for it to exit, which get us:
>> ...
>> ERROR: : spawn id exp9 not open
>> ...
>>
>> Fix this using gdb_exit_cleanup.  Likewise in gdb.threads/killed.exp.
>> ---
>>   gdb/testsuite/gdb.base/quit-live.exp | 1 +
>>   gdb/testsuite/gdb.threads/killed.exp | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.base/quit-live.exp b/gdb/testsuite/gdb.base/quit-live.exp
>> index 14f87f9b7f8..8dae4fe2aca 100644
>> --- a/gdb/testsuite/gdb.base/quit-live.exp
>> +++ b/gdb/testsuite/gdb.base/quit-live.exp
>> @@ -152,6 +152,7 @@ proc quit_with_live_inferior {appear_how extra_inferior quit_how} {
>>   		gdb_test_multiple "" $test {
>>   		    eof {
>>   			pass $test
>> +			gdb_exit_cleanup
> 
> There's another place in this test script which detects eof from GDB,
> but you've not added the gdb_exit_cleanup call there.
> 
> Should we also be patching that location?  If not why not?  And if the
> answer is there's a good reason why not, then I think it is worth
> mentioning in the commit message, and as a comment at that location in
> the code.
> 

Hi Andrew,

thanks for the review(s).

The focus of this commit is to address a specific error, and the 
addition in quit_with_live_inferior fixes it.

I did not encounter the same error in test_quit_with_sig, but it's 
probably a good idea to apply the same pattern.

[ It might even be necessary on msys2, but AFAIR currently the test-case 
fails in such a way that the eof is not reached.  I briefly tried making 
the testcase work using some kill equivalent, but that didn't work out. ]

Anyway, I've added the gdb_exit_cleanup in test_quit_with_sig, and I've 
updated the commit message to:
...
     Fix this in quit_with_live_inferior using gdb_exit_cleanup.
     Likewise in gdb.threads/killed.exp.

     While we're at it, also add default_gdb_exit for another eof clause
     in gdb.base/quit-live.exp, in proc test_quit_with_sig.
...
and pushed.

Thanks,
- Tom

> Thanks,
> Andrew
>