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

Jan Kratochvil
2013-10-06T19:51:19Z

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