RFA: remote.c : allow long monitor cmds + allow user to C-c

RFA: remote.c : allow long monitor cmds + allow user to C-c

[Philippe Waroquiers]

The remote stub can implement monitor commands which are not
known by gdb. Such monitor commands can take a long time
to execute. An example of this is the "leak_search" monitor
command implemented in the Valgrind gdbserver.

Currently, gdb will timeout on such a monitor command.
The remote stub however will continue to execute the
command and send the output later. Gdb and the remote
stub can then be desynchronised : gdb sends a packet,
and the reply read from the stub is a previous packet.

The change below uses getpkt_sane to detect a timeout.
In this case, it continues the loop.
A QUIT; is inserted in the loop to allow the user
to stop handling the current command. possibly
still creating a desynchronisation between gdb and the stub
but that will be upon user request.

Regression tested on linux x86, with and without gdbserver.

Ok to apply ?

Philippe

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13761
diff -r1.13761 ChangeLog
0a1,5
> 2012-01-22  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* remote.c (remote_rcmd): use getpkt_sane to detect timeout
> 	and continue the loop. Add QUIT statement.
> 
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.478
diff -r1.478 remote.c
8585a8586
>       QUIT;			/* Allow user to bail out with ^C.  */
8587c8588,8596
<       getpkt (&rs->buf, &rs->buf_size, 0);
---
>       if (getpkt_sane (&rs->buf, &rs->buf_size, 0) == -1)
>         { 
>           /* Timeout. Continue to (try to) read responses.
>              This is better than stopping with an error, assuming the stub
>              is still executing the (long) monitor command.
>              If needed, the user can interrupt gdb using C-c, obtaining
>              an effect similar to stop on timeout.  */
>           continue;
>         }

Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Pedro Alves]

Patches in the default diff format are practically unreadable.

  http://sourceware.org/ml/gdb-patches/2012-01/msg00757.html

Please use "cvs diff -up", or put "diff -up" in your ~/.cvsrc file.

-- 
Pedro Alves

Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Philippe Waroquiers]

On Mon, 2012-01-23 at 12:31 +0000, Pedro Alves wrote:
> Please use "cvs diff -up", or put "diff -up" in your ~/.cvsrc file.
Oops, 2nd trial.

The remote stub can implement monitor commands which are not
known by gdb. Such monitor commands can take a long time
to execute. An example of this is the "leak_search" monitor
command implemented in the Valgrind gdbserver.

Currently, gdb will timeout on such a long monitor command.
The remote stub however will continue to execute the
command and send the output later. Gdb and the remote
stub can then be desynchronised : gdb sends a packet,
and the reply read from the stub is the one of a previous packet.

The change below uses getpkt_sane to detect a timeout.
In this case, it continues the loop.
A QUIT; is inserted in the loop to allow the user
to stop handling the current command. possibly
still creating a desynchronisation between gdb and the stub
but that will be upon user request.

Regression tested on linux x86, with and without gdbserver.

Ok to apply ?

Philippe

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13761
diff -u -p -r1.13761 ChangeLog
--- gdb/ChangeLog	20 Jan 2012 10:31:25 -0000	1.13761
+++ gdb/ChangeLog	23 Jan 2012 20:22:22 -0000
@@ -1,3 +1,8 @@
+2012-01-22  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+
+	* remote.c (remote_rcmd): use getpkt_sane to detect timeout
+	and continue the loop. Add QUIT statement.
+
 2012-01-20  Ulrich Weigand  <ulrich.weigand@linaro.org>
 
 	* NEWS: Document remote "info proc" and "generate-core-file".
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.478
diff -u -p -r1.478 remote.c
--- gdb/remote.c	20 Jan 2012 09:47:32 -0000	1.478
+++ gdb/remote.c	23 Jan 2012 20:22:24 -0000
@@ -8583,8 +8583,17 @@ remote_rcmd (char *command,
       char *buf;
 
       /* XXX - see also remote_get_noisy_reply().  */
+      QUIT;			/* Allow user to bail out with ^C.  */
       rs->buf[0] = '\0';
-      getpkt (&rs->buf, &rs->buf_size, 0);
+      if (getpkt_sane (&rs->buf, &rs->buf_size, 0) == -1)
+        { 
+          /* Timeout. Continue to (try to) read responses.
+             This is better than stopping with an error, assuming the stub
+             is still executing the (long) monitor command.
+             If needed, the user can interrupt gdb using C-c, obtaining
+             an effect similar to stop on timeout.  */
+          continue;
+        }
       buf = rs->buf;
       if (buf[0] == '\0')
 	error (_("Target does not support this command."));

Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Sergio Durigan Junior]

Hello Philippe,

Thanks for the patch.  Do you have copyright assignment?

Some formatting nits.

On Monday, January 23 2012, Philippe Waroquiers wrote:

> On Mon, 2012-01-23 at 12:31 +0000, Pedro Alves wrote:
>> Please use "cvs diff -up", or put "diff -up" in your ~/.cvsrc file.
> Oops, 2nd trial.
>
> Index: gdb/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.13761
> diff -u -p -r1.13761 ChangeLog
> --- gdb/ChangeLog	20 Jan 2012 10:31:25 -0000	1.13761
> +++ gdb/ChangeLog	23 Jan 2012 20:22:22 -0000
> @@ -1,3 +1,8 @@
> +2012-01-22  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> +
> +	* remote.c (remote_rcmd): use getpkt_sane to detect timeout
> +	and continue the loop. Add QUIT statement.

First letter shall be uppercase.  Two spaces after period, before
beginning a new sentence.  The date must also be updated if/when you
commit.

> Index: gdb/remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.478
> diff -u -p -r1.478 remote.c
> --- gdb/remote.c	20 Jan 2012 09:47:32 -0000	1.478
> +++ gdb/remote.c	23 Jan 2012 20:22:24 -0000

>        rs->buf[0] = '\0';
> -      getpkt (&rs->buf, &rs->buf_size, 0);
> +      if (getpkt_sane (&rs->buf, &rs->buf_size, 0) == -1)
> +        { 
> +          /* Timeout. Continue to (try to) read responses.
> +             This is better than stopping with an error, assuming the stub
> +             is still executing the (long) monitor command.
> +             If needed, the user can interrupt gdb using C-c, obtaining
> +             an effect similar to stop on timeout.  */
> +          continue;
> +        }

Same formatting issues here in this comment: two spaces after period.

I know almost nothing about this part of the code, but as far as I have
seen, the patch looks OK.  I am not a maintainer, however.

-- 
Sergio

Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Philippe Waroquiers]

On Tue, 2012-01-24 at 01:48 -0200, Sergio Durigan Junior wrote:
> Thanks for the patch.  Do you have copyright assignment?
Yes, all is ok on this aspect.

> Some formatting nits.
Will do the changes ...

> I know almost nothing about this part of the code, but as far as I have
> seen, the patch looks OK.  I am not a maintainer, however.
and will wait for more feedback & approval.

Thanks for the review

Philippe

(ping) Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Philippe Waroquiers]

On Tue, 2012-01-24 at 01:48 -0200, Sergio Durigan Junior wrote:

> Some formatting nits.
Fixed in the updated patch below.

> I know almost nothing about this part of the code, but as far as I have
> seen, the patch looks OK.  I am not a maintainer, however.
RFA ping ?

Thanks

Philippe


Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13804
diff -u -p -r1.13804 ChangeLog
--- gdb/ChangeLog	2 Feb 2012 20:19:02 -0000	1.13804
+++ gdb/ChangeLog	2 Feb 2012 22:05:35 -0000
@@ -1,3 +1,8 @@
+2012-02-02  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+
+	* remote.c (remote_rcmd): Use getpkt_sane to detect timeout
+	and continue the loop.  Add QUIT statement.
+
 2012-02-02  Doug Evans  <dje@google.com>
 
 	* blockframe.c (find_pc_partial_function_gnu_ifunc): Change type of
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.481
diff -u -p -r1.481 remote.c
--- gdb/remote.c	2 Feb 2012 18:04:29 -0000	1.481
+++ gdb/remote.c	2 Feb 2012 22:05:36 -0000
@@ -8590,8 +8590,17 @@ remote_rcmd (char *command,
       char *buf;
 
       /* XXX - see also remote_get_noisy_reply().  */
+      QUIT;			/* Allow user to bail out with ^C.  */
       rs->buf[0] = '\0';
-      getpkt (&rs->buf, &rs->buf_size, 0);
+      if (getpkt_sane (&rs->buf, &rs->buf_size, 0) == -1)
+        { 
+          /* Timeout.  Continue to (try to) read responses.
+             This is better than stopping with an error, assuming the stub
+             is still executing the (long) monitor command.
+             If needed, the user can interrupt gdb using C-c, obtaining
+             an effect similar to stop on timeout.  */
+          continue;
+        }
       buf = rs->buf;
       if (buf[0] == '\0')
 	error (_("Target does not support this command."));

Re: (ping) Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Stan Shebs]

On 2/2/12 2:13 PM, Philippe Waroquiers wrote:
> On Tue, 2012-01-24 at 01:48 -0200, Sergio Durigan Junior wrote:
>
>> Some formatting nits.
> Fixed in the updated patch below.
>
>> I know almost nothing about this part of the code, but as far as I have
>> seen, the patch looks OK.  I am not a maintainer, however.
> RFA ping ?

Looks reasonable to me, let's put it in.  I don't suppose there's any 
easy way to make a testsuite test for it?

Stan

>
> Thanks
>
> Philippe
>
>
> Index: gdb/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.13804
> diff -u -p -r1.13804 ChangeLog
> --- gdb/ChangeLog	2 Feb 2012 20:19:02 -0000	1.13804
> +++ gdb/ChangeLog	2 Feb 2012 22:05:35 -0000
> @@ -1,3 +1,8 @@
> +2012-02-02  Philippe Waroquiers<philippe.waroquiers@skynet.be>
> +
> +	* remote.c (remote_rcmd): Use getpkt_sane to detect timeout
> +	and continue the loop.  Add QUIT statement.
> +
>   2012-02-02  Doug Evans<dje@google.com>
>
>   	* blockframe.c (find_pc_partial_function_gnu_ifunc): Change type of
> Index: gdb/remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.481
> diff -u -p -r1.481 remote.c
> --- gdb/remote.c	2 Feb 2012 18:04:29 -0000	1.481
> +++ gdb/remote.c	2 Feb 2012 22:05:36 -0000
> @@ -8590,8 +8590,17 @@ remote_rcmd (char *command,
>         char *buf;
>
>         /* XXX - see also remote_get_noisy_reply().  */
> +      QUIT;			/* Allow user to bail out with ^C.  */
>         rs->buf[0] = '\0';
> -      getpkt (&rs->buf,&rs->buf_size, 0);
> +      if (getpkt_sane (&rs->buf,&rs->buf_size, 0) == -1)
> +        {
> +          /* Timeout.  Continue to (try to) read responses.
> +             This is better than stopping with an error, assuming the stub
> +             is still executing the (long) monitor command.
> +             If needed, the user can interrupt gdb using C-c, obtaining
> +             an effect similar to stop on timeout.  */
> +          continue;
> +        }
>         buf = rs->buf;
>         if (buf[0] == '\0')
>   	error (_("Target does not support this command."));
>
>
>
>
>
>
>
>

Re: (ping) Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Philippe Waroquiers]

> Looks reasonable to me, let's put it in.  I don't suppose there's any 
> easy way to make a testsuite test for it?

Thanks for the feedback, will commit.

Effectively, no easy way to have a testsuite test for that, as a remote command
taking a long time is needed for that.

A possible approach would be to write a test checking if a valgrind >= 3.7.0
is available (as Valgrind has a 'v.wait xxxx' milliseconds monitor command).
Not clear to me just this is worth the effort. However, the added bonus
would be to have in gdb one (or more) tests checking the connection
with the Valgrind gdbserver.

Currently, only the Valgrind test suite has tests testing the connection between its
gdbserver and gdb.

Philippe

Re: (ping) Re: RFA: remote.c : allow long monitor cmds + allow user to C-c

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> A possible approach would be to write a test checking if a
Philippe> valgrind >= 3.7.0 is available (as Valgrind has a 'v.wait
Philippe> xxxx' milliseconds monitor command).  Not clear to me just
Philippe> this is worth the effort. However, the added bonus would be to
Philippe> have in gdb one (or more) tests checking the connection with
Philippe> the Valgrind gdbserver.

I think it would be worthwhile, if you are willing.

There is already one valgrind test in gdb, but it doesn't test the new
embedded gdbserver work.  See gdb/testsuite/gdb.base/valgrind-db-attach.exp

Tom