Re: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges

[Carl Love via Gdb-patches <gdb-patches@sourceware.org>]

GDB maintainers:

Just a ping on the patch.  I noticed I did miss address the patch
initially. This is a patch for GDB.

                Carl 


On Wed, 2022-02-23 at 08:39 -0800, Carl Love wrote:
> GCC maintainers:
> 
> The following patch was posted by Luis Machado on 2/1/2021.  There
> was
> a little discussion on the patch but it was never fully reviewed and
> approved.  The email for Luis <luis.machado@linaro.org> no longer
> works.
> 
> As of 2/21/2022, the patch does not compile.  I made a small fix to
> get
> it to compile.  I commented out the original line in gdb/infrun.c and
> added a new line with the fix and the comment //carll fix to indicate
> what I changed.  Clearly, the comment needs to be removed if the
> patch
> is accepted but I wanted to show what I changed.
> 
> That said, I tested the patch on Powerpc and it fixed the 5 test
> failures in gdb.reverse/solib-precsave.exp and 5 test failures in
> gdb.reverse/solib-reverse.exp.  I tested on Intel 64-bit.  The two
> tests solib-precsave.exp and solib-reverse.exp both initially passed
> on
> Intel.  No additional regression failures were seen with the patch.
> 
> Please let me know if you have comments on the patch or if it is
> acceptable as is.  Thank you.
> 
>                      Carl Love
> -----------------------------------------------------------
> 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 fix
> +
> +
> +	  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
>  	  && 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;
> +    }
> +
> +  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);