This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/3] async remote notification 'Trace'.


On 01/08/2013 01:46 PM, Yao Qi wrote:
> On 01/08/2013 03:20 AM, Pedro Alves wrote:
>> On 12/19/2012 10:54 AM, Yao Qi wrote:
>>>> +static void
>>>> +remote_notif_trace_parse (struct notif_client *self, char *buf,
>>>> +			  struct notif_event *event)
>>>> +{
>>>> +  if (strncmp (buf, "stop:", 5) == 0)
>>>> +    parse_trace_status (buf + 5 + 1, current_trace_status ());
>> Why "+ 1" ?
>>
>
> To skip "stop:T" in BUF, as parse_trace_status parses BUF after 'T'.
> I add a line of comment here to explain this.

Comments on that below.

>
>>>> +  else
>>>> +    error (_("Unknown trace notification."));
>> What will be the upgrade path for new notifications?
>> Should gdb reply some sort of error to the remote?
>>
>
> Current notification mechanism doesn't have a way for GDB to reply
> error to the remote for after parsing a notification.  On the other
> hand, the user manual reads "Recipients should silently ignore
> corrupted notifications and notifications they do not understand.",
> so I think it would be OK to just ignore it.

I think that's a hole in the "spec".  If GDB just ignores, and
doesn't send the ack, then the remote will get stuck, and not
send any other notification.  It looks to me that we either
need to make GDB send a "ack-but-I-didn't-understand", or,
I think perhaps better, or in addition, make GDB and the remote
negotiate the set of supported notifications upfront.

That raises the question: new gdbserver with old gdb must
be getting the trace notifications stuck, because gdb is
not acking them, right?  It so happens that gdbserver doesn't
implement a timeout and retry mechanism today (it could,
notifications were designed with that in mind, for non-reliable
transports, like serial/rs232).

>
> 2013-01-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* notif.c (notifs): Add "notif_trace".
> 	* notif.h (ontif_trace): Declare.
> 	* tracepoint.c [!IN_PRCESS_AGENT]: Include "notif.h".

Typo.

> 	(notif_reply_trace): New.
> 	(notif_trace): New variable.
> 	(stop_tracing) [!IN_PROCESS_AGENT]: Call notif_push.
> gdb:
> 
> 2012-01-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* Makefile.in (REMOTE_OBS): Append remote-notif-trace.o
> 	(SFILES): Add remote-notif-trace.c
> 	* remote-notif.c (notifs): Add "notif_client_trace".
> 	* remote-notif.h (notif_client_trace): Declare.
> 	* remote-notif-trace.c: New.
> 
> gdb/doc:
> 
> 2012-01-08  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.texinfo (Packets): Add "vTraced".
> 	(Notification Packets): Add doc about "Trace" notification
> 	and "vTraced" packet.
> ---
>  gdb/Makefile.in            |    5 ++-
>  gdb/doc/gdb.texinfo        |    9 +++++
>  gdb/gdbserver/notif.c      |    1 +
>  gdb/gdbserver/notif.h      |    1 +
>  gdb/gdbserver/tracepoint.c |   27 ++++++++++++++++
>  gdb/remote-notif-trace.c   |   75 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/remote-notif.c         |    1 +
>  gdb/remote-notif.h         |    1 +
>  8 files changed, 118 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/remote-notif-trace.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index b065d41..bc2f527 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -508,7 +508,7 @@ SER_HARDWIRE = @SER_HARDWIRE@
>  # The `remote' debugging target is supported for most architectures,
>  # but not all (e.g. 960)
>  REMOTE_OBS = remote.o dcache.o tracepoint.o ax-general.o ax-gdb.o remote-fileio.o \
> -	remote-notif.o
> +	remote-notif.o remote-notif-trace.o
>  
>  # This is remote-sim.o if a simulator is to be linked in.
>  SIM_OBS = @SIM_OBS@
> @@ -732,7 +732,8 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
>  	p-exp.y p-lang.c p-typeprint.c p-valprint.c parse.c printcmd.c \
>  	proc-service.list progspace.c \
>  	prologue-value.c psymtab.c \
> -	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c reverse.c \
> +	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c \
> +	remote-notif-trace.c reverse.c \
>  	sentinel-frame.c \
>  	serial.c ser-base.c ser-unix.c skip.c \
>  	solib.c solib-target.c source.c \
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index fc95330..0eefa34 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -36253,6 +36253,8 @@ for success (@pxref{Stop Reply Packets})
>  
>  @item vStopped
>  @cindex @samp{vStopped} packet
> +@itemx vTraced
> +@cindex @samp{vTraced} packet
>  @xref{Notification Packets}.
>  
>  @item X @var{addr},@var{length}:@var{XX@dots{}}
> @@ -38135,6 +38137,7 @@ continue the tracing run, while 0 tells the target to stop tracing if
>  @value{GDBN} is no longer in the picture.
>  
>  @item qTStatus
> +@anchor{qTStatus packet}
>  @cindex @samp{qTStatus} packet
>  Ask the stub if there is a trace experiment running right now.
>  
> @@ -38658,6 +38661,12 @@ for information on how these notifications are acknowledged by
>  @value{GDBN}.
>  @tab Report an asynchronous stop event in non-stop mode.
>  
> +@item Trace
> +@tab vTraced
> +@tab @code{stop}:@var{reply}.  The @var{reply} has the form of the reply
> +to packet @samp{qTStatus}, as described in @ref{qTStatus packet}.
> +@tab Report an asynchronous trace-related event.

There are some things in the qTStatus reply (the optional bits)
that don't make that much sense for this notification, but
I agree it's just simpler to reuse, thus better.

> +
>  @end multitable
>  
>  @node Remote Non-Stop
> diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
> index e27746e..fadae11 100644
> --- a/gdb/gdbserver/notif.c
> +++ b/gdb/gdbserver/notif.c
> @@ -52,6 +52,7 @@
>  static struct notif_server *notifs[] =
>  {
>    &notif_stop,
> +  &notif_trace,
>  };
>  
>  /* Write another event or an OK, if there are no more left, to
> diff --git a/gdb/gdbserver/notif.h b/gdb/gdbserver/notif.h
> index 608b763..d794b4d 100644
> --- a/gdb/gdbserver/notif.h
> +++ b/gdb/gdbserver/notif.h
> @@ -53,6 +53,7 @@ typedef struct notif_server
>  } *notif_server_p;
>  
>  extern struct notif_server notif_stop;
> +extern struct notif_server notif_trace;
>  
>  int handle_notif_ack (char *own_buf, int packet_len);
>  void notif_write_event (struct notif_server *notif, char *own_buf);
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 0367eb4..2726183 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -3372,6 +3372,25 @@ cmd_qtstart (char *packet)
>    write_ok (packet);
>  }
>  
> +#ifndef IN_PROCESS_AGENT
> +#include "notif.h"
> +
> +static void cmd_qtstatus (char *packet);
> +
> +static void
> +notif_reply_trace (struct notif_event *event, char *own_buf)
> +{
> +  sprintf (own_buf, "stop:");
> +  cmd_qtstatus (own_buf + 5);
> +}
> +
> +struct notif_server notif_trace =
> +{
> +  "vTraced", "Trace", NULL, notif_reply_trace
> +};
> +
> +#endif
> +
>  /* End a tracing run, filling in a stop reason to report back to GDB,
>     and removing the tracepoints from the code.  */
>  
> @@ -3472,6 +3491,14 @@ stop_tracing (void)
>      }
>  
>    unpause_all (1);
> +
> +#ifndef IN_PROCESS_AGENT

Not necessary.  The IPA has its own different implementation
elsewhere in the file.

> +  {
> +    struct notif_event *event = malloc (sizeof (struct notif_event));
> +
> +    notif_push (&notif_trace, event);
> +  }
> +#endif
>  }
>  
>  static int
> diff --git a/gdb/remote-notif-trace.c b/gdb/remote-notif-trace.c
> new file mode 100644
> index 0000000..c74d9cb
> --- /dev/null
> +++ b/gdb/remote-notif-trace.c
> @@ -0,0 +1,75 @@
> +/* Async remote notification on trace.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.

2013

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   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 "defs.h"
> +#include <string.h>
> +#include "remote.h"
> +#include "tracepoint.h"
> +#include "remote-notif.h"
> +
> +static void
> +remote_notif_trace_parse (struct notif_client *self, char *buf,
> +			  struct notif_event *event)
> +{
> +  if (strncmp (buf, "stop:", 5) == 0)
> +    {
> +      struct trace_status *ts = current_trace_status ();
> +
> +      /* Skip "stop:T" in BUF.  */
> +      parse_trace_status (buf + 5 + 1, ts);

Then you should either error if you see something
not T after "stop:", or ignore it.  Not blindly skip.

> +      gdb_assert (!ts->running);

- the notification includes the whole trace status report.
- the trace status includes a field, called "T", that
  indicates whether the trace is running ("T1") or
  stopped ("T0").
- thus, "stop:T0" in "Trace:stop:T0" is redundant.
- that leads to that assertion prohibiting
  "Trace:stop:T1".

An option would be to call it "status" (as in "trace status
has changed) instead of "stop", so you'd get:

  "Trace:status:T0"

which allows for:

  "Trace:status:T1"

Another option would be to call it "T0"

  "Trace:T0"

I can live with things as is, just pointing it
out that this made me raise my eyebrows.

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]