[2/2,v5] Fix reverse stepping multiple contiguous PC ranges over the line table.

  Bruno, Simon, GDB maintainers:

Version 5, changed comments in test case func-map-to-same-line.c. 
Patch 1/2 implemented the new options for gdb_compile.  Updated the
call to proc run_tests to use the new gdb_compile options in a
foreach_with_prefix loop.

Version 4, additional fixes for gcc version check, wrap function calls
using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted by
Luis.

Version 3, added the gcc version check as discussed further from
version 2 of the patch.  Also updated the tests to check for supporting
reverse execution rather than requiring recording.  I also noticed
there were a couple more instances of a requirement check, i.e. if []
which I changed to "require" per the current style for checking on the
test requirements.


The following patch fixes issues on PowerPC with the reverse-step and
reverse-next instructions when there are multiple assignment statements
on the same line and when there are multiple function calls on the same
line. The commit log below discusses these issues in further depth. 
The discussion included what the correct operation should be for these
commands based on the GDB documentation.  The proposed patch at that
time changed how the commands worked on other platforms such as X86 in
a way they no longer matched the documentation.

The issue is the line table contains multiple entries for the same
source line.  The patch adds a function to search the line table to
find the address of the first instruction of a line.  When setup up the
reverse stepping range, the function is called to make sure the start
of the range corresponds to the address of the first instruction for
the line.  This approach was used.  When Luis initially developed the
patch, he considered merging the contiguous ranges in the line table
when reading the line tables. He decided it was better to work with the
data directly in the line table rather than creating and using a
modified version of the line table.

The following patch fixes the execution of the reveres-step and
reverse-next commands for both senarios of multiple statements on the
same line for PowerPC and aarch64-linux.  Unlike the previous patch, it
does not change the operation of the commands on other platforms, i.e.
X86.  The patch adds new test cases for both scenarios to verify they
work correctly.

The patch has been tested on PowerPC, Intel X86 and aarch64-linux with
no new regression failures. 

Please let me know if the patch is acceptable for mainline.  Thanks.

                   Carl

---------------------------------------------
Fix reverse stepping multiple contiguous PC ranges over the line table.

There are a couple of scenarios where the GDB reverse-step and reverse-next
commands do not work correctly.

Scenario 1 issue description by Luis Machado:

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.

Since we've reached ecs->event_thread->control.step_range_start, we stop
stepping backwards.

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

Scenario 2 issue described by Pedro Alves:

The following explanation of the issue was taken from the gdb mailing list
discussion of the withdrawn patch to change the behavior of the reverse-step
and reverse-next commands.  Specifically, message from Pedro Alves
<pedro@palves.net> where he demonstrates the issue where you have multiple
function calls on the same source code line:

https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html

The source line looks like:

   func1 ();  func2 ();

so stepping backwards over that line should always stop at the first
instruction of the line, not in the middle.  Let's simplify this.

Here's the full source code of my example:

(gdb) list 1
1       void func1 ()
2       {
3       }
4
5       void func2 ()
6       {
7       }
8
9       int main ()
10      {
11        func1 (); func2 ();
12      }

Compiled with:

 $ gcc reverse.c -o reverse -g3 -O0
 $ gcc -v
 ...
 gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)

Now let's debug it with target record, using current gdb git master (f3d8ae90b236),
without your patch:

 $ gdb ~/reverse
 GNU gdb (GDB) 14.0.50.20230124-git
 ...
 Reading symbols from /home/pedro/reverse...
 (gdb) start
 Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
 Starting program: /home/pedro/reverse
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

 Temporary breakpoint 1, main () at reverse.c:11
 11        func1 (); func2 ();
 (gdb) record

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp

 11        func1 (); func2 ();
 => 0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
    0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax

 12      }
    0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.

 (gdb) n
 12      }

So far so good, a "next" stepped over the whole of line 11 and stopped at line 12.

Let's confirm where we are now:

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp

 11        func1 (); func2 ();
    0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
    0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax

 12      }
 => 0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.

Good, we're at the first instruction of line 12.

Now let's undo the "next", with "reverse-next":

 (gdb) reverse-next
 11        func1 (); func2 ();

Seemingly stopped at line 11.  Let's see exactly where:

 (gdb) disassemble /s
 Dump of assembler code for function main:
 reverse.c:
 10      {
    0x000055555555513f <+0>:     endbr64
    0x0000555555555143 <+4>:     push   %rbp
    0x0000555555555144 <+5>:     mov    %rsp,%rbp

 11        func1 (); func2 ();
    0x0000555555555147 <+8>:     mov    $0x0,%eax
    0x000055555555514c <+13>:    call   0x555555555129 <func1>
 => 0x0000555555555151 <+18>:    mov    $0x0,%eax
    0x0000555555555156 <+23>:    call   0x555555555134 <func2>
    0x000055555555515b <+28>:    mov    $0x0,%eax

 12      }
    0x0000555555555160 <+33>:    pop    %rbp
    0x0000555555555161 <+34>:    ret
 End of assembler dump.
 (gdb)

And lo, we stopped in the middle of line 11!  That is a bug, we should have
stepped back all the way to the beginning of the line.  The "reverse-next"
should have fully undone the prior "next" command.

The above issues were fixed by introducing a new function that looks 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.  The new start PC for the range
is used for the control.step_range_start when setting up a step range.

The test case gdb.reverse/map-to-same-line.exp is added to test the fix
for the issues in scenario 1.

The test case gdb.reverse/func-map-to-same-line.exp was added to test the
fix for scenario 2 when the binary was compiled with and without line
table information.

bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426

Co-authored-by: Luis Machado <luis.machado@arm.com>
Co-authored-by: Carl Love <cel@us.ibm.com>
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
---
 gdb/infrun.c                                  |  57 +++++++
 gdb/symtab.c                                  |  49 ++++++
 gdb/symtab.h                                  |  16 ++
 .../gdb.reverse/func-map-to-same-line.c       |  36 ++++
 .../gdb.reverse/func-map-to-same-line.exp     | 140 ++++++++++++++++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
 .../gdb.reverse/map-to-same-line.exp          | 156 ++++++++++++++++++
 7 files changed, 512 insertions(+)
 create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
 create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c
 create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp
  

Ping.

Simon:

I know Simon said he was busy but wanted to look more at this patch. 
Just wondering if you have found any time to review the patch further.

Thanks

                           Carl 

On Tue, 2023-05-16 at 15:54 -0700, Carl Love wrote:
> Bruno, Simon, GDB maintainers:
> 
> Version 5, changed comments in test case func-map-to-same-line.c. 
> Patch 1/2 implemented the new options for gdb_compile.  Updated the
> call to proc run_tests to use the new gdb_compile options in a
> foreach_with_prefix loop.
> 
> Version 4, additional fixes for gcc version check, wrap function
> calls
> using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted
> by
> Luis.
> 
> Version 3, added the gcc version check as discussed further from
> version 2 of the patch.  Also updated the tests to check for
> supporting
> reverse execution rather than requiring recording.  I also noticed
> there were a couple more instances of a requirement check, i.e. if []
> which I changed to "require" per the current style for checking on
> the
> test requirements.
> 
> 
> The following patch fixes issues on PowerPC with the reverse-step and
> reverse-next instructions when there are multiple assignment
> statements
> on the same line and when there are multiple function calls on the
> same
> line. The commit log below discusses these issues in further depth. 
> The discussion included what the correct operation should be for
> these
> commands based on the GDB documentation.  The proposed patch at that
> time changed how the commands worked on other platforms such as X86
> in
> a way they no longer matched the documentation.
> 
> The issue is the line table contains multiple entries for the same
> source line.  The patch adds a function to search the line table to
> find the address of the first instruction of a line.  When setup up
> the
> reverse stepping range, the function is called to make sure the start
> of the range corresponds to the address of the first instruction for
> the line.  This approach was used.  When Luis initially developed the
> patch, he considered merging the contiguous ranges in the line table
> when reading the line tables. He decided it was better to work with
> the
> data directly in the line table rather than creating and using a
> modified version of the line table.
> 
> The following patch fixes the execution of the reveres-step and
> reverse-next commands for both senarios of multiple statements on the
> same line for PowerPC and aarch64-linux.  Unlike the previous patch,
> it
> does not change the operation of the commands on other platforms,
> i.e.
> X86.  The patch adds new test cases for both scenarios to verify they
> work correctly.
> 
> The patch has been tested on PowerPC, Intel X86 and aarch64-linux
> with
> no new regression failures. 
> 
> Please let me know if the patch is acceptable for mainline.  Thanks.
> 
>                    Carl
> 
> ---------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges over the line
> table.
> 
> There are a couple of scenarios where the GDB reverse-step and
> reverse-next
> commands do not work correctly.
> 
> Scenario 1 issue description by Luis Machado:
> 
> 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.
> 
> Since we've reached ecs->event_thread->control.step_range_start, we
> stop
> stepping backwards.
> 
> ---------------------------------------------------------
> 
> Scenario 2 issue described by Pedro Alves:
> 
> The following explanation of the issue was taken from the gdb mailing
> list
> discussion of the withdrawn patch to change the behavior of the
> reverse-step
> and reverse-next commands.  Specifically, message from Pedro Alves
> <pedro@palves.net> where he demonstrates the issue where you have
> multiple
> function calls on the same source code line:
> 
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
> 
> The source line looks like:
> 
>    func1 ();  func2 ();
> 
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
> 
> Here's the full source code of my example:
> 
> (gdb) list 1
> 1       void func1 ()
> 2       {
> 3       }
> 4
> 5       void func2 ()
> 6       {
> 7       }
> 8
> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }
> 
> Compiled with:
> 
>  $ gcc reverse.c -o reverse -g3 -O0
>  $ gcc -v
>  ...
>  gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
> 
> Now let's debug it with target record, using current gdb git master
> (f3d8ae90b236),
> without your patch:
> 
>  $ gdb ~/reverse
>  GNU gdb (GDB) 14.0.50.20230124-git
>  ...
>  Reading symbols from /home/pedro/reverse...
>  (gdb) start
>  Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
>  Starting program: /home/pedro/reverse
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-
> gnu/libthread_db.so.1".
> 
>  Temporary breakpoint 1, main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) record
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>  => 0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
>  (gdb) n
>  12      }
> 
> So far so good, a "next" stepped over the whole of line 11 and
> stopped at line 12.
> 
> Let's confirm where we are now:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>  => 0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
> Good, we're at the first instruction of line 12.
> 
> Now let's undo the "next", with "reverse-next":
> 
>  (gdb) reverse-next
>  11        func1 (); func2 ();
> 
> Seemingly stopped at line 11.  Let's see exactly where:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>  => 0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
>  (gdb)
> 
> And lo, we stopped in the middle of line 11!  That is a bug, we
> should have
> stepped back all the way to the beginning of the line.  The "reverse-
> next"
> should have fully undone the prior "next" command.
> 
> The above issues were fixed by introducing a new function that looks
> 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.  The new start PC for the
> range
> is used for the control.step_range_start when setting up a step
> range.
> 
> The test case gdb.reverse/map-to-same-line.exp is added to test the
> fix
> for the issues in scenario 1.
> 
> The test case gdb.reverse/func-map-to-same-line.exp was added to test
> the
> fix for scenario 2 when the binary was compiled with and without line
> table information.
> 
> bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426
> 
> Co-authored-by: Luis Machado <luis.machado@arm.com>
> Co-authored-by: Carl Love <cel@us.ibm.com>
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> ---
>  gdb/infrun.c                                  |  57 +++++++
>  gdb/symtab.c                                  |  49 ++++++
>  gdb/symtab.h                                  |  16 ++
>  .../gdb.reverse/func-map-to-same-line.c       |  36 ++++
>  .../gdb.reverse/func-map-to-same-line.exp     | 140 ++++++++++++++++
>  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
>  .../gdb.reverse/map-to-same-line.exp          | 156
> ++++++++++++++++++
>  7 files changed, 512 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-
> line.exp
>  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 efe2c00c489..31cd817c733 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -114,6 +114,9 @@ static struct async_event_handler
> *infrun_async_inferior_event_token;
>     Starts off as -1, indicating "never enabled/disabled".  */
>  static int infrun_is_async = -1;
>  
> +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> +					  struct
> execution_control_state *ecs);
> +
>  /* See infrun.h.  */
>  
>  void
> @@ -6769,6 +6772,25 @@ handle_signal_stop (struct
> execution_control_state *ecs)
>    process_event_stop_test (ecs);
>  }
>  
> +CORE_ADDR
> +update_line_range_start (CORE_ADDR pc, struct
> execution_control_state *ecs)
> +{
> +  /* The line table may have multiple entries for the same source
> code line.
> +     Given the PC, check the line table and return the PC that
> corresponds
> +     to the line table entry for the source line that PC is in.  */
> +  CORE_ADDR start_line_pc = ecs->event_thread-
> >control.step_range_start;
> +  gdb::optional<CORE_ADDR> real_range_start;
> +
> +  /* Call find_line_range_start to get the smallest address in the
> +     linetable for multiple Line X entries in the line table.  */
> +  real_range_start = find_line_range_start (pc);
> +
> +  if (real_range_start.has_value ())
> +    start_line_pc = *real_range_start;
> +
> +  return start_line_pc;
> +}
> +
>  /* Come here when we've got some debug event / signal we can explain
>     (IOW, not a random signal), and test whether it should cause a
>     stop, or whether we should resume the inferior (transparently).
> @@ -7570,6 +7592,28 @@ process_event_stop_test (struct
> execution_control_state *ecs)
>  
>        if (stop_pc_sal.is_stmt)
>  	{
> +	  if (execution_direction == EXEC_REVERSE)
> +	    {
> +	      /* We are stepping backwards make sure we have reached
> the
> +		 beginning of the line.  */
> +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +	      CORE_ADDR start_line_pc
> +		= update_line_range_start (stop_pc, ecs);
> +
> +	      if (stop_pc != start_line_pc)
> +		{
> +		  /* Have not reached the beginning of the source code
> line.
> +		     Set a step range.  Execution should stop in any
> function
> +		     calls we execute back into before reaching the
> beginning
> +		     of the line.  */
> +		  ecs->event_thread->control.step_range_start =
> start_line_pc;
> +		  ecs->event_thread->control.step_range_end = stop_pc;
> +		  set_step_info (ecs->event_thread, frame,
> stop_pc_sal);
> +		  keep_going (ecs);
> +		  return;
> +		}
> +	    }
> +
>  	  /* We are at the start of a statement.
>  
>  	     So stop.  Note that we don't stop if we step into the
> middle of a
> @@ -7632,6 +7676,19 @@ process_event_stop_test (struct
> execution_control_state *ecs)
>      set_step_info (ecs->event_thread, frame, stop_pc_sal);
>  
>    infrun_debug_printf ("keep going");
> +
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +
> +      /* Make sure the stop_pc is set to the beginning of the
> line.  */
> +      if (stop_pc != ecs->event_thread->control.step_range_start)
> +	{
> +	  stop_pc = update_line_range_start (stop_pc, ecs);
> +	  ecs->event_thread->control.step_range_start = stop_pc;
> +	}
> +    }
> +
>    keep_going (ecs);
>  }
>  
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 27611a34ec4..91d35616eb9 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3282,6 +3282,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 404d0ab30a8..f54305636da 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2346,6 +2346,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/func-map-to-same-line.c
> b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> new file mode 100644
> index 00000000000..da944874e86
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> @@ -0,0 +1,36 @@
> +/* Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <
> http://www.gnu.org/licenses/>.
> +
> +   This test is used to test the reverse-step and reverse-next
> instruction
> +   execution for a source line that contains multiple function
> calls.  */
> +
> +void
> +func1 ()
> +{
> +} /* END FUNC1 */
> +
> +void
> +func2 ()
> +{
> +} /* END FUNC2 */
> +
> +int main ()
> +{
> +  int a, b;
> +  a = 1;
> +  b = 2;
> +  func1 (); func2 ();
> +  a = a + b;     /* START REVERSE TEST */
> +}
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> new file mode 100644
> index 00000000000..89e226b0f84
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> @@ -0,0 +1,140 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <
> http://www.gnu.org/licenses/>.  */
> +
> +# This file is part of the GDB testsuite.  It tests reverse
> stepping.
> +# Lots of code borrowed from "step-test.exp".
> +
> +# This test checks to make sure there is no regression failures for
> +# the reverse-next command when stepping back over two functions in
> +# the same line.
> +
> +require supports_reverse
> +
> +# This test uses the gcc no-column-info command which was added in
> gcc 7.1.
> +
> +proc run_tests {} {
> +    global srcfile
> +    global executable
> +
> +    runto_main
> +    set target_remote [gdb_is_target_remote]
> +
> +    with_test_prefix "test1" {
> +	gdb_test_no_output "record" "turn on process record"
> +    }
> +
> +    # This regression test verifies the reverse-step and reverse-
> next commands
> +    # work properly when executing backwards thru a source line
> containing
> +    # two function calls on the same source line, i.e. func1 ();
> func2 ();
> +    # This test is compiled so the dwarf info not contain the line
> table
> +    # information.
> +
> +    # Test 1, reverse-next command
> +    # Set breakpoint at the line after the function calls.
> +    set bp_start_reverse_test [gdb_get_line_number "START REVERSE
> TEST" \
> +				   $srcfile]
> +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> +
> +    # Continue to break point for reverse-next test.
> +    # Command definition:  reverse-next [count]
> +    #   Run backward to the beginning of the previous line executed
> in the
> +    #   current (innermost) stack frame. If the line contains
> function calls,
> +    #   they will be “un-executed” without stopping. Starting from
> the first
> +    #   line of a function, reverse-next will take you back to the
> caller of
> +    #   that function, before the function was called, just as the
> normal next
> +    #   command would take you from the last line of a function back
> to its
> +    #   return to its caller 2 .
> +    gdb_continue_to_breakpoint \
> +	"test1: stopped at command reverse-next test start location" \
> +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> +
> +    # The reverse-next should step all the way back to the beginning
> of the
> +    # line, i.e. at the beginning of the func1 call.
> +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	"test1: reverse-next to line with two functions"
> +
> +    # We should be stopped at the first instruction of the line. A
> reverse-step
> +    # should step back and stop at the beginning of the previous
> line b = 2,
> +    # i.e. not in func1 ().
> +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	"test1: reverse-stepi to previous line b = 2"
> +
> +
> +    # Setup for test 2
> +    clean_restart $executable
> +    runto_main
> +
> +    with_test_prefix "test2" {
> +	gdb_test_no_output "record" "turn on process record"
> +    }
> +
> +    # Test 2, reverse-step command
> +    # Set breakpoint at the line after the function calls.
> +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> +
> +    #  Continue to the start of the reverse-step test.
> +    #  Command definition:  reverse-step [count]
> +    #    Run the program backward until control reaches the start of
> a
> +    #    different source line; then stop it, and return control to
> gdb.
> +    #    Like the step command, reverse-step will only stop at the
> beginning
> +    #    of a source line. It “un-executes” the previously executed
> source
> +    #    line. If the previous source line included calls to
> debuggable
> +    #    functions, reverse-step will step (backward) into the
> called function,
> +    #    stopping at the beginning of the last statement in the
> called
> +    #    function (typically a return statement).  Also, as with the
> step
> +    #    command, if non-debuggable functions are called, reverse-
> step will
> +    #    run thru them backward without stopping.
> +
> +    gdb_continue_to_breakpoint \
> +	"test2: stopped at command reverse-step test start location" \
> +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> +
> +    # The first reverse step should take us call of func2 ().
> +    gdb_test "reverse-step" ".*END FUNC2.*" \
> +	"test2: reverse-step into func2 "
> +
> +    # The second reverse step should take us into func1 ().
> +    gdb_test "reverse-step" ".*END FUNC1.*" \
> +	"test2: reverse-step into func1 "
> +
> +    # The third reverse step should take us call of func1 ().
> +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	"test2: reverse-step to line func1(); func2(), at call for
> func1 "
> +
> +    # We should be stopped at the first instruction of the line. A
> reverse
> +    # stepi should take us to b = 2 ().
> +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	"test2: reverse-stepi to line b = 2 "
> +}
> +
> +set srcfile  func-map-to-same-line.c
> +set executable func-map-to-same-line
> +
> +# test with and without gcc column info enabled
> +foreach_with_prefix with_column_info {yes no} {
> +    if {$with_column_info == "yes"} {
> +	set options [list debug column-info]
> +    } else {
> +	set options [list debug no-column-info]
> +    }
> +
> +    if {[build_executable "failed to prepare" $executable $srcfile \
> +	     $options] == -1} {
> +	return -1
> +    }
> +
> +    clean_restart $executable
> +    run_tests
> +}
> 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..f20d778f40e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,58 @@
> +/* Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <
> http://www.gnu.org/licenses/
>  >.  */
> +
> +/* The purpose of this test is to create a DWARF line table that
> contains two
> +   or more entries for the same line.  When stepping (forwards or
> backwards),
> +   GDB should step over the entire line and not just a particular
> entry in the
> +   line table.  */
> +
> +int
> +main ()
> +{     /* TAG: main prologue */
> +  asm ("main_label: .globl main_label");
> +  int i = 1, j = 2, k;
> +  float f1 = 2.0, f2 = 4.1, f3;
> +  const char *str_1 = "foo", *str_2 = "bar", *str_3;
> +
> +  asm ("line1: .globl line1");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
> +
> +  asm ("line2: .globl line2");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
> +
> +  asm ("line3: .globl line3");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
> +
> +  asm ("line4: .globl line4");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
> +
> +  asm ("line5: .globl line5");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
> +
> +  asm ("line6: .globl line6");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
> +
> +  asm ("line7: .globl line7");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
> +
> +  asm ("line8: .globl line8");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
> +
> +  asm ("main_return: .globl main_return");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
> +
> +  asm ("end_of_sequence: .globl end_of_sequence");
> +  return 0; /* TAG: main return */
> +}
> 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..16a359d90ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,156 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <
> http://www.gnu.org/licenses/
>  >.
> +
> +# When stepping (forwards or backwards), GDB should step over the
> entire line
> +# and not just a particular entry in the line table. This test was
> added to
> +# verify the find_line_range_start function properly sets the step
> range for a
> +# line that consists of multiple statements, i.e. multiple entries
> in the line
> +# table.  This test creates a DWARF line table that contains two
> entries for
> +# the same line to do the needed testing.
> +
> +# This test can only be run on targets which support DWARF-2 and use
> gas.
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +# The DWARF assembler requires the gcc compiler.
> +require is_c_compiler_gcc
> +
> +# This test suitable only for process that can do reverse execution
> +require supports_reverse
> +
> +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 distinct source lines
> being
> +	# mapped to the same line entry. Line 1, 5 and 8 contain 1
> statement
> +	# each.  Line 2 contains 2 statements.  Line 3 contains 3
> statements.
> +	program {
> +	    DW_LNE_set_address $main_start
> +	    line [gdb_get_line_number "TAG: main prologue"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line1
> +	    line [gdb_get_line_number "TAG: line 1" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line2
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line3
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line4
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line5
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line6
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line7
> +	    line [gdb_get_line_number "TAG: line 5" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line8
> +	    line [gdb_get_line_number "TAG: line 8" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address main_return
> +	    line [gdb_get_line_number "TAG: main return"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address end_of_sequence
> +	    DW_LNE_end_sequence
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +runto_main
> +
> +# Print the line table
> +gdb_test_multiple "maint info line-table ${testfile}" "" {
> +    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[
> \t\]+Y\[^\r\n\]*" {
> +	lappend is_stmt $expect_out(1,string)
> +	exp_continue
> +    }
> +    -re -wrap "" {
> +    }
> +}
> +
> +# Do the reverse-step test
> +gdb_test_no_output "record" "turn on process record"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-step test"
> ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
> +
> +# At this point, GDB has already recorded the execution up until the
> return
> +# statement.  Reverse-step and test if GDB transitions between lines
> in the
> +# expected order.  It should reverse-step across lines 8, 5, 3, 2
> and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to
> line $line"
> +}
> +
> +## Clean restart, test reverse-next command
> +clean_restart ${testfile}
> +runto_main
> +gdb_test_no_output "record" "turn on process record, reverst-next
> test"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-next test"
> ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
> +
> +# At this point, GDB has already recorded the execution up until the
> return
> +# statement.  Reverse-next and test if GDB transitions between lines
> in the
> +# expected order.  It should reverse-next across lines 8, 5, 3, 2
> and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to
> line $line"
> +}
  

Ping.  Hoping Simon can take a look at the patch.  Thanks.

             Carl 

On Thu, 2023-05-25 at 08:08 -0700, Carl Love wrote:
> Ping.
> 
> Simon:
> 
> I know Simon said he was busy but wanted to look more at this patch. 
> Just wondering if you have found any time to review the patch
> further.
> 
> Thanks
> 
>                            Carl 
> 
> On Tue, 2023-05-16 at 15:54 -0700, Carl Love wrote:
> > Bruno, Simon, GDB maintainers:
> > 
> > Version 5, changed comments in test case func-map-to-same-line.c. 
> > Patch 1/2 implemented the new options for gdb_compile.  Updated the
> > call to proc run_tests to use the new gdb_compile options in a
> > foreach_with_prefix loop.
> > 
> > Version 4, additional fixes for gcc version check, wrap function
> > calls
> > using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted
> > by
> > Luis.
> > 
> > Version 3, added the gcc version check as discussed further from
> > version 2 of the patch.  Also updated the tests to check for
> > supporting
> > reverse execution rather than requiring recording.  I also noticed
> > there were a couple more instances of a requirement check, i.e. if
> > []
> > which I changed to "require" per the current style for checking on
> > the
> > test requirements.
> > 
> > 
> > The following patch fixes issues on PowerPC with the reverse-step
> > and
> > reverse-next instructions when there are multiple assignment
> > statements
> > on the same line and when there are multiple function calls on the
> > same
> > line. The commit log below discusses these issues in further
> > depth. 
> > The discussion included what the correct operation should be for
> > these
> > commands based on the GDB documentation.  The proposed patch at
> > that
> > time changed how the commands worked on other platforms such as X86
> > in
> > a way they no longer matched the documentation.
> > 
> > The issue is the line table contains multiple entries for the same
> > source line.  The patch adds a function to search the line table to
> > find the address of the first instruction of a line.  When setup up
> > the
> > reverse stepping range, the function is called to make sure the
> > start
> > of the range corresponds to the address of the first instruction
> > for
> > the line.  This approach was used.  When Luis initially developed
> > the
> > patch, he considered merging the contiguous ranges in the line
> > table
> > when reading the line tables. He decided it was better to work with
> > the
> > data directly in the line table rather than creating and using a
> > modified version of the line table.
> > 
> > The following patch fixes the execution of the reveres-step and
> > reverse-next commands for both senarios of multiple statements on
> > the
> > same line for PowerPC and aarch64-linux.  Unlike the previous
> > patch,
> > it
> > does not change the operation of the commands on other platforms,
> > i.e.
> > X86.  The patch adds new test cases for both scenarios to verify
> > they
> > work correctly.
> > 
> > The patch has been tested on PowerPC, Intel X86 and aarch64-linux
> > with
> > no new regression failures. 
> > 
> > Please let me know if the patch is acceptable for
> > mainline.  Thanks.
> > 
> >                    Carl
> > 
> > ---------------------------------------------
> > Fix reverse stepping multiple contiguous PC ranges over the line
> > table.
> > 
> > There are a couple of scenarios where the GDB reverse-step and
> > reverse-next
> > commands do not work correctly.
> > 
> > Scenario 1 issue description by Luis Machado:
> > 
> > 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.
> > 
> > Since we've reached ecs->event_thread->control.step_range_start, we
> > stop
> > stepping backwards.
> > 
> > ---------------------------------------------------------
> > 
> > Scenario 2 issue described by Pedro Alves:
> > 
> > The following explanation of the issue was taken from the gdb
> > mailing
> > list
> > discussion of the withdrawn patch to change the behavior of the
> > reverse-step
> > and reverse-next commands.  Specifically, message from Pedro Alves
> > <pedro@palves.net> where he demonstrates the issue where you have
> > multiple
> > function calls on the same source code line:
> > 
> > https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
> > 
> > The source line looks like:
> > 
> >    func1 ();  func2 ();
> > 
> > so stepping backwards over that line should always stop at the
> > first
> > instruction of the line, not in the middle.  Let's simplify this.
> > 
> > Here's the full source code of my example:
> > 
> > (gdb) list 1
> > 1       void func1 ()
> > 2       {
> > 3       }
> > 4
> > 5       void func2 ()
> > 6       {
> > 7       }
> > 8
> > 9       int main ()
> > 10      {
> > 11        func1 (); func2 ();
> > 12      }
> > 
> > Compiled with:
> > 
> >  $ gcc reverse.c -o reverse -g3 -O0
> >  $ gcc -v
> >  ...
> >  gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
> > 
> > Now let's debug it with target record, using current gdb git master
> > (f3d8ae90b236),
> > without your patch:
> > 
> >  $ gdb ~/reverse
> >  GNU gdb (GDB) 14.0.50.20230124-git
> >  ...
> >  Reading symbols from /home/pedro/reverse...
> >  (gdb) start
> >  Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
> >  Starting program: /home/pedro/reverse
> >  [Thread debugging using libthread_db enabled]
> >  Using host libthread_db library "/lib/x86_64-linux-
> > gnu/libthread_db.so.1".
> > 
> >  Temporary breakpoint 1, main () at reverse.c:11
> >  11        func1 (); func2 ();
> >  (gdb) record
> > 
> >  (gdb) disassemble /s
> >  Dump of assembler code for function main:
> >  reverse.c:
> >  10      {
> >     0x000055555555513f <+0>:     endbr64
> >     0x0000555555555143 <+4>:     push   %rbp
> >     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> > 
> >  11        func1 (); func2 ();
> >  => 0x0000555555555147 <+8>:     mov    $0x0,%eax
> >     0x000055555555514c <+13>:    call   0x555555555129 <func1>
> >     0x0000555555555151 <+18>:    mov    $0x0,%eax
> >     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
> >     0x000055555555515b <+28>:    mov    $0x0,%eax
> > 
> >  12      }
> >     0x0000555555555160 <+33>:    pop    %rbp
> >     0x0000555555555161 <+34>:    ret
> >  End of assembler dump.
> > 
> >  (gdb) n
> >  12      }
> > 
> > So far so good, a "next" stepped over the whole of line 11 and
> > stopped at line 12.
> > 
> > Let's confirm where we are now:
> > 
> >  (gdb) disassemble /s
> >  Dump of assembler code for function main:
> >  reverse.c:
> >  10      {
> >     0x000055555555513f <+0>:     endbr64
> >     0x0000555555555143 <+4>:     push   %rbp
> >     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> > 
> >  11        func1 (); func2 ();
> >     0x0000555555555147 <+8>:     mov    $0x0,%eax
> >     0x000055555555514c <+13>:    call   0x555555555129 <func1>
> >     0x0000555555555151 <+18>:    mov    $0x0,%eax
> >     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
> >     0x000055555555515b <+28>:    mov    $0x0,%eax
> > 
> >  12      }
> >  => 0x0000555555555160 <+33>:    pop    %rbp
> >     0x0000555555555161 <+34>:    ret
> >  End of assembler dump.
> > 
> > Good, we're at the first instruction of line 12.
> > 
> > Now let's undo the "next", with "reverse-next":
> > 
> >  (gdb) reverse-next
> >  11        func1 (); func2 ();
> > 
> > Seemingly stopped at line 11.  Let's see exactly where:
> > 
> >  (gdb) disassemble /s
> >  Dump of assembler code for function main:
> >  reverse.c:
> >  10      {
> >     0x000055555555513f <+0>:     endbr64
> >     0x0000555555555143 <+4>:     push   %rbp
> >     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> > 
> >  11        func1 (); func2 ();
> >     0x0000555555555147 <+8>:     mov    $0x0,%eax
> >     0x000055555555514c <+13>:    call   0x555555555129 <func1>
> >  => 0x0000555555555151 <+18>:    mov    $0x0,%eax
> >     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
> >     0x000055555555515b <+28>:    mov    $0x0,%eax
> > 
> >  12      }
> >     0x0000555555555160 <+33>:    pop    %rbp
> >     0x0000555555555161 <+34>:    ret
> >  End of assembler dump.
> >  (gdb)
> > 
> > And lo, we stopped in the middle of line 11!  That is a bug, we
> > should have
> > stepped back all the way to the beginning of the line.  The
> > "reverse-
> > next"
> > should have fully undone the prior "next" command.
> > 
> > The above issues were fixed by introducing a new function that
> > looks
> > 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.  The new start PC for
> > the
> > range
> > is used for the control.step_range_start when setting up a step
> > range.
> > 
> > The test case gdb.reverse/map-to-same-line.exp is added to test the
> > fix
> > for the issues in scenario 1.
> > 
> > The test case gdb.reverse/func-map-to-same-line.exp was added to
> > test
> > the
> > fix for scenario 2 when the binary was compiled with and without
> > line
> > table information.
> > 
> > bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426
> > 
> > Co-authored-by: Luis Machado <luis.machado@arm.com>
> > Co-authored-by: Carl Love <cel@us.ibm.com>
> > Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> > ---
> >  gdb/infrun.c                                  |  57 +++++++
> >  gdb/symtab.c                                  |  49 ++++++
> >  gdb/symtab.h                                  |  16 ++
> >  .../gdb.reverse/func-map-to-same-line.c       |  36 ++++
> >  .../gdb.reverse/func-map-to-same-line.exp     | 140
> > ++++++++++++++++
> >  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
> >  .../gdb.reverse/map-to-same-line.exp          | 156
> > ++++++++++++++++++
> >  7 files changed, 512 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-
> > line.c
> >  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-
> > line.exp
> >  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 efe2c00c489..31cd817c733 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -114,6 +114,9 @@ static struct async_event_handler
> > *infrun_async_inferior_event_token;
> >     Starts off as -1, indicating "never enabled/disabled".  */
> >  static int infrun_is_async = -1;
> >  
> > +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> > +					  struct
> > execution_control_state *ecs);
> > +
> >  /* See infrun.h.  */
> >  
> >  void
> > @@ -6769,6 +6772,25 @@ handle_signal_stop (struct
> > execution_control_state *ecs)
> >    process_event_stop_test (ecs);
> >  }
> >  
> > +CORE_ADDR
> > +update_line_range_start (CORE_ADDR pc, struct
> > execution_control_state *ecs)
> > +{
> > +  /* The line table may have multiple entries for the same source
> > code line.
> > +     Given the PC, check the line table and return the PC that
> > corresponds
> > +     to the line table entry for the source line that PC is
> > in.  */
> > +  CORE_ADDR start_line_pc = ecs->event_thread-
> > > control.step_range_start;
> > +  gdb::optional<CORE_ADDR> real_range_start;
> > +
> > +  /* Call find_line_range_start to get the smallest address in the
> > +     linetable for multiple Line X entries in the line table.  */
> > +  real_range_start = find_line_range_start (pc);
> > +
> > +  if (real_range_start.has_value ())
> > +    start_line_pc = *real_range_start;
> > +
> > +  return start_line_pc;
> > +}
> > +
> >  /* Come here when we've got some debug event / signal we can
> > explain
> >     (IOW, not a random signal), and test whether it should cause a
> >     stop, or whether we should resume the inferior (transparently).
> > @@ -7570,6 +7592,28 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >  
> >        if (stop_pc_sal.is_stmt)
> >  	{
> > +	  if (execution_direction == EXEC_REVERSE)
> > +	    {
> > +	      /* We are stepping backwards make sure we have reached
> > the
> > +		 beginning of the line.  */
> > +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +	      CORE_ADDR start_line_pc
> > +		= update_line_range_start (stop_pc, ecs);
> > +
> > +	      if (stop_pc != start_line_pc)
> > +		{
> > +		  /* Have not reached the beginning of the source code
> > line.
> > +		     Set a step range.  Execution should stop in any
> > function
> > +		     calls we execute back into before reaching the
> > beginning
> > +		     of the line.  */
> > +		  ecs->event_thread->control.step_range_start =
> > start_line_pc;
> > +		  ecs->event_thread->control.step_range_end = stop_pc;
> > +		  set_step_info (ecs->event_thread, frame,
> > stop_pc_sal);
> > +		  keep_going (ecs);
> > +		  return;
> > +		}
> > +	    }
> > +
> >  	  /* We are at the start of a statement.
> >  
> >  	     So stop.  Note that we don't stop if we step into the
> > middle of a
> > @@ -7632,6 +7676,19 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >      set_step_info (ecs->event_thread, frame, stop_pc_sal);
> >  
> >    infrun_debug_printf ("keep going");
> > +
> > +  if (execution_direction == EXEC_REVERSE)
> > +    {
> > +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +
> > +      /* Make sure the stop_pc is set to the beginning of the
> > line.  */
> > +      if (stop_pc != ecs->event_thread->control.step_range_start)
> > +	{
> > +	  stop_pc = update_line_range_start (stop_pc, ecs);
> > +	  ecs->event_thread->control.step_range_start = stop_pc;
> > +	}
> > +    }
> > +
> >    keep_going (ecs);
> >  }
> >  
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 27611a34ec4..91d35616eb9 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3282,6 +3282,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 404d0ab30a8..f54305636da 100644
> > --- a/gdb/symtab.h
> > +++ b/gdb/symtab.h
> > @@ -2346,6 +2346,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/func-map-to-same-line.c
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > new file mode 100644
> > index 00000000000..da944874e86
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > @@ -0,0 +1,36 @@
> > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published
> > by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/>;.
> > +
> > +   This test is used to test the reverse-step and reverse-next
> > instruction
> > +   execution for a source line that contains multiple function
> > calls.  */
> > +
> > +void
> > +func1 ()
> > +{
> > +} /* END FUNC1 */
> > +
> > +void
> > +func2 ()
> > +{
> > +} /* END FUNC2 */
> > +
> > +int main ()
> > +{
> > +  int a, b;
> > +  a = 1;
> > +  b = 2;
> > +  func1 (); func2 ();
> > +  a = a + b;     /* START REVERSE TEST */
> > +}
> > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > new file mode 100644
> > index 00000000000..89e226b0f84
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > @@ -0,0 +1,140 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published
> > by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/>.  */
> > +
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# This test checks to make sure there is no regression failures
> > for
> > +# the reverse-next command when stepping back over two functions
> > in
> > +# the same line.
> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +
> > +proc run_tests {} {
> > +    global srcfile
> > +    global executable
> > +
> > +    runto_main
> > +    set target_remote [gdb_is_target_remote]
> > +
> > +    with_test_prefix "test1" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> > +
> > +    # This regression test verifies the reverse-step and reverse-
> > next commands
> > +    # work properly when executing backwards thru a source line
> > containing
> > +    # two function calls on the same source line, i.e. func1 ();
> > func2 ();
> > +    # This test is compiled so the dwarf info not contain the line
> > table
> > +    # information.
> > +
> > +    # Test 1, reverse-next command
> > +    # Set breakpoint at the line after the function calls.
> > +    set bp_start_reverse_test [gdb_get_line_number "START REVERSE
> > TEST" \
> > +				   $srcfile]
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    # Continue to break point for reverse-next test.
> > +    # Command definition:  reverse-next [count]
> > +    #   Run backward to the beginning of the previous line
> > executed
> > in the
> > +    #   current (innermost) stack frame. If the line contains
> > function calls,
> > +    #   they will be “un-executed” without stopping. Starting from
> > the first
> > +    #   line of a function, reverse-next will take you back to the
> > caller of
> > +    #   that function, before the function was called, just as the
> > normal next
> > +    #   command would take you from the last line of a function
> > back
> > to its
> > +    #   return to its caller 2 .
> > +    gdb_continue_to_breakpoint \
> > +	"test1: stopped at command reverse-next test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The reverse-next should step all the way back to the
> > beginning
> > of the
> > +    # line, i.e. at the beginning of the func1 call.
> > +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test1: reverse-next to line with two functions"
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse-step
> > +    # should step back and stop at the beginning of the previous
> > line b = 2,
> > +    # i.e. not in func1 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test1: reverse-stepi to previous line b = 2"
> > +
> > +
> > +    # Setup for test 2
> > +    clean_restart $executable
> > +    runto_main
> > +
> > +    with_test_prefix "test2" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> > +
> > +    # Test 2, reverse-step command
> > +    # Set breakpoint at the line after the function calls.
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    #  Continue to the start of the reverse-step test.
> > +    #  Command definition:  reverse-step [count]
> > +    #    Run the program backward until control reaches the start
> > of
> > a
> > +    #    different source line; then stop it, and return control
> > to
> > gdb.
> > +    #    Like the step command, reverse-step will only stop at the
> > beginning
> > +    #    of a source line. It “un-executes” the previously
> > executed
> > source
> > +    #    line. If the previous source line included calls to
> > debuggable
> > +    #    functions, reverse-step will step (backward) into the
> > called function,
> > +    #    stopping at the beginning of the last statement in the
> > called
> > +    #    function (typically a return statement).  Also, as with
> > the
> > step
> > +    #    command, if non-debuggable functions are called, reverse-
> > step will
> > +    #    run thru them backward without stopping.
> > +
> > +    gdb_continue_to_breakpoint \
> > +	"test2: stopped at command reverse-step test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The first reverse step should take us call of func2 ().
> > +    gdb_test "reverse-step" ".*END FUNC2.*" \
> > +	"test2: reverse-step into func2 "
> > +
> > +    # The second reverse step should take us into func1 ().
> > +    gdb_test "reverse-step" ".*END FUNC1.*" \
> > +	"test2: reverse-step into func1 "
> > +
> > +    # The third reverse step should take us call of func1 ().
> > +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test2: reverse-step to line func1(); func2(), at call for
> > func1 "
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse
> > +    # stepi should take us to b = 2 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test2: reverse-stepi to line b = 2 "
> > +}
> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> > +
> > +# test with and without gcc column info enabled
> > +foreach_with_prefix with_column_info {yes no} {
> > +    if {$with_column_info == "yes"} {
> > +	set options [list debug column-info]
> > +    } else {
> > +	set options [list debug no-column-info]
> > +    }
> > +
> > +    if {[build_executable "failed to prepare" $executable $srcfile
> > \
> > +	     $options] == -1} {
> > +	return -1
> > +    }
> > +
> > +    clean_restart $executable
> > +    run_tests
> > +}
> > 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..f20d778f40e
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> > @@ -0,0 +1,58 @@
> > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published
> > by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.  */
> > +
> > +/* The purpose of this test is to create a DWARF line table that
> > contains two
> > +   or more entries for the same line.  When stepping (forwards or
> > backwards),
> > +   GDB should step over the entire line and not just a particular
> > entry in the
> > +   line table.  */
> > +
> > +int
> > +main ()
> > +{     /* TAG: main prologue */
> > +  asm ("main_label: .globl main_label");
> > +  int i = 1, j = 2, k;
> > +  float f1 = 2.0, f2 = 4.1, f3;
> > +  const char *str_1 = "foo", *str_2 = "bar", *str_3;
> > +
> > +  asm ("line1: .globl line1");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
> > +
> > +  asm ("line2: .globl line2");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
> > +
> > +  asm ("line3: .globl line3");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
> > +
> > +  asm ("line4: .globl line4");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
> > +
> > +  asm ("line5: .globl line5");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
> > +
> > +  asm ("line6: .globl line6");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
> > +
> > +  asm ("line7: .globl line7");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
> > +
> > +  asm ("line8: .globl line8");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
> > +
> > +  asm ("main_return: .globl main_return");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
> > +
> > +  asm ("end_of_sequence: .globl end_of_sequence");
> > +  return 0; /* TAG: main return */
> > +}
> > 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..16a359d90ec
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> > @@ -0,0 +1,156 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published
> > by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.
> > +
> > +# When stepping (forwards or backwards), GDB should step over the
> > entire line
> > +# and not just a particular entry in the line table. This test was
> > added to
> > +# verify the find_line_range_start function properly sets the step
> > range for a
> > +# line that consists of multiple statements, i.e. multiple entries
> > in the line
> > +# table.  This test creates a DWARF line table that contains two
> > entries for
> > +# the same line to do the needed testing.
> > +
> > +# This test can only be run on targets which support DWARF-2 and
> > use
> > gas.
> > +load_lib dwarf.exp
> > +require dwarf2_support
> > +
> > +# The DWARF assembler requires the gcc compiler.
> > +require is_c_compiler_gcc
> > +
> > +# This test suitable only for process that can do reverse
> > execution
> > +require supports_reverse
> > +
> > +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 distinct source lines
> > being
> > +	# mapped to the same line entry. Line 1, 5 and 8 contain 1
> > statement
> > +	# each.  Line 2 contains 2 statements.  Line 3 contains 3
> > statements.
> > +	program {
> > +	    DW_LNE_set_address $main_start
> > +	    line [gdb_get_line_number "TAG: main prologue"]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line1
> > +	    line [gdb_get_line_number "TAG: line 1" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line2
> > +	    line [gdb_get_line_number "TAG: line 2" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line3
> > +	    line [gdb_get_line_number "TAG: line 2" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line4
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line5
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line6
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line7
> > +	    line [gdb_get_line_number "TAG: line 5" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line8
> > +	    line [gdb_get_line_number "TAG: line 8" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address main_return
> > +	    line [gdb_get_line_number "TAG: main return"]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address end_of_sequence
> > +	    DW_LNE_end_sequence
> > +	}
> > +    }
> > +}
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} \
> > +	[list $srcfile $asm_file] {nodebug} ] } {
> > +    return -1
> > +}
> > +
> > +runto_main
> > +
> > +# Print the line table
> > +gdb_test_multiple "maint info line-table ${testfile}" "" {
> > +    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[
> > \t\]+Y\[^\r\n\]*" {
> > +	lappend is_stmt $expect_out(1,string)
> > +	exp_continue
> > +    }
> > +    -re -wrap "" {
> > +    }
> > +}
> > +
> > +# Do the reverse-step test
> > +gdb_test_no_output "record" "turn on process record"
> > +
> > +set bp_main_return [gdb_get_line_number "TAG: main return"
> > $srcfile]
> > +gdb_breakpoint $srcfile:$bp_main_return
> > +gdb_continue_to_breakpoint  "run to end of main, reverse-step
> > test"
> > ".*$srcfile:$bp_main_return.*"
> > +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
> > +
> > +# At this point, GDB has already recorded the execution up until
> > the
> > return
> > +# statement.  Reverse-step and test if GDB transitions between
> > lines
> > in the
> > +# expected order.  It should reverse-step across lines 8, 5, 3, 2
> > and 1.
> > +foreach line {8 5 3 2 1} {
> > +    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to
> > line $line"
> > +}
> > +
> > +## Clean restart, test reverse-next command
> > +clean_restart ${testfile}
> > +runto_main
> > +gdb_test_no_output "record" "turn on process record, reverst-next
> > test"
> > +
> > +set bp_main_return [gdb_get_line_number "TAG: main return"
> > $srcfile]
> > +gdb_breakpoint $srcfile:$bp_main_return
> > +gdb_continue_to_breakpoint  "run to end of main, reverse-next
> > test"
> > ".*$srcfile:$bp_main_return.*"
> > +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
> > +
> > +# At this point, GDB has already recorded the execution up until
> > the
> > return
> > +# statement.  Reverse-next and test if GDB transitions between
> > lines
> > in the
> > +# expected order.  It should reverse-next across lines 8, 5, 3, 2
> > and 1.
> > +foreach line {8 5 3 2 1} {
> > +    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to
> > line $line"
> > +}
> 
>
  

On 5/16/23 18:54, Carl Love wrote:
> Bruno, Simon, GDB maintainers:
> 
> Version 5, changed comments in test case func-map-to-same-line.c. 
> Patch 1/2 implemented the new options for gdb_compile.  Updated the
> call to proc run_tests to use the new gdb_compile options in a
> foreach_with_prefix loop.
> 
> Version 4, additional fixes for gcc version check, wrap function calls
> using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted by
> Luis.
> 
> Version 3, added the gcc version check as discussed further from
> version 2 of the patch.  Also updated the tests to check for supporting
> reverse execution rather than requiring recording.  I also noticed
> there were a couple more instances of a requirement check, i.e. if []
> which I changed to "require" per the current style for checking on the
> test requirements.
> 
> 
> The following patch fixes issues on PowerPC with the reverse-step and
> reverse-next instructions when there are multiple assignment statements
> on the same line and when there are multiple function calls on the same
> line. The commit log below discusses these issues in further depth. 
> The discussion included what the correct operation should be for these
> commands based on the GDB documentation.  The proposed patch at that
> time changed how the commands worked on other platforms such as X86 in
> a way they no longer matched the documentation.
> 
> The issue is the line table contains multiple entries for the same
> source line.  The patch adds a function to search the line table to
> find the address of the first instruction of a line.  When setup up the
> reverse stepping range, the function is called to make sure the start
> of the range corresponds to the address of the first instruction for
> the line.  This approach was used.  When Luis initially developed the
> patch, he considered merging the contiguous ranges in the line table
> when reading the line tables. He decided it was better to work with the
> data directly in the line table rather than creating and using a
> modified version of the line table.
> 
> The following patch fixes the execution of the reveres-step and
> reverse-next commands for both senarios of multiple statements on the
> same line for PowerPC and aarch64-linux.  Unlike the previous patch, it
> does not change the operation of the commands on other platforms, i.e.
> X86.  The patch adds new test cases for both scenarios to verify they
> work correctly.
> 
> The patch has been tested on PowerPC, Intel X86 and aarch64-linux with
> no new regression failures. 
> 
> Please let me know if the patch is acceptable for mainline.  Thanks.
> 
>                    Carl
> 
> ---------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges over the line table.
> 
> There are a couple of scenarios where the GDB reverse-step and reverse-next
> commands do not work correctly.
> 
> Scenario 1 issue description by Luis Machado:
> 
> 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.
> 
> Since we've reached ecs->event_thread->control.step_range_start, we stop
> stepping backwards.
> 
> ---------------------------------------------------------
> 
> Scenario 2 issue described by Pedro Alves:
> 
> The following explanation of the issue was taken from the gdb mailing list
> discussion of the withdrawn patch to change the behavior of the reverse-step
> and reverse-next commands.  Specifically, message from Pedro Alves
> <pedro@palves.net> where he demonstrates the issue where you have multiple
> function calls on the same source code line:
> 
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
> 
> The source line looks like:
> 
>    func1 ();  func2 ();
> 
> so stepping backwards over that line should always stop at the first
> instruction of the line, not in the middle.  Let's simplify this.
> 
> Here's the full source code of my example:
> 
> (gdb) list 1
> 1       void func1 ()
> 2       {
> 3       }
> 4
> 5       void func2 ()
> 6       {
> 7       }
> 8
> 9       int main ()
> 10      {
> 11        func1 (); func2 ();
> 12      }
> 
> Compiled with:
> 
>  $ gcc reverse.c -o reverse -g3 -O0
>  $ gcc -v
>  ...
>  gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
> 
> Now let's debug it with target record, using current gdb git master (f3d8ae90b236),
> without your patch:
> 
>  $ gdb ~/reverse
>  GNU gdb (GDB) 14.0.50.20230124-git
>  ...
>  Reading symbols from /home/pedro/reverse...
>  (gdb) start
>  Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
>  Starting program: /home/pedro/reverse
>  [Thread debugging using libthread_db enabled]
>  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> 
>  Temporary breakpoint 1, main () at reverse.c:11
>  11        func1 (); func2 ();
>  (gdb) record
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>  => 0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
>  (gdb) n
>  12      }
> 
> So far so good, a "next" stepped over the whole of line 11 and stopped at line 12.
> 
> Let's confirm where we are now:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>     0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>  => 0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
> 
> Good, we're at the first instruction of line 12.
> 
> Now let's undo the "next", with "reverse-next":
> 
>  (gdb) reverse-next
>  11        func1 (); func2 ();
> 
> Seemingly stopped at line 11.  Let's see exactly where:
> 
>  (gdb) disassemble /s
>  Dump of assembler code for function main:
>  reverse.c:
>  10      {
>     0x000055555555513f <+0>:     endbr64
>     0x0000555555555143 <+4>:     push   %rbp
>     0x0000555555555144 <+5>:     mov    %rsp,%rbp
> 
>  11        func1 (); func2 ();
>     0x0000555555555147 <+8>:     mov    $0x0,%eax
>     0x000055555555514c <+13>:    call   0x555555555129 <func1>
>  => 0x0000555555555151 <+18>:    mov    $0x0,%eax
>     0x0000555555555156 <+23>:    call   0x555555555134 <func2>
>     0x000055555555515b <+28>:    mov    $0x0,%eax
> 
>  12      }
>     0x0000555555555160 <+33>:    pop    %rbp
>     0x0000555555555161 <+34>:    ret
>  End of assembler dump.
>  (gdb)
> 
> And lo, we stopped in the middle of line 11!  That is a bug, we should have
> stepped back all the way to the beginning of the line.  The "reverse-next"
> should have fully undone the prior "next" command.
> 
> The above issues were fixed by introducing a new function that looks 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.  The new start PC for the range
> is used for the control.step_range_start when setting up a step range.
> 
> The test case gdb.reverse/map-to-same-line.exp is added to test the fix
> for the issues in scenario 1.
> 
> The test case gdb.reverse/func-map-to-same-line.exp was added to test the
> fix for scenario 2 when the binary was compiled with and without line
> table information.
> 
> bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426
> 
> Co-authored-by: Luis Machado <luis.machado@arm.com>
> Co-authored-by: Carl Love <cel@us.ibm.com>
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> ---
>  gdb/infrun.c                                  |  57 +++++++
>  gdb/symtab.c                                  |  49 ++++++
>  gdb/symtab.h                                  |  16 ++
>  .../gdb.reverse/func-map-to-same-line.c       |  36 ++++
>  .../gdb.reverse/func-map-to-same-line.exp     | 140 ++++++++++++++++
>  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
>  .../gdb.reverse/map-to-same-line.exp          | 156 ++++++++++++++++++
>  7 files changed, 512 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
>  create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
>  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 efe2c00c489..31cd817c733 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -114,6 +114,9 @@ static struct async_event_handler *infrun_async_inferior_event_token;
>     Starts off as -1, indicating "never enabled/disabled".  */
>  static int infrun_is_async = -1;
>  
> +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> +					  struct execution_control_state *ecs);
> +

This forward-declaration is not needed.

>  /* See infrun.h.  */
>  
>  void
> @@ -6769,6 +6772,25 @@ handle_signal_stop (struct execution_control_state *ecs)
>    process_event_stop_test (ecs);
>  }
>  
> +CORE_ADDR
> +update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs)

Please add a comment for the function.

> +{
> +  /* The line table may have multiple entries for the same source code line.
> +     Given the PC, check the line table and return the PC that corresponds
> +     to the line table entry for the source line that PC is in.  */
> +  CORE_ADDR start_line_pc = ecs->event_thread->control.step_range_start;
> +  gdb::optional<CORE_ADDR> real_range_start;
> +
> +  /* Call find_line_range_start to get the smallest address in the
> +     linetable for multiple Line X entries in the line table.  */
> +  real_range_start = find_line_range_start (pc);
> +
> +  if (real_range_start.has_value ())
> +    start_line_pc = *real_range_start;
> +
> +  return start_line_pc;

When I read this, I wonder: why was control.step_range_start not set to
the "real" range start in the first place (not only in the context of
reverse execution, every time it is set)?  It would seem more robust
than patching it afterwards in some very specific spots.

I could see some benefits for range-stepping uses cases too (relevant
when debugging remotely).  Using your example here:

   Line X - [0x0 - 0x8]
   Line X - [0x8 - 0x10]
   Line X - [0x10 - 0x18]

Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
conditional jump to 0x5.  It seems like current GDB would send a "range
step" request to GDBserver, to step in the [0x10, 0x18[ range.  When
reaching 0x5, execution would stop, and GDB would resume it again with
the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB would
resume it with [0x8,0x10[, and so on.  If GDB could send a "range step"
request with the [0x0,0x18[ range, it would avoid those unnecessary
intermediary stop.

> +}
> +
>  /* Come here when we've got some debug event / signal we can explain
>     (IOW, not a random signal), and test whether it should cause a
>     stop, or whether we should resume the inferior (transparently).
> @@ -7570,6 +7592,28 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>        if (stop_pc_sal.is_stmt)
>  	{
> +	  if (execution_direction == EXEC_REVERSE)
> +	    {
> +	      /* We are stepping backwards make sure we have reached the
> +		 beginning of the line.  */
> +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +	      CORE_ADDR start_line_pc
> +		= update_line_range_start (stop_pc, ecs);
> +
> +	      if (stop_pc != start_line_pc)
> +		{
> +		  /* Have not reached the beginning of the source code line.
> +		     Set a step range.  Execution should stop in any function
> +		     calls we execute back into before reaching the beginning
> +		     of the line.  */
> +		  ecs->event_thread->control.step_range_start = start_line_pc;
> +		  ecs->event_thread->control.step_range_end = stop_pc;
> +		  set_step_info (ecs->event_thread, frame, stop_pc_sal);
> +		  keep_going (ecs);
> +		  return;
> +		}
> +	    }
> +
>  	  /* We are at the start of a statement.
>  
>  	     So stop.  Note that we don't stop if we step into the middle of a
> @@ -7632,6 +7676,19 @@ process_event_stop_test (struct execution_control_state *ecs)
>      set_step_info (ecs->event_thread, frame, stop_pc_sal);
>  
>    infrun_debug_printf ("keep going");
> +
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +
> +      /* Make sure the stop_pc is set to the beginning of the line.  */
> +      if (stop_pc != ecs->event_thread->control.step_range_start)
> +	{
> +	  stop_pc = update_line_range_start (stop_pc, ecs);
> +	  ecs->event_thread->control.step_range_start = stop_pc;
> +	}
> +    }
> +
>    keep_going (ecs);
>  }
>  
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 27611a34ec4..91d35616eb9 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3282,6 +3282,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);

Unnecessary parenthesis.

> +}
> +
> +/* 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;

Algorithmic complexity question: given that line tables are sorted by
address, would it work to start at the current line table item, and go
look at the previous ones until we find one that is no longer
contiguous and same line?  find_pc_line is somewhat heavy, so if we
don't need to do it repeatedly...

> +}
> +
>  /* See symtab.h.  */
>  
>  struct symtab *
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 404d0ab30a8..f54305636da 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2346,6 +2346,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.

I think that putting this example in the comment is great.  It makes it
much more obvious what the function specifically does.

> +*/
> +
> +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/func-map-to-same-line.c b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> new file mode 100644
> index 00000000000..da944874e86
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> @@ -0,0 +1,36 @@
> +/* Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +   This test is used to test the reverse-step and reverse-next instruction
> +   execution for a source line that contains multiple function calls.  */
> +
> +void
> +func1 ()
> +{
> +} /* END FUNC1 */
> +
> +void
> +func2 ()
> +{
> +} /* END FUNC2 */
> +
> +int main ()

int
main (void)

> +{
> +  int a, b;
> +  a = 1;
> +  b = 2;
> +  func1 (); func2 ();
> +  a = a + b;     /* START REVERSE TEST */
> +}
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> new file mode 100644
> index 00000000000..89e226b0f84
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> @@ -0,0 +1,140 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +# This file is part of the GDB testsuite.  It tests reverse stepping.
> +# Lots of code borrowed from "step-test.exp".
> +
> +# This test checks to make sure there is no regression failures for
> +# the reverse-next command when stepping back over two functions in
> +# the same line.
> +
> +require supports_reverse
> +
> +# This test uses the gcc no-column-info command which was added in gcc 7.1.
> +
> +proc run_tests {} {
> +    global srcfile
> +    global executable
> +
> +    runto_main

We typically check for runto_main's success:

  if { ![runto_main] } {
      return
  }

runto_main logs a FAIL on failure.  There are a few runto_mains in the
patch.

> +    set target_remote [gdb_is_target_remote]

target_remote seems unused

> +
> +    with_test_prefix "test1" {
> +	gdb_test_no_output "record" "turn on process record"
> +    }

with_test_prefix with a single test in it is really just the same as:

  gdb_test_no_output "record" "test1: turn on process record"

In fact, you have some other tests with the "test1:" or "test2:" prefix,
I think they should be moved to the with_test_prefix.  And maybe use
"next" and "step" instead of "test1" and "test2".

> +
> +    # This regression test verifies the reverse-step and reverse-next commands
> +    # work properly when executing backwards thru a source line containing
> +    # two function calls on the same source line, i.e. func1 (); func2 ();
> +    # This test is compiled so the dwarf info not contain the line table
> +    # information.
> +
> +    # Test 1, reverse-next command
> +    # Set breakpoint at the line after the function calls.
> +    set bp_start_reverse_test [gdb_get_line_number "START REVERSE TEST" \
> +				   $srcfile]
> +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> +
> +    # Continue to break point for reverse-next test.
> +    # Command definition:  reverse-next [count]
> +    #   Run backward to the beginning of the previous line executed in the
> +    #   current (innermost) stack frame. If the line contains function calls,
> +    #   they will be “un-executed” without stopping. Starting from the first
> +    #   line of a function, reverse-next will take you back to the caller of
> +    #   that function, before the function was called, just as the normal next
> +    #   command would take you from the last line of a function back to its
> +    #   return to its caller 2 .
> +    gdb_continue_to_breakpoint \
> +	"test1: stopped at command reverse-next test start location" \
> +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> +
> +    # The reverse-next should step all the way back to the beginning of the
> +    # line, i.e. at the beginning of the func1 call.
> +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	"test1: reverse-next to line with two functions"
> +
> +    # We should be stopped at the first instruction of the line. A reverse-step
> +    # should step back and stop at the beginning of the previous line b = 2,
> +    # i.e. not in func1 ().
> +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	"test1: reverse-stepi to previous line b = 2"
> +
> +
> +    # Setup for test 2
> +    clean_restart $executable
> +    runto_main
> +
> +    with_test_prefix "test2" {
> +	gdb_test_no_output "record" "turn on process record"
> +    }
> +
> +    # Test 2, reverse-step command
> +    # Set breakpoint at the line after the function calls.
> +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> +
> +    #  Continue to the start of the reverse-step test.
> +    #  Command definition:  reverse-step [count]
> +    #    Run the program backward until control reaches the start of a
> +    #    different source line; then stop it, and return control to gdb.
> +    #    Like the step command, reverse-step will only stop at the beginning
> +    #    of a source line. It “un-executes” the previously executed source
> +    #    line. If the previous source line included calls to debuggable
> +    #    functions, reverse-step will step (backward) into the called function,
> +    #    stopping at the beginning of the last statement in the called
> +    #    function (typically a return statement).  Also, as with the step
> +    #    command, if non-debuggable functions are called, reverse-step will
> +    #    run thru them backward without stopping.
> +
> +    gdb_continue_to_breakpoint \
> +	"test2: stopped at command reverse-step test start location" \
> +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> +
> +    # The first reverse step should take us call of func2 ().
> +    gdb_test "reverse-step" ".*END FUNC2.*" \
> +	"test2: reverse-step into func2 "
> +
> +    # The second reverse step should take us into func1 ().
> +    gdb_test "reverse-step" ".*END FUNC1.*" \
> +	"test2: reverse-step into func1 "
> +
> +    # The third reverse step should take us call of func1 ().
> +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	"test2: reverse-step to line func1(); func2(), at call for func1 "
> +
> +    # We should be stopped at the first instruction of the line. A reverse
> +    # stepi should take us to b = 2 ().
> +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	"test2: reverse-stepi to line b = 2 "
> +}
> +
> +set srcfile  func-map-to-same-line.c
> +set executable func-map-to-same-line

Wondering if this test should use standard_testfile (like almost every
other tests) to set these.

> +
> +# test with and without gcc column info enabled
> +foreach_with_prefix with_column_info {yes no} {
> +    if {$with_column_info == "yes"} {
> +	set options [list debug column-info]
> +    } else {
> +	set options [list debug no-column-info]
> +    }

I didn't think of this when proposing the foreach_with_prefix, but you
could perhaps use:

  foreach_with_prefix column_info_flag {column-info no-column-info}

... to avoid this boilerplate.  You can then use $column_info_flag
directly when setting options.

> +
> +    if {[build_executable "failed to prepare" $executable $srcfile \
> +	     $options] == -1} {
> +	return -1
> +    }
> +
> +    clean_restart $executable

clean_restart can go in run_tests.

> +    run_tests
> +}
> 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..f20d778f40e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,58 @@
> +/* Copyright 2008-2023 Free Software Foundation, Inc.

Just wondering if the copyright years are right.

> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/ >.  */
> +
> +/* The purpose of this test is to create a DWARF line table that contains two
> +   or more entries for the same line.  When stepping (forwards or backwards),
> +   GDB should step over the entire line and not just a particular entry in the
> +   line table.  */
> +
> +int
> +main ()

void in the parenthesis

> +{     /* TAG: main prologue */
> +  asm ("main_label: .globl main_label");
> +  int i = 1, j = 2, k;
> +  float f1 = 2.0, f2 = 4.1, f3;
> +  const char *str_1 = "foo", *str_2 = "bar", *str_3;
> +
> +  asm ("line1: .globl line1");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
> +
> +  asm ("line2: .globl line2");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
> +
> +  asm ("line3: .globl line3");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
> +
> +  asm ("line4: .globl line4");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
> +
> +  asm ("line5: .globl line5");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
> +
> +  asm ("line6: .globl line6");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
> +
> +  asm ("line7: .globl line7");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
> +
> +  asm ("line8: .globl line8");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
> +
> +  asm ("main_return: .globl main_return");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
> +
> +  asm ("end_of_sequence: .globl end_of_sequence");
> +  return 0; /* TAG: main return */
> +}
> 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..16a359d90ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,156 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/ >.
> +
> +# When stepping (forwards or backwards), GDB should step over the entire line
> +# and not just a particular entry in the line table. This test was added to
> +# verify the find_line_range_start function properly sets the step range for a
> +# line that consists of multiple statements, i.e. multiple entries in the line
> +# table.  This test creates a DWARF line table that contains two entries for
> +# the same line to do the needed testing.
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +# The DWARF assembler requires the gcc compiler.
> +require is_c_compiler_gcc
> +
> +# This test suitable only for process that can do reverse execution
> +require supports_reverse
> +
> +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 distinct source lines being
> +	# mapped to the same line entry. Line 1, 5 and 8 contain 1 statement
> +	# each.  Line 2 contains 2 statements.  Line 3 contains 3 statements.
> +	program {
> +	    DW_LNE_set_address $main_start
> +	    line [gdb_get_line_number "TAG: main prologue"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line1
> +	    line [gdb_get_line_number "TAG: line 1" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line2
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line3
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line4
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line5
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line6
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line7
> +	    line [gdb_get_line_number "TAG: line 5" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line8
> +	    line [gdb_get_line_number "TAG: line 8" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address main_return
> +	    line [gdb_get_line_number "TAG: main return"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address end_of_sequence
> +	    DW_LNE_end_sequence
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +runto_main
> +
> +# Print the line table
> +gdb_test_multiple "maint info line-table ${testfile}" "" {
> +    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[ \t\]+Y\[^\r\n\]*" {
> +	lappend is_stmt $expect_out(1,string)
> +	exp_continue
> +    }
> +    -re -wrap "" {
> +    }
> +}
> +
> +# Do the reverse-step test
> +gdb_test_no_output "record" "turn on process record"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-step test" ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
> +
> +# At this point, GDB has already recorded the execution up until the return
> +# statement.  Reverse-step and test if GDB transitions between lines in the
> +# expected order.  It should reverse-step across lines 8, 5, 3, 2 and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to line $line"
> +}
> +
> +## Clean restart, test reverse-next command
> +clean_restart ${testfile}
> +runto_main
> +gdb_test_no_output "record" "turn on process record, reverst-next test"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-next test" ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
> +
> +# At this point, GDB has already recorded the execution up until the return
> +# statement.  Reverse-next and test if GDB transitions between lines in the
> +# expected order.  It should reverse-next across lines 8, 5, 3, 2 and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to line $line"
> +}

It seems like the step and next tests are identical, so I guess it could
be factored out using:

  foreach_with_prefix method {step next} {
      ...
  }

?

Simon
  

Simon:

On Mon, 2023-06-19 at 13:58 -0400, Simon Marchi wrote:
<snip>

> 
100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -114,6 +114,9 @@ static struct async_event_handler
> > *infrun_async_inferior_event_token;
> >     Starts off as -1, indicating "never enabled/disabled".  */
> >  static int infrun_is_async = -1;
> >  
> > +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> > +					  struct
> > execution_control_state *ecs);
> > +
> 
> This forward-declaration is not needed.

I tried removing the forward-declaration and the compile fails with the
message:

   ../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:6773:1:
   error: no previous declaration for ‘CORE_ADDR
   update_line_range_start(CORE_ADDR, execution_control_state*)’ [-
   Werror=missing-declarations]
    6773 | update_line_range_start (CORE_ADDR pc, struct
   execution_control_state *ecs)
         | ^~~~~~~~~~~~~~~~~~~~~~~
   cc1plus: all warnings being treated as errors
   make[2]: *** [Makefile:1922: infrun.o] Error 1
   make[2]: Leaving directory '/home/carll/GDB/build-reverse-multiple-
   contiguous/gdb'
   make[1]: *** [Makefile:13569: all-gdb] Error 2
   make[1]: Leaving directory '/home/carll/GDB/build-reverse-multiple-
   contiguous'
   make: *** [Makefile:1005: all] Error 2

Leaving the forward declaration in the code.

   > 
> >  /* See infrun.h.  */
> >  
> >  void
> > @@ -6769,6 +6772,25 @@ handle_signal_stop (struct
> > execution_control_state *ecs)
> >    process_event_stop_test (ecs);
> >  }
> >  
> > +CORE_ADDR
> > +update_line_range_start (CORE_ADDR pc, struct
> > execution_control_state *ecs)
> 
> Please add a comment for the function.

Done.

> 
> > +{
> > +  /* The line table may have multiple entries for the same source
> > code line.
> > +     Given the PC, check the line table and return the PC that
> > corresponds
> > +     to the line table entry for the source line that PC is
> > in.  */
> > +  CORE_ADDR start_line_pc = ecs->event_thread-
> > >control.step_range_start;
> > +  gdb::optional<CORE_ADDR> real_range_start;
> > +
> > +  /* Call find_line_range_start to get the smallest address in the
> > +     linetable for multiple Line X entries in the line table.  */
> > +  real_range_start = find_line_range_start (pc);
> > +
> > +  if (real_range_start.has_value ())
> > +    start_line_pc = *real_range_start;
> > +
> > +  return start_line_pc;
> 
> When I read this, I wonder: why was control.step_range_start not set
> to
> the "real" range start in the first place (not only in the context of
> reverse execution, every time it is set)?  It would seem more robust
> than patching it afterwards in some very specific spots.
> 
> I could see some benefits for range-stepping uses cases too (relevant
> when debugging remotely).  Using your example here:
> 
>    Line X - [0x0 - 0x8]
>    Line X - [0x8 - 0x10]
>    Line X - [0x10 - 0x18]
> 
> Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
> conditional jump to 0x5.  It seems like current GDB would send a
> "range
> step" request to GDBserver, to step in the [0x10, 0x18[ range.  When
> reaching 0x5, execution would stop, and GDB would resume it again
> with
> the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB
> would
> resume it with [0x8,0x10[, and so on.  If GDB could send a "range
> step"
> request with the [0x0,0x18[ range, it would avoid those unnecessary
> intermediary stop.
> 
> > +}
> > +
> >  /* Come here when we've got some debug event / signal we can
> > explain
> >     (IOW, not a random signal), and test whether it should cause a
> >     stop, or whether we should resume the inferior (transparently).
> > @@ -7570,6 +7592,28 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >  
> >        if (stop_pc_sal.is_stmt)
> >  	{
> > +	  if (execution_direction == EXEC_REVERSE)
> > +	    {
> > +	      /* We are stepping backwards make sure we have reached
> > the
> > +		 beginning of the line.  */
> > +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +	      CORE_ADDR start_line_pc
> > +		= update_line_range_start (stop_pc, ecs);
> > +
> > +	      if (stop_pc != start_line_pc)
> > +		{
> > +		  /* Have not reached the beginning of the source code
> > line.
> > +		     Set a step range.  Execution should stop in any
> > function
> > +		     calls we execute back into before reaching the
> > beginning
> > +		     of the line.  */
> > +		  ecs->event_thread->control.step_range_start =
> > start_line_pc;
> > +		  ecs->event_thread->control.step_range_end = stop_pc;
> > +		  set_step_info (ecs->event_thread, frame,
> > stop_pc_sal);
> > +		  keep_going (ecs);
> > +		  return;
> > +		}
> > +	    }
> > +
> >  	  /* We are at the start of a statement.
> >  
> >  	     So stop.  Note that we don't stop if we step into the
> > middle of a
> > @@ -7632,6 +7676,19 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >      set_step_info (ecs->event_thread, frame, stop_pc_sal);
> >  
> >    infrun_debug_printf ("keep going");
> > +
> > +  if (execution_direction == EXEC_REVERSE)
> > +    {
> > +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +
> > +      /* Make sure the stop_pc is set to the beginning of the
> > line.  */
> > +      if (stop_pc != ecs->event_thread->control.step_range_start)
> > +	{
> > +	  stop_pc = update_line_range_start (stop_pc, ecs);
> > +	  ecs->event_thread->control.step_range_start = stop_pc;
> > +	}
> > +    }
> > +
> >    keep_going (ecs);
> >  }
> >  
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 27611a34ec4..91d35616eb9 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3282,6 +3282,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);
> 
> Unnecessary parenthesis.

Removed unnecessary parenthesis.

> 
> > +}
> > +
> > +/* 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;
> 
> Algorithmic complexity question: given that line tables are sorted by
> address, would it work to start at the current line table item, and
> go
> look at the previous ones until we find one that is no longer
> contiguous and same line?  find_pc_line is somewhat heavy, so if we
> don't need to do it repeatedly...
> 
> > +}
> > +
> >  /* See symtab.h.  */
> >  
> >  struct symtab *
> > diff --git a/gdb/symtab.h b/gdb/symtab.h
> > index 404d0ab30a8..f54305636da 100644
> > --- a/gdb/symtab.h
> > +++ b/gdb/symtab.h
> > @@ -2346,6 +2346,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.
> 
> I think that putting this example in the comment is great.  It makes
> it
> much more obvious what the function specifically does.
> 
> > +*/
> > +
> > +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/func-map-to-same-line.c
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > new file mode 100644
> > index 00000000000..da944874e86
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > @@ -0,0 +1,36 @@
> > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.
> > +
> > +   This test is used to test the reverse-step and reverse-next
> > instruction
> > +   execution for a source line that contains multiple function
> > calls.  */
> > +
> > +void
> > +func1 ()
> > +{
> > +} /* END FUNC1 */
> > +
> > +void
> > +func2 ()
> > +{
> > +} /* END FUNC2 */
> > +
> > +int main ()
> 
> int
> main (void)
> 

Fixed.

> > +{
> > +  int a, b;
> > +  a = 1;
> > +  b = 2;
> > +  func1 (); func2 ();
> > +  a = a + b;     /* START REVERSE TEST */
> > +}
> > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > new file mode 100644
> > index 00000000000..89e226b0f84
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > @@ -0,0 +1,140 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.  */
> > +
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# This test checks to make sure there is no regression failures
> > for
> > +# the reverse-next command when stepping back over two functions
> > in
> > +# the same line.
> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +
> > +proc run_tests {} {
> > +    global srcfile
> > +    global executable
> > +
> > +    runto_main
> 
> We typically check for runto_main's success:

Fixed two instances of runto_main in this test case and two in the
other test case.

> 
>   if { ![runto_main] } {
>       return
>   }
> 
> runto_main logs a FAIL on failure.  There are a few runto_mains in
> the
> patch.
> 
> > +    set target_remote [gdb_is_target_remote]
> 
> target_remote seems unused

Removed.

> 
> > +
> > +    with_test_prefix "test1" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> 
> with_test_prefix with a single test in it is really just the same as:
> 
>   gdb_test_no_output "record" "test1: turn on process record"
> 
> In fact, you have some other tests with the "test1:" or "test2:"
> prefix,
> I think they should be moved to the with_test_prefix.  And maybe use
> "next" and "step" instead of "test1" and "test2".

Yup, cleaner to have the with_test_prefix cover the whole test. 
Changed test1 to next-test and test2 to step-next.

> 
> > +
> > +    # This regression test verifies the reverse-step and reverse-
> > next commands
> > +    # work properly when executing backwards thru a source line
> > containing
> > +    # two function calls on the same source line, i.e. func1 ();
> > func2 ();
> > +    # This test is compiled so the dwarf info not contain the line
> > table
> > +    # information.
> > +
> > +    # Test 1, reverse-next command
> > +    # Set breakpoint at the line after the function calls.
> > +    set bp_start_reverse_test [gdb_get_line_number "START REVERSE
> > TEST" \
> > +				   $srcfile]
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    # Continue to break point for reverse-next test.
> > +    # Command definition:  reverse-next [count]
> > +    #   Run backward to the beginning of the previous line
> > executed in the
> > +    #   current (innermost) stack frame. If the line contains
> > function calls,
> > +    #   they will be “un-executed” without stopping. Starting from
> > the first
> > +    #   line of a function, reverse-next will take you back to the
> > caller of
> > +    #   that function, before the function was called, just as the
> > normal next
> > +    #   command would take you from the last line of a function
> > back to its
> > +    #   return to its caller 2 .
> > +    gdb_continue_to_breakpoint \
> > +	"test1: stopped at command reverse-next test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The reverse-next should step all the way back to the
> > beginning of the
> > +    # line, i.e. at the beginning of the func1 call.
> > +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test1: reverse-next to line with two functions"
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse-step
> > +    # should step back and stop at the beginning of the previous
> > line b = 2,
> > +    # i.e. not in func1 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test1: reverse-stepi to previous line b = 2"
> > +
> > +
> > +    # Setup for test 2
> > +    clean_restart $executable
> > +    runto_main
> > +
> > +    with_test_prefix "test2" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> > +
> > +    # Test 2, reverse-step command
> > +    # Set breakpoint at the line after the function calls.
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    #  Continue to the start of the reverse-step test.
> > +    #  Command definition:  reverse-step [count]
> > +    #    Run the program backward until control reaches the start
> > of a
> > +    #    different source line; then stop it, and return control
> > to gdb.
> > +    #    Like the step command, reverse-step will only stop at the
> > beginning
> > +    #    of a source line. It “un-executes” the previously
> > executed source
> > +    #    line. If the previous source line included calls to
> > debuggable
> > +    #    functions, reverse-step will step (backward) into the
> > called function,
> > +    #    stopping at the beginning of the last statement in the
> > called
> > +    #    function (typically a return statement).  Also, as with
> > the step
> > +    #    command, if non-debuggable functions are called, reverse-
> > step will
> > +    #    run thru them backward without stopping.
> > +
> > +    gdb_continue_to_breakpoint \
> > +	"test2: stopped at command reverse-step test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The first reverse step should take us call of func2 ().
> > +    gdb_test "reverse-step" ".*END FUNC2.*" \
> > +	"test2: reverse-step into func2 "
> > +
> > +    # The second reverse step should take us into func1 ().
> > +    gdb_test "reverse-step" ".*END FUNC1.*" \
> > +	"test2: reverse-step into func1 "
> > +
> > +    # The third reverse step should take us call of func1 ().
> > +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test2: reverse-step to line func1(); func2(), at call for
> > func1 "
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse
> > +    # stepi should take us to b = 2 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test2: reverse-stepi to line b = 2 "
> > +}
> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> 
> Wondering if this test should use standard_testfile (like almost
> every
> other tests) to set these.

OK, changed to use the standard_testfile.

> 
> > +
> > +# test with and without gcc column info enabled
> > +foreach_with_prefix with_column_info {yes no} {
> > +    if {$with_column_info == "yes"} {
> > +	set options [list debug column-info]
> > +    } else {
> > +	set options [list debug no-column-info]
> > +    }
> 
> I didn't think of this when proposing the foreach_with_prefix, but
> you
> could perhaps use:
> 
>   foreach_with_prefix column_info_flag {column-info no-column-info}
> 
> ... to avoid this boilerplate.  You can then use $column_info_flag
> directly when setting options.
> 
OK, that cleans things up a bit.  Changed.
+
+    if {[build_executable "failed to prepare" $executable $srcfile
\
+	     $options] == -1} {
+	return -1
+    }
+
+    clean_restart $executable

clean_restart can go in run_tests.


+    run_tests
+}
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..f20d778f40e
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,58 @@
+/* Copyright 2008-2023 Free Software Foundation, Inc.

Just wondering if the copyright years are right.

New files so yea, should just start with 2023.
> 
+
+   This program is free software; you can redistribute it and/or
modify
+   it under the terms of the GNU General Public License as
published by
+   the Free Software Foundation; either version 3 of the License,
or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public
License
+   along with this program.  If not, see <
http://www.gnu.org/licenses/
  >.  */
+
+/* The purpose of this test is to create a DWARF line table that
contains two
+   or more entries for the same line.  When stepping (forwards or
backwards),
+   GDB should step over the entire line and not just a particular
entry in the
+   line table.  */
+
+int
+main ()

void in the parenthesis

Fixed in both test files.

> 
+{     /* TAG: main prologue */
+  asm ("main_label: .globl main_label");
+  int i = 1, j = 2, k;
+  float f1 = 2.0, f2 = 4.1, f3;
+  const char *str_1 = "foo", *str_2 = "bar", *str_3;
+
+  asm ("line1: .globl line1");
+  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
+
+  asm ("line2: .globl line2");
+  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
+
+  asm ("line3: .globl line3");
+  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
+
+  asm ("line4: .globl line4");
+  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
+
+  asm ("line5: .globl line5");
+  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
+
+  asm ("line6: .globl line6");
+  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
+
+  asm ("line7: .globl line7");
+  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
+
+  asm ("line8: .globl line8");
+  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
+
+  asm ("main_return: .globl main_return");
+  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
+
+  asm ("end_of_sequence: .globl end_of_sequence");
+  return 0; /* TAG: main return */
+}
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..16a359d90ec
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,156 @@
+# Copyright 2008-2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or
modify
+# it under the terms of the GNU General Public License as
published by
+# the Free Software Foundation; either version 3 of the License,
or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public
License
+# along with this program.  If not, see <
http://www.gnu.org/licenses/
=  >.
+
+# When stepping (forwards or backwards), GDB should step over the
entire line
+# and not just a particular entry in the line table. This test was
added to
+# verify the find_line_range_start function properly sets the step
range for a
+# line that consists of multiple statements, i.e. multiple entries
in the line
+# table.  This test creates a DWARF line table that contains two
entries for
+# the same line to do the needed testing.
+
+# This test can only be run on targets which support DWARF-2 and
use gas.
+load_lib dwarf.exp
+require dwarf2_support
+
+# The DWARF assembler requires the gcc compiler.
+require is_c_compiler_gcc
+
+# This test suitable only for process that can do reverse
execution
+require supports_reverse
+
+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 distinct source lines
being
+	# mapped to the same line entry. Line 1, 5 and 8 contain 1
statement
+	# each.  Line 2 contains 2 statements.  Line 3 contains 3
statements.
+	program {
+	    DW_LNE_set_address $main_start
+	    line [gdb_get_line_number "TAG: main prologue"]
+	    DW_LNS_copy
+	    DW_LNE_set_address line1
+	    line [gdb_get_line_number "TAG: line 1" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line2
+	    line [gdb_get_line_number "TAG: line 2" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line3
+	    line [gdb_get_line_number "TAG: line 2" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line4
+	    line [gdb_get_line_number "TAG: line 3" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line5
+	    line [gdb_get_line_number "TAG: line 3" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line6
+	    line [gdb_get_line_number "TAG: line 3" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line7
+	    line [gdb_get_line_number "TAG: line 5" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address line8
+	    line [gdb_get_line_number "TAG: line 8" ]
+	    DW_LNS_copy
+	    DW_LNE_set_address main_return
+	    line [gdb_get_line_number "TAG: main return"]
+	    DW_LNS_copy
+	    DW_LNE_set_address end_of_sequence
+	    DW_LNE_end_sequence
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	[list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+runto_main
+
+# Print the line table
+gdb_test_multiple "maint info line-table ${testfile}" "" {
+    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[
\t\]+Y\[^\r\n\]*" {
+	lappend is_stmt $expect_out(1,string)
+	exp_continue
+    }
+    -re -wrap "" {
+    }
+}
+
+# Do the reverse-step test
+gdb_test_no_output "record" "turn on process record"
+
+set bp_main_return [gdb_get_line_number "TAG: main return"
$srcfile]
+gdb_breakpoint $srcfile:$bp_main_return
+gdb_continue_to_breakpoint  "run to end of main, reverse-step
test" ".*$srcfile:$bp_main_return.*"
+gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
+
+# At this point, GDB has already recorded the execution up until
the return
+# statement.  Reverse-step and test if GDB transitions between
lines in the
+# expected order.  It should reverse-step across lines 8, 5, 3, 2
and 1.
+foreach line {8 5 3 2 1} {
+    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to
line $line"
+}
+
+## Clean restart, test reverse-next command
+clean_restart ${testfile}
+runto_main
+gdb_test_no_output "record" "turn on process record, reverst-next
test"
+
+set bp_main_return [gdb_get_line_number "TAG: main return"
$srcfile]
+gdb_breakpoint $srcfile:$bp_main_return
+gdb_continue_to_breakpoint  "run to end of main, reverse-next
test" ".*$srcfile:$bp_main_return.*"
+gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
+
+# At this point, GDB has already recorded the execution up until
the return
+# statement.  Reverse-next and test if GDB transitions between
lines in the
+# expected order.  It should reverse-next across lines 8, 5, 3, 2
and 1.
+foreach line {8 5 3 2 1} {
+    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to
line $line"
+}

It seems like the step and next tests are identical, so I guess it
could
be factored out using:

  foreach_with_prefix method {step next} {
      ...
  }

?

Yup, redid the code using the foreach_with_prefix.

> 
Simon
> yR
  

Sorry, ignore this message.  Accidentally hit send before I was done
with the message.

            Carl 
--------------------------------------------------


On Thu, 2023-06-22 at 13:38 -0700, Carl Love wrote:
> Simon:
> 
> On Mon, 2023-06-19 at 13:58 -0400, Simon Marchi wrote:
> <snip>
> 
> 100644
> > > --- a/gdb/infrun.c
> > > +++ b/gdb/infrun.c
> > > @@ -114,6 +114,9 @@ static struct async_event_handler
> > > *infrun_async_inferior_event_token;
> > >     Starts off as -1, indicating "never enabled/disabled".  */
> > >  static int infrun_is_async = -1;
> > >  
> > > +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> > > +					  struct
> > > execution_control_state *ecs);
> > > +
> > 
> > This forward-declaration is not needed.
> 
> I tried removing the forward-declaration and the compile fails with
> the
> message:
> 
>    ../../binutils-gdb-reverse-multiple-
> contiguous/gdb/infrun.c:6773:1:
>    error: no previous declaration for ‘CORE_ADDR
>    update_line_range_start(CORE_ADDR, execution_control_state*)’ [-
>    Werror=missing-declarations]
>     6773 | update_line_range_start (CORE_ADDR pc, struct
>    execution_control_state *ecs)
>          | ^~~~~~~~~~~~~~~~~~~~~~~
>    cc1plus: all warnings being treated as errors
>    make[2]: *** [Makefile:1922: infrun.o] Error 1
>    make[2]: Leaving directory '/home/carll/GDB/build-reverse-
> multiple-
>    contiguous/gdb'
>    make[1]: *** [Makefile:13569: all-gdb] Error 2
>    make[1]: Leaving directory '/home/carll/GDB/build-reverse-
> multiple-
>    contiguous'
>    make: *** [Makefile:1005: all] Error 2
> 
> Leaving the forward declaration in the code.
> 
>    > 
> > >  /* See infrun.h.  */
> > >  
> > >  void
> > > @@ -6769,6 +6772,25 @@ handle_signal_stop (struct
> > > execution_control_state *ecs)
> > >    process_event_stop_test (ecs);
> > >  }
> > >  
> > > +CORE_ADDR
> > > +update_line_range_start (CORE_ADDR pc, struct
> > > execution_control_state *ecs)
> > 
> > Please add a comment for the function.
> 
> Done.
> 
> > > +{
> > > +  /* The line table may have multiple entries for the same
> > > source
> > > code line.
> > > +     Given the PC, check the line table and return the PC that
> > > corresponds
> > > +     to the line table entry for the source line that PC is
> > > in.  */
> > > +  CORE_ADDR start_line_pc = ecs->event_thread-
> > > > control.step_range_start;
> > > +  gdb::optional<CORE_ADDR> real_range_start;
> > > +
> > > +  /* Call find_line_range_start to get the smallest address in
> > > the
> > > +     linetable for multiple Line X entries in the line
> > > table.  */
> > > +  real_range_start = find_line_range_start (pc);
> > > +
> > > +  if (real_range_start.has_value ())
> > > +    start_line_pc = *real_range_start;
> > > +
> > > +  return start_line_pc;
> > 
> > When I read this, I wonder: why was control.step_range_start not
> > set
> > to
> > the "real" range start in the first place (not only in the context
> > of
> > reverse execution, every time it is set)?  It would seem more
> > robust
> > than patching it afterwards in some very specific spots.
> > 
> > I could see some benefits for range-stepping uses cases too
> > (relevant
> > when debugging remotely).  Using your example here:
> > 
> >    Line X - [0x0 - 0x8]
> >    Line X - [0x8 - 0x10]
> >    Line X - [0x10 - 0x18]
> > 
> > Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
> > conditional jump to 0x5.  It seems like current GDB would send a
> > "range
> > step" request to GDBserver, to step in the [0x10, 0x18[
> > range.  When
> > reaching 0x5, execution would stop, and GDB would resume it again
> > with
> > the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB
> > would
> > resume it with [0x8,0x10[, and so on.  If GDB could send a "range
> > step"
> > request with the [0x0,0x18[ range, it would avoid those unnecessary
> > intermediary stop.
> > 
> > > +}
> > > +
> > >  /* Come here when we've got some debug event / signal we can
> > > explain
> > >     (IOW, not a random signal), and test whether it should cause
> > > a
> > >     stop, or whether we should resume the inferior
> > > (transparently).
> > > @@ -7570,6 +7592,28 @@ process_event_stop_test (struct
> > > execution_control_state *ecs)
> > >  
> > >        if (stop_pc_sal.is_stmt)
> > >  	{
> > > +	  if (execution_direction == EXEC_REVERSE)
> > > +	    {
> > > +	      /* We are stepping backwards make sure we have reached
> > > the
> > > +		 beginning of the line.  */
> > > +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > > +	      CORE_ADDR start_line_pc
> > > +		= update_line_range_start (stop_pc, ecs);
> > > +
> > > +	      if (stop_pc != start_line_pc)
> > > +		{
> > > +		  /* Have not reached the beginning of the source code
> > > line.
> > > +		     Set a step range.  Execution should stop in any
> > > function
> > > +		     calls we execute back into before reaching the
> > > beginning
> > > +		     of the line.  */
> > > +		  ecs->event_thread->control.step_range_start =
> > > start_line_pc;
> > > +		  ecs->event_thread->control.step_range_end = stop_pc;
> > > +		  set_step_info (ecs->event_thread, frame,
> > > stop_pc_sal);
> > > +		  keep_going (ecs);
> > > +		  return;
> > > +		}
> > > +	    }
> > > +
> > >  	  /* We are at the start of a statement.
> > >  
> > >  	     So stop.  Note that we don't stop if we step into the
> > > middle of a
> > > @@ -7632,6 +7676,19 @@ process_event_stop_test (struct
> > > execution_control_state *ecs)
> > >      set_step_info (ecs->event_thread, frame, stop_pc_sal);
> > >  
> > >    infrun_debug_printf ("keep going");
> > > +
> > > +  if (execution_direction == EXEC_REVERSE)
> > > +    {
> > > +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > > +
> > > +      /* Make sure the stop_pc is set to the beginning of the
> > > line.  */
> > > +      if (stop_pc != ecs->event_thread-
> > > >control.step_range_start)
> > > +	{
> > > +	  stop_pc = update_line_range_start (stop_pc, ecs);
> > > +	  ecs->event_thread->control.step_range_start = stop_pc;
> > > +	}
> > > +    }
> > > +
> > >    keep_going (ecs);
> > >  }
> > >  
> > > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > > index 27611a34ec4..91d35616eb9 100644
> > > --- a/gdb/symtab.c
> > > +++ b/gdb/symtab.c
> > > @@ -3282,6 +3282,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);
> > 
> > Unnecessary parenthesis.
> 
> Removed unnecessary parenthesis.
> 
> > > +}
> > > +
> > > +/* 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;
> > 
> > Algorithmic complexity question: given that line tables are sorted
> > by
> > address, would it work to start at the current line table item, and
> > go
> > look at the previous ones until we find one that is no longer
> > contiguous and same line?  find_pc_line is somewhat heavy, so if we
> > don't need to do it repeatedly...
> > 
> > > +}
> > > +
> > >  /* See symtab.h.  */
> > >  
> > >  struct symtab *
> > > diff --git a/gdb/symtab.h b/gdb/symtab.h
> > > index 404d0ab30a8..f54305636da 100644
> > > --- a/gdb/symtab.h
> > > +++ b/gdb/symtab.h
> > > @@ -2346,6 +2346,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.
> > 
> > I think that putting this example in the comment is great.  It
> > makes
> > it
> > much more obvious what the function specifically does.
> > 
> > > +*/
> > > +
> > > +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/func-map-to-same-line.c
> > > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > > new file mode 100644
> > > index 00000000000..da944874e86
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > > @@ -0,0 +1,36 @@
> > > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> > > +
> > > +   This program is free software; you can redistribute it and/or
> > > modify
> > > +   it under the terms of the GNU General Public License as
> > > published by
> > > +   the Free Software Foundation; either version 3 of the
> > > License,
> > > or
> > > +   (at your option) any later version.
> > > +
> > > +   This program is distributed in the hope that it will be
> > > useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty
> > > of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +   GNU General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU General Public
> > > License
> > > +   along with this program.  If not, see <
> > > http://www.gnu.org/licenses/
> > >  >.
> > > +
> > > +   This test is used to test the reverse-step and reverse-next
> > > instruction
> > > +   execution for a source line that contains multiple function
> > > calls.  */
> > > +
> > > +void
> > > +func1 ()
> > > +{
> > > +} /* END FUNC1 */
> > > +
> > > +void
> > > +func2 ()
> > > +{
> > > +} /* END FUNC2 */
> > > +
> > > +int main ()
> > 
> > int
> > main (void)
> > 
> 
> Fixed.
> 
> > > +{
> > > +  int a, b;
> > > +  a = 1;
> > > +  b = 2;
> > > +  func1 (); func2 ();
> > > +  a = a + b;     /* START REVERSE TEST */
> > > +}
> > > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > > new file mode 100644
> > > index 00000000000..89e226b0f84
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > > @@ -0,0 +1,140 @@
> > > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > > +
> > > +# This program is free software; you can redistribute it and/or
> > > modify
> > > +# it under the terms of the GNU General Public License as
> > > published by
> > > +# the Free Software Foundation; either version 3 of the License,
> > > or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be
> > > useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public
> > > License
> > > +# along with this program.  If not, see <
> > > http://www.gnu.org/licenses/
> > >  >.  */
> > > +
> > > +# This file is part of the GDB testsuite.  It tests reverse
> > > stepping.
> > > +# Lots of code borrowed from "step-test.exp".
> > > +
> > > +# This test checks to make sure there is no regression failures
> > > for
> > > +# the reverse-next command when stepping back over two functions
> > > in
> > > +# the same line.
> > > +
> > > +require supports_reverse
> > > +
> > > +# This test uses the gcc no-column-info command which was added
> > > in
> > > gcc 7.1.
> > > +
> > > +proc run_tests {} {
> > > +    global srcfile
> > > +    global executable
> > > +
> > > +    runto_main
> > 
> > We typically check for runto_main's success:
> 
> Fixed two instances of runto_main in this test case and two in the
> other test case.
> 
> >   if { ![runto_main] } {
> >       return
> >   }
> > 
> > runto_main logs a FAIL on failure.  There are a few runto_mains in
> > the
> > patch.
> > 
> > > +    set target_remote [gdb_is_target_remote]
> > 
> > target_remote seems unused
> 
> Removed.
> 
> > > +
> > > +    with_test_prefix "test1" {
> > > +	gdb_test_no_output "record" "turn on process record"
> > > +    }
> > 
> > with_test_prefix with a single test in it is really just the same
> > as:
> > 
> >   gdb_test_no_output "record" "test1: turn on process record"
> > 
> > In fact, you have some other tests with the "test1:" or "test2:"
> > prefix,
> > I think they should be moved to the with_test_prefix.  And maybe
> > use
> > "next" and "step" instead of "test1" and "test2".
> 
> Yup, cleaner to have the with_test_prefix cover the whole test. 
> Changed test1 to next-test and test2 to step-next.
> 
> > > +
> > > +    # This regression test verifies the reverse-step and
> > > reverse-
> > > next commands
> > > +    # work properly when executing backwards thru a source line
> > > containing
> > > +    # two function calls on the same source line, i.e. func1 ();
> > > func2 ();
> > > +    # This test is compiled so the dwarf info not contain the
> > > line
> > > table
> > > +    # information.
> > > +
> > > +    # Test 1, reverse-next command
> > > +    # Set breakpoint at the line after the function calls.
> > > +    set bp_start_reverse_test [gdb_get_line_number "START
> > > REVERSE
> > > TEST" \
> > > +				   $srcfile]
> > > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > > +
> > > +    # Continue to break point for reverse-next test.
> > > +    # Command definition:  reverse-next [count]
> > > +    #   Run backward to the beginning of the previous line
> > > executed in the
> > > +    #   current (innermost) stack frame. If the line contains
> > > function calls,
> > > +    #   they will be “un-executed” without stopping. Starting
> > > from
> > > the first
> > > +    #   line of a function, reverse-next will take you back to
> > > the
> > > caller of
> > > +    #   that function, before the function was called, just as
> > > the
> > > normal next
> > > +    #   command would take you from the last line of a function
> > > back to its
> > > +    #   return to its caller 2 .
> > > +    gdb_continue_to_breakpoint \
> > > +	"test1: stopped at command reverse-next test start location" \
> > > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > > +
> > > +    # The reverse-next should step all the way back to the
> > > beginning of the
> > > +    # line, i.e. at the beginning of the func1 call.
> > > +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> > > +	"test1: reverse-next to line with two functions"
> > > +
> > > +    # We should be stopped at the first instruction of the line.
> > > A
> > > reverse-step
> > > +    # should step back and stop at the beginning of the previous
> > > line b = 2,
> > > +    # i.e. not in func1 ().
> > > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > > +	"test1: reverse-stepi to previous line b = 2"
> > > +
> > > +
> > > +    # Setup for test 2
> > > +    clean_restart $executable
> > > +    runto_main
> > > +
> > > +    with_test_prefix "test2" {
> > > +	gdb_test_no_output "record" "turn on process record"
> > > +    }
> > > +
> > > +    # Test 2, reverse-step command
> > > +    # Set breakpoint at the line after the function calls.
> > > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > > +
> > > +    #  Continue to the start of the reverse-step test.
> > > +    #  Command definition:  reverse-step [count]
> > > +    #    Run the program backward until control reaches the
> > > start
> > > of a
> > > +    #    different source line; then stop it, and return control
> > > to gdb.
> > > +    #    Like the step command, reverse-step will only stop at
> > > the
> > > beginning
> > > +    #    of a source line. It “un-executes” the previously
> > > executed source
> > > +    #    line. If the previous source line included calls to
> > > debuggable
> > > +    #    functions, reverse-step will step (backward) into the
> > > called function,
> > > +    #    stopping at the beginning of the last statement in the
> > > called
> > > +    #    function (typically a return statement).  Also, as with
> > > the step
> > > +    #    command, if non-debuggable functions are called,
> > > reverse-
> > > step will
> > > +    #    run thru them backward without stopping.
> > > +
> > > +    gdb_continue_to_breakpoint \
> > > +	"test2: stopped at command reverse-step test start location" \
> > > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > > +
> > > +    # The first reverse step should take us call of func2 ().
> > > +    gdb_test "reverse-step" ".*END FUNC2.*" \
> > > +	"test2: reverse-step into func2 "
> > > +
> > > +    # The second reverse step should take us into func1 ().
> > > +    gdb_test "reverse-step" ".*END FUNC1.*" \
> > > +	"test2: reverse-step into func1 "
> > > +
> > > +    # The third reverse step should take us call of func1 ().
> > > +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > > +	"test2: reverse-step to line func1(); func2(), at call for
> > > func1 "
> > > +
> > > +    # We should be stopped at the first instruction of the line.
> > > A
> > > reverse
> > > +    # stepi should take us to b = 2 ().
> > > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > > +	"test2: reverse-stepi to line b = 2 "
> > > +}
> > > +
> > > +set srcfile  func-map-to-same-line.c
> > > +set executable func-map-to-same-line
> > 
> > Wondering if this test should use standard_testfile (like almost
> > every
> > other tests) to set these.
> 
> OK, changed to use the standard_testfile.
> 
> > > +
> > > +# test with and without gcc column info enabled
> > > +foreach_with_prefix with_column_info {yes no} {
> > > +    if {$with_column_info == "yes"} {
> > > +	set options [list debug column-info]
> > > +    } else {
> > > +	set options [list debug no-column-info]
> > > +    }
> > 
> > I didn't think of this when proposing the foreach_with_prefix, but
> > you
> > could perhaps use:
> > 
> >   foreach_with_prefix column_info_flag {column-info no-column-info}
> > 
> > ... to avoid this boilerplate.  You can then use $column_info_flag
> > directly when setting options.
> > 
> OK, that cleans things up a bit.  Changed.
> +
> +    if {[build_executable "failed to prepare" $executable $srcfile
> \
> +	     $options] == -1} {
> +	return -1
> +    }
> +
> +    clean_restart $executable
> 
> clean_restart can go in run_tests.
> 
> 
> +    run_tests
> +}
> 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..f20d778f40e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,58 @@
> +/* Copyright 2008-2023 Free Software Foundation, Inc.
> 
> Just wondering if the copyright years are right.
> 
> New files so yea, should just start with 2023.
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as
> published by
> +   the Free Software Foundation; either version 3 of the License,
> or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public
> License
> +   along with this program.  If not, see <
> http://www.gnu.org/licenses/
>   >.  */
> +
> +/* The purpose of this test is to create a DWARF line table that
> contains two
> +   or more entries for the same line.  When stepping (forwards or
> backwards),
> +   GDB should step over the entire line and not just a particular
> entry in the
> +   line table.  */
> +
> +int
> +main ()
> 
> void in the parenthesis
> 
> Fixed in both test files.
> 
> +{     /* TAG: main prologue */
> +  asm ("main_label: .globl main_label");
> +  int i = 1, j = 2, k;
> +  float f1 = 2.0, f2 = 4.1, f3;
> +  const char *str_1 = "foo", *str_2 = "bar", *str_3;
> +
> +  asm ("line1: .globl line1");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
> +
> +  asm ("line2: .globl line2");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
> +
> +  asm ("line3: .globl line3");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
> +
> +  asm ("line4: .globl line4");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
> +
> +  asm ("line5: .globl line5");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
> +
> +  asm ("line6: .globl line6");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
> +
> +  asm ("line7: .globl line7");
> +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
> +
> +  asm ("line8: .globl line8");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
> +
> +  asm ("main_return: .globl main_return");
> +  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
> +
> +  asm ("end_of_sequence: .globl end_of_sequence");
> +  return 0; /* TAG: main return */
> +}
> 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..16a359d90ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,156 @@
> +# Copyright 2008-2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as
> published by
> +# the Free Software Foundation; either version 3 of the License,
> or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public
> License
> +# along with this program.  If not, see <
> http://www.gnu.org/licenses/
> =  >.
> +
> +# When stepping (forwards or backwards), GDB should step over the
> entire line
> +# and not just a particular entry in the line table. This test was
> added to
> +# verify the find_line_range_start function properly sets the step
> range for a
> +# line that consists of multiple statements, i.e. multiple entries
> in the line
> +# table.  This test creates a DWARF line table that contains two
> entries for
> +# the same line to do the needed testing.
> +
> +# This test can only be run on targets which support DWARF-2 and
> use gas.
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +# The DWARF assembler requires the gcc compiler.
> +require is_c_compiler_gcc
> +
> +# This test suitable only for process that can do reverse
> execution
> +require supports_reverse
> +
> +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 distinct source lines
> being
> +	# mapped to the same line entry. Line 1, 5 and 8 contain 1
> statement
> +	# each.  Line 2 contains 2 statements.  Line 3 contains 3
> statements.
> +	program {
> +	    DW_LNE_set_address $main_start
> +	    line [gdb_get_line_number "TAG: main prologue"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line1
> +	    line [gdb_get_line_number "TAG: line 1" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line2
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line3
> +	    line [gdb_get_line_number "TAG: line 2" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line4
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line5
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line6
> +	    line [gdb_get_line_number "TAG: line 3" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line7
> +	    line [gdb_get_line_number "TAG: line 5" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address line8
> +	    line [gdb_get_line_number "TAG: line 8" ]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address main_return
> +	    line [gdb_get_line_number "TAG: main return"]
> +	    DW_LNS_copy
> +	    DW_LNE_set_address end_of_sequence
> +	    DW_LNE_end_sequence
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +runto_main
> +
> +# Print the line table
> +gdb_test_multiple "maint info line-table ${testfile}" "" {
> +    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[
> \t\]+Y\[^\r\n\]*" {
> +	lappend is_stmt $expect_out(1,string)
> +	exp_continue
> +    }
> +    -re -wrap "" {
> +    }
> +}
> +
> +# Do the reverse-step test
> +gdb_test_no_output "record" "turn on process record"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return"
> $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-step
> test" ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
> +
> +# At this point, GDB has already recorded the execution up until
> the return
> +# statement.  Reverse-step and test if GDB transitions between
> lines in the
> +# expected order.  It should reverse-step across lines 8, 5, 3, 2
> and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to
> line $line"
> +}
> +
> +## Clean restart, test reverse-next command
> +clean_restart ${testfile}
> +runto_main
> +gdb_test_no_output "record" "turn on process record, reverst-next
> test"
> +
> +set bp_main_return [gdb_get_line_number "TAG: main return"
> $srcfile]
> +gdb_breakpoint $srcfile:$bp_main_return
> +gdb_continue_to_breakpoint  "run to end of main, reverse-next
> test" ".*$srcfile:$bp_main_return.*"
> +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
> +
> +# At this point, GDB has already recorded the execution up until
> the return
> +# statement.  Reverse-next and test if GDB transitions between
> lines in the
> +# expected order.  It should reverse-next across lines 8, 5, 3, 2
> and 1.
> +foreach line {8 5 3 2 1} {
> +    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to
> line $line"
> +}
> 
> It seems like the step and next tests are identical, so I guess it
> could
> be factored out using:
> 
>   foreach_with_prefix method {step next} {
>       ...
>   }
> 
> ?
> 
> Yup, redid the code using the foreach_with_prefix.
> 
> Simon
> > yR
  

On 6/22/23 16:38, Carl Love wrote:
> Simon:
> 
> On Mon, 2023-06-19 at 13:58 -0400, Simon Marchi wrote:
> <snip>
> 
>>
> 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -114,6 +114,9 @@ static struct async_event_handler
>>> *infrun_async_inferior_event_token;
>>>     Starts off as -1, indicating "never enabled/disabled".  */
>>>  static int infrun_is_async = -1;
>>>  
>>> +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
>>> +					  struct
>>> execution_control_state *ecs);
>>> +
>>
>> This forward-declaration is not needed.
> 
> I tried removing the forward-declaration and the compile fails with the
> message:
> 
>    ../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:6773:1:
>    error: no previous declaration for ‘CORE_ADDR
>    update_line_range_start(CORE_ADDR, execution_control_state*)’ [-
>    Werror=missing-declarations]
>     6773 | update_line_range_start (CORE_ADDR pc, struct
>    execution_control_state *ecs)
>          | ^~~~~~~~~~~~~~~~~~~~~~~
>    cc1plus: all warnings being treated as errors
>    make[2]: *** [Makefile:1922: infrun.o] Error 1
>    make[2]: Leaving directory '/home/carll/GDB/build-reverse-multiple-
>    contiguous/gdb'
>    make[1]: *** [Makefile:13569: all-gdb] Error 2
>    make[1]: Leaving directory '/home/carll/GDB/build-reverse-multiple-
>    contiguous'
>    make: *** [Makefile:1005: all] Error 2
> 
> Leaving the forward declaration in the code.

Because you need to put "static" at the other place (line 6813).

Simon
  

Simon:

Thanks for the reply to my previous unintended send of this note before
it was done.  I did fix the forward declaration as you said.  The issue
with removing it was making sure the actual declaration is static.


On Mon, 2023-06-19 at 13:58 -0400, Simon Marchi wrote:
> On 5/16/23 18:54, Carl Love wrote:

<snip>

> > 

> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index efe2c00c489..31cd817c733 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -114,6 +114,9 @@ static struct async_event_handler
> > *infrun_async_inferior_event_token;
> >     Starts off as -1, indicating "never enabled/disabled".  */
> >  static int infrun_is_async = -1;
> >  
> > +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> > +					  struct
> > execution_control_state *ecs);
> > +
> 
> This forward-declaration is not needed.

Per second email, removed the forward declaration and made the actual
declaration static.  

> 
> >  /* See infrun.h.  */
> >  
> >  void
> > @@ -6769,6 +6772,25 @@ handle_signal_stop (struct
> > execution_control_state *ecs)
> >    process_event_stop_test (ecs);
> >  }
> >  
> > +CORE_ADDR
> > +update_line_range_start (CORE_ADDR pc, struct
> > execution_control_state *ecs)
> 
> Please add a comment for the function.

OK, added comment.

> 
> > +{
> > +  /* The line table may have multiple entries for the same source
> > code line.
> > +     Given the PC, check the line table and return the PC that
> > corresponds
> > +     to the line table entry for the source line that PC is
> > in.  */
> > +  CORE_ADDR start_line_pc = ecs->event_thread-
> > >control.step_range_start;
> > +  gdb::optional<CORE_ADDR> real_range_start;
> > +
> > +  /* Call find_line_range_start to get the smallest address in the
> > +     linetable for multiple Line X entries in the line table.  */
> > +  real_range_start = find_line_range_start (pc);
> > +
> > +  if (real_range_start.has_value ())
> > +    start_line_pc = *real_range_start;
> > +
> > +  return start_line_pc;
> 
> When I read this, I wonder: why was control.step_range_start not set
> to
> the "real" range start in the first place (not only in the context of
> reverse execution, every time it is set)?  It would seem more robust
> than patching it afterwards in some very specific spots.
> 
> I could see some benefits for range-stepping uses cases too (relevant
> when debugging remotely).  Using your example here:
> 
>    Line X - [0x0 - 0x8]
>    Line X - [0x8 - 0x10]
>    Line X - [0x10 - 0x18]
> 
> Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
> conditional jump to 0x5.  It seems like current GDB would send a
> "range
> step" request to GDBserver, to step in the [0x10, 0x18[ range.  When
> reaching 0x5, execution would stop, and GDB would resume it again
> with
> the [0x0,0x8[ range.  When reaching 0x8, it would stop again, GDB
> would
> resume it with [0x8,0x10[, and so on.  If GDB could send a "range
> step"
> request with the [0x0,0x18[ range, it would avoid those unnecessary
> intermediary stop.
> 

We looked at trying to set control.step_range_start to the real start
range initially.  Ulrich brought this point up in our internal review
of the patch.  

So, when I am in function finish_backward() in infcmd.c I have no way
to determine what the previous PC was.  If I assume it was the previous
value, i.e. pc - 4byes (on PowerPC).  I get a gdb internal error.  It
seems that I am not allowed to change the line range to something that
does not include the current pc value.  

   ../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:2740:
   internal-error: resume_1:
   Assertion `pc_in_thread_step_range (pc, tp)' failed.

In order to make that work, we concluded that it would probably entail
a much bigger change to how reverse execution works which would be
beyond the scope of what this patch is trying to fix.  So, being able
to do what I believe you want to do is in theory possible but it would
require a larger, independent change to what this patch is trying to
fix.


> > +}
> > +
> >  /* Come here when we've got some debug event / signal we can
> > explain
> >     (IOW, not a random signal), and test whether it should cause a
> >     stop, or whether we should resume the inferior (transparently).
> > @@ -7570,6 +7592,28 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >  
> >        if (stop_pc_sal.is_stmt)
> >  	{
> > +	  if (execution_direction == EXEC_REVERSE)
> > +	    {
> > +	      /* We are stepping backwards make sure we have reached
> > the
> > +		 beginning of the line.  */
> > +	      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +	      CORE_ADDR start_line_pc
> > +		= update_line_range_start (stop_pc, ecs);
> > +
> > +	      if (stop_pc != start_line_pc)
> > +		{
> > +		  /* Have not reached the beginning of the source code
> > line.
> > +		     Set a step range.  Execution should stop in any
> > function
> > +		     calls we execute back into before reaching the
> > beginning
> > +		     of the line.  */
> > +		  ecs->event_thread->control.step_range_start =
> > start_line_pc;
> > +		  ecs->event_thread->control.step_range_end = stop_pc;
> > +		  set_step_info (ecs->event_thread, frame,
> > stop_pc_sal);
> > +		  keep_going (ecs);
> > +		  return;
> > +		}
> > +	    }
> > +
> >  	  /* We are at the start of a statement.
> >  
> >  	     So stop.  Note that we don't stop if we step into the
> > middle of a
> > @@ -7632,6 +7676,19 @@ process_event_stop_test (struct
> > execution_control_state *ecs)
> >      set_step_info (ecs->event_thread, frame, stop_pc_sal);
> >  
> >    infrun_debug_printf ("keep going");
> > +
> > +  if (execution_direction == EXEC_REVERSE)
> > +    {
> > +      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> > +
> > +      /* Make sure the stop_pc is set to the beginning of the
> > line.  */
> > +      if (stop_pc != ecs->event_thread->control.step_range_start)
> > +	{
> > +	  stop_pc = update_line_range_start (stop_pc, ecs);
> > +	  ecs->event_thread->control.step_range_start = stop_pc;
> > +	}
> > +    }
> > +
> >    keep_going (ecs);
> >  }
> >  
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index 27611a34ec4..91d35616eb9 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3282,6 +3282,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);
> 
> Unnecessary parenthesis.

Fixed.


> 
> > +}
> > +
> > +/* 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;
> 
> Algorithmic complexity question: given that line tables are sorted by
> address, would it work to start at the current line table item, and
> go
> look at the previous ones until we find one that is no longer
> contiguous and same line?  find_pc_line is somewhat heavy, so if we
> don't need to do it repeatedly...  

Yes, line tables are sorted by address.  The issue with just looking
back thru the current line table is branches/function calls.  Suppose
we arrived at the current the source code line for a given entry in the
line table via a branch or a function call. If we looked at the source
code line number for the previous entry in the line table, it may be
the same in this case if we jumped into the line.  The pc for the
previous entry in the line table would not for the line we came from
because we arrived via a branch/function call.  The address we came
from could even be in another symbol table.  We need to walk back one
pc at a time, to see where we came from, and checking to see if the
symbol table entry changed or the line number changed to determine
where the beginning of the line is.  So, we need to call
find_pc_line(), to find out if the previous PC is in a different line
or if it corresponds to a different symbol table.

Just searching back in the "current" line table for a line number
change to figure out the first pc in the line is not sufficient.  I
don't see anyway to avoid calling find_pc_line() to determine if the
symbol table for the previous PC changed or not.

Note, I did play with just walking back thru the line table and that
does work for the test cases. But I don't believe it will work in
general.  The pointer to the base of the linetable for the symbol table
and the maximum number of lines in the linetable are accessible in
function find_pc_sect_line. Given that information, then we search the
line table for the first linetable entry that corresponds to the line
number we want and then return the pc for that linetable entry.  We
would need to do a binary search since we don't know the actual offset
into the table for the current pc.  The time (log base 2 of the
linetable size) to do the search is obviously a function of the size of
the linetable.  If we have function find_pc_sect_line (which is called
by our intial call to find_pc_line()) record the offset into the line
table where the current line number occurs and store it in struct
symtab, then we can very efficiently start at the correct linetable
entry and walk back as you suggested.  But what we can't tell is if the
execution history actually came in via the pc values in the earlier
linetable entries.  If execution didn't come in via an earlier entry in
the linetable, the returned pc for the first linetable entry of the
desired source code line may never be reached during our reverse
execution.  Obviously in that case, gdb will not stop at the correct
location.
 
> 
> > +}
> > +
> >  /* See symtab.h.  */
> >  
> >  struct symtab *
> > diff --git a/gdb/symtab.h b/gdb/symtab.h
> > index 404d0ab30a8..f54305636da 100644
> > --- a/gdb/symtab.h
> > +++ b/gdb/symtab.h
> > @@ -2346,6 +2346,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.
> 
> I think that putting this example in the comment is great.  It makes
> it
> much more obvious what the function specifically does.
> 
> > +*/
> > +
> > +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/func-map-to-same-line.c
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > new file mode 100644
> > index 00000000000..da944874e86
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > @@ -0,0 +1,36 @@
> > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.
> > +
> > +   This test is used to test the reverse-step and reverse-next
> > instruction
> > +   execution for a source line that contains multiple function
> > calls.  */
> > +
> > +void
> > +func1 ()
> > +{
> > +} /* END FUNC1 */
> > +
> > +void
> > +func2 ()
> > +{
> > +} /* END FUNC2 */
> > +
> > +int main ()
> 
> int
> main (void)
> 

Fixed.

> > +{
> > +  int a, b;
> > +  a = 1;
> > +  b = 2;
> > +  func1 (); func2 ();
> > +  a = a + b;     /* START REVERSE TEST */
> > +}
> > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > new file mode 100644
> > index 00000000000..89e226b0f84
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > @@ -0,0 +1,140 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >  >.  */
> > +
> > +# This file is part of the GDB testsuite.  It tests reverse
> > stepping.
> > +# Lots of code borrowed from "step-test.exp".
> > +
> > +# This test checks to make sure there is no regression failures
> > for
> > +# the reverse-next command when stepping back over two functions
> > in
> > +# the same line.
> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +
> > +proc run_tests {} {
> > +    global srcfile
> > +    global executable
> > +
> > +    runto_main
> 
> We typically check for runto_main's success:

Fixed a couple instances of runto_main in this test case and in the
other test case.

> 
>   if { ![runto_main] } {
>       return
>   }
> 
> runto_main logs a FAIL on failure.  There are a few runto_mains in
> the
> patch.
> 
> > +    set target_remote [gdb_is_target_remote]
> 
> target_remote seems unused

Removed.

> 
> > +
> > +    with_test_prefix "test1" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> 
> with_test_prefix with a single test in it is really just the same as:
> 
>   gdb_test_no_output "record" "test1: turn on process record"
> 
> In fact, you have some other tests with the "test1:" or "test2:"
> prefix,
> I think they should be moved to the with_test_prefix.  And maybe use
> "next" and "step" instead of "test1" and "test2".

Yup, cleaner to have the with_test_prefix cover the whole test. 
Changed test1 to next-test and test2 to step-next.

> 
> > +
> > +    # This regression test verifies the reverse-step and reverse-
> > next commands
> > +    # work properly when executing backwards thru a source line
> > containing
> > +    # two function calls on the same source line, i.e. func1 ();
> > func2 ();
> > +    # This test is compiled so the dwarf info not contain the line
> > table
> > +    # information.
> > +
> > +    # Test 1, reverse-next command
> > +    # Set breakpoint at the line after the function calls.
> > +    set bp_start_reverse_test [gdb_get_line_number "START REVERSE
> > TEST" \
> > +				   $srcfile]
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    # Continue to break point for reverse-next test.
> > +    # Command definition:  reverse-next [count]
> > +    #   Run backward to the beginning of the previous line
> > executed in the
> > +    #   current (innermost) stack frame. If the line contains
> > function calls,
> > +    #   they will be “un-executed” without stopping. Starting from
> > the first
> > +    #   line of a function, reverse-next will take you back to the
> > caller of
> > +    #   that function, before the function was called, just as the
> > normal next
> > +    #   command would take you from the last line of a function
> > back to its
> > +    #   return to its caller 2 .
> > +    gdb_continue_to_breakpoint \
> > +	"test1: stopped at command reverse-next test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The reverse-next should step all the way back to the
> > beginning of the
> > +    # line, i.e. at the beginning of the func1 call.
> > +    gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test1: reverse-next to line with two functions"
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse-step
> > +    # should step back and stop at the beginning of the previous
> > line b = 2,
> > +    # i.e. not in func1 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test1: reverse-stepi to previous line b = 2"
> > +
> > +
> > +    # Setup for test 2
> > +    clean_restart $executable
> > +    runto_main
> > +
> > +    with_test_prefix "test2" {
> > +	gdb_test_no_output "record" "turn on process record"
> > +    }
> > +
> > +    # Test 2, reverse-step command
> > +    # Set breakpoint at the line after the function calls.
> > +    gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > +    #  Continue to the start of the reverse-step test.
> > +    #  Command definition:  reverse-step [count]
> > +    #    Run the program backward until control reaches the start
> > of a
> > +    #    different source line; then stop it, and return control
> > to gdb.
> > +    #    Like the step command, reverse-step will only stop at the
> > beginning
> > +    #    of a source line. It “un-executes” the previously
> > executed source
> > +    #    line. If the previous source line included calls to
> > debuggable
> > +    #    functions, reverse-step will step (backward) into the
> > called function,
> > +    #    stopping at the beginning of the last statement in the
> > called
> > +    #    function (typically a return statement).  Also, as with
> > the step
> > +    #    command, if non-debuggable functions are called, reverse-
> > step will
> > +    #    run thru them backward without stopping.
> > +
> > +    gdb_continue_to_breakpoint \
> > +	"test2: stopped at command reverse-step test start location" \
> > +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > +    # The first reverse step should take us call of func2 ().
> > +    gdb_test "reverse-step" ".*END FUNC2.*" \
> > +	"test2: reverse-step into func2 "
> > +
> > +    # The second reverse step should take us into func1 ().
> > +    gdb_test "reverse-step" ".*END FUNC1.*" \
> > +	"test2: reverse-step into func1 "
> > +
> > +    # The third reverse step should take us call of func1 ().
> > +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +	"test2: reverse-step to line func1(); func2(), at call for
> > func1 "
> > +
> > +    # We should be stopped at the first instruction of the line. A
> > reverse
> > +    # stepi should take us to b = 2 ().
> > +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> > +	"test2: reverse-stepi to line b = 2 "
> > +}
> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> 
> Wondering if this test should use standard_testfile (like almost
> every
> other tests) to set these.

OK, changed to use the standard_testfile.

> 
> > +
> > +# test with and without gcc column info enabled
> > +foreach_with_prefix with_column_info {yes no} {
> > +    if {$with_column_info == "yes"} {
> > +	set options [list debug column-info]
> > +    } else {
> > +	set options [list debug no-column-info]
> > +    }
> 
> I didn't think of this when proposing the foreach_with_prefix, but
> you
> could perhaps use:
> 
>   foreach_with_prefix column_info_flag {column-info no-column-info}
> 
> ... to avoid this boilerplate.  You can then use $column_info_flag
> directly when setting options.

Yes, that makes things a bit cleaner.  Changed.


> 
> > +
> > +    if {[build_executable "failed to prepare" $executable $srcfile
> > \
> > +	     $options] == -1} {
> > +	return -1
> > +    }
> > +
> > +    clean_restart $executable
> 
> clean_restart can go in run_tests.

Moved.

> 
> > +    run_tests
> > +}
> > 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..f20d778f40e
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> > @@ -0,0 +1,58 @@
> > +/* Copyright 2008-2023 Free Software Foundation, Inc.
> 
> Just wondering if the copyright years are right.

These are new test files so, yea, should start with 2023.

> 
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> >   >.  */
> > +
> > +/* The purpose of this test is to create a DWARF line table that
> > contains two
> > +   or more entries for the same line.  When stepping (forwards or
> > backwards),
> > +   GDB should step over the entire line and not just a particular
> > entry in the
> > +   line table.  */
> > +
> > +int
> > +main ()
> 
> void in the parenthesis

Fixed in both tests.

> 
> > +{     /* TAG: main prologue */
> > +  asm ("main_label: .globl main_label");
> > +  int i = 1, j = 2, k;
> > +  float f1 = 2.0, f2 = 4.1, f3;
> > +  const char *str_1 = "foo", *str_2 = "bar", *str_3;
> > +
> > +  asm ("line1: .globl line1");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 1 */
> > +
> > +  asm ("line2: .globl line2");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 2 */
> > +
> > +  asm ("line3: .globl line3");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 3 */
> > +
> > +  asm ("line4: .globl line4");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 4 */
> > +
> > +  asm ("line5: .globl line5");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 5 */
> > +
> > +  asm ("line6: .globl line6");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 6 */
> > +
> > +  asm ("line7: .globl line7");
> > +  k = i; f3 = f1; str_3 = str_1;    /* TAG: line 7 */
> > +
> > +  asm ("line8: .globl line8");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: line 8 */
> > +
> > +  asm ("main_return: .globl main_return");
> > +  k = j; f3 = f2; str_3 = str_2;    /* TAG: main return */
> > +
> > +  asm ("end_of_sequence: .globl end_of_sequence");
> > +  return 0; /* TAG: main return */
> > +}
> > 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..16a359d90ec
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> > @@ -0,0 +1,156 @@
> > +# Copyright 2008-2023 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as
> > published by
> > +# the Free Software Foundation; either version 3 of the License,
> > or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/
> > =  >.
> > +
> > +# When stepping (forwards or backwards), GDB should step over the
> > entire line
> > +# and not just a particular entry in the line table. This test was
> > added to
> > +# verify the find_line_range_start function properly sets the step
> > range for a
> > +# line that consists of multiple statements, i.e. multiple entries
> > in the line
> > +# table.  This test creates a DWARF line table that contains two
> > entries for
> > +# the same line to do the needed testing.
> > +
> > +# This test can only be run on targets which support DWARF-2 and
> > use gas.
> > +load_lib dwarf.exp
> > +require dwarf2_support
> > +
> > +# The DWARF assembler requires the gcc compiler.
> > +require is_c_compiler_gcc
> > +
> > +# This test suitable only for process that can do reverse
> > execution
> > +require supports_reverse
> > +
> > +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 distinct source lines
> > being
> > +	# mapped to the same line entry. Line 1, 5 and 8 contain 1
> > statement
> > +	# each.  Line 2 contains 2 statements.  Line 3 contains 3
> > statements.
> > +	program {
> > +	    DW_LNE_set_address $main_start
> > +	    line [gdb_get_line_number "TAG: main prologue"]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line1
> > +	    line [gdb_get_line_number "TAG: line 1" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line2
> > +	    line [gdb_get_line_number "TAG: line 2" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line3
> > +	    line [gdb_get_line_number "TAG: line 2" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line4
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line5
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line6
> > +	    line [gdb_get_line_number "TAG: line 3" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line7
> > +	    line [gdb_get_line_number "TAG: line 5" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address line8
> > +	    line [gdb_get_line_number "TAG: line 8" ]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address main_return
> > +	    line [gdb_get_line_number "TAG: main return"]
> > +	    DW_LNS_copy
> > +	    DW_LNE_set_address end_of_sequence
> > +	    DW_LNE_end_sequence
> > +	}
> > +    }
> > +}
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} \
> > +	[list $srcfile $asm_file] {nodebug} ] } {
> > +    return -1
> > +}
> > +
> > +runto_main
> > +
> > +# Print the line table
> > +gdb_test_multiple "maint info line-table ${testfile}" "" {
> > +    -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[
> > \t\]+Y\[^\r\n\]*" {
> > +	lappend is_stmt $expect_out(1,string)
> > +	exp_continue
> > +    }
> > +    -re -wrap "" {
> > +    }
> > +}
> > +
> > +# Do the reverse-step test
> > +gdb_test_no_output "record" "turn on process record"
> > +
> > +set bp_main_return [gdb_get_line_number "TAG: main return"
> > $srcfile]
> > +gdb_breakpoint $srcfile:$bp_main_return
> > +gdb_continue_to_breakpoint  "run to end of main, reverse-step
> > test" ".*$srcfile:$bp_main_return.*"
> > +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-step test"
> > +
> > +# At this point, GDB has already recorded the execution up until
> > the return
> > +# statement.  Reverse-step and test if GDB transitions between
> > lines in the
> > +# expected order.  It should reverse-step across lines 8, 5, 3, 2
> > and 1.
> > +foreach line {8 5 3 2 1} {
> > +    gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to
> > line $line"
> > +}
> > +
> > +## Clean restart, test reverse-next command
> > +clean_restart ${testfile}
> > +runto_main
> > +gdb_test_no_output "record" "turn on process record, reverst-next
> > test"
> > +
> > +set bp_main_return [gdb_get_line_number "TAG: main return"
> > $srcfile]
> > +gdb_breakpoint $srcfile:$bp_main_return
> > +gdb_continue_to_breakpoint  "run to end of main, reverse-next
> > test" ".*$srcfile:$bp_main_return.*"
> > +gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-next test"
> > +
> > +# At this point, GDB has already recorded the execution up until
> > the return
> > +# statement.  Reverse-next and test if GDB transitions between
> > lines in the
> > +# expected order.  It should reverse-next across lines 8, 5, 3, 2
> > and 1.
> > +foreach line {8 5 3 2 1} {
> > +    gdb_test "reverse-next" ".*TAG: line $line.*" "reverse next to
> > line $line"
> > +}
> 
> It seems like the step and next tests are identical, so I guess it
> could
> be factored out using:
> 
>   foreach_with_prefix method {step next} {
>       ...
>   }
> 

Yes, changed to use foreach_with_prefix.

                     Carl