RE: [PATCH v3 12/12] btrace: Store function segments as objects.

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

Hi Simon,

Thanks for reviewing!

> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@polymtl.ca]
> Sent: Wednesday, May 10, 2017 7:09 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 12/12] btrace: Store function segments as objects.
> 
> On 2017-05-09 02:55, Tim Wiederhake wrote:
> > 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> >
> > gdb/ChangeLog:
> > 	* btrace.c:
> > 	* btrace.h:
> > 	* record-btrace.c:
> 
> IWBN if you could be more explicit :).

Whoops.

> >
> > ---
> >  gdb/btrace.c        | 94
> > ++++++++++++++++++++++++++---------------------------
> >  gdb/btrace.h        |  7 ++--
> >  gdb/record-btrace.c | 10 +++---
> >  3 files changed, 56 insertions(+), 55 deletions(-)
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index 7b82000..4c7020d 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -155,14 +155,27 @@ ftrace_call_num_insn (const struct
> > btrace_function* bfun)
> >  /* Return the function segment with the given NUMBER or NULL if no
> > such segment
> >     exists.  BTINFO is the branch trace information for the current
> > thread.  */
> >
> > -static struct btrace_function *
> > +const static struct btrace_function *
> 
> It would make more sense to put the "static" first.

Done.

> >  ftrace_find_call_by_number (const struct btrace_thread_info *btinfo,
> >  			    unsigned int number)
> >  {
> >    if (number == 0 || number > btinfo->functions.size ())
> >      return NULL;
> >
> > -  return btinfo->functions[number - 1];
> > +  return &btinfo->functions[number - 1];
> > +}
> > +
> > +/* Return the function segment with the given NUMBER or NULL if no
> > such segment
> > +   exists.  BTINFO is the branch trace information for the current
> > thread.  */
> 
> It took me a surprisingly high amount of seconds to understand that this
> was a const version of the function below.  To avoid reapeating the
> comment and to make it clear it's the same thing, you can replace the
> comment of the const version to something like:
> 
>    /* A const version of the function above.  */

Done.

> > +
> > +static struct btrace_function *
> > +ftrace_find_call_by_number (struct btrace_thread_info *btinfo,
> > +			    unsigned int number)
> > +{
> > +  if (number == 0 || number > btinfo->functions.size ())
> > +    return NULL;
> > +
> > +  return &btinfo->functions[number - 1];
> >  }
> >
> >  /* Return non-zero if BFUN does not match MFUN and FUN,
> > @@ -214,37 +227,33 @@ ftrace_function_switched (const struct
> > btrace_function *bfun,
> >  /* Allocate and initialize a new branch trace function segment at the
> > end of
> >     the trace.
> >     BTINFO is the branch trace information for the current thread.
> > -   MFUN and FUN are the symbol information we have for this function.
> > */
> > +   MFUN and FUN are the symbol information we have for this function.
> > +   This invalidates all struct btrace_function pointer currently held.
> >  */
> >
> >  static struct btrace_function *
> >  ftrace_new_function (struct btrace_thread_info *btinfo,
> >  		     struct minimal_symbol *mfun,
> >  		     struct symbol *fun)
> >  {
> > -  struct btrace_function *bfun;
> > -
> > -  bfun = XCNEW (struct btrace_function);
> > -
> > -  bfun->msym = mfun;
> > -  bfun->sym = fun;
> > +  struct btrace_function bfun {mfun, fun, 0, 0, 0, NULL, 0, 0, 0, 0,
> > 0};
> 
> I think it would be much better to add a simple constructor to
> btrace_function.  For the fields that should simply be zero'ed, you can
> initialize fields directly, like we do in many other places (e.g. class
> inferior).

Having a proper constructor would definitely be beneficial here.
Nevertheless, I would do such a change in a separate patch set.

> >
> >    if (btinfo->functions.empty ())
> >      {
> >        /* Start counting at one.  */
> > -      bfun->number = 1;
> > -      bfun->insn_offset = 1;
> > +      bfun.number = 1;
> > +      bfun.insn_offset = 1;
> >      }
> >    else
> >      {
> > -      struct btrace_function *prev = btinfo->functions.back ();
> > +      struct btrace_function *prev = &btinfo->functions.back ();
> >
> > -      bfun->number = prev->number + 1;
> > -      bfun->insn_offset = prev->insn_offset + ftrace_call_num_insn
> > (prev);
> > -      bfun->level = prev->level;
> > +      bfun.number = prev->number + 1;
> > +      bfun.insn_offset = prev->insn_offset + ftrace_call_num_insn
> > (prev);
> > +      bfun.level = prev->level;
> >      }
> >
> > -  btinfo->functions.push_back (bfun);
> > -  return bfun;
> > +  btinfo->functions.push_back (std::move (bfun));
> > +  return &btinfo->functions.back ();
> 
> I could be mistaken, but I don't think the move is very useful here,
> since all fields of btrace_function are trivial (?).  You could use
> emplace_back instead:
> 
>    btinfo->functions.emplace_back (mfun, fun);
>    btrace_function &bfun = btinfo->functions.back ();
> 
>    ...
> 
>    return &bfun;
> 
> or
> 
>    unsigned int number, insn_offset;
>    unsigned int insn_offset = prev->insn_offset + ftrace_call_num_insn
> (prev);
>    int level = prev->level;
> 
>    if (btinfo->functions.empty ())
>      {
>        /* Start counting at one.  */
>        number = 1;
>        insn_offset = 1;
> 
>        level = 0;
>      }
>    else
>      {
>        struct btrace_function *prev = &btinfo->functions.back ();
> 
>        number = prev->number + 1;
>        insn_offset = prev->insn_offset + ftrace_call_num_insn (prev);
>        level = prev->level;
>      }
> 
>    btinfo->functions.emplace_back (mfun, fun, number, insn_offset,
> level);
> 
>    return &btinfo->functions.back ();

You are right, the std::move is quiete pointless. Sadly, we can't use
emplace_back() yet until btrace_function gets a constructor. Removed
the move.

> 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