This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v3 12/12] btrace: Store function segments as objects.
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