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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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