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

Jan Kratochvil
2013-12-11T19:58:30Z

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