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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

This title too should say btrace_function.

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.

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.  But because the same bfun 
variable is reused, it will iterate forward from the start 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.

Thanks,

Simon