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

[Luis Machado <luis.machado@arm.com>]

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!