[PATCH v3 05/12] btrace: Use function segment index in insn iterator.
Wiederhake, Tim
tim.wiederhake@intel.com
Wed May 10 11:46:00 GMT 2017
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
More information about the Gdb-patches
mailing list