This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH 04/11] btrace: Use function segment index in call iterator.
> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Friday, February 17, 2017 2:26 PM
> To: gdb-patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH 04/11] btrace: Use function segment index in call iterator.
Hello Tim,
>
> 2017-02-17 Tim Wiederhake <tim.wiederhake@intel.com>
>
> gdb/ChangeLog
>
> * btrace.c (btrace_ends_with_single_insn): New function.
> (btrace_call_get, btrace_call_number, btrace_call_begin,
> btrace_call_end, btrace_call_next, btrace_call_prev,
> btrace_find_call_by_number): Use
> index into call segment vector instead of pointer.
Most, if not all, should still fit onto the previous line.
> +static int
> +btrace_ends_with_single_insn (const struct btrace_thread_info* btinfo)
The space ' ' goes on the other side of the '*', i.e. "const struct btrace_thread_info *btinfo".
> +{
> + const btrace_function *bfun;
> +
> + if (VEC_empty (btrace_fun_p, btinfo->functions))
> + return 0;
> +
> + bfun = VEC_last (btrace_fun_p, btinfo->functions);
> + return ftrace_call_num_insn (bfun) == 1;
Shouldn't we check for gaps? They also count as one instruction.
> /* See btrace.h. */
> @@ -2528,28 +2546,12 @@ btrace_call_get (const struct btrace_call_iterator *it)
> unsigned int
> btrace_call_number (const struct btrace_call_iterator *it)
> {
> - const struct btrace_thread_info *btinfo;
> - const struct btrace_function *bfun;
> - unsigned int insns;
> -
> - btinfo = it->btinfo;
> - bfun = it->function;
> - if (bfun != NULL)
> - return bfun->number;
> -
> - /* For the end iterator, i.e. bfun == NULL, we return one more than the
> - number of the last function. */
> - bfun = btinfo->end;
> - insns = VEC_length (btrace_insn_s, bfun->insn);
> + const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
>
> - /* If the function contains only a single instruction (i.e. the current
> - instruction), it will be skipped and its number is already the number
> - we seek. */
> - if (insns == 1)
> - return bfun->number;
> + if ((it->call_index == length) && btrace_ends_with_single_insn (it->btinfo))
> + return length;
Please leave the comment or modify it to better match the new code. This is
otherwise quite hard to read.
> /* See btrace.h. */
> @@ -2558,14 +2560,15 @@ void
> btrace_call_begin (struct btrace_call_iterator *it,
> const struct btrace_thread_info *btinfo)
> {
> - const struct btrace_function *bfun;
> -
> - bfun = btinfo->begin;
> - if (bfun == NULL)
> + if (VEC_empty (btrace_fun_p, btinfo->functions))
> error (_("No trace."));
>
> it->btinfo = btinfo;
> - it->function = bfun;
> + it->call_index = 0;
> +
> + if ((VEC_length (btrace_fun_p, it->btinfo->functions) == 1)
> + && (btrace_ends_with_single_insn (btinfo)))
> + it->call_index = 1;
We didn't have such a check before. Why is this needed? Was this a bug before?
> /* See btrace.h. */
> @@ -2589,35 +2589,29 @@ btrace_call_end (struct btrace_call_iterator *it,
> unsigned int
> btrace_call_next (struct btrace_call_iterator *it, unsigned int stride)
> {
> - const struct btrace_function *bfun;
> - unsigned int steps;
> + const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
>
> - bfun = it->function;
> - steps = 0;
> - while (bfun != NULL)
> + if (it->call_index + stride < length - 1)
> {
> - const struct btrace_function *next;
> - unsigned int insns;
> -
> - next = bfun->flow.next;
> - if (next == NULL)
> - {
> - /* Ignore the last function if it only contains a single
> - (i.e. the current) instruction. */
> - insns = VEC_length (btrace_insn_s, bfun->insn);
> - if (insns == 1)
> - steps -= 1;
> - }
> -
> - if (stride == steps)
> - break;
> -
> - bfun = next;
> - steps += 1;
> + it->call_index += stride;
> + }
No {} if there's a single statement left.
> + else if (it->call_index + stride == length - 1)
> + {
> + if (btrace_ends_with_single_insn (it->btinfo))
> + it->call_index = length;
> + else
> + it->call_index += stride;
> + }
> + else
> + {
> + if (btrace_ends_with_single_insn (it->btinfo))
> + stride = length - it->call_index - 1;
> + else
> + stride = length - it->call_index;
> + it->call_index = length;
> }
Can we merge the two last cases?
Please add a comment explaining that we're ignoring that last function segment
if it only contains the current instruction.
> /* See btrace.h. */
> @@ -2625,48 +2619,30 @@ btrace_call_next (struct btrace_call_iterator *it,
> unsigned int stride)
> unsigned int
> btrace_call_prev (struct btrace_call_iterator *it, unsigned int stride)
> {
> - const struct btrace_thread_info *btinfo;
> - const struct btrace_function *bfun;
> - unsigned int steps;
> + const unsigned int length = VEC_length (btrace_fun_p, it->btinfo->functions);
> + int steps;
> +
> + if (length == 0 || stride == 0)
> + return 0;
>
> - bfun = it->function;
> steps = 0;
>
> - if (bfun == NULL)
> + if (it->call_index >= length)
"> length" is really an internal error, isn't it?
> {
> - unsigned int insns;
> -
> - btinfo = it->btinfo;
> - bfun = btinfo->end;
> - if (bfun == NULL)
> - return 0;
> -
> - /* Ignore the last function if it only contains a single
> - (i.e. the current) instruction. */
> - insns = VEC_length (btrace_insn_s, bfun->insn);
> - if (insns == 1)
> - bfun = bfun->flow.prev;
> -
> - if (bfun == NULL)
> - return 0;
> + if (btrace_ends_with_single_insn (it->btinfo))
> + it->call_index = length - 2;
> + else
> + it->call_index = length - 1;
Please keep the comment or add a new one to explain why we're doing this.
LENGTH - 2 may be negative (or, rather, very big) if the trace only contained
the current instruction.
> - steps += 1;
> + steps = 1;
> + stride -= 1;
> }
Please add a comment to explain this.
> - while (steps < stride)
> - {
> - const struct btrace_function *prev;
> + if (it->call_index < stride)
> + stride = it->call_index;
>
> - prev = bfun->flow.prev;
> - if (prev == NULL)
> - break;
> -
> - bfun = prev;
> - steps += 1;
> - }
> -
> - it->function = bfun;
> - return steps;
> + it->call_index -= stride;
> + return steps + stride;
> }
I think I understand what this is doing but it wasn't obvious. Maybe the
comment on the STEPS and STRIDE adjustment above suffices to explain it.
> /* Branch trace iteration state for "record instruction-history". */
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index f7683f2..ba83be0 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1110,8 +1110,8 @@ record_btrace_call_history (struct target_ops *self, int
> size, int int_flags)
> replay = btinfo->replay;
> if (replay != NULL)
> {
> - begin.function = replay->function;
> begin.btinfo = btinfo;
> + begin.call_index = replay->function->number - 1;
> }
No {} when there's only one statement left.
Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928