Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Pedro Alves <palves at redhat dot com>]

On 02/22/2013 03:22 PM, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> We might want to add support for LBR branch tracing in the future.
> 
> To prepare for this, I would specify the branch trace recording method
> in the Qbtrace packet when requesting branch tracing.
> 
> The supported branch trace recording methods will further be listed
> in response to a QSupported packet.
> 
> The patch works but I'm not sure whether it is in the spirit of
> the remote serial protocol.

I'm not sure I have a definite answer.  I'll just shoot some
answers/questions, and let's see if we can converse on something.

Hmm:

   add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace],
-       "Qbtrace", "enable-btrace", 0);
+       "Qbtrace:bts", "enable-btrace", 0);

How would you change this when another method in addition to bts
is added?

What if a remote stub sends in "Qbtrace:foo", but no mention of
bts?  The remote understands "Qbtrace:off", but this does not
express that.

> 
> 
> This patch does not enable GDB to support different recording methods.
> GDB still only supports BTS-based branch trace recording.
> 
> But I hope this will avoid having to change the remote serial protocol
> later on when we want to add the new recording method.
> 
> 
> 2013-02-22 Markus Metzger  <markus.t.metzger@intel.com>

Double space after date too.

> ---
>  gdb/doc/gdb.texinfo    |   10 +++++++---
>  gdb/gdbserver/server.c |    6 +++---
>  gdb/remote.c           |    4 ++--
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index d6842f1..ec934a5 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -37570,7 +37570,10 @@ rather than reporting the hit to @value{GDBN}.
>  The remote stub understands the @samp{qbtrace} packet.
>  
>  @item Qbtrace
> -The remote stub understands the @samp{Qbtrace} packet.
> +The remote stub understands the @samp{Qbtrace} packet.  The branch
> +trace recording method will be specified after the packet name
> +separated by a single colon (@code{:}).  There will be one entry for
> +each supported branch trace recording method.

How are the multiple supported methods separated?

I think s/will be/is/ is better in the manual.

Hmm, I think the user reading this is left wondering what
exactly are the supported methods.

qSupported already has a way of passing arguments:

@table @samp
@item @var{name}=@var{value}
The remote protocol feature @var{name} is supported, and associated
with the specified @var{value}.  The format of @var{value} depends
on the feature, but it must not include a semicolon.
@item @var{name}+

So, that'd be, e.g.:

 Qbtrace=bts,foo,bar

An existing feature that supports multiple values is:

 @item xmlRegisters
 This feature indicates that @value{GDBN} supports the XML target
 description.  If the stub sees @samp{xmlRegisters=} with target
 specific strings separated by a comma, it will report register
 description.

>  
>  @end table
>  
> @@ -37951,8 +37954,9 @@ No new branch trace data is available.
>  A badly formed request or an error was encountered.
>  @end table
>  
> -@item Qbtrace:on
> -Enable branch tracing for the current thread.
> +@item Qbtrace:@var{method}
> +Enable branch tracing for the current thread using the @var{method}
> +branch trace recording method.

I think a mention / cross reference to the corresponding qSupported
packet description would be nice.

>  
>  Reply:
>  @table @samp
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index c335af7..debef5d 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -463,12 +463,12 @@ handle_btrace_general_set (char *own_buf)
>  
>    err = NULL;
>  
> -  if (strcmp (op, "on") == 0)
> +  if (strcmp (op, "bts") == 0)
>      err = handle_btrace_enable (thread);
>    else if (strcmp (op, "off") == 0)
>      err = handle_btrace_disable (thread);
>    else
> -    err = "E.Bad Qbtrace operation. Use on or off.";
> +    err = "E.Bad Qbtrace operation. Use bts or off.";
>  
>    if (err != 0)
>      strcpy (own_buf, err);
> @@ -1828,7 +1828,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>        if (target_supports_btrace ())
>  	{
>  	  strcat (own_buf, ";qbtrace+");
> -	  strcat (own_buf, ";Qbtrace+");
> +	  strcat (own_buf, ";Qbtrace:bts+");
>  	  strcat (own_buf, ";qXfer:btrace:read+");
>  	}
>  
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 6f95125..2322ece 100755
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -11164,7 +11164,7 @@ send_Qbtrace (ptid_t ptid, int enable)
>    set_general_thread (ptid);
>  
>    buf += xsnprintf (buf, endbuf - buf, "%s:", packet->name);
> -  buf += xsnprintf (buf, endbuf - buf, enable ? "on" : "off");
> +  buf += xsnprintf (buf, endbuf - buf, enable ? "bts" : "off");
>    putpkt (rs->buf);
>    getpkt (&rs->buf, &rs->buf_size, 0);
>  
> @@ -11928,7 +11928,7 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>         "qbtrace", "query-btrace", 0);
>  
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace],
> -       "Qbtrace", "enable-btrace", 0);
> +       "Qbtrace:bts", "enable-btrace", 0);
>  
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace],
>         "qXfer:btrace", "read-btrace", 0);
> 


-- 
Pedro Alves