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

Carl Love via Gdb-patches
2022-05-31T15:12:48Z

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