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

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

On 2017-05-11 03:21, Tim Wiederhake wrote:
>  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;
> +  int level;
> +  unsigned int number, insn_offset;
>
>    if (btinfo->functions.empty ())
>      {
>        /* Start counting at one.  */
> -      bfun->number = 1;
> -      bfun->insn_offset = 1;
> +      level = 0;
> +      number = 1;
> +      insn_offset = 1;

The "Start counting at one" is confusing now, because of the assignment 
"level = 0".  The comment chould be improved to say what we start 
counting at one.

> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index 01f1888..1e6a7d1 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -135,6 +135,14 @@ enum btrace_pt_error
>     We do not allow function segments without instructions otherwise.  
> */
>  struct btrace_function
>  {
> +  btrace_function (struct minimal_symbol *msym_, struct symbol *sym_,
> +		   unsigned int number_, unsigned int insn_offset_, int level_)
> +    : msym (msym_), sym (sym_), prev (0), next (0), up (0), insn 
> (NULL),
> +      errcode (0), insn_offset (insn_offset_), number (number_),
> level (level_),
> +      flags (0)
> +  {
> +  }

Please initialize the fields with a default value directly in-place:

struct btrace_function
{
  ...
  struct minimal_symbol *msym = NULL;
  struct symbol *sym = NULL;
  ...

};

It's cleaner and more DRY.


> +
>    /* The full and minimal symbol for the function.  Both may be NULL.  
> */
>    struct minimal_symbol *msym;
>    struct symbol *sym;
> @@ -325,9 +333,10 @@ struct btrace_thread_info
>    /* The raw branch trace data for the below branch trace.  */
>    struct btrace_data data;
>
> -  /* Vector of pointer to decoded function segments.  These are in 
> execution
> -     order with the first element == BEGIN and the last element == 
> END.  */
> -  std::vector<btrace_function *> functions;
> +  /* Vector of decoded function segments in execution flow order.  
> Note that
> +     the numbering for btrace function segments starts with 1, so 
> function
> +     segment i will be at index (i - 1).  */
> +  std::vector<btrace_function> functions;

This change should go in the patch that removes the begin and end 
fields.

Thanks,

Simon