diff mbox series

[RFC,gdb/testsuite] Fix inferior pid in gdb.server/server-kill.exp

Message ID 20221011140150.GA15277@delia.home
State Dropped
Headers
Series [RFC,gdb/testsuite] Fix inferior pid in gdb.server/server-kill.exp |

Commit Message

Tom de Vries Oct. 11, 2022, 2:01 p.m. UTC 
  Hi,

Consider this patch in gdb.server/server-kill.exp:
...
 proc kill_server {} {
     global server_pid

+    remote_exec target "pstree -a -p $server_pid"
     remote_exec target "kill -9 $server_pid"
 }
...

We have for kill_pid_of=server, as expected, the gdbserver killed:
...
Executing on target: pstree -a -p 6969    (timeout = 300)
builtin_spawn -ignore SIGHUP pstree -a -p 6969^M
gdbserver,6969^M
  `-server-kill,6976^M
Executing on target: kill -9 6969    (timeout = 300)
builtin_spawn -ignore SIGHUP kill -9 6969^M
...

But for kill_pid_of=inferior, we also have the gdbserver killed:
...
Executing on target: pstree -a -p 6805    (timeout = 300)
builtin_spawn -ignore SIGHUP pstree -a -p 6805^M
gdbserver,6805^M
  `-server-kill,6812^M
Executing on target: kill -9 6805    (timeout = 300)
builtin_spawn -ignore SIGHUP kill -9 6805^M
...

The proc prepare contains the pid extraction code:
...
    if { $::kill_pid_of == "inferior" } {
        # Get the pid of GDBServer.
        set test "p server_pid"
        set server_pid 0
        gdb_test_multiple $test $test {
            -re " = ($decimal)\r\n$gdb_prompt $" {
                set server_pid $expect_out(1,string)
                pass $test
            }
        }
    } else {
        set server_pid [exp_pid -i $::server_spawn_id]
    }
...

The bit for $::kill_pid_of == "inferior" looks like the correct code to
extract the gdbserver pid (it prints the parent PID of the inferior).

The bit for $::kill_pid_of == "server" does work for local target, but for
remote target using say target board remote-gdbserver-on-localhost, we have:
...
Executing on target: pstree -a -p 10354    (timeout = 300)
builtin_spawn [open ...]^M
ssh,10354 -l vries localhost/gdbserver --once localhost:2350 s
XYZ0ZYX
Executing on target: kill -9 10354    (timeout = 300)
builtin_spawn [open ...]^M
XYZ0ZYX
...
In other words, we're not killing the gdbserver, but the ssh session (a
problem which was already fixed once in commit f90183d7e31 ("Get GDBserver
pid on remote target")).

Fix this by:
- using the $::kill_pid_of == "inferior" bit for the gdbserver pid, and
- using the inferior command for the the inferior pid.

This introduces the following 4 FAILs:
...
FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_tstatus: tstatus
FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_nosyms: bt
FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_syms: bt
FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_stepi: stepi
...

The first in more detail:
...
(gdb) status-packet on
tstatus^M
No trace has been run on the target.^M
Collected 0 trace frames.^M
Trace buffer has 5242880 bytes of 5242880 bytes free (0% full).^M
Trace will stop if GDB disconnects.^M
Not looking at any trace frame.^M
(gdb) FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_tstatus: tstatus
...

The test-case contains the comment:
...
 # When KILL_PID_OF is set to 'inferior' then the pid we kill is that
 # of the inferior running under gdbserver, when this process dies
 # gdbserver itself will exit.
...
but that doesn't seem to be happening.

I've added a sleep 60 after the kill to rule out any timing issues, and indeed
after 60 seconds the gdbserver is still running.

I don't know gdbserver well enough to decide whether the test-case assumption
is wrong and we need to fix the test-case, or there's a gdbserver problem
which is known or for which we can file a PR.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix inferior pid in gdb.server/server-kill.exp

---
 gdb/testsuite/gdb.server/server-kill.exp | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Tom de Vries Oct. 25, 2022, 12:20 p.m. UTC | #1 
On 10/11/22 16:01, Tom de Vries wrote:
> Hi,
> 
> Consider this patch in gdb.server/server-kill.exp:
> ...
>   proc kill_server {} {
>       global server_pid
> 
> +    remote_exec target "pstree -a -p $server_pid"
>       remote_exec target "kill -9 $server_pid"
>   }
> ...
> 
> We have for kill_pid_of=server, as expected, the gdbserver killed:
> ...
> Executing on target: pstree -a -p 6969    (timeout = 300)
> builtin_spawn -ignore SIGHUP pstree -a -p 6969^M
> gdbserver,6969^M
>    `-server-kill,6976^M
> Executing on target: kill -9 6969    (timeout = 300)
> builtin_spawn -ignore SIGHUP kill -9 6969^M
> ...
> 
> But for kill_pid_of=inferior, we also have the gdbserver killed:
> ...
> Executing on target: pstree -a -p 6805    (timeout = 300)
> builtin_spawn -ignore SIGHUP pstree -a -p 6805^M
> gdbserver,6805^M
>    `-server-kill,6812^M
> Executing on target: kill -9 6805    (timeout = 300)
> builtin_spawn -ignore SIGHUP kill -9 6805^M
> ...
> 
> The proc prepare contains the pid extraction code:
> ...
>      if { $::kill_pid_of == "inferior" } {
>          # Get the pid of GDBServer.
>          set test "p server_pid"
>          set server_pid 0
>          gdb_test_multiple $test $test {
>              -re " = ($decimal)\r\n$gdb_prompt $" {
>                  set server_pid $expect_out(1,string)
>                  pass $test
>              }
>          }
>      } else {
>          set server_pid [exp_pid -i $::server_spawn_id]
>      }
> ...
> 
> The bit for $::kill_pid_of == "inferior" looks like the correct code to
> extract the gdbserver pid (it prints the parent PID of the inferior).
> 
> The bit for $::kill_pid_of == "server" does work for local target, but for
> remote target using say target board remote-gdbserver-on-localhost, we have:
> ...
> Executing on target: pstree -a -p 10354    (timeout = 300)
> builtin_spawn [open ...]^M
> ssh,10354 -l vries localhost/gdbserver --once localhost:2350 s
> XYZ0ZYX
> Executing on target: kill -9 10354    (timeout = 300)
> builtin_spawn [open ...]^M
> XYZ0ZYX
> ...
> In other words, we're not killing the gdbserver, but the ssh session (a
> problem which was already fixed once in commit f90183d7e31 ("Get GDBserver
> pid on remote target")).
> 
> Fix this by:
> - using the $::kill_pid_of == "inferior" bit for the gdbserver pid, and
> - using the inferior command for the the inferior pid.
> 
> This introduces the following 4 FAILs:
> ...
> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_tstatus: tstatus
> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_nosyms: bt
> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_syms: bt
> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_stepi: stepi
> ...
> 
> The first in more detail:
> ...
> (gdb) status-packet on
> tstatus^M
> No trace has been run on the target.^M
> Collected 0 trace frames.^M
> Trace buffer has 5242880 bytes of 5242880 bytes free (0% full).^M
> Trace will stop if GDB disconnects.^M
> Not looking at any trace frame.^M
> (gdb) FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_tstatus: tstatus
> ...
> 
> The test-case contains the comment:
> ...
>   # When KILL_PID_OF is set to 'inferior' then the pid we kill is that
>   # of the inferior running under gdbserver, when this process dies
>   # gdbserver itself will exit.
> ...
> but that doesn't seem to be happening.
> 
> I've added a sleep 60 after the kill to rule out any timing issues, and indeed
> after 60 seconds the gdbserver is still running.
> 
> I don't know gdbserver well enough to decide whether the test-case assumption
> is wrong and we need to fix the test-case, or there's a gdbserver problem
> which is known or for which we can file a PR.
> 
> Tested on x86_64-linux.
> 
> Any comments?
> 

Ping.

Thanks,
- Tom

> [gdb/testsuite] Fix inferior pid in gdb.server/server-kill.exp
> 
> ---
>   gdb/testsuite/gdb.server/server-kill.exp | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
> index 93daf482907..05f4f21bc0c 100644
> --- a/gdb/testsuite/gdb.server/server-kill.exp
> +++ b/gdb/testsuite/gdb.server/server-kill.exp
> @@ -75,10 +75,10 @@ proc prepare {} {
>       gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"]
>       gdb_continue_to_breakpoint "after server_pid assignment"
>   
> -    if { $::kill_pid_of == "inferior" } {
> +    set server_pid 0
> +    if { $::kill_pid_of == "server" } {
>   	# Get the pid of GDBServer.
>   	set test "p server_pid"
> -	set server_pid 0
>   	gdb_test_multiple $test $test {
>   	    -re " = ($decimal)\r\n$gdb_prompt $" {
>   		set server_pid $expect_out(1,string)
> @@ -86,7 +86,13 @@ proc prepare {} {
>   	    }
>   	}
>       } else {
> -	set server_pid [exp_pid -i $::server_spawn_id]
> +	set test "inferior"
> +	gdb_test_multiple $test $test {
> +	    -re -wrap "Current inferior is 1 \\\[process ($decimal)\\\].*" {
> +		set server_pid $expect_out(1,string)
> +		pass $test
> +	    }
> +	}
>       }
>   
>       if {$server_pid == 0} {
Tom de Vries March 10, 2023, 3:40 p.m. UTC | #2 
On 10/25/22 14:20, Tom de Vries wrote:
> On 10/11/22 16:01, Tom de Vries wrote:
>> Hi,
>>
>> Consider this patch in gdb.server/server-kill.exp:
>> ...
>>   proc kill_server {} {
>>       global server_pid
>>
>> +    remote_exec target "pstree -a -p $server_pid"
>>       remote_exec target "kill -9 $server_pid"
>>   }
>> ...
>>
>> We have for kill_pid_of=server, as expected, the gdbserver killed:
>> ...
>> Executing on target: pstree -a -p 6969    (timeout = 300)
>> builtin_spawn -ignore SIGHUP pstree -a -p 6969^M
>> gdbserver,6969^M
>>    `-server-kill,6976^M
>> Executing on target: kill -9 6969    (timeout = 300)
>> builtin_spawn -ignore SIGHUP kill -9 6969^M
>> ...
>>
>> But for kill_pid_of=inferior, we also have the gdbserver killed:
>> ...
>> Executing on target: pstree -a -p 6805    (timeout = 300)
>> builtin_spawn -ignore SIGHUP pstree -a -p 6805^M
>> gdbserver,6805^M
>>    `-server-kill,6812^M
>> Executing on target: kill -9 6805    (timeout = 300)
>> builtin_spawn -ignore SIGHUP kill -9 6805^M
>> ...
>>
>> The proc prepare contains the pid extraction code:
>> ...
>>      if { $::kill_pid_of == "inferior" } {
>>          # Get the pid of GDBServer.
>>          set test "p server_pid"
>>          set server_pid 0
>>          gdb_test_multiple $test $test {
>>              -re " = ($decimal)\r\n$gdb_prompt $" {
>>                  set server_pid $expect_out(1,string)
>>                  pass $test
>>              }
>>          }
>>      } else {
>>          set server_pid [exp_pid -i $::server_spawn_id]
>>      }
>> ...
>>
>> The bit for $::kill_pid_of == "inferior" looks like the correct code to
>> extract the gdbserver pid (it prints the parent PID of the inferior).
>>
>> The bit for $::kill_pid_of == "server" does work for local target, but 
>> for
>> remote target using say target board remote-gdbserver-on-localhost, we 
>> have:
>> ...
>> Executing on target: pstree -a -p 10354    (timeout = 300)
>> builtin_spawn [open ...]^M
>> ssh,10354 -l vries localhost/gdbserver --once localhost:2350 s
>> XYZ0ZYX
>> Executing on target: kill -9 10354    (timeout = 300)
>> builtin_spawn [open ...]^M
>> XYZ0ZYX
>> ...
>> In other words, we're not killing the gdbserver, but the ssh session (a
>> problem which was already fixed once in commit f90183d7e31 ("Get 
>> GDBserver
>> pid on remote target")).
>>
>> Fix this by:
>> - using the $::kill_pid_of == "inferior" bit for the gdbserver pid, and
>> - using the inferior command for the the inferior pid.
>>
>> This introduces the following 4 FAILs:
>> ...
>> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_tstatus: 
>> tstatus
>> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: 
>> test_unwind_nosyms: bt
>> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: 
>> test_unwind_syms: bt
>> FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_stepi: stepi
>> ...
>>
>> The first in more detail:
>> ...
>> (gdb) status-packet on
>> tstatus^M
>> No trace has been run on the target.^M
>> Collected 0 trace frames.^M
>> Trace buffer has 5242880 bytes of 5242880 bytes free (0% full).^M
>> Trace will stop if GDB disconnects.^M
>> Not looking at any trace frame.^M
>> (gdb) FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: 
>> test_tstatus: tstatus
>> ...
>>
>> The test-case contains the comment:
>> ...
>>   # When KILL_PID_OF is set to 'inferior' then the pid we kill is that
>>   # of the inferior running under gdbserver, when this process dies
>>   # gdbserver itself will exit.
>> ...
>> but that doesn't seem to be happening.
>>
>> I've added a sleep 60 after the kill to rule out any timing issues, 
>> and indeed
>> after 60 seconds the gdbserver is still running.
>>
>> I don't know gdbserver well enough to decide whether the test-case 
>> assumption
>> is wrong and we need to fix the test-case, or there's a gdbserver problem
>> which is known or for which we can file a PR.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>>
> 
> Ping.
> 

I've ended up fixing this problem by commit 079f190d4cf 
("[gdb/testsuite] Fix gdb.server/server-kill.exp for remote target"), so 
this patch is now obsolete.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 93daf482907..05f4f21bc0c 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -75,10 +75,10 @@  proc prepare {} {
     gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"]
     gdb_continue_to_breakpoint "after server_pid assignment"
 
-    if { $::kill_pid_of == "inferior" } {
+    set server_pid 0
+    if { $::kill_pid_of == "server" } {
 	# Get the pid of GDBServer.
 	set test "p server_pid"
-	set server_pid 0
 	gdb_test_multiple $test $test {
 	    -re " = ($decimal)\r\n$gdb_prompt $" {
 		set server_pid $expect_out(1,string)
@@ -86,7 +86,13 @@  proc prepare {} {
 	    }
 	}
     } else {
-	set server_pid [exp_pid -i $::server_spawn_id]
+	set test "inferior"
+	gdb_test_multiple $test $test {
+	    -re -wrap "Current inferior is 1 \\\[process ($decimal)\\\].*" {
+		set server_pid $expect_out(1,string)
+		pass $test
+	    }
+	}
     }
 
     if {$server_pid == 0} {