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

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

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