RE: [PATCH v3 04/12] btrace: Use function segment index in call iterator.

["Wiederhake, Tim" <tim.wiederhake@intel.com>]

Hi Simon!

Thanks for the review.

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Simon Marchi
> Sent: Tuesday, May 9, 2017 2:51 PM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v3 04/12] btrace: Use function segment index in call
> iterator.
> 
> On 2017-05-09 02:55, Tim Wiederhake wrote:
> > Remove FUNCTION pointer in struct btrace_call_iterator and use an index
> > into
> > the list of function segments instead.
> >
> > 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> >
> > gdb/ChangeLog:
> >
> > 	* btrace.c (btrace_ends_with_single_insn): New function.
> > 	(btrace_call_get, btrace_call_number, btrace_call_begin,
> > 	btrace_call_end, btrace_call_next, btrace_call_prev,
> > 	btrace_find_call_by_number): Use index into call segment vector
> > 	instead of pointer.
> > 	(btrace_call_cmp): Simplify.
> > 	* btrace.h (struct btrace_call_iterator): Replace function call
> > segment
> > 	pointer with index into vector.
> > 	* record-btrace.c (record_btrace_call_history): Use index instead of
> > 	pointer.
> >
> > ---
> >  gdb/btrace.c        | 198
> > ++++++++++++++++++++++------------------------------
> >  gdb/btrace.h        |   5 +-
> >  gdb/record-btrace.c |   2 +-
> >  3 files changed, 87 insertions(+), 118 deletions(-)
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index c2d3730..5615aad 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -2527,12 +2527,33 @@ btrace_find_insn_by_number (struct
> > btrace_insn_iterator *it,
> >    return 1;
> >  }
> >
> > +/* Returns a non-zero value if the recording ends with a function
> > segment that
> > +   contains only a single (i.e. the current) instruction.  */
> > +
> > +static int
> 
> bool?

Right, changed locally.

> Otherwise, it LGTM.  I'm not familiar with the btrace specific
> computations, but it made sense to me.  I trust that you guys know what
> you are doing and have tested it enough :).

I tested the patches on i686 and x86_64 -- maybe I should have mentioned that.

> If I understand correctly, btrace_function::number is 1-based?  If so,
> it would be good to mention it somewhere (if it's not already, but I
> couldn't find it).

See the comment on btrace_thread_info::functions :).

> 
> Thanks,
> 
> Simon

Regards,
Tim
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928