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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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