Re: [patch v4 08/13] remote, btrace: add branch trace remote ops

[Pedro Alves <palves at redhat dot com>]

New packets always need documentation.  And NEWS entries too.
The new "set remote XXX" commands need docs too.

On 11/27/2012 10:49 AM, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> Add the gdb remote target operations for branch tracing. We define the following
> packets:
> 
>   qbtrace:<ptid>  query if new trace data is available for a thread
>                   returns "yes" or "no" or "Enn"

The code isn't handling "no".

> 
>   Qbtrace:on:<ptid>  enable branch tracing for one thread
>                      returns "OK" or "Enn"
> 
>   Qbtrace:off:<ptid>  disable branch tracing for one thread
>                       returns "OK" or "Enn"

Nit,  I'd put <ptid> first:

>   Qbtrace:<ptid>:on  enable branch tracing for one thread
>                      returns "OK" or "Enn"
>
>   Qbtrace:<ptid>:off  disable branch tracing for one thread
>                       returns "OK" or "Enn"


How does "btrace enable all" for new threads work with remote targets?
GDB isn't notified of new threads until they stop for some reason.

> 
>   qXfer:btrace:read:<ptid>   read the full branch trace data for one thread

No need to pass the thread in the annex.  qXfer packets use the general
context set by remote_xfer_partial -> set_current_thread.

> 	(_initialize_remote): Add btrace packets.

Say:

        (_initialize_remote): Add packet configuration for
        branch tracing.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 929d4f5..c512b93 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -67,6 +67,7 @@
>  #include "ax.h"
>  #include "ax-gdb.h"
>  #include "agent.h"
> +#include "btrace.h"

Should be mentioned on the change log too.

>  
>  /* Temp hacks for tracepoint encoding migration.  */
>  static char *target_buf;
> @@ -1292,6 +1293,9 @@ enum {
>    PACKET_qXfer_fdpic,
>    PACKET_QDisableRandomization,
>    PACKET_QAgent,
> +  PACKET_qbtrace,
> +  PACKET_Qbtrace,
> +  PACKET_qXfer_btrace,
>    PACKET_MAX
>  };
>  
> @@ -3946,6 +3950,10 @@ static struct protocol_feature remote_protocol_features[] = {
>    { "QAgent", PACKET_DISABLE, remote_supported_packet, PACKET_QAgent},
>    { "tracenz", PACKET_DISABLE,
>      remote_string_tracing_feature, -1 },
> +  { "qbtrace", PACKET_DISABLE, remote_supported_packet, PACKET_qbtrace },
> +  { "Qbtrace", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace },
> +  { "qXfer:btrace:read", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_qXfer_btrace }
>  };
>  
>  static char *remote_support_xml;
> @@ -8682,6 +8690,10 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
>        return remote_read_qxfer (ops, "uib", annex, readbuf, offset, len,
>  				&remote_protocol_packets[PACKET_qXfer_uib]);
>  
> +    case TARGET_OBJECT_BTRACE:
> +      return remote_read_qxfer (ops, "btrace", annex, readbuf, offset, len,
> +        &remote_protocol_packets[PACKET_qXfer_btrace]);
> +
>      default:
>        return -1;
>      }
> @@ -11001,6 +11013,137 @@ remote_can_use_agent (void)
>    return (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE);
>  }
>  
> +struct btrace_target_info
> +{
> +  /* The ptid of the traced thread.  */
> +  ptid_t ptid;
> +};

Hmm, why would you need to store this?  To get at a struct btrace_target_info,
you need to start from a struct thread_info, so why not just pass that
down, or pass the ptid down?

> +
> +/* Check whether the target supports branch tracing.  */
> +static int
> +remote_supports_btrace (void)
> +{
> +  if (remote_protocol_packets[PACKET_qbtrace].support != PACKET_ENABLE)
> +    return 0;
> +  if (remote_protocol_packets[PACKET_Qbtrace].support != PACKET_ENABLE)
> +    return 0;
> +  if (remote_protocol_packets[PACKET_qXfer_btrace].support != PACKET_ENABLE)
> +    return 0;
> +
> +  return 1;
> +}
> +
> +/* Enable branch tracing for @ptid.  */
> +static struct btrace_target_info *
> +remote_enable_btrace (ptid_t ptid)
> +{
> +  struct packet_config *packet = &remote_protocol_packets[PACKET_Qbtrace];
> +  struct remote_state *rs = get_remote_state ();
> +  struct btrace_target_info *tinfo = NULL;
> +  char *buf = rs->buf;
> +  char *endbuf = rs->buf + get_remote_packet_size ();
> +
> +  if (packet->support != PACKET_ENABLE)
> +    error (_("Target does not support branch tracing."));
> +
> +  buf += xsnprintf (buf, endbuf - buf, "%s", packet->name);
> +  buf += xsnprintf (buf, endbuf - buf, ":on:");
> +  buf = write_ptid (buf, endbuf, ptid);
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, &rs->buf_size, 0);
> +
> +  if (packet_ok (rs->buf, packet) != PACKET_OK)
> +    error (_("Could not enable branch tracing for %s."),
> +           target_pid_to_str (ptid));
> +
> +  tinfo = xzalloc (sizeof (*tinfo));
> +  tinfo->ptid = ptid;
> +
> +  return tinfo;
> +}
> +
> +/* Disable branch tracing.  */
> +static void
> +remote_disable_btrace (struct btrace_target_info *tinfo)
> +{
> +  struct packet_config *packet = &remote_protocol_packets[PACKET_Qbtrace];
> +  struct remote_state *rs = get_remote_state ();
> +  char *buf = rs->buf;
> +  char *endbuf = rs->buf + get_remote_packet_size ();
> +
> +  if (packet->support != PACKET_ENABLE)
> +    error (_("Target does not support branch tracing."));
> +
> +  buf += xsnprintf (buf, endbuf - buf, "%s", packet->name);
> +  buf += xsnprintf (buf, endbuf - buf, ":off:");
> +  buf = write_ptid (buf, endbuf, tinfo->ptid);
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, &rs->buf_size, 0);
> +
> +  if (packet_ok (rs->buf, packet) != PACKET_OK)
> +    error (_("Could not disable branch tracing for %s."),
> +           target_pid_to_str (tinfo->ptid));
> +
> +  xfree (tinfo);
> +}

remote_enable_btrace/remote_disable_btrace are almost identical.
You could factor this out to a function that takes a on/off boolean.


-- 
Pedro Alves