Re: [patch v9 01/23] thread, btrace: add generic branch trace support

Doug Evans
2013-03-06T21:11:41Z

Markus Metzger writes:
> Add branch trace information to struct thread_info to hold the branch trace
> information for that thread.
>
> Add functions to enable, disable, clear, and fetch a thread's branch trace.
>
> 2013-03-04 Markus Metzger <markus.t.metzger@intel.com>
>
> * target.h: Include btrace.h.
> (struct target_ops): Add btrace ops.
> * target.c (update_current_target): Initialize btrace ops.
> (target_supports_btrace): New function.
> (target_enable_btrace): New function.
> (target_disable_btrace): New function.
> (target_read_btrace): New function.
> (target_btrace_has_changed): New function.
> * btrace.h: New file.
> * btrace.c: New file.
> * Makefile.in: Add btrace.c.
> * gdbthread.h: Include btrace.h.
> (struct thread_info): Add btrace field.
> * thread.c: Include btrace.h.
> (clear_thread_inferior_resources): Call btrace_disable.
> * infcmd.c: Include btrace.h.
> (detach_command): Call btrace_disconnect.
> * common/btrace-common.h: New file.

Hi. Just some nits.

> +/* Branch trace iteration state for "record function-call-history". */
> +struct btrace_func_iterator
> +{
> + /* The function index range [begin; end[ that has been covered last time.

Typo: end]

> +/* A branch trace block.
> +
> + This represents a block of sequential control-flow. Adjacent blocks will be
> + connected via calls, returns, or jumps. The latter can be direct or
> + indirect, conditional or unconditional. Branches can further be
> + asynchronous, e.g. interrupts. */
> +struct btrace_block
> +{
> + /* The address of the first instruction in the block. */
> + CORE_ADDR begin;
> +
> + /* The address of the last instruction in the block. */
> + CORE_ADDR end;
> +};

Can you elaborate in the docs for "end" what it is?
E.g., on an ISA with only 4 byte instructions, and the block contains
two instructions, is end == begin+4 or begin+7 or begin+8?
I'd guess that it's begin+4, but IWBN if the comment
removed all doubt.

> +/* See target.h. */

Blank line between function comment and definition.
[Here and elsewhere.]

> +int
> +target_supports_btrace (void)
> +{
> + struct target_ops *t;
> +
> + for (t = current_target.beneath; t != NULL; t = t->beneath)
> + if (t->to_supports_btrace != NULL)
> + return t->to_supports_btrace ();
> +
> + return 0;
> +}