Re: [patch v6 19/21] btrace, gdbserver: read branch trace incrementally

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 20 Sep 2013 13:30:37 +0200, Markus Metzger wrote:
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
[...]
> @@ -717,27 +717,149 @@ btrace_teardown (struct thread_info *tp)
>    btrace_clear (tp);
>  }
>  
> +/* Adjust the block trace in order to stitch old and new trace together.
> +   Return 0 on success, -1 otherwise.  */

Document parameters (what is input, what is output).


> +
> +static int
> +btrace_stitch_trace (VEC (btrace_block_s) **btrace,
> +		     const struct btrace_thread_info *btinfo)
> +{
> +  struct btrace_function *end;
> +  struct btrace_insn *insn;
> +  btrace_block_s *block;
> +
> +  /* If we don't have trace, there's nothing to do.  */
> +  if (VEC_empty (btrace_block_s, *btrace))
> +    return 0;
> +
> +  end = btinfo->end;
> +  gdb_assert (end != NULL);
> +
> +  block = VEC_last (btrace_block_s, *btrace);
> +  insn = VEC_last (btrace_insn_s, end->insn);

At least call block and insn somehow specific from where they come from.
Maybe btrace_block and btinfo_end.  Also end should be called btinfo_end (if
the extra variable still makes sense in such case).

I would even call it new_btrace and old_btinfo with variables old_end etc.


> +
> +  /* If the current PC at the end of the block is the same as in our current
> +     trace, there are two explanations:
> +       1. we executed the instruction and some branch brought us back.
> +       2. we have not made any progress.
> +     In the first case, the delta trace vector should contain at least two
> +     entries.
> +     In the second case, the delta trace vector should contain exactly one
> +     entry for the partial block containing the current PC.  Remove it.  */
> +  if (block->end == insn->pc && VEC_length (btrace_block_s, *btrace) == 1)
> +    {
> +      VEC_pop (btrace_block_s, *btrace);
> +      return 0;
> +    }
> +
> +  DEBUG ("stitching %s to %s", ftrace_print_insn_addr (insn),
> +	 core_addr_to_string_nz (block->end));
> +
> +  /* Do a simple sanity check to make sure we don't accidentally end up
> +     with a bad block.  This should not occur in practice.  */
> +  if (block->end < insn->pc)
> +    {
> +      warning (_("Error while trying to read delta trace.  Falling back to "
> +		 "a full read."));
> +      return -1;
> +    }
> +
> +  /* We adjust the last block to start at the end of our current trace.  */
> +  gdb_assert (block->begin == 0);
> +  block->begin = insn->pc;
> +
> +  /* We simply pop the last insn so we can insert it again as part of
> +     the normal branch trace computation.
> +     Since instruction iterators are based on indices in the instructions
> +     vector, we don't leave any pointers dangling.  */
> +  DEBUG ("pruning insn at %s for stitching", ftrace_print_insn_addr (insn));
> +
> +  VEC_pop (btrace_insn_s, end->insn);
> +
> +  /* The instructions vector may become empty temporarily if this has
> +     been the only instruction in this function segment.
> +     This violates the invariant but will be remedied shortly.  */
> +  return 0;
> +}
[...]
> --- a/gdb/common/linux-btrace.h
> +++ b/gdb/common/linux-btrace.h
> @@ -70,8 +70,12 @@ extern struct btrace_target_info *linux_enable_btrace (ptid_t ptid);
>  /* Disable branch tracing and deallocate @tinfo.  */
>  extern int linux_disable_btrace (struct btrace_target_info *tinfo);
>  
> -/* Read branch trace data.  */
> -extern VEC (btrace_block_s) *linux_read_btrace (struct btrace_target_info *,
> -						enum btrace_read_type);
> +/* Read branch trace data for the thread indicated by BTINFO into BTRACE
> +   using the TYPE read method.
> +   The branch trace will start with the most recent block and continue
> +   towards older blocks.  */

It could also state from to_read_btrace these two things:
	The vector is cleared before any new data is added.
	Returns 0 on success; a negative error code, otherwise.

And in fact the comment can be merged to to_read_btrace and here to have only:
  /* See to_read_btrace.  */


> +extern int linux_read_btrace (VEC (btrace_block_s) **btrace,
> +			      struct btrace_target_info *btinfo,
> +			      enum btrace_read_type type);
>  
>  #endif /* LINUX_BTRACE_H */
[...]
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5722,17 +5722,27 @@ linux_low_enable_btrace (ptid_t ptid)
>    return tinfo;
>  }
>  
> -/* Read branch trace data as btrace xml document.  */
> +/* The read_btrace target method.  */

/* See to_read_btrace target method.  */

(so that ctags and other tools work; I made a "typo" in the review)


>  
> -static void
> +static int
>  linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
>  		       int type)
>  {
[...]
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -4199,18 +4199,19 @@ target_teardown_btrace (struct btrace_target_info *btinfo)
>  
>  /* See target.h.  */
>  
> -VEC (btrace_block_s) *
> -target_read_btrace (struct btrace_target_info *btinfo,
> +int
> +target_read_btrace (VEC (btrace_block_s) **btrace,
> +		    struct btrace_target_info *btinfo,
>  		    enum btrace_read_type type)
>  {
>    struct target_ops *t;
>  
>    for (t = current_target.beneath; t != NULL; t = t->beneath)
>      if (t->to_read_btrace != NULL)
> -      return t->to_read_btrace (btinfo, type);
> +      return t->to_read_btrace (btrace, btinfo, type);
>  
>    tcomplain ();
> -  return NULL;
> +  return ENOSYS;

This should be "return -ENOSYS".


>  }
>  
>  /* See target.h.  */
[...]


Thanks,
Jan