[PATCH] Skip server-kill.exp on remote target

[PATCH] Skip server-kill.exp on remote target

[Yao Qi]

Hi,
We see the following fail in the real remote testing...

(gdb) Executing on target: kill -9 29808    (timeout = 300)
spawn [open ...]^M
sh: 1: kill: No such process

XYZ1ZYX
tstatus^M
Trace can not be run on this target.^M
(gdb) FAIL: gdb.server/server-kill.exp: tstatus

The test tries to kill gdbserver in this way:

set server_pid [exp_pid -i [board_info target fileid]]
remote_exec target "kill -9 $server_pid"

in native testing, we'll get the pid of spawned gdbserver, however, in
remote testing, we'll get the pid of ssh session, since we start
gdbserver on the remote target through ssh.  The pid on build doesn't
exist on target.

I thought about different ways to handle remote target, for example:

 - use command ps on remote target to gdbserver pid, but it doesn't work
   if we have multiple gdbserver instances during testing,
 - port number can be regarded as an id to differentiate different
   instances, but port number is unknown to each test case.  Likewise,
   we can't use 'pkill' due to the same reason.
 - close the gdbserver channel, but gdbserver doesn't exit on the target,

then, I have to skip this test for remote target.

gdb/testsuite:

2014-09-30  Yao Qi  <yao@codesourcery.com>

	* gdb.server/server-kill.exp: Skip it if the target is remote.
---
 gdb/testsuite/gdb.server/server-kill.exp | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 5d31103..45af850 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -23,6 +23,11 @@ if {[skip_gdbserver_tests]} {
     return 0
 }
 
+# We can't get the pid of gdbserver on the remote target.
+if [is_remote target] then {
+    return 0
+}
+
 if { [prepare_for_testing ${testfile}.exp ${testfile}] } {
     return -1
 }
-- 
1.9.3

Re: [PATCH] Skip server-kill.exp on remote target

[Yao Qi]

Yao Qi <yao@codesourcery.com> writes:

> gdb/testsuite:
>
> 2014-09-30  Yao Qi  <yao@codesourcery.com>
>
> 	* gdb.server/server-kill.exp: Skip it if the target is remote.

Ping.

-- 
Yao (齐尧)

Re: [PATCH] Skip server-kill.exp on remote target

[Pedro Alves]

On 09/30/2014 02:10 PM, Yao Qi wrote:
> I thought about different ways to handle remote target, for example:
> 
>  - use command ps on remote target to gdbserver pid, but it doesn't work
>    if we have multiple gdbserver instances during testing,
>  - port number can be regarded as an id to differentiate different
>    instances, but port number is unknown to each test case.  Likewise,
>    we can't use 'pkill' due to the same reason.
>  - close the gdbserver channel, but gdbserver doesn't exit on the target,

gdbserver will be the parent of the tested process.  So how about
tweaking the test program to do:

int server_pid;

int main ()
{
   server_pid = getppid ();
}

and then simply read "server_pid" out of the inferior ?

Thanks,
Pedro Alves

[PATCH 0/3] Tweak server-kill.exp for remote target

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> gdbserver will be the parent of the tested process.  So how about
> tweaking the test program to do:
>
> int server_pid;
>
> int main ()
> {
>    server_pid = getppid ();
> }
>
> and then simply read "server_pid" out of the inferior ?

That is a good idea!  Patch 2/3 is about this.  Patch 1/3 is a clean
up.  Patch 3/3 is to fix a fail we've seen on target doesn't support
tracepoint.

*** BLURB HERE ***

Yao Qi (3):
  Clean up server-kill.exp
  Get GDBserver pid on remote target
  Use command "monitor set debug 0" to check the connection

 gdb/testsuite/gdb.server/server-kill.c   | 10 +++++++++-
 gdb/testsuite/gdb.server/server-kill.exp | 23 +++++++++++++++++------
 2 files changed, 26 insertions(+), 7 deletions(-)

-- 
1.9.3

[PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Yao Qi]

Nowadays, we are using command "tstatus" to send a packet to GDBserver
in order to check the connection.  However, on the target doesn't
support tracepoint, the following error is emitted before sending any
packet to GDBserver.

tstatus^M
Trace can not be run on this target.^M
(gdb) FAIL: gdb.server/server-kill.exp: tstatus

We have to choose other commands which exists on different targets, and
send a RSP packet to GDBserver.  "monitor set debug 0" is a good one.

gdb/testsuite:

2014-10-08  Yao Qi  <yao@codesourcery.com>

	* gdb.server/server-kill.exp: Execute command "monitor set debug 0"
	instead of "tstatus".
---
 gdb/testsuite/gdb.server/server-kill.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 0f808a9..a2e65a4 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -53,4 +53,4 @@ remote_exec target "kill -9 $server_pid"
 
 # Force GDB to talk with GDBserver, so that we can get the
 # "connection closed" error.
-gdb_test "tstatus" {Remote connection closed|Remote communication error\.  Target disconnected\.: Connection reset by peer\.}
+gdb_test "monitor set debug 0" {Remote connection closed|Remote communication error\.  Target disconnected\.: Connection reset by peer\.}
-- 
1.9.3

Re: [PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Pedro Alves]

On 10/08/2014 07:27 AM, Yao Qi wrote:
> Nowadays, we are using command "tstatus" to send a packet to GDBserver
> in order to check the connection.  However, on the target doesn't
> support tracepoint, the following error is emitted before sending any
> packet to GDBserver.
> 
> tstatus^M
> Trace can not be run on this target.^M
> (gdb) FAIL: gdb.server/server-kill.exp: tstatus
> 
> We have to choose other commands which exists on different targets, and
> send a RSP packet to GDBserver.  "monitor set debug 0" is a good one.
> 

Hmm, are you sure GDB didn't really send any packet to GDBserver?

That error is printed by trace_status_command, but only after calling
target_get_trace_status, which seems to me should be sending a packet.

remote_get_trace_status's exception swallowing should be
letting TARGET_CLOSE_ERROR pass through.  Is that not working?
I'm a bit confused because it seems like if that didn't work,
we should see a "qTStatus:..." error line in the logs.

So what is preventing the packet from being sent?

Thanks,
Pedro Alves

Re: [PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> That error is printed by trace_status_command, but only after calling
> target_get_trace_status, which seems to me should be sending a packet.
>
> remote_get_trace_status's exception swallowing should be
> letting TARGET_CLOSE_ERROR pass through.  Is that not working?
> I'm a bit confused because it seems like if that didn't work,
> we should see a "qTStatus:..." error line in the logs.
>
> So what is preventing the packet from being sent?

qTStatus is disabled after receiving the empty reply during connecting
to the remote target.

(gdb) set debug remote 1
(gdb) target remote :4444
...
Sending packet: $qTStatus#49...Packet received:                                                                                                               
Packet qTStatus (trace-status) is NOT supported
...
(gdb) show remote trace-status-packet 
Support for the `qTStatus' packet is auto-detected, currently disabled.

When the test executes command "tstatus" again, remote_get_trace_status
returns -1 at the very beginning,

  if (packet_support (PACKET_qTStatus) == PACKET_DISABLE)
    return -1;

so no RSP packet is sent out.

(gdb) set debug remote 1
(gdb) tstatus 
Trace can not be run on this target.

-- 
Yao (齐尧)

Re: [PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Pedro Alves]

On 10/09/2014 04:01 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> That error is printed by trace_status_command, but only after calling
>> target_get_trace_status, which seems to me should be sending a packet.
>>
>> remote_get_trace_status's exception swallowing should be
>> letting TARGET_CLOSE_ERROR pass through.  Is that not working?
>> I'm a bit confused because it seems like if that didn't work,
>> we should see a "qTStatus:..." error line in the logs.
>>
>> So what is preventing the packet from being sent?
> 
> qTStatus is disabled after receiving the empty reply during connecting
> to the remote target.

Ah, thanks.  I missed that we request the trace status on connection.

So I think we should do "set remote trace-status-packet on" before
the "tstatus".

I know that the test originally didn't use "tstatus",
but it's really the packet we should be testing for exercising
that remote_get_trace_status's exception swallowing code,
which was very much related to the original bug this test covered,
see:

  https://sourceware.org/bugzilla/show_bug.cgi?id=15275

  https://sourceware.org/ml/gdb-patches/2013-03/msg00691.html

Thanks,
Pedro Alves

Re: [PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> So I think we should do "set remote trace-status-packet on" before
> the "tstatus".
>

Sure.

> I know that the test originally didn't use "tstatus",
> but it's really the packet we should be testing for exercising
> that remote_get_trace_status's exception swallowing code,
> which was very much related to the original bug this test covered,
> see:
>
>   https://sourceware.org/bugzilla/show_bug.cgi?id=15275
>
>   https://sourceware.org/ml/gdb-patches/2013-03/msg00691.html

Thanks for the information.  I remember the first half of the
story, but forget the second half.  How about the patch below?

-- 
Yao (齐尧)

Subject: [PATCH] Enable qTStatus packet in case it is disabled

Nowadays, we are using command "tstatus" to send a packet to GDBserver
in order to check the connection.  However, on the target doesn't
support tracepoint, the following error is emitted before sending any
packet to GDBserver.

tstatus^M
Trace can not be run on this target.^M
(gdb) FAIL: gdb.server/server-kill.exp: tstatus

qTStatus is disabled after receiving the empty reply during connecting
to the remote target.  When the test executes command "tstatus" again,
remote_get_trace_status returns -1 at the very beginning, and no RSP
packet is sent out.

This patch is to enable qTStatus packet again.

gdb/testsuite:

2014-10-10  Yao Qi  <yao@codesourcery.com>

	* gdb.server/server-kill.exp: Execute command
	"set remote trace-status-packet on" before "tstatus".

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 83c11f2..53d8e9a 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -52,6 +52,11 @@ if {$server_pid == "" } {
 
 remote_exec target "kill -9 $server_pid"
 
+# Enable trace status packet which is disabled after the connection
+# if the remote target doesn't support tracepoint at all.  Otherwise,
+# no RSP packet is sent out.
+gdb_test_no_output "set remote trace-status-packet on"
+
 # Force GDB to talk with GDBserver, so that we can get the
 # "connection closed" error.
 gdb_test "tstatus" {Remote connection closed|Remote communication error\.  Target disconnected\.: Connection reset by peer\.}

Re: [PATCH 3/3] Use command "monitor set debug 0" to check the connection

[Pedro Alves]

On 10/10/2014 02:48 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>> I know that the test originally didn't use "tstatus",
>> but it's really the packet we should be testing for exercising
>> that remote_get_trace_status's exception swallowing code,
>> which was very much related to the original bug this test covered,
>> see:
>>
>>   https://sourceware.org/bugzilla/show_bug.cgi?id=15275
>>
>>   https://sourceware.org/ml/gdb-patches/2013-03/msg00691.html
> 
> Thanks for the information.  I remember the first half of the
> story, but forget the second half.  How about the patch below?

Looks good.

Thanks,
Pedro Alves

[PATCH 2/3] Get GDBserver pid on remote target

[Yao Qi]

Hi,
We see the following fail in the real remote testing...

(gdb) Executing on target: kill -9 29808    (timeout = 300)
spawn [open ...]^M
sh: 1: kill: No such process

The test tries to kill gdbserver in this way:

set server_pid [exp_pid -i [board_info target fileid]]
remote_exec target "kill -9 $server_pid"

in native testing, we'll get the pid of spawned gdbserver, however, in
remote testing, we'll get the pid of ssh session, since we start
gdbserver on the remote target through ssh.  The pid on build doesn't
exist on target.

In this patch, we tweak server-kill.c to get the parent pid, which is
the pid of GDBserver.  GDB gets it and kill GDBserver on target.

gdb/testsuite:

2014-10-08  Yao Qi  <yao@codesourcery.com>

	* gdb.server/server-kill.c: Include sys/types.h and unistd.h.
	(main): Call getppid.
	* gdb.server/server-kill.exp: Set breakpoint on line "i = 0;"
	and continue to it.  Read variable "server_pid".
---
 gdb/testsuite/gdb.server/server-kill.c   | 10 +++++++++-
 gdb/testsuite/gdb.server/server-kill.exp | 17 ++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.server/server-kill.c b/gdb/testsuite/gdb.server/server-kill.c
index 8a7c74e..8369024b 100644
--- a/gdb/testsuite/gdb.server/server-kill.c
+++ b/gdb/testsuite/gdb.server/server-kill.c
@@ -15,10 +15,18 @@
    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 <sys/types.h>
+#include <unistd.h>
+
+int server_pid;
+
 int
 main (void)
 {
-  int i = 0;
+  int i;
+
+  server_pid = getppid ();
+  i = 0;
 
   return i;
 }
diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index b23c2b5..0f808a9 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -33,7 +33,22 @@ gdb_test "disconnect" ".*"
 
 gdbserver_run ""
 
-set server_pid [exp_pid -i [board_info target fileid]]
+# Continue past server_pid assignment.
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"]
+gdb_continue_to_breakpoint "after server_pid assignment"
+
+# Get the pid of GDBServer.
+set test "p server_pid"
+gdb_test_multiple $test $test {
+    -re " = ($decimal)\r\n$gdb_prompt $" {
+	set server_pid $expect_out(1,string)
+	pass $test
+    }
+    -re "$gdb_prompt $" {
+	fail $test
+	return -1
+    }
+}
 remote_exec target "kill -9 $server_pid"
 
 # Force GDB to talk with GDBserver, so that we can get the
-- 
1.9.3

Re: [PATCH 2/3] Get GDBserver pid on remote target

[Pedro Alves]

On 10/08/2014 07:27 AM, Yao Qi wrote:
> Hi,
> We see the following fail in the real remote testing...
> 
> (gdb) Executing on target: kill -9 29808    (timeout = 300)
> spawn [open ...]^M
> sh: 1: kill: No such process
> 
> The test tries to kill gdbserver in this way:
> 
> set server_pid [exp_pid -i [board_info target fileid]]
> remote_exec target "kill -9 $server_pid"
> 
> in native testing, we'll get the pid of spawned gdbserver, however, in
> remote testing, we'll get the pid of ssh session, since we start
> gdbserver on the remote target through ssh.  The pid on build doesn't
> exist on target.
> 
> In this patch, we tweak server-kill.c to get the parent pid, which is
> the pid of GDBserver.  GDB gets it and kill GDBserver on target.
> 
> gdb/testsuite:
> 
> 2014-10-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.server/server-kill.c: Include sys/types.h and unistd.h.
> 	(main): Call getppid.
> 	* gdb.server/server-kill.exp: Set breakpoint on line "i = 0;"
> 	and continue to it.  Read variable "server_pid".
> ---
>  gdb/testsuite/gdb.server/server-kill.c   | 10 +++++++++-
>  gdb/testsuite/gdb.server/server-kill.exp | 17 ++++++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.server/server-kill.c b/gdb/testsuite/gdb.server/server-kill.c
> index 8a7c74e..8369024b 100644
> --- a/gdb/testsuite/gdb.server/server-kill.c
> +++ b/gdb/testsuite/gdb.server/server-kill.c
> @@ -15,10 +15,18 @@
>     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 <sys/types.h>
> +#include <unistd.h>
> +
> +int server_pid;
> +
>  int
>  main (void)
>  {
> -  int i = 0;
> +  int i;
> +
> +  server_pid = getppid ();
> +  i = 0;
>  
>    return i;
>  }
> diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
> index b23c2b5..0f808a9 100644
> --- a/gdb/testsuite/gdb.server/server-kill.exp
> +++ b/gdb/testsuite/gdb.server/server-kill.exp
> @@ -33,7 +33,22 @@ gdb_test "disconnect" ".*"
>  
>  gdbserver_run ""
>  
> -set server_pid [exp_pid -i [board_info target fileid]]
> +# Continue past server_pid assignment.
> +gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"]
> +gdb_continue_to_breakpoint "after server_pid assignment"
> +
> +# Get the pid of GDBServer.
> +set test "p server_pid"
> +gdb_test_multiple $test $test {
> +    -re " = ($decimal)\r\n$gdb_prompt $" {
> +	set server_pid $expect_out(1,string)
> +	pass $test
> +    }
> +    -re "$gdb_prompt $" {
> +	fail $test
> +	return -1
> +    }
> +}

If gdb_test_multiple catches an internal error, or a timeout, this
won't return, and below we'll end up using $server_pid without it
being set.  So please do the usual (also note "GDBserver" typo):

# Get GDBserver's pid.

set server_pid ""
set test "p server_pid"
gdb_test_multiple $test $test {
    -re " = ($decimal)\r\n$gdb_prompt $" {
	set server_pid $expect_out(1,string)
	pass $test
    }
}
if {$server_pid == "" } {
    return -1
}

This is OK with that change.

I think mingw doesn't have getppid; so as is this will be losing
coverage there.  gnulib doesn't appear to have something we could borrow,
unfortunately.  I think we could emulate it with NtQueryInformationProcess,
but it's fine with me to leave it for another pass.

Thanks,
Pedro Alves

[PATCH 1/3] Clean up server-kill.exp

[Yao Qi]

This patch is to remove some lines which looks unnecessary.  These
lines were added when server-kill.exp was added.  In the version 1,
https://sourceware.org/ml/gdb-patches/2013-03/msg00691.html the test
calls runto_main and delete breakpoint on main,

+if ![runto_main] {
+    return -1
+}
+
+# Otherwise the breakpoint at 'main' would not cause insert breakpoints during
+# first step.
+delete_breakpoints

However, in the version 2
https://sourceware.org/ml/gdb-patches/2013-03/msg00854.html runto_main
is removed but delete_breakpoints is still there.  AFAICS, the line of
delete_breakpoints can be removed too.

gdb/testsuite:

2014-10-08  Yao Qi  <yao@codesourcery.com>

	* gdb.server/server-kill.exp: Remove "delete_breakpoints".
---
 gdb/testsuite/gdb.server/server-kill.exp | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 5d31103..b23c2b5 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -33,10 +33,6 @@ gdb_test "disconnect" ".*"
 
 gdbserver_run ""
 
-# Otherwise the breakpoint at 'main' would not cause insert
-# breakpoints during first step.
-delete_breakpoints
-
 set server_pid [exp_pid -i [board_info target fileid]]
 remote_exec target "kill -9 $server_pid"
 
-- 
1.9.3

Re: [PATCH 1/3] Clean up server-kill.exp

[Pedro Alves]

On 10/08/2014 07:27 AM, Yao Qi wrote:
> This patch is to remove some lines which looks unnecessary.  These
> lines were added when server-kill.exp was added.  In the version 1,
> https://sourceware.org/ml/gdb-patches/2013-03/msg00691.html the test
> calls runto_main and delete breakpoint on main,
> 
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Otherwise the breakpoint at 'main' would not cause insert breakpoints during
> +# first step.
> +delete_breakpoints
> 
> However, in the version 2
> https://sourceware.org/ml/gdb-patches/2013-03/msg00854.html runto_main
> is removed but delete_breakpoints is still there.  AFAICS, the line of
> delete_breakpoints can be removed too.
> 
> gdb/testsuite:
> 
> 2014-10-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.server/server-kill.exp: Remove "delete_breakpoints".

OK.

Thanks,
Pedro Alves

Re: [PATCH 0/3] Tweak server-kill.exp for remote target

[Yao Qi]

Yao Qi <yao@codesourcery.com> writes:

> That is a good idea!  Patch 2/3 is about this.  Patch 1/3 is a clean
> up.  Patch 3/3 is to fix a fail we've seen on target doesn't support
> tracepoint.

These three patches are pushed in.

-- 
Yao (齐尧)