Re: [PATCH v3 1/8] btrace: Count gaps as one instruction explicitly.

[Doug Evans <dje@google.com>]

Tim Wiederhake writes:
  > This gives all instructions, including gaps, a unique number.  Add a  
function
  > to retrieve the error code if a btrace instruction iterator points to an
  > invalid instruction.
  >
  > 2016-11-21  Tim Wiederhake  <tim.wiederhake@intel.com>
  >
  > gdb/ChangeLog:
  >
  > 	* btrace.c (ftrace_call_num_insn, btrace_insn_get_error): New function.
  > 	(ftrace_new_function, btrace_insn_number, btrace_insn_cmp,
  > 	btrace_find_insn_by_number): Remove special case for gaps.
  > 	* btrace.h (btrace_insn_get_error): New export.
  > 	(btrace_insn_number, btrace_find_insn_by_number): Adjust comment.
  > 	* record-btrace.c (btrace_insn_history): Print number for gaps.
  > 	(record_btrace_info, record_btrace_goto): Handle gaps.
  >
  >
  > ---
  >  gdb/btrace.c        | 83  
+++++++++++++++++------------------------------------
  >  gdb/btrace.h        |  9 ++++--
  >  gdb/record-btrace.c | 34 ++++++++--------------
  >  3 files changed, 45 insertions(+), 81 deletions(-)
  >
  > diff --git a/gdb/btrace.c b/gdb/btrace.c
  > index 39d537c..7df1b00 100644
  > --- a/gdb/btrace.c
  > +++ b/gdb/btrace.c
  > @@ -141,6 +141,20 @@ ftrace_debug (const struct btrace_function *bfun,  
const char *prefix)
  >  		prefix, fun, file, level, ibegin, iend);
  >  }
  >
  > +/* Return the number of instructions in a given function call segment.   
*/
  > +
  > +static unsigned int
  > +ftrace_call_num_insn (const struct btrace_function* bfun)
  > +{
  > +  if (bfun == NULL)
  > +    return 0;
  > +
  > +  if (bfun->errcode != 0)
  > +    return 1;

Hi.

It's not immediately clear why this test is here and why we return 1.
I'm sure there's a reason, and perhaps I just missed the place
in the code that makes it clear why, but if such a comment doesn't already
exist can you add a comment here explaining why this test is here
and why we return 1? [It'll help at least this reader more quickly
understand the why of things.]

  > +
  > +  return VEC_length (btrace_insn_s, bfun->insn);
  > +}
  > +
  >  /* Return non-zero if BFUN does not match MFUN and FUN,
  >     return zero otherwise.  */
  >
  > @@ -216,8 +230,7 @@ ftrace_new_function (struct btrace_function *prev,
  >        prev->flow.next = bfun;
  >
  >        bfun->number = prev->number + 1;
  > -      bfun->insn_offset = (prev->insn_offset
  > -			   + VEC_length (btrace_insn_s, prev->insn));
  > +      bfun->insn_offset = prev->insn_offset + ftrace_call_num_insn  
(prev);
  >        bfun->level = prev->level;
  >      }
  >
  > @@ -2150,18 +2163,18 @@ btrace_insn_get (const struct  
btrace_insn_iterator *it)
  >
  >  /* See btrace.h.  */
  >
  > -unsigned int
  > -btrace_insn_number (const struct btrace_insn_iterator *it)
  > +int
  > +btrace_insn_get_error (const struct btrace_insn_iterator *it)
  >  {
  > -  const struct btrace_function *bfun;
  > -
  > -  bfun = it->function;
  > +  return it->function->errcode;
  > +}
  >
  > -  /* Return zero if the iterator points to a gap in the trace.  */
  > -  if (bfun->errcode != 0)
  > -    return 0;
  > +/* See btrace.h.  */
  >
  > -  return bfun->insn_offset + it->index;
  > +unsigned int
  > +btrace_insn_number (const struct btrace_insn_iterator *it)
  > +{
  > +  return it->function->insn_offset + it->index;
  >  }

Previously btrace_insn_number watched for bfun->errcode.
Now it does not, which is ok in and of itself.
But I don't see anything in the function comment that says
something like "Do not call this if errcode is non-zero." or some such.
Or maybe in places where this will be called errcode is irrelevant
or must be zero (if the latter then an assert here would be nice)?

  >
  >  /* See btrace.h.  */
  > @@ -2356,37 +2369,6 @@ btrace_insn_cmp (const struct  
btrace_insn_iterator *lhs,
  >    lnum = btrace_insn_number (lhs);
  >    rnum = btrace_insn_number (rhs);
  >
  > -  /* A gap has an instruction number of zero.  Things are getting more
  > -     complicated if gaps are involved.
  > -
  > -     We take the instruction number offset from the iterator's function.
  > -     This is the number of the first instruction after the gap.
  > -
  > -     This is OK as long as both lhs and rhs point to gaps.  If only one  
of
  > -     them does, we need to adjust the number based on the other's  
regular
  > -     instruction number.  Otherwise, a gap might compare equal to an
  > -     instruction.  */
  > -
  > -  if (lnum == 0 && rnum == 0)
  > -    {
  > -      lnum = lhs->function->insn_offset;
  > -      rnum = rhs->function->insn_offset;
  > -    }
  > -  else if (lnum == 0)
  > -    {
  > -      lnum = lhs->function->insn_offset;
  > -
  > -      if (lnum == rnum)
  > -	lnum -= 1;
  > -    }
  > -  else if (rnum == 0)
  > -    {
  > -      rnum = rhs->function->insn_offset;
  > -
  > -      if (rnum == lnum)
  > -	rnum -= 1;
  > -    }
  > -
  >    return (int) (lnum - rnum);
  >  }
  >
  > @@ -2398,26 +2380,15 @@ btrace_find_insn_by_number (struct  
btrace_insn_iterator *it,
  >  			    unsigned int number)
  >  {
  >    const struct btrace_function *bfun;
  > -  unsigned int end, length;
  >
  >    for (bfun = btinfo->end; bfun != NULL; bfun = bfun->flow.prev)
  > -    {
  > -      /* Skip gaps. */
  > -      if (bfun->errcode != 0)
  > -	continue;
  > -
  > -      if (bfun->insn_offset <= number)
  > -	break;
  > -    }
  > +    if (bfun->insn_offset <= number)
  > +      break;
  >
  >    if (bfun == NULL)
  >      return 0;
  >
  > -  length = VEC_length (btrace_insn_s, bfun->insn);
  > -  gdb_assert (length > 0);
  > -
  > -  end = bfun->insn_offset + length;
  > -  if (end <= number)
  > +  if (bfun->insn_offset + ftrace_call_num_insn (bfun) <= number)
  >      return 0;
  >
  >    it->function = bfun;
  > diff --git a/gdb/btrace.h b/gdb/btrace.h
  > index 202b986..114242d 100644
  > --- a/gdb/btrace.h
  > +++ b/gdb/btrace.h
  > @@ -405,9 +405,12 @@ extern void parse_xml_btrace_conf (struct  
btrace_config *conf, const char *xml);
  >  extern const struct btrace_insn *
  >    btrace_insn_get (const struct btrace_insn_iterator *);
  >
  > +/* Return the error code for a branch trace instruction iterator.   
Returns zero
  > +   if there is no error, i.e. the instruction is valid.  */
  > +extern int btrace_insn_get_error (const struct btrace_insn_iterator *);
  > +
  >  /* Return the instruction number for a branch trace iterator.
  > -   Returns one past the maximum instruction number for the end iterator.
  > -   Returns zero if the iterator does not point to a valid instruction.   
*/
  > +   Returns one past the maximum instruction number for the end  
iterator.  */
  >  extern unsigned int btrace_insn_number (const struct  
btrace_insn_iterator *);
  >
  >  /* Initialize a branch trace instruction iterator to point to the  
begin/end of
  > @@ -433,7 +436,7 @@ extern unsigned int btrace_insn_prev (struct  
btrace_insn_iterator *,
  >  extern int btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
  >  			    const struct btrace_insn_iterator *rhs);
  >
  > -/* Find an instruction in the function branch trace by its number.
  > +/* Find an instruction or gap in the function branch trace by its  
number.
  >     If the instruction is found, initialize the branch trace instruction
  >     iterator to point to this instruction and return non-zero.
  >     Return zero otherwise.  */
  > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
  > index 7c0e39f..6c50bab 100644
  > --- a/gdb/record-btrace.c
  > +++ b/gdb/record-btrace.c
  > @@ -439,28 +439,12 @@ record_btrace_info (struct target_ops *self)
  >        calls = btrace_call_number (&call);
  >
  >        btrace_insn_end (&insn, btinfo);
  > -
  >        insns = btrace_insn_number (&insn);
  > -      if (insns != 0)
  > -	{
  > -	  /* The last instruction does not really belong to the trace.  */
  > -	  insns -= 1;
  > -	}
  > -      else
  > -	{
  > -	  unsigned int steps;
  >
  > -	  /* Skip gaps at the end.  */
  > -	  do
  > -	    {
  > -	      steps = btrace_insn_prev (&insn, 1);
  > -	      if (steps == 0)
  > -		break;
  > -
  > -	      insns = btrace_insn_number (&insn);
  > -	    }
  > -	  while (insns == 0);
  > -	}
  > +      /* If the last instruction is not a gap, it is the current  
instruction
  > +	 that is not actually part of the record.  */
  > +      if (btrace_insn_get (&insn) != NULL)
  > +	insns -= 1;
  >
  >        gaps = btinfo->ngaps;
  >      }
  > @@ -736,7 +720,11 @@ btrace_insn_history (struct ui_out *uiout,
  >  	  /* We have trace so we must have a configuration.  */
  >  	  gdb_assert (conf != NULL);
  >
  > -	  btrace_ui_out_decode_error (uiout, it.function->errcode,
  > +	  ui_out_field_fmt (uiout, "insn-number", "%u",
  > +			    btrace_insn_number (&it));
  > +	  ui_out_text (uiout, "\t");
  > +
  > +	  btrace_ui_out_decode_error (uiout, btrace_insn_get_error (&it),
  >  				      conf->format);
  >  	}
  >        else
  > @@ -2827,7 +2815,9 @@ record_btrace_goto (struct target_ops *self,  
ULONGEST insn)
  >    tp = require_btrace_thread ();
  >
  >    found = btrace_find_insn_by_number (&it, &tp->btrace, number);
  > -  if (found == 0)
  > +
  > +  /* Check if the instruction could not be found or is a gap.  */
  > +  if (found == 0 || btrace_insn_get (&it) == NULL)
  >      error (_("No such instruction."));
  >
  >    record_btrace_set_replay (tp, &it);
  > --
  > 2.7.4
  >