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

Pedro Alves
2012-11-28T20:32:32Z

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