Re: [patch v6 08/12] gdbserver, btrace: add generic btrace support

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 17 Dec 2012 17:01:37 +0100, markus.t.metzger@intel.com wrote:
[...]
> +/* Handle the "Qbtrace" packet.  */
> +
> +static int
> +handle_btrace_general_set (char *own_buf)
> +{
> +  struct thread_info *thread;
> +  const char *err;
> +  char *op;
> +
> +  if (strncmp ("Qbtrace:", own_buf, strlen ("Qbtrace:")) != 0)
> +    return 0;
> +
> +  op = own_buf + strlen ("Qbtrace:");
> +
> +  if (!target_supports_btrace ())
> +    {
> +      strcpy (own_buf, "E.Target does not support branch tracing.");
> +      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.");
> +      return -1;
> +    }
> +
> +  thread = find_thread_ptid (general_thread);
> +  if (thread == NULL)
> +    {
> +      strcpy (own_buf, "E.No such thread.");
> +      return -1;
> +    }
> +
> +  err = NULL;
> +
> +  if (strncmp ("on", op, strlen ("on")) == 0)

Here

> +    err = handle_btrace_enable (thread);
> +  else if (strncmp ("off", op, strlen ("off")) == 0)

and here should be just strcmp, otherwise it falsely matches for example
future keyword "official" or whatever.


> +    err = handle_btrace_disable (thread);
> +  else
> +    err = "E.Bad Qbtrace operation. Use on or off.";
> +
> +  if (err != 0)
> +    strcpy (own_buf, err);
> +  else
> +    write_ok (own_buf);
> +
> +  return 1;
> +}
> +
[...]
> +/* Handle the "qbtrace" packet.  */
> +
> +static int
> +handle_btrace_query (char *own_buf)
> +{
> +  if (strncmp ("qbtrace:", own_buf, strlen ("qbtrace:")) == 0)
> +    {
> +      char *ptid_str = own_buf + strlen ("qbtrace:");
> +      ptid_t ptid = read_ptid (ptid_str, NULL);
> +      struct thread_info *thread = find_thread_ptid (ptid);
> +
> +      if (thread == NULL)
> +          strcpy (own_buf, "E.No such thread.");

Incorrect indentation.

> +      else if (thread->btrace == NULL)
> +          strcpy (own_buf, "E.Btrace not enabled.");

Incorrect indentation.

> +      else
> +        strcpy (own_buf,
> +                (target_btrace_has_changed (thread->btrace) ? "yes" : "no"));

Use tabs, not spaces.  Also below and everywhere in this patch (or patchset?)


[...]
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
[...]
> +#define target_supports_btrace()                                        \
> +  (the_target->supports_btrace ? (*the_target->supports_btrace) () : 0)
> +
> +#define target_enable_btrace(ptid)              \

Use tabs for the \ backslash indentation.

> +  (*the_target->enable_btrace) (ptid)
> +
> +#define target_disable_btrace(tinfo)            \
> +  (*the_target->disable_btrace) (tinfo)
> +
> +#define target_btrace_has_changed(tinfo)        \
> +  (*the_target->btrace_has_changed) (tinfo)
> +
> +#define target_read_btrace(tinfo, buffer)       \
> +  (*the_target->read_btrace) (tinfo, buffer)
> +
>  /* Start non-stop mode, returns 0 on success, -1 on failure.   */
>  
>  int start_non_stop (int nonstop);
> -- 
> 1.7.6.5


Thanks,
Jan