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

["Wiederhake, Tim" <tim dot wiederhake at intel dot com>]

Hi Simon,

Thanks for reviewing!

> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@polymtl.ca]
> Sent: Wednesday, May 10, 2017 4:20 AM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v3 05/12] btrace: Use function segment index in insn
> iterator.
> 
> On 2017-05-09 02:55, Tim Wiederhake wrote:
> > Remove FUNCTION pointer in struct btrace_insn_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_insn_get, btrace_insn_get_error,
> > btrace_insn_number,
> > 	btrace_insn_begin, btrace_insn_end, btrace_insn_next,
> > btrace_insn_prev,
> > 	btrace_find_insn_by_number): Replace function segment pointer with
> > 	index.
> > 	(btrace_insn_cmp): Simplify.
> > 	* btrace.h: (struct btrace_insn_iterator) Rename index to
> > 	insn_index.  Replace function segment pointer with index into
> function
> > 	segment vector.
> > 	* record-btrace.c (record_btrace_call_history): Replace function
> > 	segment pointer use with index.
> > 	(record_btrace_frame_sniffer): Retrieve function call segment
> through
> > 	vector.
> > 	(record_btrace_set_replay): Remove defunc't safety check.
> 
> Looks good, just a few comments below.
> 
> > @@ -2468,12 +2474,21 @@ int
> >  btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
> >  		 const struct btrace_insn_iterator *rhs)
> >  {
> > -  unsigned int lnum, rnum;
> > +  gdb_assert (lhs->btinfo == rhs->btinfo);
> >
> > -  lnum = btrace_insn_number (lhs);
> > -  rnum = btrace_insn_number (rhs);
> > +  if (lhs->call_index > rhs->call_index)
> > +    return 1;
> > +
> > +  if (lhs->call_index < rhs->call_index)
> > +    return -1;
> > +
> > +  if (lhs->insn_index > rhs->insn_index)
> > +    return 1;
> > +
> > +  if (lhs->insn_index < rhs->insn_index)
> > +    return -1;
> >
> > -  return (int) (lnum - rnum);
> > +  return 0;
> >  }
> 
> I the number of comparisons could be reduced by doing:
> 
>    int
>    btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
>                     const struct btrace_insn_iterator *rhs)
>    {
>      gdb_assert (lhs->btinfo == rhs->btinfo);
> 
>      if (lhs->call_index != rhs->call_index)
>        return lhs->call_index - rhs->call_index;
> 
>      return lhs->insn_index - rhs->insn_index;
>    }

You're right. Changed locally, thanks!

> 
> >
> >  /* See btrace.h.  */
> > @@ -2522,8 +2537,8 @@ btrace_find_insn_by_number (struct
> > btrace_insn_iterator *it,
> >      }
> >
> >    it->btinfo = btinfo;
> > -  it->function = bfun;
> > -  it->index = number - bfun->insn_offset;
> > +  it->call_index = bfun->number - 1;
> > +  it->insn_index = number - bfun->insn_offset;
> >    return 1;
> >  }
> >
> > diff --git a/gdb/btrace.h b/gdb/btrace.h
> > index ef2c781..ca79667 100644
> > --- a/gdb/btrace.h
> > +++ b/gdb/btrace.h
> > @@ -195,12 +195,11 @@ struct btrace_insn_iterator
> >    /* The branch trace information for this thread.  Will never be
> > NULL.  */
> >    const struct btrace_thread_info *btinfo;
> >
> > -  /* The branch trace function segment containing the instruction.
> > -     Will never be NULL.  */
> > -  const struct btrace_function *function;
> 
> Just an idea, you could factor out those
> 
>    it->btinfo->functions[it->call_index]
> 
> in a small helper method in btrace_insn_iterator:
> 
> btrace_function *function ()
> {
>    return this->btinfo->functions[this->call_index];
> }

I'd like to postpone all further C++-ifications to a separate patch set.

> > @@ -2691,7 +2691,7 @@ record_btrace_set_replay (struct thread_info *tp,
> >
> >    btinfo = &tp->btrace;
> >
> > -  if (it == NULL || it->function == NULL)
> > +  if (it == NULL)
> 
> Not sure, but wouldn't the equivalent check be that call_index <
> btinfo->functions.size () ?

The comment on btrace_insn_iterator::function used to read: "The branch trace function segment containing the instruction. Will never be NULL". The check for it->function == NULL was a defensive measure but not necessary in terms of program behavior.

> 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