[PING][PATCH] Fix reverse stepping multiple contiguous PC ranges
Carl Love
cel@us.ibm.com
Fri Jul 9 17:54:41 GMT 2021
GDB maintainers:
I have applied this patch from Luis and tested it on PPC64. It fixes
the issues I was investigating for test gdb.reverse/step-reverse.exp.
The patch also fixes the PPC64 errors on the following tests:
gdb.reverse/step-precsave.exp
gdb.reverse/solib-reverse.exp
gdb.reverse/solib-precsave.exp
I have tested this patch on Power 9, Power 10 and X86 64-bit. The
patch does not introduce any additional regressions on PPC64.
Carl Love
On Thu, 2021-06-10 at 08:49 -0300, Luis Machado wrote:
> Ping!
> On 4/7/21 12:23 PM, Luis Machado wrote:
> > Ping!
> >
> > On 3/19/21 3:17 PM, Luis Machado wrote:
> > > Polite ping.
> > >
> > > On 3/12/21 12:36 PM, Luis Machado wrote:
> > > > Polite ping.
> > > >
> > > > On 3/4/21 11:30 AM, Luis Machado wrote:
> > > > > On 2/26/21 3:58 PM, Luis Machado wrote:
> > > > > > On 2/11/21 8:38 AM, Luis Machado wrote:
> > > > > > > On 2/1/21 4:19 PM, Luis Machado wrote:
> > > > > > > > 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 <luis.machado@linaro.org>
> > > > > > > >
> > > > > > > > * 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.
> > > > > > > > ---
> > > > > > > > gdb/infrun.c | 22 +++++++++++++++++++++-
> > > > > > > > gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++
> > > > > > > > gdb/symtab.h | 16 ++++++++++++++++
> > > > > > > > 3 files changed, 72 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > > > > > > > index e070eff33d..5bb268529d 100644
> > > > > > > > --- a/gdb/infrun.c
> > > > > > > > +++ b/gdb/infrun.c
> > > > > > > > @@ -6534,11 +6534,31 @@ process_event_stop_test
> > > > > > > > (struct
> > > > > > > > execution_control_state *ecs)
> > > > > > > > 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);
> > > > > > > > +
> > > > > > > > + 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-
> > > > > > > > >suspend.stop_pc;
> > > > > > > > - if (stop_pc == ecs->event_thread-
> > > > > > > > >control.step_range_start
> > > > > > > > + if (stop_pc == range_start
> > > > > > > > && stop_pc != ecs->stop_func_start
> > > > > > > > && execution_direction == EXEC_REVERSE)
> > > > > > > > end_stepping_range (ecs);
> > > > > > > > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > > > > > > > index 7ffb52a943..e8f5301951 100644
> > > > > > > > --- a/gdb/symtab.c
> > > > > > > > +++ b/gdb/symtab.c
> > > > > > > > @@ -3382,6 +3382,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;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return prev_pc;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > /* See symtab.h. */
> > > > > > > > struct symtab *
> > > > > > > > diff --git a/gdb/symtab.h b/gdb/symtab.h
> > > > > > > > index f060e0ebc1..659cb46292 100644
> > > > > > > > --- a/gdb/symtab.h
> > > > > > > > +++ b/gdb/symtab.h
> > > > > > > > @@ -1916,6 +1916,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