RE: [PATCH v3 10/12] btrace: Replace struct btrace_thread_info::segment.

["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 6:15 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 10/12] btrace: Replace struct
> btrace_thread_info::segment.
> 
> This title too should say btrace_function.

Fixed.

> On 2017-05-09 02:55, Tim Wiederhake wrote:
> > This used to hold a pair of pointers to the previous and next function
> > segment
> > that belong to this function call.  Replace with a pair of indices into
> > the
> > vector of function segments.
> >
> > 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> >
> > gdb/ChangeLog:
> >
> > 	* btrace.c (ftrace_fixup_caller, ftrace_new_return,
> > ftrace_connect_bfun,
> > 	ftrace_bridge_gap): Replace references to
> btrace_thread_info::segment
> > 	with btrace_thread_info::next_segment and
> > 	btrace_thread_info::prev_segment.
> > 	* btrace.h: Remove struct btrace_func_link.
> > 	(struct btrace_function): Replace pair of function segment pointers
> > 	with pair of indices.
> > 	* python/py-record-btrace.c (btpy_call_prev_sibling,
> > 	btpy_call_next_sibling): Replace references to
> > 	btrace_thread_info::segment with btrace_thread_info::next_segment
> and
> > 	btrace_thread_info::prev_segment.
> > 	* record-btrace.c (record_btrace_frame_this_id): Same.
> >
> > ---
> >  gdb/btrace.c                  | 47
> > ++++++++++++++++++++++++++-----------------
> >  gdb/btrace.h                  | 17 ++++++----------
> >  gdb/python/py-record-btrace.c |  8 ++++----
> >  gdb/record-btrace.c           |  4 ++--
> >  4 files changed, 40 insertions(+), 36 deletions(-)
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index f57bbf9..921cb64 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -271,20 +271,29 @@ ftrace_update_caller (struct btrace_function
> > *bfun,
> >  /* Fix up the caller for all segments of a function.  */
> >
> >  static void
> > -ftrace_fixup_caller (struct btrace_function *bfun,
> > +ftrace_fixup_caller (struct btrace_thread_info *btinfo,
> > +		     struct btrace_function *bfun,
> >  		     struct btrace_function *caller,
> >  		     enum btrace_function_flag flags)
> >  {
> > -  struct btrace_function *prev, *next;
> > +  unsigned int prev, next;
> >
> > +  prev = bfun->prev;
> > +  next = bfun->next;
> >    ftrace_update_caller (bfun, caller, flags);
> >
> >    /* Update all function segments belonging to the same function.  */
> > -  for (prev = bfun->segment.prev; prev != NULL; prev =
> > prev->segment.prev)
> > -    ftrace_update_caller (prev, caller, flags);
> > +  for (; prev != 0; prev = bfun->prev)
> > +    {
> > +      bfun = ftrace_find_call_by_number (btinfo, prev);
> > +      ftrace_update_caller (bfun, caller, flags);
> > +    }
> >
> > -  for (next = bfun->segment.next; next != NULL; next =
> > next->segment.next)
> > -    ftrace_update_caller (next, caller, flags);
> > +  for (; next != 0; next = bfun->next)
> > +    {
> > +      bfun = ftrace_find_call_by_number (btinfo, next);
> > +      ftrace_update_caller (bfun, caller, flags);
> > +    }
> 
> Could you define the loop variables in the for like this?
> 
>    for (unsigned int prev = bfun->prev; prev != 0; prev = bfun->prev)
>      {
>        bfun = ftrace_find_call_by_number (btinfo, prev);
>        ftrace_update_caller (bfun, caller, flags);
>      }
> 
> Unless is it important to capture the value of bfun->prev/next before
> calling ftrace_update_caller?  This way their scope is limited to where
> they are used.

This is what the function would look like:
<snip>
  ftrace_update_caller (bfun, caller, flags);

  for (unsigned int i = bfun->prev; i != 0;)
    {
      struct btrace_function *tmp = ftrace_find_call_by_number (btinfo, i);
      ftrace_update_caller (tmp, caller, flags);
      i = tmp->prev;
    }
  
  for (unsigned int i = bfun->next; i != 0;)
    {
      struct btrace_function *tmp = ftrace_find_call_by_number (btinfo, i);
      ftrace_update_caller (tmp, caller, flags);
      i = tmp->next;
    }
</snip>

IMO, this isn't any better. If I pull out the struct btrace_function* tmp
declaration, it would look like this:
<snip>
  struct btrace_function *tmp;
  
  ftrace_update_caller (bfun, caller, flags);

  for (unsigned int i = bfun->prev; i != 0; i = tmp->prev)
    {
      tmp = ftrace_find_call_by_number (btinfo, i);
      ftrace_update_caller (tmp, caller, flags);
    }
  
  for (unsigned int i = bfun->next; i != 0; i = tmp->next)
    {
      tmp = ftrace_find_call_by_number (btinfo, i);
      ftrace_update_caller (tmp, caller, flags);
    }
</snip>

I'd leave it as is for now and see how this code changes in a
*drum roll* future C++-ification patch series.

> Btw, this is another thing that could be rewritten nicely if
> btrace_function had a backlink to btrace_thread_info, something like:
> 
>    for (btrace_function *it = bfun; it != NULL; it = it->next_segment ())
>      ftrace_update_caller (it, caller, flags);
> 
> Btw #2, I thing this function could be more efficient (or maybe I don't
> understand as well as I think).  If bfun at function entry is in the
> middle of a long list of segments, it will start from there and iterate
> backwards until it hits the first segment.

Correct so far.

> But because the same bfun
> variable is reused, it will iterate forward from the start

We saved PREV and NEXT beforehand and use BFUN as a temporary variable
afterwards. The second "for" loop starts at NEXT, which is one past the
original "middle of the long list of segments".

> until the
> end, whereas it only needed to iterate from the original bfun.  Using a
> temporary loop variable to avoid modifying bfun would correct that.
> > @@ -948,7 +957,7 @@ ftrace_bridge_gap (struct btrace_thread_info
> > *btinfo,
> >  static void
> >  btrace_bridge_gaps (struct thread_info *tp, VEC (bfun_s) **gaps)
> >  {
> > -  struct btrace_thread_info *btinfo;
> > +  struct btrace_thread_info *btinfo = &tp->btrace;
> 
> btinfo is now assigned twice in this function.

Good catch, thank you!

> 
> 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