This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet
- From: Pedro Alves <palves at redhat dot com>
- To: markus dot t dot metzger at intel dot com
- Cc: jan dot kratochvil at redhat dot com, gdb-patches at sourceware dot org, markus dot t dot metzger at gmail dot com
- Date: Mon, 25 Feb 2013 20:32:45 +0000
- Subject: Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet
- References: <1361546562-26827-1-git-send-email-markus.t.metzger@intel.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