Re: [patch v9 07/23] gdbserver, btrace: add generic btrace support

Jan Kratochvil
2013-03-06T13:22:23Z

On Mon, 04 Mar 2013 18:05:54 +0100, Markus Metzger wrote:
> Add support to gdbserver to understand branch trace related packages.
>
> 2013-03-04 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.
[...]
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
[...]
> +/* Handle qXfer:btrace:read. */
> +
> +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;
> +
> + if (the_target->read_btrace == NULL || writebuf != NULL)
> + return -2;
> +
> + if (!target_running () || annex[0] != '\0')
> + return -1;
> +
> + if (ptid_equal (general_thread, null_ptid)
> + || ptid_equal (general_thread, minus_one_ptid))
> + {
> + strcpy (own_buf, "E.Must select a single thread.");

While Pedro suggested the "E.XXX" error codes in xfer function they do not
work as returning -1 will overwrite it by "E01" and any other return code also
cannot be used.

Maybe just add a new return value -3, describe it in the struct qxfer->qxfer
comment and handle it in handle_qxfer <"read"> and <"write">.
else if (n < 0)
write_enn (own_buf);
->
else if (n == -3)
{
/* Keep the error code in OWN_BUF. */
}
else if (n < 0)
write_enn (own_buf);

And BTW the error handling in remote.c is not correct:
if (packet_ok (rs->buf, packet) == PACKET_ERROR)
error (_("Could not enable branch tracing for %s: %s"),
target_pid_to_str (ptid), rs->buf);

Sending packet: $Qbtrace:bts#45...Packet received: E.Target does not support branch tracing.
->
Could not enable branch tracing for Thread 18498: E.Target does not support branch tracing.

There needs to be also some:
if (rs->buf[1] == '.')
and use
rs->buf + 2;
(rs->buf is 0-terminated).

> + return -1;
> + }
> +
> + thread = find_thread_ptid (general_thread);
> + if (thread == NULL)
> + {
> + strcpy (own_buf, "E.No such thread.");
> + return -1;
> + }
> +
> + if (thread->btrace == NULL)
> + {
> + strcpy (own_buf, "E.Btrace not enabled.");
> + return -1;
> + }
> +
> + if (offset == 0)
> + {
> + buffer_free (&cache);
> +
> + target_read_btrace (thread->btrace, &cache);
> + }
> + else if (offset > cache.used_size)
> + {
> + buffer_free (&cache);
> + return -1;
> + }
> +
> + if (len > cache.used_size - offset)
> + len = cache.used_size - offset;
> +
> + memcpy (readbuf, cache.buffer + offset, len);
> +
> + return len;
> +}
> +

Otherwise OK for the diff against Pedro's review.

Thanks,
Jan