* Re: [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table
2022-06-09 13:04 [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table Luis Machado
@ 2022-06-09 15:18 ` will schmidt
2022-06-16 21:13 ` [PING] " Carl Love
2022-06-21 16:52 ` Ulrich Weigand
2 siblings, 0 replies; 4+ messages in thread
From: will schmidt @ 2022-06-09 15:18 UTC (permalink / raw)
To: Luis Machado, gdb-patches; +Cc: cel, rogealve, blarsen, Ulrich.Weigand
On Thu, 2022-06-09 at 14:04 +0100, Luis Machado wrote:
> Sending as a clean new thread.
>
> v6:
> - Fix misc typos and augment the commit message.
> - Updated source file comments.
Thanks for the answers and updates. I don't have any further
questions or comments. I am not an approver, but LGTM.
Thanks
-Will
>
> v5:
> - Updated test case comments on the purpose of the test.
> - Add test to check system supports record-replay.
> - Removed now unnecessary record 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 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.
>
> These GCC changes came into effect with commits
> 497b7c47042d542ae48d10badf0c3d0088f6f798 and
> and 0029b929c9719a9794492915206314308fbdf03a.
>
> 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 | 54 +++++++
> .../gdb.reverse/map-to-same-line.exp | 141 ++++++++++++++++++
> 5 files changed, 281 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 8564986f66d..9625ad7aa8a 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3390,6 +3390,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 ac902a4cbbe..500faf7d93b 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2286,6 +2286,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..89a1898bdc2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,54 @@
> +/* 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 source file provides dummy code that is only meant to force the
> + compiler to generate instructions for the various labels. The TAG markers
> + are used by the testcase to generate DWARF information and line tables. */
> +
> +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..fd958ba114b
> --- /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 multiple 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"
> +}
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PING] [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table
2022-06-09 13:04 [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table Luis Machado
2022-06-09 15:18 ` will schmidt
@ 2022-06-16 21:13 ` Carl Love
2022-06-21 16:52 ` Ulrich Weigand
2 siblings, 0 replies; 4+ messages in thread
From: Carl Love @ 2022-06-16 21:13 UTC (permalink / raw)
To: Luis Machado, gdb-patches; +Cc: rogealve, will_schmidt, blarsen, Ulrich.Weigand
GDB developers:
Ping? Just wondering if anyone has had a chance to look at this patch.
Carl
-----------------------------
On Thu, 2022-06-09 at 14:04 +0100, Luis Machado wrote:
> Sending as a clean new thread.
>
> v6:
> - Fix misc typos and augment the commit message.
> - Updated source file comments.
>
> v5:
> - Updated test case comments on the purpose of the test.
> - Add test to check system supports record-replay.
> - Removed now unnecessary record 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 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.
>
> These GCC changes came into effect with commits
> 497b7c47042d542ae48d10badf0c3d0088f6f798 and
> and 0029b929c9719a9794492915206314308fbdf03a.
>
> 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 | 54 +++++++
> .../gdb.reverse/map-to-same-line.exp | 141
> ++++++++++++++++++
> 5 files changed, 281 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 8564986f66d..9625ad7aa8a 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3390,6 +3390,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 ac902a4cbbe..500faf7d93b 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2286,6 +2286,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..89a1898bdc2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,54 @@
> +/* 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 source file provides dummy code that is only meant to force
> the
> + compiler to generate instructions for the various labels. The
> TAG markers
> + are used by the testcase to generate DWARF information and line
> tables. */
> +
> +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..fd958ba114b
> --- /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
> multiple 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"
> +}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table
2022-06-09 13:04 [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table Luis Machado
2022-06-09 15:18 ` will schmidt
2022-06-16 21:13 ` [PING] " Carl Love
@ 2022-06-21 16:52 ` Ulrich Weigand
2 siblings, 0 replies; 4+ messages in thread
From: Ulrich Weigand @ 2022-06-21 16:52 UTC (permalink / raw)
To: gdb-patches, luis.machado
Cc: Rogerio Alves Cardoso, will_schmidt, blarsen, cel
Luis Machado <luis.machado@arm.com> wrote:
Carl asked me to look into this as well, so a couple of comments:
>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.
I've been wondering why the forward-stepping case works. It looks
like this is because of the code at the end of process_event_stop_test,
which verifies whether we have stepped to a different line, and if not,
just continues stepping.
It seems it would be preferable to have the same mechanism for both
forward- and reverse-stepping, but that doesn't work because of
this piece of code you point out:
>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);
In my view, this code is already questionable. This is under the
overall control of "pc_in_thread_step_range", and the point of
this is to check that we're still supposed to step! But even
though the test returns true, we now add a special case where
we stop stepping after all ...
Maybe we should not hard-code the end_stepping_range call here, but
rath
er just fall through to the rest of process_event_step_test
to handle
whatever other case we may encounter? Or equivalently,
have
pc_in_thread_step_range do a
start < pc <= end
comparison in the
EXEC_REVERSE case, instead of
start <= pc < end
[ But maybe this is also better done as independent cleanup,
I'm not saying we necessarily have to do it this way ... ]
>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.
Maybe not immediately when reading the line tables; we might
want to actually use the column granularity at some point.
But at the point where we set up the stepping range in
prepare_one_step, we could increase the range *there*.
This might not only be somewhat simpler, but also improve
performance since the check is only done once and not every
time we run through process_event_stop_test (and it might
even also improve the performance of forward-stepping in
those cases if we increase the range downwards too).
>+ 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);
As an aside, if we keep this approach, it would be cleaner
to have the whole logic under a single "if EXEC_REVERSE".
(There is no point in choosing a value for the "range_start"
variable in EXEC_FORWARD if it is never used.)
Bye,
Ulrich
^ permalink raw reply [flat|nested] 4+ messages in thread