Re: [PATCH v3 05/12] btrace: Use function segment index in insn iterator.

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

On 2017-05-09 02:55, Tim Wiederhake wrote:
> Remove FUNCTION pointer in struct btrace_insn_iterator and use an index 
> into
> the list of function segments instead.
> 
> 2017-05-09  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* btrace.c: (btrace_insn_get, btrace_insn_get_error, 
> btrace_insn_number,
> 	btrace_insn_begin, btrace_insn_end, btrace_insn_next, 
> btrace_insn_prev,
> 	btrace_find_insn_by_number): Replace function segment pointer with
> 	index.
> 	(btrace_insn_cmp): Simplify.
> 	* btrace.h: (struct btrace_insn_iterator) Rename index to
> 	insn_index.  Replace function segment pointer with index into function
> 	segment vector.
> 	* record-btrace.c (record_btrace_call_history): Replace function
> 	segment pointer use with index.
> 	(record_btrace_frame_sniffer): Retrieve function call segment through
> 	vector.
> 	(record_btrace_set_replay): Remove defunc't safety check.

Looks good, just a few comments below.

> @@ -2468,12 +2474,21 @@ int
>  btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
>  		 const struct btrace_insn_iterator *rhs)
>  {
> -  unsigned int lnum, rnum;
> +  gdb_assert (lhs->btinfo == rhs->btinfo);
> 
> -  lnum = btrace_insn_number (lhs);
> -  rnum = btrace_insn_number (rhs);
> +  if (lhs->call_index > rhs->call_index)
> +    return 1;
> +
> +  if (lhs->call_index < rhs->call_index)
> +    return -1;
> +
> +  if (lhs->insn_index > rhs->insn_index)
> +    return 1;
> +
> +  if (lhs->insn_index < rhs->insn_index)
> +    return -1;
> 
> -  return (int) (lnum - rnum);
> +  return 0;
>  }

I the number of comparisons could be reduced by doing:

   int
   btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
                    const struct btrace_insn_iterator *rhs)
   {
     gdb_assert (lhs->btinfo == rhs->btinfo);

     if (lhs->call_index != rhs->call_index)
       return lhs->call_index - rhs->call_index;

     return lhs->insn_index - rhs->insn_index;
   }

> 
>  /* See btrace.h.  */
> @@ -2522,8 +2537,8 @@ btrace_find_insn_by_number (struct
> btrace_insn_iterator *it,
>      }
> 
>    it->btinfo = btinfo;
> -  it->function = bfun;
> -  it->index = number - bfun->insn_offset;
> +  it->call_index = bfun->number - 1;
> +  it->insn_index = number - bfun->insn_offset;
>    return 1;
>  }
> 
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index ef2c781..ca79667 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -195,12 +195,11 @@ struct btrace_insn_iterator
>    /* The branch trace information for this thread.  Will never be 
> NULL.  */
>    const struct btrace_thread_info *btinfo;
> 
> -  /* The branch trace function segment containing the instruction.
> -     Will never be NULL.  */
> -  const struct btrace_function *function;

Just an idea, you could factor out those

   it->btinfo->functions[it->call_index]

in a small helper method in btrace_insn_iterator:

btrace_function *function ()
{
   return this->btinfo->functions[this->call_index];
}

> @@ -2691,7 +2691,7 @@ record_btrace_set_replay (struct thread_info *tp,
> 
>    btinfo = &tp->btrace;
> 
> -  if (it == NULL || it->function == NULL)
> +  if (it == NULL)

Not sure, but wouldn't the equivalent check be that call_index < 
btinfo->functions.size () ?

Thanks,

Simon