RE: [PATCH 04/11] btrace: Use function segment index in call iterator.

["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 04/11] btrace: Use function segment index in call iterator.

Hello Tim,

> 
> 2017-02-17  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/ChangeLog
> 
> 	* btrace.c (btrace_ends_with_single_insn): New function.
> 	(btrace_call_get, btrace_call_number, btrace_call_begin,
> 	btrace_call_end, btrace_call_next, btrace_call_prev,
> 	btrace_find_call_by_number): Use
> 	index into call segment vector instead of pointer.

Most, if not all, should still fit onto the previous line.


> +static int
> +btrace_ends_with_single_insn (const struct btrace_thread_info* btinfo)

The space ' ' goes on the other side of the '*', i.e. "const struct btrace_thread_info *btinfo".


> +{
> +  const btrace_function *bfun;
> +
> +  if (VEC_empty (btrace_fun_p, btinfo->functions))
> +    return 0;
> +
> +  bfun = VEC_last (btrace_fun_p, btinfo->functions);
> +  return ftrace_call_num_insn (bfun) == 1;

Shouldn't we check for gaps?  They also count as one instruction.

 
>  /* See btrace.h.  */
> @@ -2528,28 +2546,12 @@ btrace_call_get (const struct btrace_call_iterator *it)
>  unsigned int
>  btrace_call_number (const struct btrace_call_iterator *it)
>  {
> -  const struct btrace_thread_info *btinfo;
> -  const struct btrace_function *bfun;
> -  unsigned int insns;
> -
> -  btinfo = it->btinfo;
> -  bfun = it->function;
> -  if (bfun != NULL)
> -    return bfun->number;
> -
> -  /* For the end iterator, i.e. bfun == NULL, we return one more than the
> -     number of the last function.  */
> -  bfun = btinfo->end;
> -  insns = VEC_length (btrace_insn_s, bfun->insn);
> +  const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
> 
> -  /* If the function contains only a single instruction (i.e. the current
> -     instruction), it will be skipped and its number is already the number
> -     we seek.  */
> -  if (insns == 1)
> -    return bfun->number;
> +  if ((it->call_index == length) && btrace_ends_with_single_insn (it->btinfo))
> +    return length;

Please leave the comment or modify it to better match the new code.  This is
otherwise quite hard to read.


>  /* See btrace.h.  */
> @@ -2558,14 +2560,15 @@ void
>  btrace_call_begin (struct btrace_call_iterator *it,
>  		   const struct btrace_thread_info *btinfo)
>  {
> -  const struct btrace_function *bfun;
> -
> -  bfun = btinfo->begin;
> -  if (bfun == NULL)
> +  if (VEC_empty (btrace_fun_p, btinfo->functions))
>      error (_("No trace."));
> 
>    it->btinfo = btinfo;
> -  it->function = bfun;
> +  it->call_index = 0;
> +
> +  if ((VEC_length (btrace_fun_p, it->btinfo->functions) == 1)
> +      && (btrace_ends_with_single_insn (btinfo)))
> +    it->call_index = 1;

We didn't have such a check before.  Why is this needed?  Was this a bug before?


>  /* See btrace.h.  */
> @@ -2589,35 +2589,29 @@ btrace_call_end (struct btrace_call_iterator *it,
>  unsigned int
>  btrace_call_next (struct btrace_call_iterator *it, unsigned int stride)
>  {
> -  const struct btrace_function *bfun;
> -  unsigned int steps;
> +  const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
> 
> -  bfun = it->function;
> -  steps = 0;
> -  while (bfun != NULL)
> +  if (it->call_index + stride < length - 1)
>      {
> -      const struct btrace_function *next;
> -      unsigned int insns;
> -
> -      next = bfun->flow.next;
> -      if (next == NULL)
> -	{
> -	  /* Ignore the last function if it only contains a single
> -	     (i.e. the current) instruction.  */
> -	  insns = VEC_length (btrace_insn_s, bfun->insn);
> -	  if (insns == 1)
> -	    steps -= 1;
> -	}
> -
> -      if (stride == steps)
> -	break;
> -
> -      bfun = next;
> -      steps += 1;
> +      it->call_index += stride;
> +    }

No {} if there's a single statement left.


> +  else if (it->call_index + stride == length - 1)
> +    {
> +      if (btrace_ends_with_single_insn (it->btinfo))
> +	it->call_index = length;
> +      else
> +	it->call_index += stride;
> +    }
> +  else
> +    {
> +      if (btrace_ends_with_single_insn (it->btinfo))
> +	stride = length - it->call_index - 1;
> +      else
> +	stride = length - it->call_index;
> +      it->call_index = length;
>      }

Can we merge the two last cases?

Please add a comment explaining that we're ignoring that last function segment
if it only contains the current instruction.


>  /* See btrace.h.  */
> @@ -2625,48 +2619,30 @@ btrace_call_next (struct btrace_call_iterator *it,
> unsigned int stride)
>  unsigned int
>  btrace_call_prev (struct btrace_call_iterator *it, unsigned int stride)
>  {
> -  const struct btrace_thread_info *btinfo;
> -  const struct btrace_function *bfun;
> -  unsigned int steps;
> +  const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
> +  int steps;
> +
> +  if (length == 0 || stride == 0)
> +    return 0;
> 
> -  bfun = it->function;
>    steps = 0;
> 
> -  if (bfun == NULL)
> +  if (it->call_index >= length)

"> length" is really an internal error, isn't it?


>      {
> -      unsigned int insns;
> -
> -      btinfo = it->btinfo;
> -      bfun = btinfo->end;
> -      if (bfun == NULL)
> -	return 0;
> -
> -      /* Ignore the last function if it only contains a single
> -	 (i.e. the current) instruction.  */
> -      insns = VEC_length (btrace_insn_s, bfun->insn);
> -      if (insns == 1)
> -	bfun = bfun->flow.prev;
> -
> -      if (bfun == NULL)
> -	return 0;
> +      if (btrace_ends_with_single_insn (it->btinfo))
> +	it->call_index = length - 2;
> +      else
> +	it->call_index = length - 1;

Please keep the comment or add a new one to explain why we're doing this.

LENGTH - 2 may be negative (or, rather, very big) if the trace only contained
the current instruction.


> -      steps += 1;
> +      steps = 1;
> +      stride -= 1;
>      }

Please add a comment to explain this.


> -  while (steps < stride)
> -    {
> -      const struct btrace_function *prev;
> +  if (it->call_index < stride)
> +    stride = it->call_index;
> 
> -      prev = bfun->flow.prev;
> -      if (prev == NULL)
> -	break;
> -
> -      bfun = prev;
> -      steps += 1;
> -    }
> -
> -  it->function = bfun;
> -  return steps;
> +  it->call_index -= stride;
> +  return steps + stride;
>  }

I think I understand what this is doing but it wasn't obvious.  Maybe the
comment on the STEPS and STRIDE adjustment above suffices to explain it.


>  /* Branch trace iteration state for "record instruction-history".  */
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index f7683f2..ba83be0 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1110,8 +1110,8 @@ record_btrace_call_history (struct target_ops *self, int
> size, int int_flags)
>        replay = btinfo->replay;
>        if (replay != NULL)
>  	{
> -	  begin.function = replay->function;
>  	  begin.btinfo = btinfo;
> +	  begin.call_index = replay->function->number - 1;
>  	}

No {} when there's only one statement left.

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