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

[Simon Marchi <simon.marchi@polymtl.ca>]

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 :).

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

>  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.  */

> +
> +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).

> 
>    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 ();

Thanks,

Simon