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

Jan Kratochvil
2012-12-17T20:43:26Z

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