Re: [PATCH v3 1/8] btrace: Count gaps as one instruction explicitly.

Doug Evans
2016-11-30T23:34:16Z

Tim Wiederhake writes:
> This gives all instructions, including gaps, a unique number. Add a
function
> to retrieve the error code if a btrace instruction iterator points to an
> invalid instruction.
>
> 2016-11-21 Tim Wiederhake <tim.wiederhake@intel.com>
>
> gdb/ChangeLog:
>
> * btrace.c (ftrace_call_num_insn, btrace_insn_get_error): New function.
> (ftrace_new_function, btrace_insn_number, btrace_insn_cmp,
> btrace_find_insn_by_number): Remove special case for gaps.
> * btrace.h (btrace_insn_get_error): New export.
> (btrace_insn_number, btrace_find_insn_by_number): Adjust comment.
> * record-btrace.c (btrace_insn_history): Print number for gaps.
> (record_btrace_info, record_btrace_goto): Handle gaps.
>
>
> ---
> gdb/btrace.c | 83
+++++++++++++++++------------------------------------
> gdb/btrace.h | 9 ++++--
> gdb/record-btrace.c | 34 ++++++++--------------
> 3 files changed, 45 insertions(+), 81 deletions(-)
>
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 39d537c..7df1b00 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -141,6 +141,20 @@ ftrace_debug (const struct btrace_function *bfun,
const char *prefix)
> prefix, fun, file, level, ibegin, iend);
> }
>
> +/* Return the number of instructions in a given function call segment.
*/
> +
> +static unsigned int
> +ftrace_call_num_insn (const struct btrace_function* bfun)
> +{
> + if (bfun == NULL)
> + return 0;
> +
> + if (bfun->errcode != 0)
> + return 1;

Hi.

It's not immediately clear why this test is here and why we return 1.
I'm sure there's a reason, and perhaps I just missed the place
in the code that makes it clear why, but if such a comment doesn't already
exist can you add a comment here explaining why this test is here
and why we return 1? [It'll help at least this reader more quickly
understand the why of things.]

> +
> + return VEC_length (btrace_insn_s, bfun->insn);
> +}
> +
> /* Return non-zero if BFUN does not match MFUN and FUN,
> return zero otherwise. */
>
> @@ -216,8 +230,7 @@ ftrace_new_function (struct btrace_function *prev,
> prev->flow.next = bfun;
>
> bfun->number = prev->number + 1;
> - bfun->insn_offset = (prev->insn_offset
> - + VEC_length (btrace_insn_s, prev->insn));
> + bfun->insn_offset = prev->insn_offset + ftrace_call_num_insn
(prev);
> bfun->level = prev->level;
> }
>
> @@ -2150,18 +2163,18 @@ btrace_insn_get (const struct
btrace_insn_iterator *it)
>
> /* See btrace.h. */
>
> -unsigned int
> -btrace_insn_number (const struct btrace_insn_iterator *it)
> +int
> +btrace_insn_get_error (const struct btrace_insn_iterator *it)
> {
> - const struct btrace_function *bfun;
> -
> - bfun = it->function;
> + return it->function->errcode;
> +}
>
> - /* Return zero if the iterator points to a gap in the trace. */
> - if (bfun->errcode != 0)
> - return 0;
> +/* See btrace.h. */
>
> - return bfun->insn_offset + it->index;
> +unsigned int
> +btrace_insn_number (const struct btrace_insn_iterator *it)
> +{
> + return it->function->insn_offset + it->index;
> }

Previously btrace_insn_number watched for bfun->errcode.
Now it does not, which is ok in and of itself.
But I don't see anything in the function comment that says
something like "Do not call this if errcode is non-zero." or some such.
Or maybe in places where this will be called errcode is irrelevant
or must be zero (if the latter then an assert here would be nice)?

>
> /* See btrace.h. */
> @@ -2356,37 +2369,6 @@ btrace_insn_cmp (const struct
btrace_insn_iterator *lhs,
> lnum = btrace_insn_number (lhs);
> rnum = btrace_insn_number (rhs);
>
> - /* A gap has an instruction number of zero. Things are getting more
> - complicated if gaps are involved.
> -
> - We take the instruction number offset from the iterator's function.
> - This is the number of the first instruction after the gap.
> -
> - This is OK as long as both lhs and rhs point to gaps. If only one
of
> - them does, we need to adjust the number based on the other's
regular
> - instruction number. Otherwise, a gap might compare equal to an
> - instruction. */
> -
> - if (lnum == 0 && rnum == 0)
> - {
> - lnum = lhs->function->insn_offset;
> - rnum = rhs->function->insn_offset;
> - }
> - else if (lnum == 0)
> - {
> - lnum = lhs->function->insn_offset;
> -
> - if (lnum == rnum)
> - lnum -= 1;
> - }
> - else if (rnum == 0)
> - {
> - rnum = rhs->function->insn_offset;
> -
> - if (rnum == lnum)
> - rnum -= 1;
> - }
> -
> return (int) (lnum - rnum);
> }
>
> @@ -2398,26 +2380,15 @@ btrace_find_insn_by_number (struct
btrace_insn_iterator *it,
> unsigned int number)
> {
> const struct btrace_function *bfun;
> - unsigned int end, length;
>
> for (bfun = btinfo->end; bfun != NULL; bfun = bfun->flow.prev)
> - {
> - /* Skip gaps. */
> - if (bfun->errcode != 0)
> - continue;
> -
> - if (bfun->insn_offset <= number)
> - break;
> - }
> + if (bfun->insn_offset <= number)
> + break;
>
> if (bfun == NULL)
> return 0;
>
> - length = VEC_length (btrace_insn_s, bfun->insn);
> - gdb_assert (length > 0);
> -
> - end = bfun->insn_offset + length;
> - if (end <= number)
> + if (bfun->insn_offset + ftrace_call_num_insn (bfun) <= number)
> return 0;
>
> it->function = bfun;
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index 202b986..114242d 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -405,9 +405,12 @@ extern void parse_xml_btrace_conf (struct
btrace_config *conf, const char *xml);
> extern const struct btrace_insn *
> btrace_insn_get (const struct btrace_insn_iterator *);
>
> +/* Return the error code for a branch trace instruction iterator.
Returns zero
> + if there is no error, i.e. the instruction is valid. */
> +extern int btrace_insn_get_error (const struct btrace_insn_iterator *);
> +
> /* Return the instruction number for a branch trace iterator.
> - Returns one past the maximum instruction number for the end iterator.
> - Returns zero if the iterator does not point to a valid instruction.
*/
> + Returns one past the maximum instruction number for the end
iterator. */
> extern unsigned int btrace_insn_number (const struct
btrace_insn_iterator *);
>
> /* Initialize a branch trace instruction iterator to point to the
begin/end of
> @@ -433,7 +436,7 @@ extern unsigned int btrace_insn_prev (struct
btrace_insn_iterator *,
> extern int btrace_insn_cmp (const struct btrace_insn_iterator *lhs,
> const struct btrace_insn_iterator *rhs);
>
> -/* Find an instruction in the function branch trace by its number.
> +/* Find an instruction or gap in the function branch trace by its
number.
> If the instruction is found, initialize the branch trace instruction
> iterator to point to this instruction and return non-zero.
> Return zero otherwise. */
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 7c0e39f..6c50bab 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -439,28 +439,12 @@ record_btrace_info (struct target_ops *self)
> calls = btrace_call_number (&call);
>
> btrace_insn_end (&insn, btinfo);
> -
> insns = btrace_insn_number (&insn);
> - if (insns != 0)
> - {
> - /* The last instruction does not really belong to the trace. */
> - insns -= 1;
> - }
> - else
> - {
> - unsigned int steps;
>
> - /* Skip gaps at the end. */
> - do
> - {
> - steps = btrace_insn_prev (&insn, 1);
> - if (steps == 0)
> - break;
> -
> - insns = btrace_insn_number (&insn);
> - }
> - while (insns == 0);
> - }
> + /* If the last instruction is not a gap, it is the current
instruction
> + that is not actually part of the record. */
> + if (btrace_insn_get (&insn) != NULL)
> + insns -= 1;
>
> gaps = btinfo->ngaps;
> }
> @@ -736,7 +720,11 @@ btrace_insn_history (struct ui_out *uiout,
> /* We have trace so we must have a configuration. */
> gdb_assert (conf != NULL);
>
> - btrace_ui_out_decode_error (uiout, it.function->errcode,
> + ui_out_field_fmt (uiout, "insn-number", "%u",
> + btrace_insn_number (&it));
> + ui_out_text (uiout, "\t");
> +
> + btrace_ui_out_decode_error (uiout, btrace_insn_get_error (&it),
> conf->format);
> }
> else
> @@ -2827,7 +2815,9 @@ record_btrace_goto (struct target_ops *self,
ULONGEST insn)
> tp = require_btrace_thread ();
>
> found = btrace_find_insn_by_number (&it, &tp->btrace, number);
> - if (found == 0)
> +
> + /* Check if the instruction could not be found or is a gap. */
> + if (found == 0 || btrace_insn_get (&it) == NULL)
> error (_("No such instruction."));
>
> record_btrace_set_replay (tp, &it);
> --
> 2.7.4
>