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

[Doug Evans <dje@google.com>]

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;
 > +}