RE: [PATCH 06/11] [SQUASH] btrace: Save function calls in a vector.

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Friday, February 17, 2017 2:26 PM
> To: gdb-patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH 06/11] [SQUASH] btrace: Save function calls in a vector.

Hello Tim,

> This patch stands alone for easier review and is meant to be squashed together
> for committing.  ChangeLog will be added to the squashed commit.

Thanks.


>  /* Allocate and initialize a new branch trace function segment at the end of
>     the trace.  MFUN and FUN are the symbol information we have for this
> -   function.  */
> +   function.  This invalidates all struct btrace_function pointers held.  */

I understand that this is necessary but if we could get rid of those pointers before,
it wouldn't be that bad.  See my comments on patch 2.

Not sure if it really works but if it does, we could avoid the temporary resizing
of the functions vector and leave those patches separate.


>  static struct btrace_function *
>  ftrace_new_function (struct btrace_thread_info *btinfo,
>  		     struct minimal_symbol *mfun,
>  		     struct symbol *fun)
>  {
> -  struct btrace_function *prev, *bfun;
> +  struct btrace_function *prev = NULL, *bfun;
> 
> -  prev = btinfo->end;
> -  bfun = XCNEW (struct btrace_function);
> +  if (!VEC_empty (btrace_fun_s, btinfo->functions))
> +    prev = VEC_last (btrace_fun_s, btinfo->functions);
> +
> +  bfun = VEC_safe_push (btrace_fun_s, btinfo->functions, NULL);
> +  memset (bfun, 0, sizeof (*bfun));

This assumes that we're not reallocating the vector.  That's why we allocate a huge
vector initially.

We could also just take VEC_length, then add the new element, and refer to its
predecessor via the stored length - or use VEC_length - 1 after adding the new
element.

We'd still break other pointers that someone stores across that call, but at least
this function would be OK.  We can probably take care of (most) other pointers,
as well.


>    /* We're done if we failed to enable tracing.  */
> @@ -1550,6 +1557,7 @@ btrace_disable (struct thread_info *tp)
>    btp->target = NULL;
> 
>    btrace_clear (tp);
> +  VEC_free (btrace_fun_s, btp->functions);

Shouldn't we do this in btrace_clear?

  
> +  VEC_truncate (btrace_fun_s, btinfo->functions, 0);

Should this be VEC_free?
 
  
>  /* A branch trace instruction iterator.  */
>  struct btrace_insn_iterator
> @@ -343,9 +344,10 @@ struct btrace_thread_info
>    struct btrace_function *begin;
>    struct btrace_function *end;
> 
> -  /* Vector of pointer to decoded function segments.  These are in execution
> -     order with the first element == BEGIN and the last element == END.  */
> -  VEC (btrace_fun_p) *functions;
> +  /* Vector of decoded function call segments in execution flow order.  Note
> +     that the numbering for btrace function segments starts with 1, so function
> +     call segment i will be at index (i - 1).  */
> +  VEC (btrace_fun_s) *functions;

"Vector of function segments in ...".

Thanks,
Markus.

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