RE: [PATCH] Fix reverse stepping multiple contiguous PC ranges over the line table.

[Carl Love <cel@us.ibm.com>]

Bruno:

On Wed, 2023-05-03 at 11:53 +0200, Bruno Larsen wrote:
> On 27/04/2023 22:59, Carl Love wrote:

<snip>
> 
> Hi Carl, thanks for clarifying the intended commit message. I'm
> reacting 
> to it here because I also have some thoughts on the code, now that I 
> managed to apply it locally.
> 
> Starting on the commit message, it would be nice to have a 1-line 
> description of the problem before describing the scenarios in depth. 
> Taking the first line of the previous block is enough IMO.

Yes, agreed.  Kept the first line before the discussion of the
different failure scenarios.

> 
> > 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.
> > 
> > This happens because we have this check in
> > infrun.c:process_event_stop_test:
> > 
> >          /* When stepping backward, stop at beginning of line range
> >             (unless it's the function entry point, in which case
> >             keep going back to the call point).  */
> >          CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> >          if (stop_pc == ecs->event_thread->control.step_range_start
> >           && stop_pc != ecs->stop_func_start
> >           && execution_direction == EXEC_REVERSE)
> >            end_stepping_range (ecs);
> >          else
> >            keep_going (ecs);
> > 
> > Since we've reached ecs->event_thread->control.step_range_start, we
> > stop
> > stepping backwards.
> I think these last 3 paragraphs should be moved. I like to finish 
> commits with a description of the solution, rather than having it in
> the 
> middle of the text. Also, I think we like to avoid mentioning
> explicit 
> code in the commit text (though I might be mistaken).

OK, I moved the fix discussion to the end.  I also dropped the explicit
reference to infrun.c:process_event_stop_test.

> > The right thing to do is to look for adjacent PC ranges for the
> > same line,
> > until we notice a line change. Then we take that as the start PC of
> > the
> > range.
> > 
> > Another solution I thought about is to merge the contiguous ranges
> > when
> > we are reading the line tables. Though I'm not sure if we really
> > want to
> > process that data as opposed to keeping it as the compiler created,
> > and
> > then working around that.
> This paragraph doesn't need to be here in the final commit message
> IMO. 
> It was nice context for the mailing list but is not necessary for
> future 

OK, removed it from the commit log and need to update the mailing list
message with this.

> reference, I don't think.
> > The test case gdb.reverse/map-to-same-line.exp is added to test the
> > fix
> > for the issues in scenario 1.
> > 
> > ---------------------------------------------------------
> > 
> > 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 test cases gdb.reverse/func-map-to-same-line-no-colum-info.exp
> > and
> > gdb.reverse/func-map-to-same-line.exp were 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>
> > ---
> >   gdb/infrun.c                                  |  57 +++++++
> >   gdb/symtab.c                                  |  49 ++++++
> >   gdb/symtab.h                                  |  16 ++
> >   .../func-map-to-same-line-no-column-info.exp  | 135
> > ++++++++++++++++
> >   .../gdb.reverse/func-map-to-same-line.c       |  36 +++++
> >   .../gdb.reverse/func-map-to-same-line.exp     | 123
> > ++++++++++++++
> >   gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
> >   .../gdb.reverse/map-to-same-line.exp          | 153
> > ++++++++++++++++++
> >   8 files changed, 627 insertions(+)
> >   create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-
> > line-no-column-info.exp
> >   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 2f1c6cd694b..59374a05471 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -113,6 +113,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
> > @@ -6768,6 +6771,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 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).
> > @@ -7569,6 +7591,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
> > @@ -7631,6 +7675,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-no-
> > column-info.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line-
> > no-column-info.exp
> > new file mode 100644
> > index 00000000000..20529c90fc2
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line-no-column-
> > info.exp
> > @@ -0,0 +1,135 @@
> > +# 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.
> > +
> > +if ![supports_reverse] {
> > +    return
> > +}
> Nowadays you should use require instead of the if clause, like in 
> gdb.reverse/break-reverse.exp

OK, changed that

> > +
> > +# This test uses the gcc no-column-info command.
> Correct me if I'm wrong, but it seems to me that the other test is a 
> more generic version of this one, so this test could check for a gcc 
> recent enough to support this feature, instead of just generically
> gcc. 
> That said, gcc added it on version 
> 7(
> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0029b929c9719a
>  ), is it 
> old enough that we don't care?

GCC supports line tables, I don't know that clang or other compilers
do.  So all we really need is to check for gcc.  The line table stuff
was added a long time ago so not sure that we really need to check for
version 7 at this point.  So just checked that we are using gcc.  The
"other test" func-map-to-same-line.exp expects the line table so it
should probably also be checking that we are using gcc.

> > +if ![is_c_compiler_gcc] {
> > +    unsupported "gcc is required for this test"
> > +    return 0
> > +}
> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> > +
> > +set options [list debug additional_flags=-gno-column-info]
> > +
> > +if {[build_executable "failed to prepare" $executable $srcfile
> > $options] == -1}\
> > + {
> > +    return -1
> > +}
> > +
> > +clean_restart $executable
> > +
> > +runto_main
> > +set target_remote [gdb_is_target_remote]
> > +
> > +if [supports_process_record] {
> > +    # Activate process record/replay.
> > +    gdb_test_no_output "record" "turn on process record for test1"
> > +}
> > +
> > +# 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 \
> > +    "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 \\(\\);.*" \
> > +    "reverse-next to line with two functions"
> > +
> > +# 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-step" ".*b = 2;.*" \
> > +    "reverse-step to previous line b = 2"
> The point of this test is to confirm that we are at the very first 
> instruction of the line, right? So I would think it is better to do
> a 
> reverse-stepi here, to make sure that even walking a single
> instruction 
> we reach a different line. 

Yes, we should be on the first instruction of the line so stepi would
be a better test to prove we are really on the first instruction. 
Changed the reverse-step to reverse-stepi.

> Either that or doing what Pedro did in his 
> email: save the PC before executing the line, then do and undo the
> line 
> and confirm that PCs match exactly.
> > +
> > +
> > +# Setup for test 2
> > +# Go back to the start of the function
> > +gdb_test "reverse-continue" "a = 1;" "At start of main, setup for
> > test 2"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test1"
> > +gdb_test_no_output "record" "turn on process record for test2"
> Since you don't require process record for this test, you can't
> assume 
> these to work. I think its better to clean restart and record if the 
> process supports recording, this way you're sure to reset history no 
> matter the inferior.

No, the test requires process record.  If record is not supported, we
can't do reverse execution.  That said, doing a "clean restart" and
then record would be another way, probably better way, of clearing the
history.  Put in a clean restart rather than turning off/on the
recording to clear the log file.

> > +
> > +# Delete all breakpoints and catchpoints.
> > +delete_breakpoints
> > +
> > +
> > +# 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 \
> > +    "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" ".*}.*" \
> > +    "reverse-step into func2 "
> > +
> > +# The second reverse step should take us into func1 ().
> > +gdb_test "reverse-step" ".*}.*" \
> > +    "reverse-step into func1 "
> > +
> > +# The third reverse step should take us call of func1 ().
> > +gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +    "reverse-step to line func1(); func2(), at call for func1 "
> > +
> > +# The fourth reverse step should take us to b = 2 ().
> > +gdb_test "reverse-step" ".*b = 2;.*" \
> > +    "reverse-step to line b = 2 "
> Ditto from the other test like this. 

Yes, stepi is a better test to prove you were on the first instruction
in the line.  Changed.

> Also, I feel that, while the test 
> name for the last 2 gdb_test are different, they don't meaningfully 
> communicate which part of the test is failing. I think it would be 
> better if you differentiated them by adding "for step test" or "for
> test 
> 2" at the end of the name would make it easier to understand where 
> things went wrong when looking at the sum file.

OK, agreed.  For the above tests I added "test#," where # is either 1
or 2 to the test names.  For example:

+gdb_test "reverse-step" ".*b = 2;.*" \ 
+    "reverse-step to line b = 2 "

was changed to 
+gdb_test "reverse-stepi" ".*b = 2;.*" \
+    "test2, reverse-step to line b = 2 "

I also updated the test to identify the closing } for func1 and func2
to make it clearer in the test which function we just steped back into.

> > 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..e9787ef9ff5
> > --- /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 ()
> > +{
> > +}
> > +
> > +void
> > +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..b632a236bbe
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> > @@ -0,0 +1,123 @@
> > +# 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.
> > +
> > +if ![supports_reverse] {
> > +    return
> > +}
> 
> I'm not sure it's worth separating these 2 tests into separate
> files. 
> You could instead just have most of the test defined as a proc, and
> call 
> it twice, once after compiling the inferior with column info, the
> other 
> compiling without if gcc is used. This way it's less likely that the 
> tests will diverge over time.

OK, good point.  I created a test proc which is then called with the
binary compiled with and without line information.  The extra exp test
file was removed.
> 
> > +
> > +standard_testfile
> > +
> > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile]
> > } {
> > +    return -1
> > +}
> > +
> > +runto_main
> > +set target_remote [gdb_is_target_remote]
> > +
> > +if [supports_process_record] {
> > +    # Activate process record/replay.
> > +    gdb_test_no_output "record" "turn on process record for test1"
> > +}
> > +
> > +# 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
> > ();
> > +# The assumption for this test is the dwarf info contain the
> > column
> > +# 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 \
> > +    "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 \\(\\);.*" \
> > +    "reverse-next to line with two functions"
> > +
> > +# 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-step" ".*b = 2;.*" \
> > +    "reverse-step to previous line b = 2"
> > +
> > +
> > +# Setup for test 2
> > +# Go back to the start of the function
> > +gdb_test "reverse-continue" "a = 1;" "At start of main, setup for
> > test 2"
> > +
> > +# Turn off record to clear logs and turn on again
> > +gdb_test "record stop"  "Process record is stopped.*" \
> > +    "turn off process record for test1"
> > +gdb_test_no_output "record" "turn on process record for test2"
> > +
> > +# Delete all breakpoints and catchpoints.
> > +delete_breakpoints
> > +
> > +
> > +# 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 \
> > +    "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" ".*}.*" \
> > +    "reverse-step into func2 "
> > +
> > +# The second reverse step should take us into func1 ().
> > +gdb_test "reverse-step" ".*}.*" \
> > +    "reverse-step into func1 "
> > +
> > +# The third reverse step should take us call of func1 ().
> > +gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> > +    "reverse-step to line func1(); func2(), at call for func1 "
> > +
> > +# The fourth reverse step should take us to b = 2 ().
> > +gdb_test "reverse-step" ".*b = 2;.*" \
> > +    "reverse-step to line b = 2 "
> > 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..a01579c2a8d
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> > @@ -0,0 +1,153 @@
> > +# 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.
> > +
> > +load_lib dwarf.exp
> > +
> > +# This test can only be run on targets which support DWARF-2 and
> > use gas.
> > +if {![dwarf2_support]} {
> > +    unsupported "dwarf2 support required for this test"
> > +    return 0
> > +}
> Again, the new way to check for these is "required". And IIUC, you
> can 
> add multiple requirements into a singe require call.

OK, updated.


> > +
> > +if [get_compiler_info] {
> > +    return -1
> > +}
> > +
> > +# The DWARF assembler requires the gcc compiler.
> > +if ![is_c_compiler_gcc] {
> > +    unsupported "gcc is required for this test"
> > +    return 0
> > +}
> > +
> > +# This test suitable only for process record-replay
> > +if ![supports_process_record] {
> > +    return
> > +}
> > +
> > +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
> > +}
> > +
> > +if ![runto_main] {
> > +    return -1
> > +}
> runto_main already errors out and leaves, I think, so no need for the
> if 
> clause.

OK, updated.

> > +
> > +# 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 "" {
> > +    }
> > +}
> > +
> > +# Activate process record/replay
> > +gdb_test_no_output "record" "turn on process record"
> > +
> > +gdb_test "tbreak main_return" "Temporary breakpoint .*"
> > "breakpoint at return"
> you can set a temporary breakpoint using gdb_breakpoint "..."
> temporary, 
> no need to manually call tbreak.
> > +gdb_test "continue" "Temporary breakpoint .*" "run to end of main"
> gdb_continue_to_breakpoint can handle temporary breakpoints as well.

OK, updated the break and continue statements.

> > +gdb_test "display \$pc" ".*pc =.*" "display pc"
> > +
> > +# 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"
> > +}
> 
> I'm not sure if it is needed, but I don't think it would hurt to
> also 
> test reverse-next in a separate foreach right after this one.

OK, added a clean restart, run to the end of main then do reverse next
using a foreach line.  
 
                      Carl