* [PATCH, v6] Fix reverse stepping multiple contiguous PC ranges over the line table
@ 2022-06-09 13:04 Luis Machado
2022-06-09 15:18 ` will schmidt
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Luis Machado @ 2022-06-09 13:04 UTC (permalink / raw)
To: gdb-patches; +Cc: cel, rogealve, will_schmidt, blarsen, Ulrich.Weigand
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"
+}
--
2.25.1
^ 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: 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
end of thread, other threads:[~2022-06-21 16:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).