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

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

[Carl Love]

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);
-- 
2.32.0

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

[Carl Love]

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);

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

[Bruno Larsen]

On 2/23/22 13:39, Carl Love via Gdb-patches 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

Hello Carl!

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

> -----------------------------------------------------------
> 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);


-- 
Cheers!
Bruno Larsen

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

[Carl Love]

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);
> 
> 

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

[Bruno Larsen]

On 3/8/22 19:01, Carl Love wrote:
> 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 have recently worked on gdb.base/until-trailing-insns.exp, that uses the dwarf assembler quite a bit to create a line table. I am not sure how one would go about making contiguous ranges, but maybe you can find something in gdb/testsuite/lib/dwarf.exp to help you.

> 
> 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);
>>
>>
> 


-- 
Cheers!
Bruno Larsen

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

[Luis Machado]

Hi Carl,

Sorry for the e-mail bounce.

I can revisit this one, as I still see the same failures. Hopefully I'll 
have some time to address some of the concerns from past reviews. I'll 
need to refresh my memory on what it was about. :-)

Regards,
Luis

On 3/8/22 22:01, Carl Love via Gdb-patches wrote:
> 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);
>>
>>
> 

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

[Carl Love]

Bruno:

On Wed, 2022-03-09 at 09:23 -0300, Bruno Larsen wrote:
> > > 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 have recently worked on gdb.base/until-trailing-insns.exp, that
> uses the dwarf assembler quite a bit to create a line table. I am not
> sure how one would go about making contiguous ranges, but maybe you
> can find something in gdb/testsuite/lib/dwarf.exp to help you.

I have studied until-trailing-insns.exp, dwarf.exp and the DWARF
Debugging Informat Format Version 5 document.  I really don't see how
to write the test case you desire.

The issue is gdb is supposed to step in reverse one statement at a
time.  The two testcases that the patch fix have two statements on the
same line.  Specifically from gdb.reverse/solib-reverse.c it has the
source code line:  b[0] = 6;   b[1] = 9;.  It is this line where the
reverse step fails to stop in between the two statements.  

I tried
dumping the DWARF info for a very simple program with two statements on
the same line as in the two test cases that fail to see what GCC
generates.  The test program is:

int main() {
  int i,j;
  i = 1; j=3;
  printf("i=%d, j=%d\n", i, j);
}

gcc -g  -O0 test2.c -o test2
objdump -g -W -S -d test2  > test2.dump

The interesting dump information is:

 int main() {
 7fc:   02 00 4c 3c     addis   r2,r12,2
 800:   04 77 42 38     addi    r2,r2,30468
 804:   a6 02 08 7c     mflr    r0
 808:   10 00 01 f8     std     r0,16(r1)
 80c:   f8 ff e1 fb     std     r31,-8(r1)
 810:   81 ff 21 f8     stdu    r1,-128(r1)
 814:   78 0b 3f 7c     mr      r31,r1
  int i,j;
  i = 1; j=3;
 818:   01 00 20 39     li      r9,1                    // Store i = 1
 81c:   68 00 3f 91     stw     r9,104(r31)
 820:   03 00 20 39     li      r9,3                    // Store j = 3
 824:   6c 00 3f 91     stw     r9,108(r31)
  printf("i=%d, j=%d\n", i, j);
 
Line Number Statements:
  [0x00000040]  Set column to 12
  [0x00000042]  Extended opcode 2: set Address to 0x7fc
  [0x0000004d]  Special opcode 10: advance Address by 0 to 0x7fc and Line by 5 to 6
  [0x0000004e]  Set column to 5
  [0x00000050]  Special opcode 105: advance Address by 28 to 0x818 and Line by 2 to 8
  [0x00000051]  Set column to 11
  [0x00000053]  Special opcode 33: advance Address by 8 to 0x820 and Line by 0 to 8
  [0x00000054]  Set column to 3
  [0x00000056]  Special opcode 34: advance Address by 8 to 0x828 and Line by 1 to 9
  [0x00000057]  Set column to 1
  [0x00000059]  Special opcode 146: advance Address by 40 to 0x850 and Line by 1 to 10
  [0x0000005a]  Advance PC by 36 to 0x874
  [0x0000005c]  Extended opcode 1: End of Sequence


Here I can see that "[0x00000053]  Special opcode 33: advance Address
by 8 to 0x820 and Line by 0 to 8"  refers to address 0x820 in the
binary which is the assembly statement for j=3.  Also, it says advance
Line by 0, i.e. it is the same line number as the previous statement
i=1.

The line entries in the line table, i.e. Line Number, are different.

In your message, you said to create a test case that has "continuous PC
ranges."  I don't see where/how that would be reflected in the DWARF
info as dumped above?  I don't see GCC generating information/code with
continuous PC ranges.  I have not found anything that would enable me
to do that.  Each assembly statement has a unique PC value, but the
DWARF info does track the source Line number separately.  

Clearly GCC is generating DWARF that specifies the statements are in
the same line but with different PC values.  I have not found anything
that would help me "making contiguous ranges" that you refere to.  I
really don't see how a new DWARF test is going to be different or
better then the existing C tests?  Perhaps you could explain more as to
the value of this new test and why the existing two C code tests which
the patch fixes are not sufficient to detect a future gdb regression? 
Sorry, I just don't see how to create the test you propose and at this
point, and I am not seeing the value in it.  I don't claim to be an
expert on DWARF so it is entirely possible I am missing something here.

Thanks for your feedback and help on this.

                      Carl

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

[Luis Machado]

Hi!

On 3/8/22 20:21, Bruno Larsen via Gdb-patches wrote:
> On 2/23/22 13:39, Carl Love via Gdb-patches 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
> 
> Hello Carl!
> 
> 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
> 
>> -----------------------------------------------------------
>> 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.
> 

Do you mean moving the range_start check downwards below the 
ecs->stop_func_start check?

>>         && 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.
> 

I went through this again, and I don't think prev-sal.line needs to be 
checked. At this point we know current_sal.line is sane, so anything 
that differs from the current line, we bail out and return the address 
we currently have.

Does that make sense?

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

[Bruno Larsen]

On 3/10/22 08:13, Luis Machado wrote:
> Hi!
> 
> On 3/8/22 20:21, Bruno Larsen via Gdb-patches wrote:
>> On 2/23/22 13:39, Carl Love via Gdb-patches 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
>>
>> Hello Carl!
>>
>> 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
>>
>>> -----------------------------------------------------------
>>> 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.
>>
> 
> Do you mean moving the range_start check downwards below the ecs->stop_func_start check?

I meant that they can be in the same line:
	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;
>>
>> 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.
>>
> 
> I went through this again, and I don't think prev-sal.line needs to be checked. At this point we know current_sal.line is sane, so anything that differs from the current line, we bail out and return the address we currently have.
> 
> Does that make sense?
> 
It does.  My main point was asking if finding a 0 was cause for leaving with an error, instead of leaving with the answer that could be wrong. But the base answer is already wrong, so this would probably be a less wrong answer, so this is fine

-- 
Cheers!
Bruno Larsen

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

[Luis Machado]

On 3/10/22 13:19, Bruno Larsen wrote:
> On 3/10/22 08:13, Luis Machado wrote:
>> Hi!
>>
>> On 3/8/22 20:21, Bruno Larsen via Gdb-patches wrote:
>>> On 2/23/22 13:39, Carl Love via Gdb-patches 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
>>>
>>> Hello Carl!
>>>
>>> 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
>>>
>>>> -----------------------------------------------------------
>>>> 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.
>>>
>>
>> Do you mean moving the range_start check downwards below the 
>> ecs->stop_func_start check?
> 
> I meant that they can be in the same line:
>      if (stop_pc == range_start && stop_pc != ecs->stop_func_start
> 

Got it. I can do that.

>>
>>>>         && 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.
>>>
>>
>> I went through this again, and I don't think prev-sal.line needs to be 
>> checked. At this point we know current_sal.line is sane, so anything 
>> that differs from the current line, we bail out and return the address 
>> we currently have.
>>
>> Does that make sense?
>>
> It does.  My main point was asking if finding a 0 was cause for leaving 
> with an error, instead of leaving with the answer that could be wrong. 
> But the base answer is already wrong, so this would probably be a less 
> wrong answer, so this is fine
> 

Finding a line 0 could be either valid or invalid. If the line table is 
sane and we find 0 because there is no line number for a particular PC, 
it is valid, and we've reached the end of the lookup.

If we find 0 because the line table is missing an entry for the PC we're 
looking for, then the answer is incorrect. But that would be a problem 
with the line table. Not much GDB can do other than warn.

I'll refresh/re-test this patch and will address some previous comments 
Simon had.

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

[Bruno Larsen]

Carl:

On 3/9/22 17:52, Carl Love wrote:
> Bruno:
> 
> On Wed, 2022-03-09 at 09:23 -0300, Bruno Larsen wrote:
>>>> 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 have recently worked on gdb.base/until-trailing-insns.exp, that
>> uses the dwarf assembler quite a bit to create a line table. I am not
>> sure how one would go about making contiguous ranges, but maybe you
>> can find something in gdb/testsuite/lib/dwarf.exp to help you.
> 
> I have studied until-trailing-insns.exp, dwarf.exp and the DWARF
> Debugging Informat Format Version 5 document.  I really don't see how
> to write the test case you desire.
> 
> The issue is gdb is supposed to step in reverse one statement at a
> time.  The two testcases that the patch fix have two statements on the
> same line.  Specifically from gdb.reverse/solib-reverse.c it has the
> source code line:  b[0] = 6;   b[1] = 9;.  It is this line where the
> reverse step fails to stop in between the two statements.
> 
> I tried
> dumping the DWARF info for a very simple program with two statements on
> the same line as in the two test cases that fail to see what GCC
> generates.  The test program is:
> 
> int main() {
>    int i,j;
>    i = 1; j=3;
>    printf("i=%d, j=%d\n", i, j);
> }
> 
> gcc -g  -O0 test2.c -o test2
> objdump -g -W -S -d test2  > test2.dump
> 
> The interesting dump information is:
> 
>   int main() {
>   7fc:   02 00 4c 3c     addis   r2,r12,2
>   800:   04 77 42 38     addi    r2,r2,30468
>   804:   a6 02 08 7c     mflr    r0
>   808:   10 00 01 f8     std     r0,16(r1)
>   80c:   f8 ff e1 fb     std     r31,-8(r1)
>   810:   81 ff 21 f8     stdu    r1,-128(r1)
>   814:   78 0b 3f 7c     mr      r31,r1
>    int i,j;
>    i = 1; j=3;
>   818:   01 00 20 39     li      r9,1                    // Store i = 1
>   81c:   68 00 3f 91     stw     r9,104(r31)
>   820:   03 00 20 39     li      r9,3                    // Store j = 3
>   824:   6c 00 3f 91     stw     r9,108(r31)
>    printf("i=%d, j=%d\n", i, j);
>   
> Line Number Statements:
>    [0x00000040]  Set column to 12
>    [0x00000042]  Extended opcode 2: set Address to 0x7fc
>    [0x0000004d]  Special opcode 10: advance Address by 0 to 0x7fc and Line by 5 to 6
>    [0x0000004e]  Set column to 5
>    [0x00000050]  Special opcode 105: advance Address by 28 to 0x818 and Line by 2 to 8
>    [0x00000051]  Set column to 11
>    [0x00000053]  Special opcode 33: advance Address by 8 to 0x820 and Line by 0 to 8
>    [0x00000054]  Set column to 3
>    [0x00000056]  Special opcode 34: advance Address by 8 to 0x828 and Line by 1 to 9
>    [0x00000057]  Set column to 1
>    [0x00000059]  Special opcode 146: advance Address by 40 to 0x850 and Line by 1 to 10
>    [0x0000005a]  Advance PC by 36 to 0x874
>    [0x0000005c]  Extended opcode 1: End of Sequence
> 
> 
> Here I can see that "[0x00000053]  Special opcode 33: advance Address
> by 8 to 0x820 and Line by 0 to 8"  refers to address 0x820 in the
> binary which is the assembly statement for j=3.  Also, it says advance
> Line by 0, i.e. it is the same line number as the previous statement
> i=1.
> 
> The line entries in the line table, i.e. Line Number, are different.
> 
> In your message, you said to create a test case that has "continuous PC
> ranges."  I don't see where/how that would be reflected in the DWARF
> info as dumped above?  I don't see GCC generating information/code with
> continuous PC ranges.  I have not found anything that would enable me
> to do that.  Each assembly statement has a unique PC value, but the
> DWARF info does track the source Line number separately.


The testcase I had in mind could be something simple like this:

test.c:
1 int main (){
2     asm ("main_label: .globl main_label");
3     int i,j;
4     asm ("i_assignment: .globl i_assignment");
5     i = 0; /* TAG: assignment line */
6     asm ("j_assignment: .globl j_assignment");
7     j = 0;
8     return 0;
9 }

test.exp:
...
dwarf::assemble{
...
     {DW_LNE_set_address i_assignment}
     {line [gdb_get_line_number "TAG: assignment line"]}
     {DW_LNS_copy}
     {DW_LNE_set_address j_assignment}
     {line [gdb_get_line_number "TAG: assignment line"]}
     {DW_LNS_copy}
...
}

To explain what is going on: We use the asm() labels to identify PC addresses for the assignment of variables. We use the comment /* TAG ... */ to identify the line we care about. And then we pretend that those 2 PC addresses came from the same line. Even though they didn't in the actual .c, GDB will think they did because of the dwarf information, guaranteeing that we will continue to test this for regressions. (side note: main_label needs to be there for the dwarf assembler to work correctly.)

If I understand the problem correctly, this test and dwarf assembler snippet should be able to reproduce the bug, but I am also no DWARF expert, so I may be missing something as well.

> 
> Clearly GCC is generating DWARF that specifies the statements are in
> the same line but with different PC values.  I have not found anything
> that would help me "making contiguous ranges" that you refere to.  I
> really don't see how a new DWARF test is going to be different or
> better then the existing C tests?  Perhaps you could explain more as to
> the value of this new test and why the existing two C code tests which
> the patch fixes are not sufficient to detect a future gdb regression?
> Sorry, I just don't see how to create the test you propose and at this
> point, and I am not seeing the value in it.  I don't claim to be an
> expert on DWARF so it is entirely possible I am missing something here.

The value comming from a test like this is that we can have the aarch64 behavior on any CPU architecture. This will give us more test coverage, as I can't easily test on aarch64 hardware for instance, and I imagine a lot of people may just be unable to test it. Also,it is possible that GCC will change this behavior in the future for those specific tests, and we unknowingly stop testing for this regression.

> 
> Thanks for your feedback and help on this.
> 
>                        Carl
> 
> 
> 


-- 
Cheers!
Bruno Larsen

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

[Carl Love]

Bruno, GDB maintainers:

On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
> Hello Carl!
> 
> 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

The attached patch includes a stand alone DWARF test to verify the
patch works.  The test has been verified on i386 and Powerpc. 
Additionally the two lines in infrun.c were combined onto one line as
Bruno mentioned. 

As mentioned Bruno can not approve the patch.  Hopefully on of the GDB
maintainers can give us an additional review to let us know if the
patch is acceptable.  Thanks.

                     Carl Love

--------------------------------------------------------------------

Mon Sep 17 00:00:00 2001
From: Luis Machado 
Date: Mon, 21 Feb 2022 23:11:23 +0000
Subject: [PATCH] Fix reverse stepping multiple contiguous PC ranges

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 no longer works.

As of 2/21/2022 the patch does not compile.  I made a small fix to get it t
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.
-----------------------------------------------------------

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.

The patch includes a new test that creates a DWARF entry with two statements
with the same line number to verify the patch fixes the issue.

Co-authored-by: Carl Love <cel@us.ibm.com>
---
 gdb/infrun.c                                  |  25 +++-
 gdb/symtab.c                                  |  35 ++++++
 gdb/symtab.h                                  |  16 +++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  30 +++++
 .../gdb.reverse/map-to-same-line.exp          | 114 ++++++++++++++++++
 5 files changed, 218 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e3c1db73749..09701c9e5ff 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6740,12 +6740,33 @@ 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);
+	    = 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
-	  && stop_pc != ecs->stop_func_start
+      if (stop_pc == range_start && stop_pc != ecs->stop_func_start
 	  && execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
       else
diff --git a/gdb/symtab.c b/gdb/symtab.c
index a867e1db9fd..600006c7843 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);
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c b/gdb/testsuite/gdb.reverse/map-to-same-line.c
new file mode 100644
index 00000000000..d35838eccc9
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,30 @@
+
+
+/* The purpose of this test is to create a DWARF line table that says there
+   are two assignment statements on the same line.  The expect test checks to
+   make sure gdb reverse steps over each statements individulally.  The test
+   makes sure a single reverse step doesn't step over both assignment
+   statements in the line. The expect file generates the DWARF assembly
+   statements will create a line table that with j = 3 and k = 4 listed as
+   being on the same source code line even though they actually are on
+   different lines in this source code below. Have to put them on separate
+   lines to be able to identify them and add them to the line table.  */
+int
+main (){     /* TAG: main prologue */
+  asm ("main_label: .globl main_label");
+  asm ("loop_start: .globl loop_start");
+  int i, j, k, l, m;
+
+  asm ("i_assignment: .globl i_assignment");
+  i = 1;     /* TAG: assignment i */
+  asm ("j_assignment: .globl j_assignment");
+  j = 3;     /* TAG: assignment j */
+  asm ("k_assignment: .globl k_assignment");
+  k = 4;  /* TAG: assignment k */
+  asm ("l_assignment: .globl l_assignment");
+  l = 10;     /* TAG: assignment l */
+  asm ("m_assignment: .globl m_assignment");
+  m = 11;     /* TAG: assignment m */
+  asm ("main_return: .globl main_return");
+  return 0;  /* TAG: end of main */
+}
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
new file mode 100644
index 00000000000..da407b50e94
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,114 @@
+
+# The purpose of this test is to create a DWARF line table that says the
+# two assignment statements are on the same line.  The expect test checks
+# to make sure gdb reverse steps over each statement individually, i.e.
+# not over the line containing both assignments.  */
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+
+if [get_compiler_info] {
+    return -1
+}
+
+# The DWARF assembler requires the gcc compiler.
+if {!$gcc_compiled} {
+    unsupported "gcc is required for this test"
+    return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+
+    # Find start address and length of program
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+        main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+        compile_unit {
+            {language @DW_LANG_C}
+            {name map-to-same-line.c}
+            {stmt_list $L DW_FORM_sec_offset}
+            {low_pc 0 addr}
+        } {
+            subprogram {
+                {external 1 flag}
+                {name main}
+                {low_pc $main_start addr}
+                {high_pc $main_len DW_FORM_data4}
+            }
+        }
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+        include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate the line table program with the assignments to j and k
+	# are listed on the same source line in the table.
+	program {
+	    {DW_LNE_set_address $main_start}
+            {line [gdb_get_line_number "TAG: main prologue"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address i_assignment}
+	    {line [gdb_get_line_number "TAG: assignment i"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address j_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address k_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address l_assignment}
+	    {line [gdb_get_line_number "TAG: assignment l"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address m_assignment}
+	    {line [gdb_get_line_number "TAG: assignment m"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_end_sequence}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+        [list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+set break_at_l [gdb_get_line_number "TAG: assignment l" ]
+
+gdb_test "tbreak $break_at_l" "Temporary breakpoint .*"  "breakpoint l = 10"
+
+gdb_test "continue" "Temporary breakpoint .*" "run to l = 10"
+# j = 3 and k = 4 are on the same line.  The reverse step stops at j = 3
+gdb_test "reverse-step" ".* j = 3;.*" "reverse-step to j = 3"
+gdb_test "reverse-step" ".* i = 1;.*" "reverse-step to i = 1"
-- 
2.32.0

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

[Carl Love]

Bruno, GDB maintainers:

I was asked to clean up the message a bit to clarifiy the message. 
Changing this to version 2 since it now has an additional test added to
the patch and a code fix.

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.  Luis has a new email address: luis.machado@arm.com.

As of 2/21/2022 the patch did 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.

On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
> Hello Carl!
> 
> 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

The attached patch includes a stand alone DWARF test to verify the
patch works.  The test has been verified on i386 and Powerpc. 
Additionally the two lines in infrun.c were combined onto one line as
Bruno mentioned. 

As mentioned Bruno can not approve the patch.  Hopefully on of the GDB
maintainers can give us an additional review to let us know if the
patch is acceptable.  Thanks.

                     Carl Love

------------------------------------------------
Fix reverse stepping multiple contiguous PC ranges

The following patch fixes 5 test failures in gdb.reverse/solib-precsave.exp
and 5 test failures in gdb.reverse/solib-reverse.exp.  Both testst pass on
Intel 64-bit.  The two tests solib-precsave.exp and solib-reverse.exp both
initially passed on Intel without the patch.

No additional regression failures were seen with the patch.

A new testcase was added using DWARF statements to reproduce the issue as
described below.  The new testcase has been tested on x86 and Powerpc with
the patch to that the test case fails without the patch.  The issue is fixed
on both platforms with the patch.

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 0e819553aeb..0fcfd0fb10e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6779,11 +6779,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 a867e1db9fd..600006c7843 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);
-- 
2.32.0

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

[Luis Machado]

Hi Carl,

Thanks for working on the testcase. I have some minor changes to the 
patch. Mainly incorporating Simon's suggestions from the first review.

Instead of comparing just the line, the updated code compares the line 
number and the symtab. If both match, then it means we're dealing with 
the same source file.

I'll rebase it and will post it soon.
On 3/22/22 17:05, Carl Love wrote:
> Bruno, GDB maintainers:
> 
> I was asked to clean up the message a bit to clarifiy the message.
> Changing this to version 2 since it now has an additional test added to
> the patch and a code fix.
> 
> 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.  Luis has a new email address: luis.machado@arm.com.
> 
> As of 2/21/2022 the patch did 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.
> 
> On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
>> Hello Carl!
>>
>> 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
> 
> The attached patch includes a stand alone DWARF test to verify the
> patch works.  The test has been verified on i386 and Powerpc.
> Additionally the two lines in infrun.c were combined onto one line as
> Bruno mentioned.
> 
> As mentioned Bruno can not approve the patch.  Hopefully on of the GDB
> maintainers can give us an additional review to let us know if the
> patch is acceptable.  Thanks.
> 
>                       Carl Love
> 
> ------------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges
> 
> The following patch fixes 5 test failures in gdb.reverse/solib-precsave.exp
> and 5 test failures in gdb.reverse/solib-reverse.exp.  Both testst pass on
> Intel 64-bit.  The two tests solib-precsave.exp and solib-reverse.exp both
> initially passed on Intel without the patch.
> 
> No additional regression failures were seen with the patch.
> 
> A new testcase was added using DWARF statements to reproduce the issue as
> described below.  The new testcase has been tested on x86 and Powerpc with
> the patch to that the test case fails without the patch.  The issue is fixed
> on both platforms with the patch.
> 
> 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 0e819553aeb..0fcfd0fb10e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6779,11 +6779,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 a867e1db9fd..600006c7843 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);

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

[Bruno Larsen]

On 3/22/22 14:05, Carl Love wrote:
> Bruno, GDB maintainers:
> 
> I was asked to clean up the message a bit to clarifiy the message.
> Changing this to version 2 since it now has an additional test added to
> the patch and a code fix.
> 
> 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.  Luis has a new email address: luis.machado@arm.com.
> 
> As of 2/21/2022 the patch did 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.
> 
> On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote:
>> Hello Carl!
>>
>> 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
> 
> The attached patch includes a stand alone DWARF test to verify the
> patch works.  The test has been verified on i386 and Powerpc.
> Additionally the two lines in infrun.c were combined onto one line as
> Bruno mentioned.
> 
> As mentioned Bruno can not approve the patch.  Hopefully on of the GDB
> maintainers can give us an additional review to let us know if the
> patch is acceptable.  Thanks.

Hi Carl,

I think you forgot to add the testcase to the email. Also a gentle reminder for you to remove the "//carl fix" comment and the commented line of code above.

> 
>                       Carl Love
> 
> ------------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges
> 
> The following patch fixes 5 test failures in gdb.reverse/solib-precsave.exp
> and 5 test failures in gdb.reverse/solib-reverse.exp.  Both testst pass on
> Intel 64-bit.  The two tests solib-precsave.exp and solib-reverse.exp both
> initially passed on Intel without the patch.
> 
> No additional regression failures were seen with the patch.
> 
> A new testcase was added using DWARF statements to reproduce the issue as
> described below.  The new testcase has been tested on x86 and Powerpc with
> the patch to that the test case fails without the patch.  The issue is fixed
> on both platforms with the patch.
> 
> 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 0e819553aeb..0fcfd0fb10e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6779,11 +6779,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 a867e1db9fd..600006c7843 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);


-- 
Cheers!
Bruno Larsen

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

[Carl Love]

Bruno, GDB maintainers

On Wed, 2022-03-23 at 09:20 -0300, Bruno Larsen wrote:
> I think you forgot to add the testcase to the email. Also a gentle
> reminder for you to remove the "//carl fix" comment and the commented
> line of code above.

Yes, sorry I attached the wrong patch.

Actually, I had been leaving the carll fix intentionally for any
additional reviewers.  

I updated this to V3 to make it clear that the following patch is
different, i.e. that it does have the new test case.  I also went ahead
and removed the //carll fix so everything is now clean.

                    Carl Love


----------------------------------------------------------
Fix reverse stepping multiple contiguous PC ranges

The following patch fixes 5 test failures in gdb.reverse/solib-precsave.exp
and 5 test failures in gdb.reverse/solib-reverse.exp.  Both testst pass on
Intel 64-bit.  The two tests solib-precsave.exp and solib-reverse.exp both
initially passed on Intel without the patch.

No additional regression failures were seen with the patch.

A new testcase was added using DWARF statements to reproduce the issue as
described below.  The new testcase has been tested on x86 and Powerpc with
the patch to that the test case fails without the patch.  The issue is fixed
on both platforms with the patch.

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.

The patch includes a new test that creates a DWARF entry with two statements
with the same line number to verify the patch fixes the issue.

Co-authored-by: Carl Love <cel@us.ibm.com>
---
 gdb/infrun.c                                  |  24 +++-
 gdb/symtab.c                                  |  35 ++++++
 gdb/symtab.h                                  |  16 +++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  30 +++++
 .../gdb.reverse/map-to-same-line.exp          | 114 ++++++++++++++++++
 5 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index b60c1fd238a..ad1a9695763 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6778,12 +6778,32 @@ 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->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->stop_pc ();
-      if (stop_pc == ecs->event_thread->control.step_range_start
-	  && stop_pc != ecs->stop_func_start
+      if (stop_pc == range_start && stop_pc != ecs->stop_func_start
 	  && execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
       else
diff --git a/gdb/symtab.c b/gdb/symtab.c
index a867e1db9fd..600006c7843 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);
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c b/gdb/testsuite/gdb.reverse/map-to-same-line.c
new file mode 100644
index 00000000000..d35838eccc9
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,30 @@
+
+
+/* The purpose of this test is to create a DWARF line table that says there
+   are two assignment statements on the same line.  The expect test checks to
+   make sure gdb reverse steps over each statements individulally.  The test
+   makes sure a single reverse step doesn't step over both assignment
+   statements in the line. The expect file generates the DWARF assembly
+   statements will create a line table that with j = 3 and k = 4 listed as
+   being on the same source code line even though they actually are on
+   different lines in this source code below. Have to put them on separate
+   lines to be able to identify them and add them to the line table.  */
+int
+main (){     /* TAG: main prologue */
+  asm ("main_label: .globl main_label");
+  asm ("loop_start: .globl loop_start");
+  int i, j, k, l, m;
+
+  asm ("i_assignment: .globl i_assignment");
+  i = 1;     /* TAG: assignment i */
+  asm ("j_assignment: .globl j_assignment");
+  j = 3;     /* TAG: assignment j */
+  asm ("k_assignment: .globl k_assignment");
+  k = 4;  /* TAG: assignment k */
+  asm ("l_assignment: .globl l_assignment");
+  l = 10;     /* TAG: assignment l */
+  asm ("m_assignment: .globl m_assignment");
+  m = 11;     /* TAG: assignment m */
+  asm ("main_return: .globl main_return");
+  return 0;  /* TAG: end of main */
+}
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
new file mode 100644
index 00000000000..da407b50e94
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,114 @@
+
+# The purpose of this test is to create a DWARF line table that says the
+# two assignment statements are on the same line.  The expect test checks
+# to make sure gdb reverse steps over each statement individually, i.e.
+# not over the line containing both assignments.  */
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+
+if [get_compiler_info] {
+    return -1
+}
+
+# The DWARF assembler requires the gcc compiler.
+if {!$gcc_compiled} {
+    unsupported "gcc is required for this test"
+    return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+
+    # Find start address and length of program
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+        main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+        compile_unit {
+            {language @DW_LANG_C}
+            {name map-to-same-line.c}
+            {stmt_list $L DW_FORM_sec_offset}
+            {low_pc 0 addr}
+        } {
+            subprogram {
+                {external 1 flag}
+                {name main}
+                {low_pc $main_start addr}
+                {high_pc $main_len DW_FORM_data4}
+            }
+        }
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+        include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate the line table program with the assignments to j and k
+	# are listed on the same source line in the table.
+	program {
+	    {DW_LNE_set_address $main_start}
+            {line [gdb_get_line_number "TAG: main prologue"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address i_assignment}
+	    {line [gdb_get_line_number "TAG: assignment i"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address j_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address k_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address l_assignment}
+	    {line [gdb_get_line_number "TAG: assignment l"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address m_assignment}
+	    {line [gdb_get_line_number "TAG: assignment m"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_end_sequence}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+        [list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+set break_at_l [gdb_get_line_number "TAG: assignment l" ]
+
+gdb_test "tbreak $break_at_l" "Temporary breakpoint .*"  "breakpoint l = 10"
+
+gdb_test "continue" "Temporary breakpoint .*" "run to l = 10"
+# j = 3 and k = 4 are on the same line.  The reverse step stops at j = 3
+gdb_test "reverse-step" ".* j = 3;.*" "reverse-step to j = 3"
+gdb_test "reverse-step" ".* i = 1;.*" "reverse-step to i = 1"
-- 
2.31.1

[PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[Luis Machado]

v2:
- Check if both the line and symtab match for a particular line table entry.

--

When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
the ppc backend), 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 38 will land on line 40.
- step from line 40 will land on line 42.

Reverse execution:

- step from line 42 will land on line 40.
- step from line 40 will land on line 40.
- step from line 40 will land on line 38.

The problem here is that line 40 contains two contiguous but distinct
PC ranges in the line table, like so:

Line 40 - [0x7ec ~ 0x7f4]
Line 40 - [0x7f4 ~ 0x7fc]

The two distinct ranges are generated because GCC started outputting source
column information, which GDB doesn't take into account at the moment.

When stepping forward from line 40, we skip both of these ranges and land on
line 42. When stepping backward from line 42, 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->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. Carl Love has verified that it
does fix a similar issue on ppc.

Ubuntu 18.04 doesn't actually run into these failures because the compiler
doesn't generate distinct PC ranges for the same line.

I see similar failures on x86_64 in the gdb.reverse tests
(gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). Those are
also fixed by this patch, although Carl's testcase doesn't fail for x86_64.

There seems to be a corner case where a line table has only one instruction,
and while stepping that doesn't take the same path through infrun. I think it
needs some more investigation. Therefore this is a tentative patch for now.

Co-authored-by: Carl Love <cel@us.ibm.com>
---
 gdb/infrun.c                                  |  51 +++++++-
 gdb/symtab.c                                  |  49 ++++++++
 gdb/symtab.h                                  |  16 +++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  30 +++++
 .../gdb.reverse/map-to-same-line.exp          | 114 ++++++++++++++++++
 5 files changed, 258 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index dbd82775595..cec55b0a5d7 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6716,6 +6716,11 @@ process_event_stop_test (struct execution_control_state *ecs)
      through a function epilogue and therefore must detect when
      the current-frame changes in the middle of a line.  */
 
+      infrun_debug_printf
+	("STEPPING RANGE [%s-%s]",
+	 paddress (gdbarch, ecs->event_thread->control.step_range_start),
+	 paddress (gdbarch, ecs->event_thread->control.step_range_end));
+
   if (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
 			       ecs->event_thread)
       && (execution_direction != EXEC_REVERSE
@@ -6723,7 +6728,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 			  ecs->event_thread->control.step_frame_id)))
     {
       infrun_debug_printf
-	("stepping inside range [%s-%s]",
+	("STILL INSIDE range [%s-%s]",
 	 paddress (gdbarch, ecs->event_thread->control.step_range_start),
 	 paddress (gdbarch, ecs->event_thread->control.step_range_end));
 
@@ -6732,11 +6737,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->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->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);
@@ -7237,6 +7262,28 @@ process_event_stop_test (struct execution_control_state *ecs)
 	}
     }
 
+    CORE_ADDR range_start = ecs->event_thread->control.step_range_start;
+    infrun_debug_printf ("RANGE START is at %s", paddress (gdbarch, range_start));
+    if (execution_direction == EXEC_REVERSE)
+      {
+	gdb::optional<CORE_ADDR> real_range_start
+	  = find_line_range_start (ecs->event_thread->stop_pc ());
+
+	if (real_range_start.has_value ())
+	  {
+	    range_start = *real_range_start;
+	    infrun_debug_printf ("NEW RANGE START is at %s", paddress (gdbarch, 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 == range_start
+	    && stop_pc != ecs->stop_func_start)
+	  end_stepping_range (ecs);
+      }
+
   /* We aren't done stepping.
 
      Optimize by setting the stepping range to the line.
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 804b88284eb..99abf7bd2b7 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3425,6 +3425,55 @@ find_pc_line (CORE_ADDR pc, int notcurrent)
   return sal;
 }
 
+/* Compare two symtab_and_line entries.  Return true if both have
+   the same line number and the same symtab pointer.  That means we
+   are dealing with two entries from the same line and from the same
+   source file.
+
+   Return false otherwise.  */
+
+static bool
+sal_line_symtab_matches_p (const symtab_and_line &sal1,
+			   const symtab_and_line &sal2)
+{
+  return (sal1.line == sal2.line && sal1.symtab == sal2.symtab);
+}
+
+/* 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 (!sal_line_symtab_matches_p (prev_sal, current_sal))
+    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 (!sal_line_symtab_matches_p (prev_sal, current_sal))
+	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);
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c b/gdb/testsuite/gdb.reverse/map-to-same-line.c
new file mode 100644
index 00000000000..d35838eccc9
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,30 @@
+
+
+/* The purpose of this test is to create a DWARF line table that says there
+   are two assignment statements on the same line.  The expect test checks to
+   make sure gdb reverse steps over each statements individulally.  The test
+   makes sure a single reverse step doesn't step over both assignment
+   statements in the line. The expect file generates the DWARF assembly
+   statements will create a line table that with j = 3 and k = 4 listed as
+   being on the same source code line even though they actually are on
+   different lines in this source code below. Have to put them on separate
+   lines to be able to identify them and add them to the line table.  */
+int
+main (){     /* TAG: main prologue */
+  asm ("main_label: .globl main_label");
+  asm ("loop_start: .globl loop_start");
+  int i, j, k, l, m;
+
+  asm ("i_assignment: .globl i_assignment");
+  i = 1;     /* TAG: assignment i */
+  asm ("j_assignment: .globl j_assignment");
+  j = 3;     /* TAG: assignment j */
+  asm ("k_assignment: .globl k_assignment");
+  k = 4;  /* TAG: assignment k */
+  asm ("l_assignment: .globl l_assignment");
+  l = 10;     /* TAG: assignment l */
+  asm ("m_assignment: .globl m_assignment");
+  m = 11;     /* TAG: assignment m */
+  asm ("main_return: .globl main_return");
+  return 0;  /* TAG: end of main */
+}
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
new file mode 100644
index 00000000000..da407b50e94
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,114 @@
+
+# The purpose of this test is to create a DWARF line table that says the
+# two assignment statements are on the same line.  The expect test checks
+# to make sure gdb reverse steps over each statement individually, i.e.
+# not over the line containing both assignments.  */
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+
+if [get_compiler_info] {
+    return -1
+}
+
+# The DWARF assembler requires the gcc compiler.
+if {!$gcc_compiled} {
+    unsupported "gcc is required for this test"
+    return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+
+    # Find start address and length of program
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+        main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+        compile_unit {
+            {language @DW_LANG_C}
+            {name map-to-same-line.c}
+            {stmt_list $L DW_FORM_sec_offset}
+            {low_pc 0 addr}
+        } {
+            subprogram {
+                {external 1 flag}
+                {name main}
+                {low_pc $main_start addr}
+                {high_pc $main_len DW_FORM_data4}
+            }
+        }
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+        include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate the line table program with the assignments to j and k
+	# are listed on the same source line in the table.
+	program {
+	    {DW_LNE_set_address $main_start}
+            {line [gdb_get_line_number "TAG: main prologue"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address i_assignment}
+	    {line [gdb_get_line_number "TAG: assignment i"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address j_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address k_assignment}
+	    {line [gdb_get_line_number "TAG: assignment j"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address l_assignment}
+	    {line [gdb_get_line_number "TAG: assignment l"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_set_address m_assignment}
+	    {line [gdb_get_line_number "TAG: assignment m"]}
+	    {DW_LNS_copy}
+
+	    {DW_LNE_end_sequence}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+        [list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+set break_at_l [gdb_get_line_number "TAG: assignment l" ]
+
+gdb_test "tbreak $break_at_l" "Temporary breakpoint .*"  "breakpoint l = 10"
+
+gdb_test "continue" "Temporary breakpoint .*" "run to l = 10"
+# j = 3 and k = 4 are on the same line.  The reverse step stops at j = 3
+gdb_test "reverse-step" ".* j = 3;.*" "reverse-step to j = 3"
+gdb_test "reverse-step" ".* i = 1;.*" "reverse-step to i = 1"
-- 
2.25.1

Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[will schmidt]

On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches wrote:
> v2:
> - Check if both the line and symtab match for a particular line table entry.
> 
> --
> 
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
> the ppc backend), 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 38 will land on line 40.
> - step from line 40 will land on line 42.
> 
> Reverse execution:
> 
> - step from line 42 will land on line 40.
> - step from line 40 will land on line 40.
> - step from line 40 will land on line 38.
> 
> The problem here is that line 40 contains two contiguous but distinct
> PC ranges in the line table, like so:
> 
> Line 40 - [0x7ec ~ 0x7f4]
> Line 40 - [0x7f4 ~ 0x7fc]


<snip>


> I'm not particularly happy with how we go back in the ranges (using "pc - 1").
> Feedback would be welcome.

I suppose there could be a loop of some sort that iterates backwards to
a valid line; though I'd think pc - 1 is sufficient? 

> 
> Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has verified that it
> does fix a similar issue on ppc.
> 
> Ubuntu 18.04 doesn't actually run into these failures because the compiler
> doesn't generate distinct PC ranges for the same line.
> 
> I see similar failures on x86_64 in the gdb.reverse tests
> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). Those are
> also fixed by this patch, although Carl's testcase doesn't fail for x86_64.
> 
> There seems to be a corner case where a line table has only one instruction,
> and while stepping that doesn't take the same path through infrun. I think it
> needs some more investigation. Therefore this is a tentative patch for now.


Are you (or Carl) continuing to pursue that edge case? 

Unless you think the contents here would be significantly invalidated, may
be worth moving forrward with this as at least an incremental improvement.  

> 
> Co-authored-by: Carl Love <cel@us.ibm.com>
> ---
>  gdb/infrun.c                                  |  51 +++++++-
>  gdb/symtab.c                                  |  49 ++++++++
>  gdb/symtab.h                                  |  16 +++
>  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  30 +++++
>  .../gdb.reverse/map-to-same-line.exp          | 114 ++++++++++++++++++


<snip>

Code changes all seem reasonable.    Just one comment on the commentary
here. 


> diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> new file mode 100644
> index 00000000000..da407b50e94
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,114 @@
> +
> +# The purpose of this test is to create a DWARF line table that says the
> +# two assignment statements are on the same line.  The expect test checks
> +# to make sure gdb reverse steps over each statement individually, i.e.
> +# not over the line containing both assignments.  */
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +
> +# The DWARF assembler requires the gcc compiler.
> +if {!$gcc_compiled} {
> +    unsupported "gcc is required for this test"
> +    return 0
> +}
> +
> +standard_testfile .c .S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    declare_labels integer_label L
> +
> +    # Find start address and length of program
> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +        main_start main_len
> +    set main_end "$main_start + $main_len"
> +
> +    cu {} {
> +        compile_unit {
> +            {language @DW_LANG_C}
> +            {name map-to-same-line.c}
> +            {stmt_list $L DW_FORM_sec_offset}
> +            {low_pc 0 addr}
> +        } {
> +            subprogram {
> +                {external 1 flag}
> +                {name main}
> +                {low_pc $main_start addr}
> +                {high_pc $main_len DW_FORM_data4}
> +            }
> +        }
> +    }
> +
> +    lines {version 2 default_is_stmt 1} L {
> +        include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	# Generate the line table program with the assignments to j and k
> +	# are listed on the same source line in the table.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +            {line [gdb_get_line_number "TAG: main prologue"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address i_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment i"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address j_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment j"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address k_assignment}

THough it's covered in the section comment, a comment here to clarify
 "Force the Dwarf line number for k_assignment to the j assignment here."  may be useful. 
I was explicitly looking for it, and missed it in my first scan through. :-)  



Thanks,
-Will


> +	    {line [gdb_get_line_number "TAG: assignment j"]}
> +	    {DW_LNS_copy}

> +
> +	    {DW_LNE_set_address l_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment l"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address m_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment m"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_end_sequence}
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +        [list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +if [supports_process_record] {
> +    # Activate process record/replay
> +    gdb_test_no_output "record" "turn on process record"
> +}
> +
> +set break_at_l [gdb_get_line_number "TAG: assignment l" ]
> +
> +gdb_test "tbreak $break_at_l" "Temporary breakpoint .*"  "breakpoint l = 10"
> +
> +gdb_test "continue" "Temporary breakpoint .*" "run to l = 10"
> +# j = 3 and k = 4 are on the same line.  The reverse step stops at j = 3
> +gdb_test "reverse-step" ".* j = 3;.*" "reverse-step to j = 3"
> +gdb_test "reverse-step" ".* i = 1;.*" "reverse-step to i = 1"

Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[Luis Machado]

On 4/4/22 17:55, will schmidt wrote:
> On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches wrote:
>> v2:
>> - Check if both the line and symtab match for a particular line table entry.
>>
>> --
>>
>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
>> the ppc backend), 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 38 will land on line 40.
>> - step from line 40 will land on line 42.
>>
>> Reverse execution:
>>
>> - step from line 42 will land on line 40.
>> - step from line 40 will land on line 40.
>> - step from line 40 will land on line 38.
>>
>> The problem here is that line 40 contains two contiguous but distinct
>> PC ranges in the line table, like so:
>>
>> Line 40 - [0x7ec ~ 0x7f4]
>> Line 40 - [0x7f4 ~ 0x7fc]
> 
> 
> <snip>
> 
> 
>> I'm not particularly happy with how we go back in the ranges (using "pc - 1").
>> Feedback would be welcome.
> 
> I suppose there could be a loop of some sort that iterates backwards to
> a valid line; though I'd think pc - 1 is sufficient?
> 

Yes, it seems to do the job.

>>
>> Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has verified that it
>> does fix a similar issue on ppc.
>>
>> Ubuntu 18.04 doesn't actually run into these failures because the compiler
>> doesn't generate distinct PC ranges for the same line.
>>
>> I see similar failures on x86_64 in the gdb.reverse tests
>> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). Those are
>> also fixed by this patch, although Carl's testcase doesn't fail for x86_64.
>>
>> There seems to be a corner case where a line table has only one instruction,
>> and while stepping that doesn't take the same path through infrun. I think it
>> needs some more investigation. Therefore this is a tentative patch for now.
> 
> 
> Are you (or Carl) continuing to pursue that edge case?
> 

Not at the moment unfortunately. I know that this needs to be handled in 
the fallthrough of process_event_stop_test. Carl's test doesn't seem to 
fail for x86_64 (at least for me). We need to polish the testcase a bit 
more so that we can cover that corner case as well.

Also, this is more of a RFC at this point. You'll notice some debug 
print statement in the patch. It would be nice to turn those into 
permanent debug prints, as this code doesn't seem to print too much 
detail about what it is doing.

Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[will schmidt]

On Tue, 2022-04-05 at 09:36 +0100, Luis Machado wrote:
> On 4/4/22 17:55, will schmidt wrote:
> > On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches
> > wrote:
> > > v2:
> > > - Check if both the line and symtab match for a particular line
> > > table entry.
> > > 
> > > --
> > > 
> > > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
> > > spotted on
> > > the ppc backend), 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 38 will land on line 40.
> > > - step from line 40 will land on line 42.
> > > 
> > > Reverse execution:
> > > 
> > > - step from line 42 will land on line 40.
> > > - step from line 40 will land on line 40.
> > > - step from line 40 will land on line 38.
> > > 
> > > The problem here is that line 40 contains two contiguous but
> > > distinct
> > > PC ranges in the line table, like so:
> > > 
> > > Line 40 - [0x7ec ~ 0x7f4]
> > > Line 40 - [0x7f4 ~ 0x7fc]
> > 
> > <snip>
> > 
> > 
> > > I'm not particularly happy with how we go back in the ranges
> > > (using "pc - 1").
> > > Feedback would be welcome.
> > 
> > I suppose there could be a loop of some sort that iterates
> > backwards to
> > a valid line; though I'd think pc - 1 is sufficient?
> > 
> 
> Yes, it seems to do the job.
> 
> > > Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has
> > > verified that it
> > > does fix a similar issue on ppc.
> > > 
> > > Ubuntu 18.04 doesn't actually run into these failures because the
> > > compiler
> > > doesn't generate distinct PC ranges for the same line.
> > > 
> > > I see similar failures on x86_64 in the gdb.reverse tests
> > > (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
> > > Those are
> > > also fixed by this patch, although Carl's testcase doesn't fail
> > > for x86_64.
> > > 
> > > There seems to be a corner case where a line table has only one
> > > instruction,
> > > and while stepping that doesn't take the same path through
> > > infrun. I think it
> > > needs some more investigation. Therefore this is a tentative
> > > patch for now.
> > 
> > Are you (or Carl) continuing to pursue that edge case?
> > 
> 
> Not at the moment unfortunately. I know that this needs to be handled
> in 
> the fallthrough of process_event_stop_test. Carl's test doesn't seem
> to 
> fail for x86_64 (at least for me). We need to polish the testcase a
> bit 
> more so that we can cover that corner case as well.
> 
> Also, this is more of a RFC at this point. You'll notice some debug 
> print statement in the patch. It would be nice to turn those into 
> permanent debug prints, as this code doesn't seem to print too much 
> detail about what it is doing.

OK,
thanks.

I did notice the debug printfs.   Since they were actually using
the infrun_debug_printf helper, versus actual printf calls,  I assumed
they were deliberate and meant for the long term.  :-)
I wasn't going to
nit on the uppercase content of the debug printfs, I figure since they
are actually debug for special circumstances the debug was fine as
presented.  :-)
I'm generally a fan of adding more debug output.

Thanks
-Will

Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[Luis Machado]

On 4/5/22 16:15, will schmidt wrote:
> On Tue, 2022-04-05 at 09:36 +0100, Luis Machado wrote:
>> On 4/4/22 17:55, will schmidt wrote:
>>> On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches
>>> wrote:
>>>> v2:
>>>> - Check if both the line and symtab match for a particular line
>>>> table entry.
>>>>
>>>> --
>>>>
>>>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
>>>> spotted on
>>>> the ppc backend), 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 38 will land on line 40.
>>>> - step from line 40 will land on line 42.
>>>>
>>>> Reverse execution:
>>>>
>>>> - step from line 42 will land on line 40.
>>>> - step from line 40 will land on line 40.
>>>> - step from line 40 will land on line 38.
>>>>
>>>> The problem here is that line 40 contains two contiguous but
>>>> distinct
>>>> PC ranges in the line table, like so:
>>>>
>>>> Line 40 - [0x7ec ~ 0x7f4]
>>>> Line 40 - [0x7f4 ~ 0x7fc]
>>>
>>> <snip>
>>>
>>>
>>>> I'm not particularly happy with how we go back in the ranges
>>>> (using "pc - 1").
>>>> Feedback would be welcome.
>>>
>>> I suppose there could be a loop of some sort that iterates
>>> backwards to
>>> a valid line; though I'd think pc - 1 is sufficient?
>>>
>>
>> Yes, it seems to do the job.
>>
>>>> Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has
>>>> verified that it
>>>> does fix a similar issue on ppc.
>>>>
>>>> Ubuntu 18.04 doesn't actually run into these failures because the
>>>> compiler
>>>> doesn't generate distinct PC ranges for the same line.
>>>>
>>>> I see similar failures on x86_64 in the gdb.reverse tests
>>>> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
>>>> Those are
>>>> also fixed by this patch, although Carl's testcase doesn't fail
>>>> for x86_64.
>>>>
>>>> There seems to be a corner case where a line table has only one
>>>> instruction,
>>>> and while stepping that doesn't take the same path through
>>>> infrun. I think it
>>>> needs some more investigation. Therefore this is a tentative
>>>> patch for now.
>>>
>>> Are you (or Carl) continuing to pursue that edge case?
>>>
>>
>> Not at the moment unfortunately. I know that this needs to be handled
>> in
>> the fallthrough of process_event_stop_test. Carl's test doesn't seem
>> to
>> fail for x86_64 (at least for me). We need to polish the testcase a
>> bit
>> more so that we can cover that corner case as well.
>>
>> Also, this is more of a RFC at this point. You'll notice some debug
>> print statement in the patch. It would be nice to turn those into
>> permanent debug prints, as this code doesn't seem to print too much
>> detail about what it is doing.
> 
> OK,
> thanks.
> 
> I did notice the debug printfs.   Since they were actually using
> the infrun_debug_printf helper, versus actual printf calls,  I assumed
> they were deliberate and meant for the long term.  :-)
> I wasn't going to
> nit on the uppercase content of the debug printfs, I figure since they
> are actually debug for special circumstances the debug was fine as
> presented.  :-)

The upper case is so the text stands out from the rest of the debugging 
output. :-)

> I'm generally a fan of adding more debug output.

I think we should, in this case. Otherwise GDB will silently pick a 
different stepping range without making it clear why it did it.