Re: [patch v4 09/13] gdbserver, btrace: add generic btrace support

[Pedro Alves <palves@redhat.com>]

On 11/27/2012 10:49 AM, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> Add support to gdbserver to understand branch trace related packages.
> 
> 2012-11-27 Markus Metzger <markus.t.metzger@intel.com>
> 
> gdbserver/
> 	* target.h (struct target_ops): Add btrace ops.
> 	(target_supports_btrace): New macro.
> 	(target_btrace_has_changed): New macro.
> 	(target_enable_btrace): New macro.
> 	(target_disable_btrace): New macro.
> 	(target_read_btrace): New macro.
> 	* gdbthread.h (struct thread_info): Add btrace field.
> 	* server.c (handle_btrace_general_set): New function.
> 	(handle_btrace_enable): New function.
> 	(handle_btrace_disable): New function.
> 	(handle_general_set): Call handle_btrace_general_set.
> 	(handle_qxfer_btrace): New function.
> 	(struct qxfer qxfer_packets[]): Add btrace entry.
> 	(handle_btrace_query): New function.
> 	(handle_query): Add btrace to supported query, call handle_btrace_query.
> 	* inferiors.c (remove_thread): Disable btrace.
> 
> 
> ---
>  gdb/gdbserver/gdbthread.h |    5 ++
>  gdb/gdbserver/inferiors.c |    3 +
>  gdb/gdbserver/server.c    |  167 +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/gdbserver/target.h    |   33 +++++++++
>  4 files changed, 208 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
> index adc23da..6238ed2 100644
> --- a/gdb/gdbserver/gdbthread.h
> +++ b/gdb/gdbserver/gdbthread.h
> @@ -22,6 +22,8 @@
>  
>  #include "server.h"
>  
> +struct btrace_target_info;
> +
>  struct thread_info
>  {
>    struct inferior_list_entry entry;
> @@ -58,6 +60,9 @@ struct thread_info
>     Each item in the list holds the current step of the while-stepping
>     action.  */
>    struct wstep_state *while_stepping;
> +
> +  /* Branch trace target information for this thread.  */
> +  struct btrace_target_info *btrace;
>  };
>  
>  extern struct inferior_list all_threads;
> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index 76abaf5..76f5731 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -161,6 +161,9 @@ free_one_thread (struct inferior_list_entry *inf)
>  void
>  remove_thread (struct thread_info *thread)
>  {
> +  if (thread->btrace)
> +    target_disable_btrace (thread->btrace);
> +
>    remove_inferior (&all_threads, (struct inferior_list_entry *) thread);
>    free_one_thread (&thread->entry);
>  }
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index fae9199..2758d40 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -442,6 +442,82 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
>  
>  /* Handle all of the extended 'Q' packets.  */

This comment above should be kept close to the
corresponding function.

>  
> +/* Handle btrace enabling.  */
> +static int
> +handle_btrace_enable (char *ptid_str)
> +{
> +  ptid_t ptid = read_ptid (ptid_str, NULL);
> +  struct thread_info *thread = find_thread_ptid (ptid);
> +  int err = 1;
> +
> +  if (!thread)
> +    fprintf (stderr, "No such thread: %s.\n", ptid_str);
> +  else if (thread->btrace)
> +    fprintf (stderr, "Btrace already enabled for %s.\n", ptid_str);
> +  else if (!the_target->enable_btrace)
> +    fprintf (stderr, "Target does not support branch tracing.");
> +  else
> +    {
> +      thread->btrace = target_enable_btrace (thread->entry.id);
> +      if (!thread->btrace)
> +        fprintf (stderr, "Could not enable btrace for %s.\n", ptid_str);
> +      else
> +        err = 0;
> +    }
> +
> +  return err;

Instead of printing those errors to stderr, you can pass them
down to GDB, by sending "E.ERRTXT" instead of E01.  (See
packet_check_result in remote.c).

> +}
> +
> +/* Handle btrace disabling.  */
> +static int
> +handle_btrace_disable (char *ptid_str)
> +{
> +  ptid_t ptid = read_ptid (ptid_str, NULL);
> +  struct thread_info *thread = find_thread_ptid (ptid);
> +  int err = 1;
> +
> +  if (!thread)
> +    fprintf (stderr, "No such thread: %s.\n", ptid_str);
> +  else if (!thread->btrace)
> +    fprintf (stderr, "Branch tracing not enabled for %s.\n", ptid_str);
> +  else if (!the_target->disable_btrace)
> +    fprintf (stderr, "Target does not support branch tracing.");
> +  else if (target_disable_btrace (thread->btrace) != 0)
> +    fprintf (stderr, "Could not disable branch tracing for %s.\n", ptid_str);
> +  else
> +    {
> +      thread->btrace = NULL;
> +      err = 0;
> +    }
> +
> +  return err;
> +}

Same comment about handle_btrace_enable/handle_btrace_disable
being almost identical.

> +
> +/* Handle the "Qbtrace" packet.  */
> +static int
> +handle_btrace_general_set (char *own_buf)
> +{
> +  char *operation = own_buf + strlen ("Qbtrace:");
> +  int err = 1;
> +
> +  if (strncmp ("Qbtrace:", own_buf, strlen ("Qbtrace:")) != 0)
> +    return 0;
> +
> +  if (strncmp ("on:", operation, strlen ("on:")) == 0)
> +    err = handle_btrace_enable (operation + strlen ("on:"));
> +  else if (strncmp ("off:", operation, strlen ("off:")) == 0)
> +    err = handle_btrace_disable (operation + strlen ("off:"));
> +  else
> +    fprintf (stderr, "Unknown btrace operation: %s. Use on or off.\n", own_buf);
> +
> +  if (err)
> +    write_enn (own_buf);
> +  else
> +    write_ok (own_buf);
> +
> +  return 1;
> +}
> +
>  static void
>  handle_general_set (char *own_buf)
>  {
> @@ -596,6 +672,9 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (handle_btrace_general_set (own_buf))
> +    return;
> +
>    /* Otherwise we didn't know what packet it was.  Say we didn't
>       understand it.  */
>    own_buf[0] = 0;
> @@ -1295,6 +1374,55 @@ handle_qxfer_fdpic (const char *annex, gdb_byte *readbuf,
>    return (*the_target->read_loadmap) (annex, offset, readbuf, len);
>  }
>  
> +/* Handle branch trace data transfer.  */
> +static int
> +handle_qxfer_btrace (const char *annex,
> +		     gdb_byte *readbuf, const gdb_byte *writebuf,
> +		     ULONGEST offset, LONGEST len)
> +{
> +  static struct buffer cache;
> +  struct thread_info *thread;
> +  ptid_t ptid;
> +
> +  if (!the_target->read_btrace || writebuf)
> +    return -2;
> +
> +  if (!target_running () || !annex)
> +    return -1;
> +
> +  ptid = read_ptid ((char *) annex, NULL);
> +  thread = find_thread_ptid (ptid);
> +  if (!thread)
> +    {
> +      fprintf (stderr, "No such thread: %s\n", annex);
> +      return -1;
> +    }
> +  else if (!thread->btrace)
> +    {
> +      fprintf (stderr, "Btrace not enabled for %s\n", annex);
> +      return -1;
> +    }
> +
> +  if (!offset)
> +    {
> +      buffer_free (&cache);
> +
> +      target_read_btrace (thread->btrace, &cache);
> +    }
> +  else if (offset >= cache.used_size)
> +    {
> +      buffer_free (&cache);
> +      return 0;

I think this should be -1.

> +    }
> +
> +  if (len > (cache.used_size - offset))
> +    len = cache.used_size - offset;

Unnecessary parens.

> +
> +  memcpy (readbuf, cache.buffer + offset, len);
> +
> +  return len;
> +}
> +
>  static const struct qxfer qxfer_packets[] =
>    {
>      { "auxv", handle_qxfer_auxv },
> @@ -1308,6 +1436,7 @@ static const struct qxfer qxfer_packets[] =
>      { "statictrace", handle_qxfer_statictrace },
>      { "threads", handle_qxfer_threads },
>      { "traceframe-info", handle_qxfer_traceframe_info },
> +    { "btrace", handle_qxfer_btrace },
>    };

Please keep the list sorted.

-- 
Pedro Alves