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

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

[Luis Machado]

v4:
- Updated testcase to make it a bit longer so it can exercise reverse-stepping
  multiple times.
- Cleaned up debugging prints.

v3:
- Updated testcase.  The format for writing the DWARF program body in the
  testcase expect file changed.
  See commit gdb/testsuite/dwarf: simplify line number program syntax
  (commit d4c4a2298cad06ca71cfef725f5248f68205f0be)

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

--

When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
the ppc backend), I noticed some failures in gdb.reverse/solib-precsave.exp
and gdb.reverse/solib-reverse.exp.

The failure happens around the following code:

38  b[1] = shr2(17);		/* middle part two */
40  b[0] = 6;   b[1] = 9;	/* generic statement, end part two */
42  shr1 ("message 1\n");	/* shr1 one */

Normal execution:

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

Reverse execution:

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

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

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

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

When stepping forward from line 40, we skip both of these ranges and land on
line 42. When stepping backward from line 42, we stop at the start PC of the
second (or first, going backwards) range of line 40.

This happens because we have this check in infrun.c:process_event_stop_test:

      /* When stepping backward, stop at beginning of line range
	 (unless it's the function entry point, in which case
	 keep going back to the call point).  */
      CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
      if (stop_pc == ecs->event_thread->control.step_range_start
	  && stop_pc != ecs->stop_func_start
	  && execution_direction == EXEC_REVERSE)
	end_stepping_range (ecs);
      else
	keep_going (ecs);

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

The right thing to do is to look for adjacent PC ranges for the same line,
until we notice a line change. Then we take that as the start PC of the
range.

Another solution I thought about is to merge the contiguous ranges when
we are reading the line tables. Though I'm not sure if we really want to process
that data as opposed to keeping it as the compiler created, and then working
around that.

In any case, the following patch addresses this problem.

Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love has
verified that it does fix a similar issue on ppc.

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

I see similar failures on x86_64 in the gdb.reverse tests
(gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). Those are
also fixed by this patch.

The included testcase (based on a test Carl wrote) exercises this problem for
Arm, ppc and x86. It shows full passes with the patch applied.

Co-authored-by: Carl Love <cel@us.ibm.com>
---
 gdb/infrun.c                                  |  22 ++-
 gdb/symtab.c                                  |  49 +++++++
 gdb/symtab.h                                  |  16 +++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
 .../gdb.reverse/map-to-same-line.exp          | 136 ++++++++++++++++++
 5 files changed, 277 insertions(+), 1 deletion(-)
 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 02c98b50c8c..e9e14e58745 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6917,11 +6917,31 @@ process_event_stop_test (struct execution_control_state *ecs)
 	 have software watchpoints).  */
       ecs->event_thread->control.may_range_step = 1;
 
+      /* When we are stepping inside a particular line range, in reverse,
+	 and we are sitting at the first address of that range, we need to
+	 check if this address also shows up in another line range as the
+	 end address.
+
+	 If so, we need to check what line such a step range points to.
+	 If it points to the same line as the current step range, that
+	 means we need to keep going in order to reach the first address
+	 of the line range.  We repeat this until we eventually get to the
+	 first address of a particular line we're stepping through.  */
+      CORE_ADDR range_start = ecs->event_thread->control.step_range_start;
+      if (execution_direction == EXEC_REVERSE)
+	{
+	  gdb::optional<CORE_ADDR> real_range_start
+	    = find_line_range_start (ecs->event_thread->stop_pc ());
+
+	  if (real_range_start.has_value ())
+	    range_start = *real_range_start;
+	}
+
       /* When stepping backward, stop at beginning of line range
 	 (unless it's the function entry point, in which case
 	 keep going back to the call point).  */
       CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
-      if (stop_pc == ecs->event_thread->control.step_range_start
+      if (stop_pc == range_start
 	  && stop_pc != ecs->stop_func_start
 	  && execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 4b33d6c91af..de4cb5dd0eb 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3433,6 +3433,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 b1cf84f756f..226fe8803db 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2285,6 +2285,22 @@ extern struct symtab_and_line find_pc_line (CORE_ADDR, int);
 extern struct symtab_and_line find_pc_sect_line (CORE_ADDR,
 						 struct obj_section *, int);
 
+/* Given PC, and assuming it is part of a range of addresses that is part of a
+   line, go back through the linetable and find the starting PC of that
+   line.
+
+   For example, suppose we have 3 PC ranges for line X:
+
+   Line X - [0x0 - 0x8]
+   Line X - [0x8 - 0x10]
+   Line X - [0x10 - 0x18]
+
+   If we call the function with PC == 0x14, we want to return 0x0, as that is
+   the starting PC of line X, and the ranges are contiguous.
+*/
+
+extern gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);
+
 /* Wrapper around find_pc_line to just return the symtab.  */
 
 extern struct symtab *find_pc_line_symtab (CORE_ADDR);
diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c b/gdb/testsuite/gdb.reverse/map-to-same-line.c
new file mode 100644
index 00000000000..fa751ffe842
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,55 @@
+/* Copyright 2022 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");
+  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..efaa60a957f
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,136 @@
+# Copyright 2022 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.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+
+if [get_compiler_info] {
+    return -1
+}
+
+# The DWARF assembler requires the gcc compiler.
+if {!$gcc_compiled} {
+    unsupported "gcc is required for this test"
+    return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+
+    # Find start address and length of program
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name map-to-same-line.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+	    {low_pc 0 addr}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{low_pc $main_start addr}
+		{high_pc $main_len DW_FORM_data4}
+	    }
+	}
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate the line table program with 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_end_sequence
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	[list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+if [supports_process_record] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "turn on process record"
+}
+
+gdb_test "tbreak main_return" "Temporary breakpoint .*" "breakpoint at return"
+gdb_test "continue" "Temporary breakpoint .*" "run to end of main"
+
+# 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"
+}
-- 
2.25.1

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

[Bruno Larsen]

Hi Luis and Carl!

Sorry about not answering about this earlier, I had a few rushed weeks, but I have some nits about the .exp file

On 5/6/22 05:55, Luis Machado wrote:
> v4:
> - Updated testcase to make it a bit longer so it can exercise reverse-stepping
>    multiple times.
> - Cleaned up debugging prints.
> 

<snip>

> 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..efaa60a957f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,136 @@
> +# Copyright 2022 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.
> +

Could you add a comment here after the copyright blurb explaining why this testcase exists? Something similar to the find_line_range_start comment should be enough, jsut so the next unrelated person who looks at this code in 2 years has some context as to what is going on.

> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +
> +# The DWARF assembler requires the gcc compiler.
> +if {!$gcc_compiled} {
> +    unsupported "gcc is required for this test"
> +    return 0
> +}

There should probably be a test here for supports_reverse. Or supports_process_record. I'm not sure what the difference is between these 2 checks.

> +
> +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_end_sequence
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +if [supports_process_record] {
> +    # Activate process record/replay
> +    gdb_test_no_output "record" "turn on process record"
> +}
since we would have tested above if the target supports recording/reversing, we should be good to just use record here, without the if clause. I suggest doing it this way because there is no point in this whole test if the target doesn't support reverse execution.

> +
> +gdb_test "tbreak main_return" "Temporary breakpoint .*" "breakpoint at return"
> +gdb_test "continue" "Temporary breakpoint .*" "run to end of main"
> +
> +# 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"
> +}

Cheers!
Bruno Larsen

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

[Carl Love]

Bruno:

On Fri, 2022-05-06 at 12:04 -0300, Bruno Larsen wrote:

<snip>

> >  >.
> > +
> > +# 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.
> > +
> 
> Could you add a comment here after the copyright blurb explaining why
> this testcase exists? Something similar to the find_line_range_start
> comment should be enough, jsut so the next unrelated person who looks
> at this code in 2 years has some context as to what is going on.

OK, updated the comment to say the purpose is to test the
find_line_range_start functionality.  I did reorganize the comment a
bit to make things flow better.  Specifically, started with the
expected behaviour of GDB followed by testing find_line_range_start to
ensure GDB works as expected.

> 
> > +load_lib dwarf.exp
> > +
> > +# This test can only be run on targets which support DWARF-2 and
> > use gas.
> > +if {![dwarf2_support]} {
> > +    unsupported "dwarf2 support required for this test"
> > +    return 0
> > +}
> > +
> > +if [get_compiler_info] {
> > +    return -1
> > +}
> > +
> > +# The DWARF assembler requires the gcc compiler.
> > +if {!$gcc_compiled} {
> > +    unsupported "gcc is required for this test"
> > +    return 0
> > +}
> 
> There should probably be a test here for supports_reverse. Or
> supports_process_record. I'm not sure what the difference is between
> these 2 checks.

Yes, I added if ![supports_process_record] test here to make sure the
system supports the record and replay.  

<snip>
> 
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} \
> > +	[list $srcfile $asm_file] {nodebug} ] } {
> > +    return -1
> > +}
> > +
> > +if ![runto_main] {
> > +    return -1
> > +}
> > +
> > +if [supports_process_record] {
> > +    # Activate process record/replay
> > +    gdb_test_no_output "record" "turn on process record"
> > +}
> since we would have tested above if the target supports
> recording/reversing, we should be good to just use record here,
> without the if clause. I suggest doing it this way because there is
> no point in this whole test if the target doesn't support reverse
> execution.

Yes, removed the supports_process_record test.
> > 

Will post v5 of the patch for review.

Thanks for the input. 

                          Carl Love

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

[Carl Love]

Bruno, GDB maintainers:

The patch has been updated per the comments on the testcase from Bruno.

The patch was retested on Power 10 to ensure the test still works
correctly.

Please let us know if there are any additional comments or if the patch
is ready to commit.  We thank you for your help with this patch.

                   Carl Love
--------------------------------------------------------
Fix reverse stepping multiple contiguous PC ranges over the line table
v5:- Updated test case comments on the purpose of the test.- Add test
to check system supports record-replay.- Removed now unnecessary reord
test when activating record.
v4:- Updated testcase to make it a bit longer so it can exercise
reverse-stepping  multiple times.- Cleaned up debugging prints.
v3:- Updated testcase.  The format for writing the DWARF program body
in the  testcase expect file changed.  See commit gdb/testsuite/dwarf:
simplify line number program syntax  (commit
d4c4a2298cad06ca71cfef725f5248f68205f0be)
v2:- Check if both the line and symtab match for a particular line
table entry.
--
When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
spotted onthe ppc backend), I noticed some failures in
gdb.reverse/solib-precsave.expand 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 distinctPC
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
sourcecolumn 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 online 42. When stepping backward from line 42, we stop at the
start PC of thesecond (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
stopstepping backwards.
The right thing to do is to look for adjacent PC ranges for the same
line,until we notice a line change. Then we take that as the start PC
of therange.
Another solution I thought about is to merge the contiguous ranges
whenwe are reading the line tables. Though I'm not sure if we really
want to processthat data as opposed to keeping it as the compiler
created, and then workingaround that.
In any case, the following patch addresses this problem.
Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love
hasverified that it does fix a similar issue on ppc.
Ubuntu 18.04 doesn't actually run into these failures because the
compilerdoesn't generate distinct PC ranges for the same line.
I see similar failures on x86_64 in the gdb.reverse
tests(gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
Those arealso fixed by this patch.
The included testcase (based on a test Carl wrote) exercises this
problem forArm, ppc and x86. It shows full passes with the patch
applied.
Co-authored-by: Carl Love <cel@us.ibm.com>---
gdb/infrun.c                                  |  22 ++-
gdb/symtab.c                                  |  49 ++++++
gdb/symtab.h                                  |  16 ++
gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
.../gdb.reverse/map-to-same-line.exp          | 141 ++++++++++++++++++
5 files changed, 282 insertions(+), 1 deletion(-) 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.cindex 6e5853ef42a..82c28961aeb
100644--- a/gdb/infrun.c+++ b/gdb/infrun.c@@ -6955,11 +6955,31 @@ if
(ecs->event_thread->control.proceed_to_finish 	 have software
watchpoints).  */       ecs->event_thread->control.may_range_step = 1;
+      /* When we are stepping inside a particular line range, in
reverse,+	 and we are sitting at the first address of that range,
we need to+	 check if this address also shows up in another line
range as the+	 end address.++	 If so, we need to check what line such
a step range points to.+	 If it points to the same line as the
current step range, that+	 means we need to keep going in order
to reach the first address+	 of the line range.  We repeat this
until we eventually get to the+	 first address of a particular line
we're stepping through.  */+      CORE_ADDR range_start = ecs-
>event_thread->control.step_range_start;+      if (execution_direction
== EXEC_REVERSE)+	{+	  gdb::optional<CORE_ADDR>
real_range_start+	    = find_line_range_start (ecs->event_thread-
>stop_pc ());++	  if (real_range_start.has_value ())+	    range_start
= *real_range_start;+	}+       /* When stepping backward, stop at
beginning of line range 	 (unless it's the function entry point,
in which case 	 keep going back to the call
point).  */       CORE_ADDR stop_pc = ecs->event_thread->stop_pc
();-      if (stop_pc == ecs->event_thread-
>control.step_range_start+      if (stop_pc == range_start 	  &&
stop_pc != ecs->stop_func_start 	  && execution_direction ==
EXEC_REVERSE) 	end_stepping_range (ecs);diff --git a/gdb/symtab.c
b/gdb/symtab.cindex 4b33d6c91af..de4cb5dd0eb 100644---
a/gdb/symtab.c+++ b/gdb/symtab.c@@ -3433,6 +3433,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.hindex b1cf84f756f..226fe8803db 100644---
a/gdb/symtab.h+++ b/gdb/symtab.h@@ -2285,6 +2285,22 @@ extern struct
symtab_and_line find_pc_line (CORE_ADDR, int); extern struct
symtab_and_line find_pc_sect_line (CORE_ADDR, 				
		 struct obj_section *, int); +/* Given PC, and assuming
it is part of a range of addresses that is part of a+   line, go back
through the linetable and find the starting PC of that+   line.++   For
example, suppose we have 3 PC ranges for line X:++   Line X - [0x0 -
0x8]+   Line X - [0x8 - 0x10]+   Line X - [0x10 - 0x18]++   If we call
the function with PC == 0x14, we want to return 0x0, as that is+   the
starting PC of line X, and the ranges are contiguous.+*/++extern
gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);+ /*
Wrapper around find_pc_line to just return the symtab.  */  extern
struct symtab *find_pc_line_symtab (CORE_ADDR);diff --git
a/gdb/testsuite/gdb.reverse/map-to-same-line.c
b/gdb/testsuite/gdb.reverse/map-to-same-line.cnew file mode 100644index
00000000000..dd9f9f8a400--- /dev/null+++
b/gdb/testsuite/gdb.reverse/map-to-same-line.c@@ -0,0 +1,55 @@+/*
Copyright 2022 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");+  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.expnew file mode
100644index 00000000000..c3fb859be55--- /dev/null+++
b/gdb/testsuite/gdb.reverse/map-to-same-line.exp@@ -0,0 +1,141 @@+#
Copyright 2022 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+}++if [get_compiler_info] {+    return -1+}++# The DWARF assembler
requires the gcc compiler.+if {!$gcc_compiled} {+    unsupported "gcc
is required for this test"+    return 0+}++# 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_st
art 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_end_sequence+	}+    }+}++if {
[prepare_for_testing "failed to prepare" ${testfile} \+	[list $srcfile
$asm_file] {nodebug} ] } {+    return -1+}++if ![runto_main]
{+    return -1+}++# Activate process record/replay+gdb_test_no_output
"record" "turn on process record"++gdb_test "tbreak main_return"
"Temporary breakpoint .*" "breakpoint at return"+gdb_test "continue"
"Temporary breakpoint .*" "run to end of main"++# 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"+}-- 2.31.1

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

[Carl Love]

GDB maintainers:

We have addressed the comments from Bruno. Unfortunately, Bruno is not
a maintainer and can't give approval for the patch.  We are hoping a
maintainer can review the patch and provide us feedback.  

Thank you for your time.

                       Carl Love
---------------------------------------------------

On Fri, 2022-05-06 at 09:48 -0700, Carl Love via Gdb-patches wrote:
> Bruno, GDB maintainers:
> 
> The patch has been updated per the comments on the testcase from
> Bruno.
> 
> The patch was retested on Power 10 to ensure the test still works
> correctly.
> 
> Please let us know if there are any additional comments or if the
> patch
> is ready to commit.  We thank you for your help with this patch.
> 
>                    Carl Love
> --------------------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges over the line
> table
> v5:- Updated test case comments on the purpose of the test.- Add test
> to check system supports record-replay.- Removed now unnecessary
> reord
> test when activating record.
> v4:- Updated testcase to make it a bit longer so it can exercise
> reverse-stepping  multiple times.- Cleaned up debugging prints.
> v3:- Updated testcase.  The format for writing the DWARF program body
> in the  testcase expect file changed.  See commit
> gdb/testsuite/dwarf:
> simplify line number program syntax  (commit
> d4c4a2298cad06ca71cfef725f5248f68205f0be)
> v2:- Check if both the line and symtab match for a particular line
> table entry.
> --
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
> spotted onthe ppc backend), I noticed some failures in
> gdb.reverse/solib-precsave.expand 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
> distinctPC
> 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
> sourcecolumn 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 online 42. When stepping backward from line 42, we stop at the
> start PC of thesecond (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
> stopstepping backwards.
> The right thing to do is to look for adjacent PC ranges for the same
> line,until we notice a line change. Then we take that as the start PC
> of therange.
> Another solution I thought about is to merge the contiguous ranges
> whenwe are reading the line tables. Though I'm not sure if we really
> want to processthat data as opposed to keeping it as the compiler
> created, and then workingaround that.
> In any case, the following patch addresses this problem.
> Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love
> hasverified that it does fix a similar issue on ppc.
> Ubuntu 18.04 doesn't actually run into these failures because the
> compilerdoesn't generate distinct PC ranges for the same line.
> I see similar failures on x86_64 in the gdb.reverse
> tests(gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
> Those arealso fixed by this patch.
> The included testcase (based on a test Carl wrote) exercises this
> problem forArm, ppc and x86. It shows full passes with the patch
> applied.
> Co-authored-by: Carl Love <cel@us.ibm.com>---
> gdb/infrun.c                                  |  22 ++-
> gdb/symtab.c                                  |  49 ++++++
> gdb/symtab.h                                  |  16 ++
> gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
> .../gdb.reverse/map-to-same-line.exp          | 141
> ++++++++++++++++++
> 5 files changed, 282 insertions(+), 1 deletion(-) 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.cindex
> 6e5853ef42a..82c28961aeb
> 100644--- a/gdb/infrun.c+++ b/gdb/infrun.c@@ -6955,11 +6955,31 @@ if
> (ecs->event_thread->control.proceed_to_finish 	 have software
> watchpoints).  */       ecs->event_thread->control.may_range_step =
> 1;
> +      /* When we are stepping inside a particular line range, in
> reverse,+	 and we are sitting at the first address of that range,
> we need to+	 check if this address also shows up in another line
> range as the+	 end address.++	 If so, we need to check what line
> such
> a step range points to.+	 If it points to the same line as the
> current step range, that+	 means we need to keep going in order
> to reach the first address+	 of the line range.  We repeat this
> until we eventually get to the+	 first address of a particular
> line
> we're stepping through.  */+      CORE_ADDR range_start = ecs-
> > event_thread->control.step_range_start;+      if
> > (execution_direction
> == EXEC_REVERSE)+	{+	  gdb::optional<CORE_ADDR>
> real_range_start+	    = find_line_range_start (ecs->event_thread-
> > stop_pc ());++	  if (real_range_start.has_value ())+	   
> >  range_start
> = *real_range_start;+	}+       /* When stepping backward, stop at
> beginning of line range 	 (unless it's the function entry point,
> in which case 	 keep going back to the call
> point).  */       CORE_ADDR stop_pc = ecs->event_thread->stop_pc
> ();-      if (stop_pc == ecs->event_thread-
> > control.step_range_start+      if (stop_pc == range_start 	  &&
> stop_pc != ecs->stop_func_start 	  && execution_direction ==
> EXEC_REVERSE) 	end_stepping_range (ecs);diff --git
> a/gdb/symtab.c
> b/gdb/symtab.cindex 4b33d6c91af..de4cb5dd0eb 100644---
> a/gdb/symtab.c+++ b/gdb/symtab.c@@ -3433,6 +3433,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.hindex b1cf84f756f..226fe8803db 100644---
> a/gdb/symtab.h+++ b/gdb/symtab.h@@ -2285,6 +2285,22 @@ extern struct
> symtab_and_line find_pc_line (CORE_ADDR, int); extern struct
> symtab_and_line find_pc_sect_line (CORE_ADDR, 			
> 	
> 		 struct obj_section *, int); +/* Given PC, and assuming
> it is part of a range of addresses that is part of a+   line, go back
> through the linetable and find the starting PC of
> that+   line.++   For
> example, suppose we have 3 PC ranges for line X:++   Line X - [0x0 -
> 0x8]+   Line X - [0x8 - 0x10]+   Line X - [0x10 - 0x18]++   If we
> call
> the function with PC == 0x14, we want to return 0x0, as that
> is+   the
> starting PC of line X, and the ranges are contiguous.+*/++extern
> gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);+ /*
> Wrapper around find_pc_line to just return the symtab.  */  extern
> struct symtab *find_pc_line_symtab (CORE_ADDR);diff --git
> a/gdb/testsuite/gdb.reverse/map-to-same-line.c
> b/gdb/testsuite/gdb.reverse/map-to-same-line.cnew file mode
> 100644index
> 00000000000..dd9f9f8a400--- /dev/null+++
> b/gdb/testsuite/gdb.reverse/map-to-same-line.c@@ -0,0 +1,55 @@+/*
> Copyright 2022 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");+  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.expnew file mode
> 100644index 00000000000..c3fb859be55--- /dev/null+++
> b/gdb/testsuite/gdb.reverse/map-to-same-line.exp@@ -0,0 +1,141 @@+#
> Copyright 2022 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+}++if [get_compiler_info] {+    return -1+}++# The DWARF assembler
> requires the gcc compiler.+if {!$gcc_compiled} {+    unsupported "gcc
> is required for this test"+    return 0+}++# 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_st
> art 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_addres
> s
> line1+	    line [gdb_get_line_number "TAG: line 1" ]+	    D
> W_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" ]+	    D
> W_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" ]+	    D
> W_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" ]+	    D
> W_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_end_sequence+	}+    }+}++if {
> [prepare_for_testing "failed to prepare" ${testfile} \+	[list
> $srcfile
> $asm_file] {nodebug} ] } {+    return -1+}++if ![runto_main]
> {+    return -1+}++# Activate process
> record/replay+gdb_test_no_output
> "record" "turn on process record"++gdb_test "tbreak main_return"
> "Temporary breakpoint .*" "breakpoint at return"+gdb_test "continue"
> "Temporary breakpoint .*" "run to end of main"++# 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"+}-- 2.31.1
> 
> 

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

[Luis Machado]

Ping?

On 5/13/22 18:00, Carl Love wrote:
> GDB maintainers:
> 
> We have addressed the comments from Bruno. Unfortunately, Bruno is not
> a maintainer and can't give approval for the patch.  We are hoping a
> maintainer can review the patch and provide us feedback.
> 
> Thank you for your time.
> 
>                         Carl Love
> ---------------------------------------------------
> 
> On Fri, 2022-05-06 at 09:48 -0700, Carl Love via Gdb-patches wrote:
>> Bruno, GDB maintainers:
>>
>> The patch has been updated per the comments on the testcase from
>> Bruno.
>>
>> The patch was retested on Power 10 to ensure the test still works
>> correctly.
>>
>> Please let us know if there are any additional comments or if the
>> patch
>> is ready to commit.  We thank you for your help with this patch.
>>
>>                     Carl Love
>> --------------------------------------------------------
>> Fix reverse stepping multiple contiguous PC ranges over the line
>> table
>> v5:- Updated test case comments on the purpose of the test.- Add test
>> to check system supports record-replay.- Removed now unnecessary
>> reord
>> test when activating record.
>> v4:- Updated testcase to make it a bit longer so it can exercise
>> reverse-stepping  multiple times.- Cleaned up debugging prints.
>> v3:- Updated testcase.  The format for writing the DWARF program body
>> in the  testcase expect file changed.  See commit
>> gdb/testsuite/dwarf:
>> simplify line number program syntax  (commit
>> d4c4a2298cad06ca71cfef725f5248f68205f0be)
>> v2:- Check if both the line and symtab match for a particular line
>> table entry.
>> --
>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
>> spotted onthe ppc backend), I noticed some failures in
>> gdb.reverse/solib-precsave.expand 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
>> distinctPC
>> 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
>> sourcecolumn 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 online 42. When stepping backward from line 42, we stop at the
>> start PC of thesecond (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
>> stopstepping backwards.
>> The right thing to do is to look for adjacent PC ranges for the same
>> line,until we notice a line change. Then we take that as the start PC
>> of therange.
>> Another solution I thought about is to merge the contiguous ranges
>> whenwe are reading the line tables. Though I'm not sure if we really
>> want to processthat data as opposed to keeping it as the compiler
>> created, and then workingaround that.
>> In any case, the following patch addresses this problem.
>> Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love
>> hasverified that it does fix a similar issue on ppc.
>> Ubuntu 18.04 doesn't actually run into these failures because the
>> compilerdoesn't generate distinct PC ranges for the same line.
>> I see similar failures on x86_64 in the gdb.reverse
>> tests(gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
>> Those arealso fixed by this patch.
>> The included testcase (based on a test Carl wrote) exercises this
>> problem forArm, ppc and x86. It shows full passes with the patch
>> applied.
>> Co-authored-by: Carl Love <cel@us.ibm.com>---
>> gdb/infrun.c                                  |  22 ++-
>> gdb/symtab.c                                  |  49 ++++++
>> gdb/symtab.h                                  |  16 ++
>> gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
>> .../gdb.reverse/map-to-same-line.exp          | 141
>> ++++++++++++++++++
>> 5 files changed, 282 insertions(+), 1 deletion(-) 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.cindex
>> 6e5853ef42a..82c28961aeb
>> 100644--- a/gdb/infrun.c+++ b/gdb/infrun.c@@ -6955,11 +6955,31 @@ if
>> (ecs->event_thread->control.proceed_to_finish 	 have software
>> watchpoints).  */       ecs->event_thread->control.may_range_step =
>> 1;
>> +      /* When we are stepping inside a particular line range, in
>> reverse,+	 and we are sitting at the first address of that range,
>> we need to+	 check if this address also shows up in another line
>> range as the+	 end address.++	 If so, we need to check what line
>> such
>> a step range points to.+	 If it points to the same line as the
>> current step range, that+	 means we need to keep going in order
>> to reach the first address+	 of the line range.  We repeat this
>> until we eventually get to the+	 first address of a particular
>> line
>> we're stepping through.  */+      CORE_ADDR range_start = ecs-
>>> event_thread->control.step_range_start;+      if
>>> (execution_direction
>> == EXEC_REVERSE)+	{+	  gdb::optional<CORE_ADDR>
>> real_range_start+	    = find_line_range_start (ecs->event_thread-
>>> stop_pc ());++	  if (real_range_start.has_value ())+	
>>>   range_start
>> = *real_range_start;+	}+       /* When stepping backward, stop at
>> beginning of line range 	 (unless it's the function entry point,
>> in which case 	 keep going back to the call
>> point).  */       CORE_ADDR stop_pc = ecs->event_thread->stop_pc
>> ();-      if (stop_pc == ecs->event_thread-
>>> control.step_range_start+      if (stop_pc == range_start 	  &&
>> stop_pc != ecs->stop_func_start 	  && execution_direction ==
>> EXEC_REVERSE) 	end_stepping_range (ecs);diff --git
>> a/gdb/symtab.c
>> b/gdb/symtab.cindex 4b33d6c91af..de4cb5dd0eb 100644---
>> a/gdb/symtab.c+++ b/gdb/symtab.c@@ -3433,6 +3433,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.hindex b1cf84f756f..226fe8803db 100644---
>> a/gdb/symtab.h+++ b/gdb/symtab.h@@ -2285,6 +2285,22 @@ extern struct
>> symtab_and_line find_pc_line (CORE_ADDR, int); extern struct
>> symtab_and_line find_pc_sect_line (CORE_ADDR, 			
>> 	
>> 		 struct obj_section *, int); +/* Given PC, and assuming
>> it is part of a range of addresses that is part of a+   line, go back
>> through the linetable and find the starting PC of
>> that+   line.++   For
>> example, suppose we have 3 PC ranges for line X:++   Line X - [0x0 -
>> 0x8]+   Line X - [0x8 - 0x10]+   Line X - [0x10 - 0x18]++   If we
>> call
>> the function with PC == 0x14, we want to return 0x0, as that
>> is+   the
>> starting PC of line X, and the ranges are contiguous.+*/++extern
>> gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);+ /*
>> Wrapper around find_pc_line to just return the symtab.  */  extern
>> struct symtab *find_pc_line_symtab (CORE_ADDR);diff --git
>> a/gdb/testsuite/gdb.reverse/map-to-same-line.c
>> b/gdb/testsuite/gdb.reverse/map-to-same-line.cnew file mode
>> 100644index
>> 00000000000..dd9f9f8a400--- /dev/null+++
>> b/gdb/testsuite/gdb.reverse/map-to-same-line.c@@ -0,0 +1,55 @@+/*
>> Copyright 2022 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");+  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.expnew file mode
>> 100644index 00000000000..c3fb859be55--- /dev/null+++
>> b/gdb/testsuite/gdb.reverse/map-to-same-line.exp@@ -0,0 +1,141 @@+#
>> Copyright 2022 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+}++if [get_compiler_info] {+    return -1+}++# The DWARF assembler
>> requires the gcc compiler.+if {!$gcc_compiled} {+    unsupported "gcc
>> is required for this test"+    return 0+}++# 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_st
>> art 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_addres
>> s
>> line1+	    line [gdb_get_line_number "TAG: line 1" ]+	    D
>> W_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" ]+	    D
>> W_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" ]+	    D
>> W_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" ]+	    D
>> W_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_end_sequence+	}+    }+}++if {
>> [prepare_for_testing "failed to prepare" ${testfile} \+	[list
>> $srcfile
>> $asm_file] {nodebug} ] } {+    return -1+}++if ![runto_main]
>> {+    return -1+}++# Activate process
>> record/replay+gdb_test_no_output
>> "record" "turn on process record"++gdb_test "tbreak main_return"
>> "Temporary breakpoint .*" "breakpoint at return"+gdb_test "continue"
>> "Temporary breakpoint .*" "run to end of main"++# 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"+}-- 2.31.1
>>
>>
> 

RE: [PING 2][PATCH, v5] Fix reverse stepping multiple contiguous PC ranges over the line table

[Carl Love]

Ping?

On Mon, 2022-05-23 at 11:12 +0100, Luis Machado wrote:
> Ping?
> 
> On 5/13/22 18:00, Carl Love wrote:
> > GDB maintainers:
> > 
> > We have addressed the comments from Bruno. Unfortunately, Bruno is
> > not
> > a maintainer and can't give approval for the patch.  We are hoping
> > a
> > maintainer can review the patch and provide us feedback.
> > 
> > Thank you for your time.
> > 
> >                         Carl Love
> > ---------------------------------------------------
> > 
> > On Fri, 2022-05-06 at 09:48 -0700, Carl Love via Gdb-patches wrote:
> > > Bruno, GDB maintainers:
> > > 
> > > The patch has been updated per the comments on the testcase from
> > > Bruno.
> > > 
> > > The patch was retested on Power 10 to ensure the test still works
> > > correctly.
> > > 
> > > Please let us know if there are any additional comments or if the
> > > patch
> > > is ready to commit.  We thank you for your help with this patch.
> > > 
> > >                     Carl Love
> > > --------------------------------------------------------
> > > Fix reverse stepping multiple contiguous PC ranges over the line
> > > table
> > > v5:- Updated test case comments on the purpose of the test.- Add
> > > test
> > > to check system supports record-replay.- Removed now unnecessary
> > > reord
> > > test when activating record.
> > > v4:- Updated testcase to make it a bit longer so it can exercise
> > > reverse-stepping  multiple times.- Cleaned up debugging prints.
> > > v3:- Updated testcase.  The format for writing the DWARF program
> > > body
> > > in the  testcase expect file changed.  See commit
> > > gdb/testsuite/dwarf:
> > > simplify line number program syntax  (commit
> > > d4c4a2298cad06ca71cfef725f5248f68205f0be)
> > > v2:- Check if both the line and symtab match for a particular
> > > line
> > > table entry.
> > > --
> > > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
> > > spotted onthe ppc backend), I noticed some failures in
> > > gdb.reverse/solib-precsave.expand 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
> > > distinctPC
> > > 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
> > > sourcecolumn 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 online 42. When stepping backward from line 42, we stop at
> > > the
> > > start PC of thesecond (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_ra
> > > nge
> > > (ecs);      else	keep_going (ecs);
> > > Since we've reached ecs->event_thread->control.step_range_start,
> > > we
> > > stopstepping backwards.
> > > The right thing to do is to look for adjacent PC ranges for the
> > > same
> > > line,until we notice a line change. Then we take that as the
> > > start PC
> > > of therange.
> > > Another solution I thought about is to merge the contiguous
> > > ranges
> > > whenwe are reading the line tables. Though I'm not sure if we
> > > really
> > > want to processthat data as opposed to keeping it as the compiler
> > > created, and then workingaround that.
> > > In any case, the following patch addresses this problem.
> > > Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl
> > > Love
> > > hasverified that it does fix a similar issue on ppc.
> > > Ubuntu 18.04 doesn't actually run into these failures because the
> > > compilerdoesn't generate distinct PC ranges for the same line.
> > > I see similar failures on x86_64 in the gdb.reverse
> > > tests(gdb.reverse/step-reverse.exp and gdb.reverse/step-
> > > reverse.exp).
> > > Those arealso fixed by this patch.
> > > The included testcase (based on a test Carl wrote) exercises this
> > > problem forArm, ppc and x86. It shows full passes with the patch
> > > applied.
> > > Co-authored-by: Carl Love <cel@us.ibm.com>---
> > > gdb/infrun.c                                  |  22 ++-
> > > gdb/symtab.c                                  |  49 ++++++
> > > gdb/symtab.h                                  |  16 ++
> > > gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
> > > .../gdb.reverse/map-to-same-line.exp          | 141
> > > ++++++++++++++++++
> > > 5 files changed, 282 insertions(+), 1 deletion(-) 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.cindex
> > > 6e5853ef42a..82c28961aeb
> > > 100644--- a/gdb/infrun.c+++ b/gdb/infrun.c@@ -6955,11 +6955,31 @@
> > > if
> > > (ecs->event_thread->control.proceed_to_finish 	 have software
> > > watchpoints).  */       ecs->event_thread->control.may_range_step 
> > > =
> > > 1;
> > > +      /* When we are stepping inside a particular line range, in
> > > reverse,+	 and we are sitting at the first address of that range,
> > > we need to+	 check if this address also shows up in another
> > > line
> > > range as the+	 end address.++	 If so, we need to check
> > > what line
> > > such
> > > a step range points to.+	 If it points to the same line as the
> > > current step range, that+	 means we need to keep going in order
> > > to reach the first address+	 of the line range.  We repeat
> > > this
> > > until we eventually get to the+	 first address of a particular
> > > line
> > > we're stepping through.  */+      CORE_ADDR range_start = ecs-
> > > > event_thread->control.step_range_start;+      if
> > > > (execution_direction
> > > == EXEC_REVERSE)+	{+	  gdb::optional<CORE_ADDR>
> > > real_range_start+	    = find_line_range_start (ecs->event_thread-
> > > > stop_pc ());++	  if (real_range_start.has_value ())+	
> > > >   range_start
> > > = *real_range_start;+	}+       /* When stepping backward,
> > > stop at
> > > beginning of line range 	 (unless it's the function entry point,
> > > in which case 	 keep going back to the call
> > > point).  */       CORE_ADDR stop_pc = ecs->event_thread->stop_pc
> > > ();-      if (stop_pc == ecs->event_thread-
> > > > control.step_range_start+      if (stop_pc == range_start 	
> > > >   &&
> > > stop_pc != ecs->stop_func_start 	  && execution_direction ==
> > > EXEC_REVERSE) 	end_stepping_range (ecs);diff --git
> > > a/gdb/symtab.c
> > > b/gdb/symtab.cindex 4b33d6c91af..de4cb5dd0eb 100644---
> > > a/gdb/symtab.c+++ b/gdb/symtab.c@@ -3433,6 +3433,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.hindex b1cf84f756f..226fe8803db 100644---
> > > a/gdb/symtab.h+++ b/gdb/symtab.h@@ -2285,6 +2285,22 @@ extern
> > > struct
> > > symtab_and_line find_pc_line (CORE_ADDR, int); extern struct
> > > symtab_and_line find_pc_sect_line (CORE_ADDR, 			
> > > 	
> > > 		 struct obj_section *, int); +/* Given PC, and assuming
> > > it is part of a range of addresses that is part of a+   line, go
> > > back
> > > through the linetable and find the starting PC of
> > > that+   line.++   For
> > > example, suppose we have 3 PC ranges for line X:++   Line X -
> > > [0x0 -
> > > 0x8]+   Line X - [0x8 - 0x10]+   Line X - [0x10 - 0x18]++   If we
> > > call
> > > the function with PC == 0x14, we want to return 0x0, as that
> > > is+   the
> > > starting PC of line X, and the ranges are contiguous.+*/++extern
> > > gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR pc);+
> > > /*
> > > Wrapper around find_pc_line to just return the
> > > symtab.  */  extern
> > > struct symtab *find_pc_line_symtab (CORE_ADDR);diff --git
> > > a/gdb/testsuite/gdb.reverse/map-to-same-line.c
> > > b/gdb/testsuite/gdb.reverse/map-to-same-line.cnew file mode
> > > 100644index
> > > 00000000000..dd9f9f8a400--- /dev/null+++
> > > b/gdb/testsuite/gdb.reverse/map-to-same-line.c@@ -0,0 +1,55 @@+/*
> > > Copyright 2022 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");+  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.expnew file mode
> > > 100644index 00000000000..c3fb859be55--- /dev/null+++
> > > b/gdb/testsuite/gdb.reverse/map-to-same-line.exp@@ -0,0 +1,141
> > > @@+#
> > > Copyright 2022 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+}++if [get_compiler_info] {+    return -1+}++# The DWARF
> > > assembler
> > > requires the gcc compiler.+if {!$gcc_compiled} {+    unsupported
> > > "gcc
> > > is required for this test"+    return 0+}++# 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_st
> > > art 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_ad
> > > dres
> > > s
> > > line1+	    line [gdb_get_line_number "TAG: line 1" ]+	 
> > >    D
> > > W_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" ]+	 
> > >    D
> > > W_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" ]+	 
> > >    D
> > > W_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" ]+	 
> > >    D
> > > W_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_end_sequence+	}+    }+}++if {
> > > [prepare_for_testing "failed to prepare" ${testfile} \+	[list
> > > $srcfile
> > > $asm_file] {nodebug} ] } {+    return -1+}++if ![runto_main]
> > > {+    return -1+}++# Activate process
> > > record/replay+gdb_test_no_output
> > > "record" "turn on process record"++gdb_test "tbreak main_return"
> > > "Temporary breakpoint .*" "breakpoint at return"+gdb_test
> > > "continue"
> > > "Temporary breakpoint .*" "run to end of main"++# 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"+}-- 2.31.1
> > > 
> > > 

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

[will schmidt]

On Fri, 2022-05-06 at 09:48 -0700, Carl Love wrote:
> Bruno, GDB maintainers:
> 
> The patch has been updated per the comments on the testcase from
> Bruno.
> 
> The patch was retested on Power 10 to ensure the test still works
> correctly.
> 
> Please let us know if there are any additional comments or if the
> patch is ready to commit. We thank you for your help with this patch.
> 
> 

Assorted nits below, but unless my comments reveal something, LGTM. 




> 
> Carl Love
> --------------------------------------------------------
> Fix reverse stepping multiple contiguous PC ranges over the line
> table
> 
> v5:
> - Updated test case comments on the purpose of the test.
> - Add test to check system supports record-replay.
> - Removed now unnecessary reord test when activating record.


s/reord/record/  :-)

> v4:
> - Updated testcase to make it a bit longer so it can exercise
> reverse-stepping
>   multiple times.
> - Cleaned up debugging prints.
> 
> v3:
> - Updated testcase.  The format for writing the DWARF program body in
> the
>   testcase expect file changed.
>   See commit gdb/testsuite/dwarf: simplify line number program syntax
>   (commit d4c4a2298cad06ca71cfef725f5248f68205f0be)
> 
> v2:
> - Check if both the line and symtab match for a particular line table
> entry.
> 
> --
> 
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
> spotted on
> the ppc backend), I noticed some failures in gdb.reverse/solib-
> precsave.exp
> and gdb.reverse/solib-reverse.exp.
> 
> The failure happens around the following code:
> 
> 38  b[1] = shr2(17);		/* middle part two */
> 40  b[0] = 6;   b[1] = 9;	/* generic statement, end part two */
> 42  shr1 ("message 1\n");	/* shr1 one */
> 
> Normal execution:
> 
> - step from line 38 will land on line 40.
> - step from line 40 will land on line 42.
> 
> Reverse execution:
> 
> - step from line 42 will land on line 40.
> - step from line 40 will land on line 40.
> - step from line 40 will land on line 38.
> 
> The problem here is that line 40 contains two contiguous but distinct
> PC ranges in the line table, like so:
> 
> Line 40 - [0x7ec ~ 0x7f4]
> Line 40 - [0x7f4 ~ 0x7fc]


Curious, what incantation is used to collect the PC range info?


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


Not critical for this, but is there a general sense for when GCC
started generating the source column info?


> When stepping forward from line 40, we skip both of these ranges and
> land on
> line 42. When stepping backward from line 42, we stop at the start PC
> of the
> second (or first, going backwards) range of line 40.
> 
> This happens because we have this check in
> infrun.c:process_event_stop_test:
> 
>       /* When stepping backward, stop at beginning of line range
> 	 (unless it's the function entry point, in which case
> 	 keep going back to the call point).  */
>       CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>       if (stop_pc == ecs->event_thread->control.step_range_start
> 	  && stop_pc != ecs->stop_func_start
> 	  && execution_direction == EXEC_REVERSE)
> 	end_stepping_range (ecs);
>       else
> 	keep_going (ecs);
> 
> Since we've reached ecs->event_thread->control.step_range_start, we
> stop
> stepping backwards.
> 
> The right thing to do is to look for adjacent PC ranges for the same
> line,
> until we notice a line change. Then we take that as the start PC of
> the
> range.
> 
> Another solution I thought about is to merge the contiguous ranges
> when
> we are reading the line tables. Though I'm not sure if we really want
> to process
> that data as opposed to keeping it as the compiler created, and then
> working
> around that.
> 
> In any case, the following patch addresses this problem.
> 
> Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love
> has
> verified that it does fix a similar issue on ppc.
> 
> Ubuntu 18.04 doesn't actually run into these failures because the
> compiler
> doesn't generate distinct PC ranges for the same line.
> 
> I see similar failures on x86_64 in the gdb.reverse tests
> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
> Those are
> also fixed by this patch.
> 
> The included testcase (based on a test Carl wrote) exercises this
> problem for
> Arm, ppc and x86. It shows full passes with the patch applied.
> 
> Co-authored-by: Carl Love <
> cel@us.ibm.com
> >


> ---
>  gdb/infrun.c                                  |  22 ++-
>  gdb/symtab.c                                  |  49 ++++++
>  gdb/symtab.h                                  |  16 ++
>  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
>  .../gdb.reverse/map-to-same-line.exp          | 141
> ++++++++++++++++++
>  5 files changed, 282 insertions(+), 1 deletion(-)
>  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 6e5853ef42a..82c28961aeb 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6955,11 +6955,31 @@ if (ecs->event_thread-
> >control.proceed_to_finish
>  	 have software watchpoints).  */
>        ecs->event_thread->control.may_range_step = 1;
>  
> +      /* When we are stepping inside a particular line range, in
> reverse,

Nit: Could be rephrased as "When we are reverse-stepping inside a
particular line range"


> +	 and we are sitting at the first address of that range, we need
> to
> +	 check if this address also shows up in another line range as
> the
> +	 end address.
> +
> +	 If so, we need to check what line such a step range points to.
> +	 If it points to the same line as the current step range, that
> +	 means we need to keep going in order to reach the first
> address
> +	 of the line range.  We repeat this until we eventually get to
> the
> +	 first address of a particular line we're stepping through.  */
> +      CORE_ADDR range_start = ecs->event_thread-
> >control.step_range_start;
> +      if (execution_direction == EXEC_REVERSE)
> +	{
> +	  gdb::optional<CORE_ADDR> real_range_start
> +	    = find_line_range_start (ecs->event_thread->stop_pc ());
> +
> +	  if (real_range_start.has_value ())
> +	    range_start = *real_range_start;
> +	}
> +
>        /* When stepping backward, stop at beginning of line range
>  	 (unless it's the function entry point, in which case
>  	 keep going back to the call point).  */
>        CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> -      if (stop_pc == ecs->event_thread->control.step_range_start
> +      if (stop_pc == range_start
>  	  && stop_pc != ecs->stop_func_start
>  	  && execution_direction == EXEC_REVERSE)
>  	end_stepping_range (ecs);

ok



> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 4b33d6c91af..de4cb5dd0eb 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3433,6 +3433,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);
> +}

ok


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

I assume our lines start at 1 so this will never false positive?

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

ok


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

ok

> +}
> +
>  /* See symtab.h.  */
>  
>  struct symtab *
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index b1cf84f756f..226fe8803db 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2285,6 +2285,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.
> +*/

ok

> +
> +extern gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR
> pc);
> +
>  /* Wrapper around find_pc_line to just return the symtab.  */
>  
>  extern struct symtab *find_pc_line_symtab (CORE_ADDR);
> diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c
> b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> new file mode 100644
> index 00000000000..dd9f9f8a400
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,55 @@
> +/* Copyright 2022 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 URL will need to be fixed up.  Likely a side effect of the IBM
email infrastructure that does not exist in the patch itself.   :-)



> +/* 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");
> +  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..c3fb859be55
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,141 @@
> +# Copyright 2022 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/
>  >.
> +


Same.



> +# 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
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +
> +# The DWARF assembler requires the gcc compiler.
> +if {!$gcc_compiled} {
> +    unsupported "gcc is required for this test"
> +    return 0
> +}
> +
> +# 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_end_sequence
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Activate process record/replay
> +gdb_test_no_output "record" "turn on process record"
> +
> +gdb_test "tbreak main_return" "Temporary breakpoint .*" "breakpoint
> at return"
> +gdb_test "continue" "Temporary breakpoint .*" "run to end of main"
> +
> +# 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"
> +}
> -- 
> 2.31.1
> 

ok.

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

[Luis Machado]

Hi Will,

On 6/7/22 18:11, will schmidt wrote:
> On Fri, 2022-05-06 at 09:48 -0700, Carl Love wrote:
>> Bruno, GDB maintainers:
>>
>> The patch has been updated per the comments on the testcase from
>> Bruno.
>>
>> The patch was retested on Power 10 to ensure the test still works
>> correctly.
>>
>> Please let us know if there are any additional comments or if the
>> patch is ready to commit. We thank you for your help with this patch.
>>
>>
> 
> Assorted nits below, but unless my comments reveal something, LGTM.
> 
> 
> 
> 
>>
>> Carl Love
>> --------------------------------------------------------
>> Fix reverse stepping multiple contiguous PC ranges over the line
>> table
>>
>> v5:
>> - Updated test case comments on the purpose of the test.
>> - Add test to check system supports record-replay.
>> - Removed now unnecessary reord test when activating record.
> 
> 
> s/reord/record/  :-)
> 
>> v4:
>> - Updated testcase to make it a bit longer so it can exercise
>> reverse-stepping
>>    multiple times.
>> - Cleaned up debugging prints.
>>
>> v3:
>> - Updated testcase.  The format for writing the DWARF program body in
>> the
>>    testcase expect file changed.
>>    See commit gdb/testsuite/dwarf: simplify line number program syntax
>>    (commit d4c4a2298cad06ca71cfef725f5248f68205f0be)
>>
>> v2:
>> - Check if both the line and symtab match for a particular line table
>> entry.
>>
>> --
>>
>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
>> spotted on
>> the ppc backend), I noticed some failures in gdb.reverse/solib-
>> precsave.exp
>> and gdb.reverse/solib-reverse.exp.
>>
>> The failure happens around the following code:
>>
>> 38  b[1] = shr2(17);		/* middle part two */
>> 40  b[0] = 6;   b[1] = 9;	/* generic statement, end part two */
>> 42  shr1 ("message 1\n");	/* shr1 one */
>>
>> Normal execution:
>>
>> - step from line 38 will land on line 40.
>> - step from line 40 will land on line 42.
>>
>> Reverse execution:
>>
>> - step from line 42 will land on line 40.
>> - step from line 40 will land on line 40.
>> - step from line 40 will land on line 38.
>>
>> The problem here is that line 40 contains two contiguous but distinct
>> PC ranges in the line table, like so:
>>
>> Line 40 - [0x7ec ~ 0x7f4]
>> Line 40 - [0x7f4 ~ 0x7fc]
> 
> 
> Curious, what incantation is used to collect the PC range info?
> 
> 

You can use the maintenance info line-table.

>> The two distinct ranges are generated because GCC started outputting
>> source
>> column information, which GDB doesn't take into account at the
>> moment.
> 
> 
> Not critical for this, but is there a general sense for when GCC
> started generating the source column info?
> 
> 

A couple commits, from these patches here:

https://gcc.gnu.org/pipermail/gcc-patches/2017-February/469643.html
https://gcc.gnu.org/pipermail/gcc-patches/2017-February/469644.html

>> When stepping forward from line 40, we skip both of these ranges and
>> land on
>> line 42. When stepping backward from line 42, we stop at the start PC
>> of the
>> second (or first, going backwards) range of line 40.
>>
>> This happens because we have this check in
>> infrun.c:process_event_stop_test:
>>
>>        /* When stepping backward, stop at beginning of line range
>> 	 (unless it's the function entry point, in which case
>> 	 keep going back to the call point).  */
>>        CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>>        if (stop_pc == ecs->event_thread->control.step_range_start
>> 	  && stop_pc != ecs->stop_func_start
>> 	  && execution_direction == EXEC_REVERSE)
>> 	end_stepping_range (ecs);
>>        else
>> 	keep_going (ecs);
>>
>> Since we've reached ecs->event_thread->control.step_range_start, we
>> stop
>> stepping backwards.
>>
>> The right thing to do is to look for adjacent PC ranges for the same
>> line,
>> until we notice a line change. Then we take that as the start PC of
>> the
>> range.
>>
>> Another solution I thought about is to merge the contiguous ranges
>> when
>> we are reading the line tables. Though I'm not sure if we really want
>> to process
>> that data as opposed to keeping it as the compiler created, and then
>> working
>> around that.
>>
>> In any case, the following patch addresses this problem.
>>
>> Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love
>> has
>> verified that it does fix a similar issue on ppc.
>>
>> Ubuntu 18.04 doesn't actually run into these failures because the
>> compiler
>> doesn't generate distinct PC ranges for the same line.
>>
>> I see similar failures on x86_64 in the gdb.reverse tests
>> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
>> Those are
>> also fixed by this patch.
>>
>> The included testcase (based on a test Carl wrote) exercises this
>> problem for
>> Arm, ppc and x86. It shows full passes with the patch applied.
>>
>> Co-authored-by: Carl Love <
>> cel@us.ibm.com
>>>
> 
> 
>> ---
>>   gdb/infrun.c                                  |  22 ++-
>>   gdb/symtab.c                                  |  49 ++++++
>>   gdb/symtab.h                                  |  16 ++
>>   gdb/testsuite/gdb.reverse/map-to-same-line.c  |  55 +++++++
>>   .../gdb.reverse/map-to-same-line.exp          | 141
>> ++++++++++++++++++
>>   5 files changed, 282 insertions(+), 1 deletion(-)
>>   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 6e5853ef42a..82c28961aeb 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -6955,11 +6955,31 @@ if (ecs->event_thread-
>>> control.proceed_to_finish
>>   	 have software watchpoints).  */
>>         ecs->event_thread->control.may_range_step = 1;
>>   
>> +      /* When we are stepping inside a particular line range, in
>> reverse,
> 
> Nit: Could be rephrased as "When we are reverse-stepping inside a
> particular line range"
> 
> 

I agree that looks better in isolation. But if you check the code, you'll notice
it seems to favor "in reverse". I just went with the current "standard".

I don't have a strong opinion for either of those.

>> +	 and we are sitting at the first address of that range, we need
>> to
>> +	 check if this address also shows up in another line range as
>> the
>> +	 end address.
>> +
>> +	 If so, we need to check what line such a step range points to.
>> +	 If it points to the same line as the current step range, that
>> +	 means we need to keep going in order to reach the first
>> address
>> +	 of the line range.  We repeat this until we eventually get to
>> the
>> +	 first address of a particular line we're stepping through.  */
>> +      CORE_ADDR range_start = ecs->event_thread-
>>> control.step_range_start;
>> +      if (execution_direction == EXEC_REVERSE)
>> +	{
>> +	  gdb::optional<CORE_ADDR> real_range_start
>> +	    = find_line_range_start (ecs->event_thread->stop_pc ());
>> +
>> +	  if (real_range_start.has_value ())
>> +	    range_start = *real_range_start;
>> +	}
>> +
>>         /* When stepping backward, stop at beginning of line range
>>   	 (unless it's the function entry point, in which case
>>   	 keep going back to the call point).  */
>>         CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
>> -      if (stop_pc == ecs->event_thread->control.step_range_start
>> +      if (stop_pc == range_start
>>   	  && stop_pc != ecs->stop_func_start
>>   	  && execution_direction == EXEC_REVERSE)
>>   	end_stepping_range (ecs);
> 
> ok
> 
> 
> 
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 4b33d6c91af..de4cb5dd0eb 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -3433,6 +3433,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);
>> +}
> 
> ok
> 
> 
>> +
>> +/* 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 {};
> 
> I assume our lines start at 1 so this will never false positive?
> 

Right, as documented in gdb/symtab.h:

   /* Line number.  Line numbers start at 1 and proceed through symtab->nlines.
      0 is never a valid line number; it is used to indicate that line number
      information is not available.  */
   int line = 0;


>> +
>> +  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;
> 
> ok
> 
> 
>> +
>> +  /* 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;
> 
> ok
> 
>> +}
>> +
>>   /* See symtab.h.  */
>>   
>>   struct symtab *
>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index b1cf84f756f..226fe8803db 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2285,6 +2285,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.
>> +*/
> 
> ok
> 
>> +
>> +extern gdb::optional<CORE_ADDR> find_line_range_start (CORE_ADDR
>> pc);
>> +
>>   /* Wrapper around find_pc_line to just return the symtab.  */
>>   
>>   extern struct symtab *find_pc_line_symtab (CORE_ADDR);
>> diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c
>> b/gdb/testsuite/gdb.reverse/map-to-same-line.c
>> new file mode 100644
>> index 00000000000..dd9f9f8a400
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
>> @@ -0,0 +1,55 @@
>> +/* Copyright 2022 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 URL will need to be fixed up.  Likely a side effect of the IBM
> email infrastructure that does not exist in the patch itself.   :-)
> 
> 

I have a local copy where this isn't a problem. Pesky e-mail servers.

> 
>> +/* 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");
>> +  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..c3fb859be55
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
>> @@ -0,0 +1,141 @@
>> +# Copyright 2022 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/
>>   >.
>> +
> 
> 
> Same.
> 
> 
> 
>> +# 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
>> +}
>> +
>> +if [get_compiler_info] {
>> +    return -1
>> +}
>> +
>> +# The DWARF assembler requires the gcc compiler.
>> +if {!$gcc_compiled} {
>> +    unsupported "gcc is required for this test"
>> +    return 0
>> +}
>> +
>> +# 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_end_sequence
>> +	}
>> +    }
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" ${testfile} \
>> +	[list $srcfile $asm_file] {nodebug} ] } {
>> +    return -1
>> +}
>> +
>> +if ![runto_main] {
>> +    return -1
>> +}
>> +
>> +# Activate process record/replay
>> +gdb_test_no_output "record" "turn on process record"
>> +
>> +gdb_test "tbreak main_return" "Temporary breakpoint .*" "breakpoint
>> at return"
>> +gdb_test "continue" "Temporary breakpoint .*" "run to end of main"
>> +
>> +# 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"
>> +}
>> -- 
>> 2.31.1
>>
> 
> ok.
> 
> 

I'll send a v6 with the fixups.

Thanks!

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

[Carl Love]

Simon:

On Thu, 2023-05-11 at 12:01 -0400, Simon Marchi wrote:
> > 

<snip>

> I'd like to help reviewing this, but I don't have much time at the
> moment, so just a few comments on one test to start with.
> 
> > +   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
> 
> Use /* */ for comments, for consistency with the rest of the code
> base.
> 

OK, changed all instances of // comment.

> > +
> > +void
> > +func2 ()
> > +{
> > +} // END FUNC2
> > +
> > +int main ()
> > +{
> > +  int a, b;
> > +  a = 1;
> > +  b = 2;
> > +  func1 (); func2 ();
> > +  a = a + b;     // START REVERSE TEST
> > +}
> > 

<snip>

> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +if {![test_compiler_info {gcc-*}]
> > +    || [test_compiler_info {gcc-[1-6]-*}]} {
> > +    return
> > +}
> 
> I would prefer not to filter out by compiler explicitly like that.
> It would be useful for the test to run with other compilers too.

OK, implemented the gdb_compile options.  Moved the specific gcc
version test there.

> 
> > +
> > +proc run_tests {} {
> > +    global srcfile

<snip>

> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> > +
> > +# test with gcc column info enabled
> > +set options [list debug additional_flags=]
> > +
> > +if {[build_executable "failed to prepare" $executable $srcfile
> > $options] == -1}\
> > + {
> > +    return -1
> > +}
> > +
> > +clean_restart $executable
> > +
> > +with_test_prefix "with-column-info" {
> > +    run_tests
> > +}
> 
> So, the above assumes that the compiler generates column-info by
> default, which has not historically been the case for GCC (it started
> to
> emit columns by default with version 8, according to my
> tests).  Other
> compilers may choose to not emit them by default.
> 
> I think it would make sense to make gdb_compile recognize the new
> "column-info" and "no-column-info" options, which would translate to
> the
> right flags for the given compiler.  gdb_compile already handles the
> nitty gritty details of choosing compiler flags for specific compiler
> versions.  This way, individual tests don't contain compiler flags
> that
> are possibly compiler-specific.

OK, I think I have this implemented as suggested.  It does seem to
work.  I added both gcc and clang support.  Per the link to clang which
does have line info options.

I put the new gdb_compile option support in a separate patch, followed
by the updated "Fix reverse stepping multiple.... " patch.

I tested the test case with:

  make check  RUNTESTFLAGS='CC_FOR_TARGET=clang GDB=.../gdb gdb.reverse/func-map-to-same-line.exp' > out

and 

  make check  RUNTESTFLAGS='GDB=.../gdb gdb.reverse/func-map-to-same-line.exp' > out

I looked in the gdb/testsuite/gdb.log file to verify that the compiler
line (gcc or clang) explicitly has the  -gcolumn-info for the first
test and  -gno-column-info for the second test.  The expected number
of success were seen for the test with gcc and clang.  It all looks
like it works.

> 
> > +
> > +#test with gcc column info disabled
> > +set options [list debug additional_flags=-gno-column-info]
> > +
> > +if {[build_executable "failed to prepare" $executable $srcfile
> > $options] == -1}\
> > + {
> > +    return -1
> > +}
> > +
> > +set $executable executable_without_column_info
> > +clean_restart $executable
> > +
> > +with_test_prefix "no-column-info" {
> > +    run_tests
> > +}
> 
> This would probably be a good use for foreach_with_prefix (if you can
> make it work), to make things more compact:
> 
>   foreach_with_prefix with_column_info {yes no} {
>   }
> 
> ... or something like that.

Changed to the foreach_with_prefix.  

Thanks for the review.

Will post version 5 as a series of two patches.

           Carl 

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

[Simon Marchi]

> I was going to suggest something similar in an earlier revision, but
> when I tried to look for how to control it in clang, I couldn't see it
> at all, that's why I thought it was OK to restrict it to gcc only. Can
> clang (or other compilers for that matter) emit this information?clang does, yes, with the same flags as gcc:

https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-gcolumn-info

> Also, how would gdb_compile handle if the current compiler doesn't
> support a given option, but the others do? Should it loudly fail, or
> silently ignore the "broken" option? If the second, I guess there is
> no harm in allowing clang to run these tests and testing the same
> scenario twice

I'm not sure, you'd have to look at how other options are handled.

But intuitively, if gdb_compile isn't able to fulfill your request, then
it should fail.  For instance, if you used no-column-info with gcc 6
(which doesn't support column info at all), gdb_compile should succeed,
even if there isn't an option to disable column info with that compiler.
If you used column-info with gcc 6, gdb_compile would fail.

If there exists some compiler that always emits column info, with no
option to turn it off, then column-info would work with that compiler,
but no-column-info wouldn't.

To start with, I think it's fine to make column-info and no-column-info
map to -gcolumn-info and -gno-column-info.  And then, you can probably
teach gdb_compile about older gccs and older clangs that don't support
column info.

If people test with other compilers that don't support -gcolumn-info or
-gno-column-info, the test won't compile.  They can then contribute
support for the column-info / no-column-info options for that compiler.

Simon

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

[Bruno Larsen]

On 11/05/2023 18:01, Simon Marchi wrote:
>> +with_test_prefix "with-column-info" {
>> +    run_tests
>> +}
> So, the above assumes that the compiler generates column-info by
> default, which has not historically been the case for GCC (it started to
> emit columns by default with version 8, according to my tests).  Other
> compilers may choose to not emit them by default.
Yes, column info started being generated in gcc7 and was made default in 
gcc 8
>
> I think it would make sense to make gdb_compile recognize the new
> "column-info" and "no-column-info" options, which would translate to the
> right flags for the given compiler.  gdb_compile already handles the
> nitty gritty details of choosing compiler flags for specific compiler
> versions.  This way, individual tests don't contain compiler flags that
> are possibly compiler-specific.
>
I was going to suggest something similar in an earlier revision, but 
when I tried to look for how to control it in clang, I couldn't see it 
at all, that's why I thought it was OK to restrict it to gcc only. Can 
clang (or other compilers for that matter) emit this information?

Also, how would gdb_compile handle if the current compiler doesn't 
support a given option, but the others do? Should it loudly fail, or 
silently ignore the "broken" option? If the second, I guess there is no 
harm in allowing clang to run these tests and testing the same scenario 
twice

-- 
Cheers,
Bruno

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

[Simon Marchi]

I'd like to help reviewing this, but I don't have much time at the
moment, so just a few comments on one test to start with.

> 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..412ab180943
> --- /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

Use /* */ for comments, for consistency with the rest of the code base.

> +
> +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..da5ee282053
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-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/>.  */
> +
> +# 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.
> +if {![test_compiler_info {gcc-*}]
> +    || [test_compiler_info {gcc-[1-6]-*}]} {
> +    return
> +}

I would prefer not to filter out by compiler explicitly like that.
It would be useful for the test to run with other compilers too.

> +
> +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 gcc column info enabled
> +set options [list debug additional_flags=]
> +
> +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
> + {
> +    return -1
> +}
> +
> +clean_restart $executable
> +
> +with_test_prefix "with-column-info" {
> +    run_tests
> +}

So, the above assumes that the compiler generates column-info by
default, which has not historically been the case for GCC (it started to
emit columns by default with version 8, according to my tests).  Other
compilers may choose to not emit them by default.

I think it would make sense to make gdb_compile recognize the new
"column-info" and "no-column-info" options, which would translate to the
right flags for the given compiler.  gdb_compile already handles the
nitty gritty details of choosing compiler flags for specific compiler
versions.  This way, individual tests don't contain compiler flags that
are possibly compiler-specific.

> +
> +#test with gcc column info disabled
> +set options [list debug additional_flags=-gno-column-info]
> +
> +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
> + {
> +    return -1
> +}
> +
> +set $executable executable_without_column_info
> +clean_restart $executable
> +
> +with_test_prefix "no-column-info" {
> +    run_tests
> +}

This would probably be a good use for foreach_with_prefix (if you can
make it work), to make things more compact:

  foreach_with_prefix with_column_info {yes no} {
  }

... or something like that.

Simon

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

[Carl Love]

Bruno, GDB maintainers:

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     | 156 ++++++++++++++++++
 gdb/testsuite/gdb.reverse/map-to-same-line.c  |  58 +++++++
 .../gdb.reverse/map-to-same-line.exp          | 156 ++++++++++++++++++
 7 files changed, 528 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..412ab180943
--- /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..da5ee282053
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/func-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/>.  */
+
+# 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.
+if {![test_compiler_info {gcc-*}]
+    || [test_compiler_info {gcc-[1-6]-*}]} {
+    return
+}
+
+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 gcc column info enabled
+set options [list debug additional_flags=]
+
+if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
+ {
+    return -1
+}
+
+clean_restart $executable
+
+with_test_prefix "with-column-info" {
+    run_tests
+}
+
+#test with gcc column info disabled
+set options [list debug additional_flags=-gno-column-info]
+
+if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
+ {
+    return -1
+}
+
+set $executable executable_without_column_info
+clean_restart $executable
+
+with_test_prefix "no-column-info" {
+    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"
+}
-- 
2.37.2