[PATCH] Updated, fix reverse stepping multiple contiguous PC ranges
Carl Love
cel@us.ibm.com
Tue Mar 8 22:01:54 GMT 2022
On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
<snip>
>
>
> Thanks for looking at this. Since I don't test on aarch64 often, I am
> not sure if I see regressions or racy testcases, but it does fix the
> issue you mentioned, and there doesn't seem to be regressions on
> x86_64 hardware. I have a few nits, but the main feedback is: could
> you add a testcase for this, using the dwarf assembler and manually
> creating contiguous PC ranges, so we can confirm that this is not
> regressed in the future on any hardware?
>
> Also, I can't approve a patch, but with the testcase this patch is
> mostly ok by me
>
I have not writen anything in dwarf assembler in the past. Off the top
of my head, don't know how to create such a test case. It does seem
that there are the two testcases gdb.reverse/solib-precsave.exp and
gdb.reverse/solib-reverse.exp which the patch fixes. I guess the dwarf
assembly test would be similar to the C level code.
Do you have an example of how to write dwarf assembly or a pointer to a
tutorial on writting dwarf?
I will work on the two comments you had on the patch. Thanks for your
time reviewing the patch.
Carl
> > -----------------------------------------------------------
> > From: Luis Machado <luis.machado@linaro.org>
> > Date: Mon, 21 Feb 2022 23:11:23 +0000
> > Subject: [PATCH] Fix reverse stepping multiple contiguous PC ranges
> >
> > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04, I
> > noticed some
> > failures in gdb.reverse/solib-precsave.exp and gdb.reverse/solib-
> > reverse.exp.
> >
> > The failure happens around the following code:
> >
> > 38 b[1] = shr2(17); /* middle part two */
> > 40 b[0] = 6; b[1] = 9; /* generic statement, end part two
> > */
> > 42 shr1 ("message 1\n"); /* shr1 one */
> >
> > Normal execution:
> >
> > - step from line 1 will land on line 2.
> > - step from line 2 will land on line 3.
> >
> > Reverse execution:
> >
> > - step from line 3 will land on line 2.
> > - step from line 2 will land on line 2.
> > - step from line 2 will land on line 1.
> >
> > The problem here is that line 40 contains two contiguous but
> > distinct
> > PC ranges, like so:
> >
> > Line 40 - [0x7ec ~ 0x7f4]
> > Line 40 - [0x7f4 ~ 0x7fc]
> >
> > When stepping forward from line 2, we skip both of these ranges and
> > land on
> > line 42. When stepping backward from line 3, we stop at the start
> > PC of the
> > second (or first, going backwards) range of line 40.
> >
> > This happens because we have this check in
> > infrun.c:process_event_stop_test:
> >
> > /* When stepping backward, stop at beginning of line range
> > (unless it's the function entry point, in which case
> > keep going back to the call point). */
> > CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
> > if (stop_pc == ecs->event_thread->control.step_range_start
> > && stop_pc != ecs->stop_func_start
> > && execution_direction == EXEC_REVERSE)
> > end_stepping_range (ecs);
> > else
> > keep_going (ecs);
> >
> > Since we've reached ecs->event_thread->control.step_range_start, we
> > stop
> > stepping backwards.
> >
> > The right thing to do is to look for adjacent PC ranges for the
> > same line,
> > until we notice a line change. Then we take that as the start PC of
> > the
> > range.
> >
> > Another solution I thought about is to merge the contiguous ranges
> > when
> > we are reading the line tables. Though I'm not sure if we really
> > want to process
> > that data as opposed to keeping it as the compiler created, and
> > then working
> > around that.
> >
> > In any case, the following patch addresses this problem.
> >
> > I'm not particularly happy with how we go back in the ranges (using
> > "pc - 1").
> > Feedback would be welcome.
> >
> > Validated on aarch64-linux/Ubuntu 20.04/18.04.
> >
> > Ubuntu 18.04 doesn't actually run into these failures because the
> > compiler
> > doesn't generate distinct PC ranges for the same line.
> >
> > gdb/ChangeLog:
> >
> > YYYY-MM-DD Luis Machado
> >
> > * infrun.c (process_event_stop_test): Handle backward
> > stepping
> > across multiple ranges for the same line.
> > * symtab.c (find_line_range_start): New function.
> > * symtab.h (find_line_range_start): New prototype.
> >
> >
> > Co-authored-by: Carl Love <cel@us.ibm.com>
> > ---
> > gdb/infrun.c | 24 +++++++++++++++++++++++-
> > gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++
> > gdb/symtab.h | 16 ++++++++++++++++
> > 3 files changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 376a541faf6..997042d3e45 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -6782,11 +6782,33 @@ if (ecs->event_thread-
> > >control.proceed_to_finish
> > have software watchpoints). */
> > ecs->event_thread->control.may_range_step = 1;
> >
> > + /* When we are stepping inside a particular line range, in
> > reverse,
> > + and we are sitting at the first address of that range, we need
> > to
> > + check if this address also shows up in another line range as
> > the
> > + end address.
> > +
> > + If so, we need to check what line such a step range points to.
> > + If it points to the same line as the current step range, that
> > + means we need to keep going in order to reach the first
> > address
> > + of the line range. We repeat this until we eventually get to
> > the
> > + first address of a particular line we're stepping through. */
> > + CORE_ADDR range_start = ecs->event_thread-
> > >control.step_range_start;
> > + if (execution_direction == EXEC_REVERSE)
> > + {
> > + gdb::optional<CORE_ADDR> real_range_start
> > + // = find_line_range_start (ecs->event_thread-
> > >suspend.stop_pc);
> > + = find_line_range_start (ecs->event_thread-
> > >stop_pc()); //carll fi> +
> > +
> > + if (real_range_start.has_value ())
> > + range_start = *real_range_start;
> > + }
> > +
> > /* When stepping backward, stop at beginning of line range
> > (unless it's the function entry point, in which case
> > keep going back to the call point). */
> > CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > - if (stop_pc == ecs->event_thread->control.step_range_start
> > + if (stop_pc == range_start
> > && stop_pc != ecs->stop_func_start
>
> I think this could be moved to the line above.
>
> > && execution_direction == EXEC_REVERSE)
> > end_stepping_range (ecs);
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 1a39372aad0..c40739919d1 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3425,6 +3425,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent)
> > return sal;
> > }
> >
> > +/* See symtah.h. */
> > +
> > +gdb::optional<CORE_ADDR>
> > +find_line_range_start (CORE_ADDR pc)
> > +{
> > + struct symtab_and_line current_sal = find_pc_line (pc, 0);
> > +
> > + if (current_sal.line == 0)
> > + return {};
> > +
> > + struct symtab_and_line prev_sal = find_pc_line (current_sal.pc -
> > 1, 0);
> > +
> > + /* If the previous entry is for a different line, that means we
> > are already
> > + at the entry with the start PC for this line. */
> > + if (prev_sal.line != current_sal.line)
> > + return current_sal.pc;
> > +
> > + /* Otherwise, keep looking for entries for the same line but
> > with
> > + smaller PC's. */
> > + bool done = false;
> > + CORE_ADDR prev_pc;
> > + while (!done)
> > + {
> > + prev_pc = prev_sal.pc;
> > +
> > + prev_sal = find_pc_line (prev_pc - 1, 0);
> > +
> > + /* Did we notice a line change? If so, we are done with the
> > search. */
> > + if (prev_sal.line != current_sal.line)
> > + done = true;
>
> Shouldn't prev_sal.line also be checked here and return an empty
> optional? I am not sure when that happens, so please enlighten me if
> there is no need to check.
>
> > + }
> > +
> > + return prev_pc;
> > +}
> > +
> > /* See symtab.h. */
> >
> > struct symtab *
> > diff --git a/gdb/symtab.h b/gdb/symtab.h
> > index d12eee6e9d8..4d893a8a3b8 100644
> > --- a/gdb/symtab.h
> > +++ b/gdb/symtab.h
> > @@ -2149,6 +2149,22 @@ extern struct symtab_and_line find_pc_line
> > (CORE_ADDR, int);
> > extern struct symtab_and_line find_pc_sect_line (CORE_ADDR,
> > struct obj_section *,
> > int);
> >
> > +/* Given PC, and assuming it is part of a range of addresses that
> > is part of a
> > + line, go back through the linetable and find the starting PC of
> > that
> > + line.
> > +
> > + For example, suppose we have 3 PC ranges for line X:
> > +
> > + Line X - [0x0 - 0x8]
> > + Line X - [0x8 - 0x10]
> > + Line X - [0x10 - 0x18]
> > +
> > + If we call the function with PC == 0x14, we want to return 0x0,
> > as that is
> > + the starting PC of line X, and the ranges are contiguous.
> > +*/
> > +
> > +extern gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR
> > pc);
> > +
> > /* Wrapper around find_pc_line to just return the symtab. */
> >
> > extern struct symtab *find_pc_line_symtab (CORE_ADDR);
>
>
More information about the Gdb-patches
mailing list