Re: [patch v7 22/24] btrace, gdbserver: read branch trace incrementally

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

On Fri, 29 Nov 2013 15:38:17 +0100, Markus Metzger wrote:
> @@ -717,27 +717,152 @@ btrace_teardown (struct thread_info *tp)
>    btrace_clear (tp);
>  }
>  
> +/* Adjust the block trace in order to stitch old and new trace together.
> +   BTRACE is the new delta trace between the last and the current stop.
> +   BTINFO is the old branch trace until the last stop.
> +   May modifx BTRACE as well as the existing trace in BTINFO.

modify

> +   Return 0 on success, -1 otherwise.  */
> +
> +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;
> +}
[...]


Thanks,
Jan