* [ PATCH 0/3] Fix GDB reverse execution behavior
[not found] ` <ee4ed81911a3196327c2aea3fa9a77f910a5e798.camel@linux.ibm.com>
@ 2023-11-22 23:33 ` Carl Love
2023-11-30 11:36 ` Guinevere Larsen
2023-11-22 23:33 ` [ PATCH 1/3] " Carl Love
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-11-22 23:33 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: cel, Pedro Alves, Tom Tromey, Simon Marchi
GDB developers:
The patch "[PATCH 2/2 ver 8] Fix reverse stepping multiple contiguous
PC ranges over the line table",
https://sourceware.org/pipermail/gdb-patches/2023-August/201889.html
was posted and pinged several times without any response from the
developers. It was reviewed by Guinevere Larsen. The original patch 2
was intended to fix failures on PowerPC and AArch64. An additional
architecture independent change and test was added per feedback from
the community.
Based on some private discussions on how best to get the patch
approved, it was suggested that I should break the patch in two to make
it easier to review. Specifically, separate out the PowerPC and
AArch64 fixes from the generic architecture independent changes. The
original patch 2 has been split up as suggested. The hope is this will
make it easier to review and get approval.
The two new patches are functionally identical to the previously posted
patch 2.
The patch series now consists of three patches.
Patch 1, adds a new option to gdb_compile to either generate or not
generate the line table information. No change from the prior version.
Patch 2, fix the specific GDB issues with reverse stepping over a line
with multiple statements in the same line for PowerPC and AArch64.
Behavior on X86-64 did not change.
Patch 3, fix the behavior of GDB for all architectures when executing
the next command in reverse on a line that contains multiple function
calls.
The series of three patches has been tested on PowerPC, X86 and AArch64
with no regression errors. Please let me know if these patches are
acceptable for mainline. Note, Luis Machado did the AArch64 testing.
Thanks.
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
* [ PATCH 1/3] Fix GDB reverse execution behavior
[not found] ` <ee4ed81911a3196327c2aea3fa9a77f910a5e798.camel@linux.ibm.com>
2023-11-22 23:33 ` [ PATCH 0/3] Fix GDB reverse execution behavior Carl Love
@ 2023-11-22 23:33 ` Carl Love
2023-11-29 11:44 ` Luis Machado
2023-11-22 23:33 ` [ PATCH 2/3] " Carl Love
2023-11-22 23:33 ` [PATCH 3/3] " Carl Love
3 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-11-22 23:33 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: Carl Love, Pedro Alves, Tom Tromey, Simon Marchi
GDB maintainers:
This patch in the series is unchanged from the previously posted
version 2 patch of patch 1 in the previous patch series.
Version 2, updated the compiler check and handling for gcc version 6
and earlier. Retested on Power 10.
Per the comments on version 4 for the gdb.reverse/func-map-to-same-
line.exp, I have added support to proc gdb_compile to enable or disable
generating line information as part of the debug information. The two
new options are column-info and no-column-info.
This patch implements the new options for gdb_compile.
These options have been tested with patch 2 and patch 3 on PowerPC with
the GCC and clang compilers.
Please let me know if the patch is acceptable for mainline. Thanks.
Carl
------------------------------------------------------------
Add gdb_compile options column-info and no-column-info
This patch adds two new options to gdb_compile to specify if the compile
should or should not generate the line table information. The
options are supported on clang and gcc version 7 and newer.
Patch has been tested on PowerPC with both gcc and clang.
---
gdb/testsuite/lib/gdb.exp | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 63885860795..a63394d5cc0 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5150,6 +5150,8 @@ proc quote_for_host { args } {
# debug information
# - text_segment=addr: Tell the linker to place the text segment at ADDR.
# - build-id: Ensure the final binary includes a build-id.
+# - column-info/no-column-info: Enable/Disable generation of column table
+# information.
#
# And here are some of the not too obscure options understood by DejaGnu that
# influence the compilation:
@@ -5359,6 +5361,38 @@ proc gdb_compile {source dest type options} {
} else {
error "Don't know how to handle text_segment option."
}
+ } elseif { $opt == "column-info" } {
+ # If GCC or clang does not support column-info, compilation
+ # will fail and the usupported column-info option will be
+ # reported as such.
+ if {[test_compiler_info {gcc-*}]} {
+ lappend new_options "additional_flags=-gcolumn-info"
+
+ } elseif {[test_compiler_info {clang-*}]} {
+ lappend new_options "additional_flags=-gcolumn-info"
+
+ } else {
+ error "Option gcolumn-info option not supported by compiler."
+ }
+
+ } elseif { $opt == "no-column-info" } {
+ if {[test_compiler_info {gcc-*}]} {
+ if {[test_compiler_info {gcc-[1-6]-*}]} {
+ # In this case, don't add the compile line option and
+ # the result will be the same as using no-column-info
+ # on a version that supports the option.
+ warning "gdb_compile option no-column-info not supported, ignoring."
+ } else {
+ lappend new_options "additional_flags=-gno-column-info"
+ }
+
+ } elseif {[test_compiler_info {clang-*}]} {
+ lappend new_options "additional_flags=-gno-column-info"
+
+ } else {
+ error "Option gno-column-info option not supported by compiler."
+ }
+
} else {
lappend new_options $opt
}
--
2.41.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [ PATCH 2/3] Fix GDB reverse execution behavior
[not found] ` <ee4ed81911a3196327c2aea3fa9a77f910a5e798.camel@linux.ibm.com>
2023-11-22 23:33 ` [ PATCH 0/3] Fix GDB reverse execution behavior Carl Love
2023-11-22 23:33 ` [ PATCH 1/3] " Carl Love
@ 2023-11-22 23:33 ` Carl Love
2023-11-29 11:44 ` Luis Machado
2023-11-22 23:33 ` [PATCH 3/3] " Carl Love
3 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-11-22 23:33 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: Carl Love, Pedro Alves, Tom Tromey, Simon Marchi
GDB maintainers:
The following patch fixes issues on PowerPC and AArch64 with the
reverse-step and reverse-next instructions when there are multiple
assignment statements on the same line.
The patch adds a function to search the line table to find the address
of the first instruction of the line. When setting up the
reverse stepping range, the function is called to make sure the start
of the range corresponds to the address of the first instruction for
the line.
The following patch fixes the execution of the reverse-step and
reverse-next commands for both senarios of multiple statements on the
same line for PowerPC and aarch64-linux. The patch does not change the
behavior on other architectures such as X86-64 where the issue does not
occur when executing in reverse.
The patch adds a new test case gdb.reverse/map-to-same-line.exp to
verify the fix.
The patch has been tested on PowerPC and Intel X86 and AArch64 with no
new regression failures.
Please let me know if the patch is acceptable for mainline. Thanks.
Carl
------------------------------------------------
PowerPC and aarch64: Fix reverse stepping failure
When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
the ppc backend), there are 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.
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.
Since we've reached ecs->event_thread->control.step_range_start, we stop
stepping backwards.
The above issues were fixed by introducing a new function that looks 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. The new start PC for the range
is used for the control.step_range_start when setting up a step range.
The test case gdb.reverse/map-to-same-line.exp is added to test the fix
for the above reverse step issues.
Patch has been tested on PowerPC, X86 and AArch64 with no regressions.
---
gdb/infrun.c | 57 +++++++
gdb/symtab.c | 50 ++++++
gdb/symtab.h | 17 ++
gdb/testsuite/gdb.reverse/map-to-same-line.c | 58 +++++++
.../gdb.reverse/map-to-same-line.exp | 153 ++++++++++++++++++
5 files changed, 335 insertions(+)
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 62b306ff347..27251d3023e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -116,6 +116,8 @@ static struct async_event_handler *infrun_async_inferior_event_token;
/* Stores whether infrun_async was previously enabled or disabled.
Starts off as -1, indicating "never enabled/disabled". */
static int infrun_is_async = -1;
+static CORE_ADDR update_line_range_start (CORE_ADDR pc,
+ struct execution_control_state *ecs);
/* See infrun.h. */
@@ -7279,6 +7281,27 @@ handle_signal_stop (struct execution_control_state *ecs)
process_event_stop_test (ecs);
}
+/* Return the address for the beginning of the line. */
+
+CORE_ADDR
+update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs)
+{
+ /* The line table may have multiple entries for the same source code line.
+ Given the PC, check the line table and return the PC that corresponds
+ to the line table entry for the source line that PC is in. */
+ CORE_ADDR start_line_pc = ecs->event_thread->control.step_range_start;
+ std::optional<CORE_ADDR> real_range_start;
+
+ /* Call find_line_range_start to get the smallest address in the
+ linetable for multiple Line X entries in the line table. */
+ real_range_start = find_line_range_start (pc);
+
+ if (real_range_start.has_value ())
+ start_line_pc = *real_range_start;
+
+ return start_line_pc;
+}
+
/* Come here when we've got some debug event / signal we can explain
(IOW, not a random signal), and test whether it should cause a
stop, or whether we should resume the inferior (transparently).
@@ -8093,6 +8116,29 @@ process_event_stop_test (struct execution_control_state *ecs)
if (stop_pc_sal.is_stmt)
{
+ if (execution_direction == EXEC_REVERSE)
+ {
+ /* We are stepping backwards make sure we have reached the
+ beginning of the line. */
+ CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
+ CORE_ADDR start_line_pc
+ = update_line_range_start (stop_pc, ecs);
+
+ if (stop_pc != start_line_pc)
+ {
+ /* Have not reached the beginning of the source code line.
+ Set a step range. Execution should stop in any function
+ calls we execute back into before reaching the beginning
+ of the line. */
+ ecs->event_thread->control.step_range_start
+ = start_line_pc;
+ ecs->event_thread->control.step_range_end = stop_pc;
+ set_step_info (ecs->event_thread, frame, stop_pc_sal);
+ keep_going (ecs);
+ return;
+ }
+ }
+
/* We are at the start of a statement.
So stop. Note that we don't stop if we step into the middle of a
@@ -8155,6 +8201,17 @@ process_event_stop_test (struct execution_control_state *ecs)
set_step_info (ecs->event_thread, frame, stop_pc_sal);
infrun_debug_printf ("keep going");
+
+ if (execution_direction == EXEC_REVERSE)
+ {
+ CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
+
+ /* Make sure the stop_pc is set to the beginning of the line. */
+ if (stop_pc != ecs->event_thread->control.step_range_start)
+ ecs->event_thread->control.step_range_start
+ = update_line_range_start (stop_pc, ecs);
+ }
+
keep_going (ecs);
}
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 5ec56f4f2af..70ef7d56878 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -73,6 +73,7 @@
#include "gdbsupport/gdb_string_view.h"
#include "gdbsupport/pathstuff.h"
#include "gdbsupport/common-utils.h"
+#include <optional>
/* Forward declarations for local functions. */
@@ -3311,6 +3312,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. */
+
+std::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 searching. */
+ 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 8dfc873b1c9..9ef09e22902 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -38,6 +38,7 @@
#include "gdb-demangle.h"
#include "split-name.h"
#include "frame.h"
+#include <optional>
/* Opaque declarations. */
struct ui_file;
@@ -2359,6 +2360,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 std::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..3086e849231
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
@@ -0,0 +1,58 @@
+/* Copyright 2023 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 (void)
+{ /* 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");
+ k = j; f3 = f2; str_3 = str_2; /* TAG: main return */
+
+ asm ("end_of_sequence: .globl end_of_sequence");
+ 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..63f8c9c76b3
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
@@ -0,0 +1,153 @@
+# Copyright 2023 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.
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+load_lib dwarf.exp
+require dwarf2_support
+
+# The DWARF assembler requires the gcc compiler.
+require is_c_compiler_gcc
+
+# This test suitable only for process that can do reverse execution
+require supports_reverse
+
+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_set_address end_of_sequence
+ DW_LNE_end_sequence
+ }
+ }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+ [list $srcfile $asm_file] {nodebug} ] } {
+ return -1
+}
+
+if { ![runto_main] } {
+ return
+}
+
+# Print the line table
+gdb_test_multiple "maint info line-table ${testfile}" "" {
+ -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[ \t\]+Y\[^\r\n\]*" {
+ lappend is_stmt $expect_out(1,string)
+ exp_continue
+ }
+ -re -wrap "" {
+ }
+}
+
+# Do the reverse-step and reverse-next tests
+foreach_with_prefix cmd {step next} {
+ gdb_test_no_output "record" "turn on process record, test $cmd"
+
+ set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
+ gdb_breakpoint $srcfile:$bp_main_return
+ gdb_continue_to_breakpoint "run to end of main, reverse-$cmd test" ".*$srcfile:$bp_main_return.*"
+ gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-$cmd test"
+
+ # At this point, GDB has already recorded the execution up until the return
+ # statement. Reverse and test if GDB transitions between lines in the
+ # expected order. It should reverse-step or reverse-next across lines 8,
+ # 5, 3, 2 and 1.
+ foreach line {8 5 3 2 1} {
+ gdb_test "reverse-$cmd" ".*TAG: line $line.*" "reverse $cmd to line $line"
+ }
+
+ if {$cmd =="step"} {
+ ## Clean restart, test reverse-next command
+ clean_restart ${testfile}
+
+ if { ![runto_main] } {
+ return
+ }
+ }
+}
--
2.41.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] Fix GDB reverse execution behavior
[not found] ` <ee4ed81911a3196327c2aea3fa9a77f910a5e798.camel@linux.ibm.com>
` (2 preceding siblings ...)
2023-11-22 23:33 ` [ PATCH 2/3] " Carl Love
@ 2023-11-22 23:33 ` Carl Love
2023-11-29 11:46 ` Luis Machado
3 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-11-22 23:33 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: Carl Love, Pedro Alves, Tom Tromey, Simon Marchi
GDB maintainers:
The following patch changes the behavior of GDB when executing the next
command in reverse over a line that contains multiple function calls on
the same source code line. Currently, GDB stops in the middle of the
line not at the beginning of the line. This patch causes GDB to stop
at the beginning of the line. The incorrect behavior was pointed out
by Pedro Alves in
https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
A new testcase, func-map-to-same-line.exp, is added to verify the
behavior of GDB.
The fix requires updating two testcases, gdb/testsuite/gdb.mi/mi-
reverse.exp and gdb.reverse/finish-reverse-next.exp. The testcases
have to issue two reverse next instructions to reach the previous
source code line. With this patch, they only require a single reverse
next instruction to reach the previous line.
The patch has been tested on PowerPC and X86-64 with no regression
errors.
Please let me know if the patch is acceptable for mainline. Thanks.
Carl
----------------------------------------------------
Fix GDB reverse-step and reverse-next command behavior
Currently GDB when executing in reverse over multiple statements in a single
line of source code, GDB stops in the middle of the line. Thus requiring
multiple commands to reach the previous line. GDB should stop at the first
instruction of the line, not in the middle of the line.
The following description of the incorrect behavior was taken from an
earlier message by Pedro Alves <pedro@palves.net>:
https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
---------------------------------
The source line looks like:
func1 (); func2 ();
in the test case:
(gdb) list 1
1 void func1 ()
2 {
3 }
4
5 void func2 ()
6 {
7 }
8
9 int main ()
10 {
11 func1 (); func2 ();
12 }
compiled with:
$ gcc reverse.c -o reverse -g3 -O0
$ gcc -v
...
gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
Now let's debug it with target record, using current gdb git master
(f3d8ae90b236),
$ gdb ~/reverse
GNU gdb (GDB) 14.0.50.20230124-git
...
Reading symbols from /home/pedro/reverse...
(gdb) start
Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
Starting program: /home/pedro/reverse
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Temporary breakpoint 1, main () at reverse.c:11
11 func1 (); func2 ();
(gdb) record
(gdb) disassemble /s
Dump of assembler code for function main:
reverse.c:
10 {
0x000055555555513f <+0>: endbr64
0x0000555555555143 <+4>: push %rbp
0x0000555555555144 <+5>: mov %rsp,%rbp
11 func1 (); func2 ();
=> 0x0000555555555147 <+8>: mov $0x0,%eax
0x000055555555514c <+13>: call 0x555555555129 <func1>
0x0000555555555151 <+18>: mov $0x0,%eax
0x0000555555555156 <+23>: call 0x555555555134 <func2>
0x000055555555515b <+28>: mov $0x0,%eax
12 }
0x0000555555555160 <+33>: pop %rbp
0x0000555555555161 <+34>: ret
End of assembler dump.
(gdb) n
12 }
So far so good, a "next" stepped over the whole of line 11 and stopped at
line 12.
Let's confirm where we are now:
(gdb) disassemble /s
Dump of assembler code for function main:
reverse.c:
10 {
0x000055555555513f <+0>: endbr64
0x0000555555555143 <+4>: push %rbp
0x0000555555555144 <+5>: mov %rsp,%rbp
11 func1 (); func2 ();
0x0000555555555147 <+8>: mov $0x0,%eax
0x000055555555514c <+13>: call 0x555555555129 <func1>
0x0000555555555151 <+18>: mov $0x0,%eax
0x0000555555555156 <+23>: call 0x555555555134 <func2>
0x000055555555515b <+28>: mov $0x0,%eax
12 }
=> 0x0000555555555160 <+33>: pop %rbp
0x0000555555555161 <+34>: ret
End of assembler dump.
Good, we're at the first instruction of line 12.
Now let's undo the "next", with "reverse-next":
(gdb) reverse-next
11 func1 (); func2 ();
Seemingly stopped at line 11. Let's see exactly where:
(gdb) disassemble /s
Dump of assembler code for function main:
reverse.c:
10 {
0x000055555555513f <+0>: endbr64
0x0000555555555143 <+4>: push %rbp
0x0000555555555144 <+5>: mov %rsp,%rbp
11 func1 (); func2 ();
0x0000555555555147 <+8>: mov $0x0,%eax
0x000055555555514c <+13>: call 0x555555555129 <func1>
=> 0x0000555555555151 <+18>: mov $0x0,%eax
0x0000555555555156 <+23>: call 0x555555555134 <func2>
0x000055555555515b <+28>: mov $0x0,%eax
12 }
0x0000555555555160 <+33>: pop %rbp
0x0000555555555161 <+34>: ret
End of assembler dump.
(gdb)
And lo, we stopped in the middle of line 11! That is a bug, we should have
stepped back all the way to the beginning of the line. The "reverse-next"
should have fully undone the prior "next" command.
--------------------
This patch fixes the incorrect GDB behavior by ensuring that GDB stops at
the first instruction in the line.
The test case gdb.reverse/func-map-to-same-line.exp is added to testsuite
to verify this fix when the line table information is and is not available.
---
gdb/infcmd.c | 13 ++
gdb/testsuite/gdb.mi/mi-reverse.exp | 5 +-
.../gdb.reverse/finish-reverse-next.exp | 40 ++---
.../gdb.reverse/func-map-to-same-line.c | 37 +++++
.../gdb.reverse/func-map-to-same-line.exp | 140 ++++++++++++++++++
5 files changed, 201 insertions(+), 34 deletions(-)
create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index cf8cd527955..1415fe55163 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -975,6 +975,19 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
&tp->control.step_range_start,
&tp->control.step_range_end);
+ if (execution_direction == EXEC_REVERSE)
+ {
+ symtab_and_line sal = find_pc_line (pc, 0);
+ symtab_and_line sal_start
+ = find_pc_line (tp->control.step_range_start, 0);
+
+ if (sal.line == sal_start.line)
+ /* Executing in reverse, the step_range_start address is in
+ the same line. We want to stop in the previous line so
+ move step_range_start before the current line. */
+ tp->control.step_range_start--;
+ }
+
/* There's a problem in gcc (PR gcc/98780) that causes missing line
table entries, which results in a too large stepping range.
Use inlined_subroutine info to make the range more narrow. */
diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
index baa53a495d7..0630b8b6c7f 100644
--- a/gdb/testsuite/gdb.mi/mi-reverse.exp
+++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
@@ -96,11 +96,8 @@ proc test_controlled_execution_reverse {} {
"reverse finish from callme"
# Test exec-reverse-next
- # It takes two steps to get back to the previous line,
- # as the first step moves us to the start of the current line,
- # and the one after that moves back to the previous line.
- mi_execute_to "exec-next --reverse 2" \
+ mi_execute_to "exec-next --reverse" \
"end-stepping-range" "main" "" \
"basics.c" $line_main_hello "" \
"reverse next to get over the call to do_nothing"
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
index 1f53b649a7d..6488f66b107 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -76,14 +76,10 @@ gdb_continue_to_breakpoint \
repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
# The reverse-finish command should stop on the function call instruction
-# which is the last instruction in the source code line. A reverse-next
-# instruction should then stop at the first instruction in the same source
-# code line. Another revers-next instruction stops at the previous source
-# code line.
+# which is the last instruction in the source code line. Another reverse-next
+# instruction stops at the previous source code line.
gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
"reverse-finish function1 LEP call from LEP "
-gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP" \
- "reverse next 1 LEP entry point function call from LEP"
gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP"
@@ -109,14 +105,10 @@ gdb_continue_to_breakpoint \
gdb_test "step" ".*int ret = 0;.*" "step test 1"
# The reverse-finish command should stop on the function call instruction
-# which is the last instruction in the source code line. A reverse-next
-# instruction should then stop at the first instruction in the same source
-# code line. Another revers-next instruction stops at the previous source
-# code line.
+# which is the last instruction in the source code line. Another reverse-next
+# instruction stops at the previous source code line.
gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
"reverse-finish function1 LEP call from function body"
-gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
- "reverse next 1 LEP from function body"
gdb_test "reverse-next" ".*b = 5;.*" \
"reverse next 2 at b = 5, from function body"
@@ -144,14 +136,10 @@ gdb_continue_to_breakpoint \
repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
# The reverse-finish command should stop on the function call instruction
-# which is the last instruction in the source code line. A reverse-next
-# instruction should then stop at the first instruction in the same source
-# code line. Another revers-next instruction stops at the previous source
-# code line.
+# which is the last instruction in the source code line. Another reverse-next
+# instruction stops at the previous source code line.
gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
"function1 GEP call call from GEP"
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
- "reverse next 1 GEP entry point function call from GEP"
gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP"
gdb_test "reverse-continue" ".*" "setup for test 4"
@@ -180,14 +168,10 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call again"
gdb_test "stepi" "{" "stepi to between GEP and LEP"
# The reverse-finish command should stop on the function call instruction
-# which is the last instruction in the source code line. A reverse-next
-# instruction should then stop at the first instruction in the same source
-# code line. Another revers-next instruction stops at the previous source
-# code line.
+# which is the last instruction in the source code line. Another reverse-next
+# instruction stops at the previous source code line.
gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
"function1 GEP call call from GEP again"
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
- "reverse next 1 GEP entry point function call from GEP again"
gdb_test "reverse-next" ".*b = 50;.*" \
"reverse next 2 at b = 50, call from GEP again"
@@ -212,13 +196,9 @@ gdb_continue_to_breakpoint \
gdb_test "step" ".*int ret = 0;.*" "step test 2"
# The reverse-finish command should stop on the function call instruction
-# which is the last instruction in the source code line. A reverse-next
-# instruction should then stop at the first instruction in the same source
-# code line. Another revers-next instruction stops at the previous source
-# code line.
+# which is the last instruction in the source code line. Another reverse-next
+# instruction stops at the previous source code line.
gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
"reverse-finish function1 GEP call, from function body "
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
- "reverse next 1 GEP call from function body"
gdb_test "reverse-next" ".*b = 50;.*" \
"reverse next 2 at b = 50 from function body"
diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.c b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
new file mode 100644
index 00000000000..17fe17af267
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
@@ -0,0 +1,37 @@
+/* Copyright 2023 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 test is used to test the reverse-step and reverse-next instruction
+ execution for a source line that contains multiple function calls. */
+
+void
+func1 (void)
+{
+} /* END FUNC1 */
+
+void
+func2 (void)
+{
+} /* END FUNC2 */
+
+int
+main (void)
+{
+ int a, b;
+ a = 1;
+ b = 2;
+ func1 (); func2 ();
+ a = a + b; /* START REVERSE TEST */
+}
diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
new file mode 100644
index 00000000000..1b23233b43e
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
@@ -0,0 +1,140 @@
+# Copyright 2023 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 file is part of the GDB testsuite. It tests reverse stepping.
+# Lots of code borrowed from "step-test.exp".
+
+# This test checks to make sure there is no regression failures for
+# the reverse-next command when stepping back over two functions in
+# the same line.
+
+require supports_reverse
+
+# This test uses the gcc no-column-info command which was added in gcc 7.1.
+
+proc run_tests {} {
+ global testfile
+
+ clean_restart ${testfile}
+
+ if { ![runto_main] } {
+ return
+ }
+
+ with_test_prefix "next-test" {
+ gdb_test_no_output "record" "turn on process record"
+
+ # This regression test verifies the reverse-step and reverse-next
+ # commands work properly when executing backwards thru a source line
+ # containing two function calls on the same source line, i.e.
+ # func1 (); func2 ();. This test is compiled so the dwarf info
+ # does not contain the line table information.
+
+ # Test 1, reverse-next command
+ # Set breakpoint at the line after the function calls.
+ set bp_start_reverse_test [gdb_get_line_number "START REVERSE TEST"]
+
+ gdb_breakpoint $bp_start_reverse_test temporary
+
+ # Continue to break point for reverse-next test.
+ # Command definition: reverse-next [count]
+ # Run backward to the beginning of the previous line executed in
+ # the current (innermost) stack frame. If the line contains
+ # function calls,they will be “un-executed” without stopping.
+ # Starting from the first line of a function, reverse-next will
+ # take you back to the caller of that function, before the
+ # function was called, just as the normal next command would take
+ # you from the last line of a function back to its return to its
+ # caller 2.
+
+ gdb_continue_to_breakpoint \
+ "stopped at command reverse-next test start location" \
+ ".*$bp_start_reverse_test\r\n.*"
+
+ # The reverse-next should step all the way back to the beginning of
+ # the line, i.e. at the beginning of the func1 call.
+ gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
+ " reverse-next to line with two functions"
+
+ # We should be stopped at the first instruction of the line. A
+ # reverse-step should step back and stop at the beginning of the
+ # previous line b = 2, i.e. not in func1 ().
+ gdb_test "reverse-stepi" ".*b = 2;.*" \
+ "reverse-stepi to previous line b = 2"
+ }
+
+ # Setup for test 2
+ clean_restart ${testfile}
+
+ if { ![runto_main] } {
+ return
+ }
+
+ with_test_prefix "step-test" {
+ gdb_test_no_output "record" "turn on process record"
+
+ # Test 2, reverse-step command
+ # Set breakpoint at the line after the function calls.
+ gdb_breakpoint $bp_start_reverse_test temporary
+
+ # Continue to the start of the reverse-step test.
+ # Command definition: reverse-step [count]
+ # Run the program backward until control reaches the start of a
+ # different source line; then stop it, and return control to gdb.
+ # Like the step command, reverse-step will only stop at the
+ # beginning of a source line. It “un-executes” the previously
+ # executed source line. If the previous source line included calls
+ # to debuggable functions, reverse-step will step (backward) into
+ # the called function, stopping at the beginning of the last
+ # statement in the called function (typically a return statement).
+ # Also, as with the step command, if non-debuggable functions are
+ # called, reverse-step will run thru them backward without
+ # stopping.
+
+ gdb_continue_to_breakpoint \
+ "stopped at command reverse-step test start location" \
+ ".*$bp_start_reverse_test\r\n.*"
+
+ # The first reverse step should take us call of func2 ().
+ gdb_test "reverse-step" ".*END FUNC2.*" \
+ "reverse-step into func2 "
+
+ # The second reverse step should take us into func1 ().
+ gdb_test "reverse-step" ".*END FUNC1.*" \
+ "reverse-step into func1 "
+
+ # The third reverse step should take us call of func1 ().
+ gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
+ "reverse-step to line func1(); func2(), at call for func1 "
+ # We should be stopped at the first instruction of the line. A
+ # reversee stepi should take us to b = 2 ().
+ gdb_test "reverse-stepi" ".*b = 2;.*" \
+ "reverse-stepi to line b = 2 "
+ }
+}
+
+standard_testfile .c
+
+# test with and without gcc column info enabled
+foreach_with_prefix column_info_flag {column-info no-column-info} {
+ set options [list debug $column_info_flag]
+
+ if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+ $options]} {
+ return -1
+ }
+
+ run_tests
+}
--
2.41.0
---------------------------------
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 2/3] Fix GDB reverse execution behavior
2023-11-22 23:33 ` [ PATCH 2/3] " Carl Love
@ 2023-11-29 11:44 ` Luis Machado
0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-11-29 11:44 UTC (permalink / raw)
To: Carl Love, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 11/22/23 23:33, Carl Love wrote:
> GDB maintainers:
>
> The following patch fixes issues on PowerPC and AArch64 with the
> reverse-step and reverse-next instructions when there are multiple
> assignment statements on the same line.
>
> The patch adds a function to search the line table to find the address
> of the first instruction of the line. When setting up the
> reverse stepping range, the function is called to make sure the start
> of the range corresponds to the address of the first instruction for
> the line.
>
> The following patch fixes the execution of the reverse-step and
> reverse-next commands for both senarios of multiple statements on the
> same line for PowerPC and aarch64-linux. The patch does not change the
> behavior on other architectures such as X86-64 where the issue does not
> occur when executing in reverse.
>
> The patch adds a new test case gdb.reverse/map-to-same-line.exp to
> verify the fix.
>
> The patch has been tested on PowerPC and Intel X86 and AArch64 with no
> new regression failures.
>
> Please let me know if the patch is acceptable for mainline. Thanks.
>
> Carl
> ------------------------------------------------
>
> PowerPC and aarch64: Fix reverse stepping failure
>
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
> the ppc backend), there are 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.
>
> 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.
>
> Since we've reached ecs->event_thread->control.step_range_start, we stop
> stepping backwards.
>
> The above issues were fixed by introducing a new function that looks 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. The new start PC for the range
> is used for the control.step_range_start when setting up a step range.
>
> The test case gdb.reverse/map-to-same-line.exp is added to test the fix
> for the above reverse step issues.
>
> Patch has been tested on PowerPC, X86 and AArch64 with no regressions.
> ---
> gdb/infrun.c | 57 +++++++
> gdb/symtab.c | 50 ++++++
> gdb/symtab.h | 17 ++
> gdb/testsuite/gdb.reverse/map-to-same-line.c | 58 +++++++
> .../gdb.reverse/map-to-same-line.exp | 153 ++++++++++++++++++
> 5 files changed, 335 insertions(+)
> 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 62b306ff347..27251d3023e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -116,6 +116,8 @@ static struct async_event_handler *infrun_async_inferior_event_token;
> /* Stores whether infrun_async was previously enabled or disabled.
> Starts off as -1, indicating "never enabled/disabled". */
> static int infrun_is_async = -1;
> +static CORE_ADDR update_line_range_start (CORE_ADDR pc,
> + struct execution_control_state *ecs);
>
> /* See infrun.h. */
>
> @@ -7279,6 +7281,27 @@ handle_signal_stop (struct execution_control_state *ecs)
> process_event_stop_test (ecs);
> }
>
> +/* Return the address for the beginning of the line. */
> +
> +CORE_ADDR
> +update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs)
> +{
> + /* The line table may have multiple entries for the same source code line.
> + Given the PC, check the line table and return the PC that corresponds
> + to the line table entry for the source line that PC is in. */
> + CORE_ADDR start_line_pc = ecs->event_thread->control.step_range_start;
> + std::optional<CORE_ADDR> real_range_start;
> +
> + /* Call find_line_range_start to get the smallest address in the
> + linetable for multiple Line X entries in the line table. */
> + real_range_start = find_line_range_start (pc);
> +
> + if (real_range_start.has_value ())
> + start_line_pc = *real_range_start;
> +
> + return start_line_pc;
> +}
> +
> /* Come here when we've got some debug event / signal we can explain
> (IOW, not a random signal), and test whether it should cause a
> stop, or whether we should resume the inferior (transparently).
> @@ -8093,6 +8116,29 @@ process_event_stop_test (struct execution_control_state *ecs)
>
> if (stop_pc_sal.is_stmt)
> {
> + if (execution_direction == EXEC_REVERSE)
> + {
> + /* We are stepping backwards make sure we have reached the
> + beginning of the line. */
> + CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> + CORE_ADDR start_line_pc
> + = update_line_range_start (stop_pc, ecs);
> +
> + if (stop_pc != start_line_pc)
> + {
> + /* Have not reached the beginning of the source code line.
> + Set a step range. Execution should stop in any function
> + calls we execute back into before reaching the beginning
> + of the line. */
> + ecs->event_thread->control.step_range_start
> + = start_line_pc;
> + ecs->event_thread->control.step_range_end = stop_pc;
> + set_step_info (ecs->event_thread, frame, stop_pc_sal);
> + keep_going (ecs);
> + return;
> + }
> + }
> +
> /* We are at the start of a statement.
>
> So stop. Note that we don't stop if we step into the middle of a
> @@ -8155,6 +8201,17 @@ process_event_stop_test (struct execution_control_state *ecs)
> set_step_info (ecs->event_thread, frame, stop_pc_sal);
>
> infrun_debug_printf ("keep going");
> +
> + if (execution_direction == EXEC_REVERSE)
> + {
> + CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
> +
> + /* Make sure the stop_pc is set to the beginning of the line. */
> + if (stop_pc != ecs->event_thread->control.step_range_start)
> + ecs->event_thread->control.step_range_start
> + = update_line_range_start (stop_pc, ecs);
> + }
> +
> keep_going (ecs);
> }
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 5ec56f4f2af..70ef7d56878 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -73,6 +73,7 @@
> #include "gdbsupport/gdb_string_view.h"
> #include "gdbsupport/pathstuff.h"
> #include "gdbsupport/common-utils.h"
> +#include <optional>
>
> /* Forward declarations for local functions. */
>
> @@ -3311,6 +3312,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. */
> +
> +std::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 searching. */
> + 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 8dfc873b1c9..9ef09e22902 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -38,6 +38,7 @@
> #include "gdb-demangle.h"
> #include "split-name.h"
> #include "frame.h"
> +#include <optional>
>
> /* Opaque declarations. */
> struct ui_file;
> @@ -2359,6 +2360,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 std::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..3086e849231
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c
> @@ -0,0 +1,58 @@
> +/* Copyright 2023 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 (void)
> +{ /* 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");
> + k = j; f3 = f2; str_3 = str_2; /* TAG: main return */
> +
> + asm ("end_of_sequence: .globl end_of_sequence");
> + 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..63f8c9c76b3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,153 @@
> +# Copyright 2023 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.
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +# The DWARF assembler requires the gcc compiler.
> +require is_c_compiler_gcc
> +
> +# This test suitable only for process that can do reverse execution
> +require supports_reverse
> +
> +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_set_address end_of_sequence
> + DW_LNE_end_sequence
> + }
> + }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> + [list $srcfile $asm_file] {nodebug} ] } {
> + return -1
> +}
> +
> +if { ![runto_main] } {
> + return
> +}
> +
> +# Print the line table
> +gdb_test_multiple "maint info line-table ${testfile}" "" {
> + -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[ \t\]+Y\[^\r\n\]*" {
> + lappend is_stmt $expect_out(1,string)
> + exp_continue
> + }
> + -re -wrap "" {
> + }
> +}
> +
> +# Do the reverse-step and reverse-next tests
> +foreach_with_prefix cmd {step next} {
> + gdb_test_no_output "record" "turn on process record, test $cmd"
> +
> + set bp_main_return [gdb_get_line_number "TAG: main return" $srcfile]
> + gdb_breakpoint $srcfile:$bp_main_return
> + gdb_continue_to_breakpoint "run to end of main, reverse-$cmd test" ".*$srcfile:$bp_main_return.*"
> + gdb_test "display \$pc" ".*pc =.*" "display pc, reverse-$cmd test"
> +
> + # At this point, GDB has already recorded the execution up until the return
> + # statement. Reverse and test if GDB transitions between lines in the
> + # expected order. It should reverse-step or reverse-next across lines 8,
> + # 5, 3, 2 and 1.
> + foreach line {8 5 3 2 1} {
> + gdb_test "reverse-$cmd" ".*TAG: line $line.*" "reverse $cmd to line $line"
> + }
> +
> + if {$cmd =="step"} {
> + ## Clean restart, test reverse-next command
> + clean_restart ${testfile}
> +
> + if { ![runto_main] } {
> + return
> + }
> + }
> +}
I have re-tested this patch on aarch64-linux and arm-linux. I don't see any regressions and it fixes
the failures I see for aarch64-linux. The tests also pass for arm-linux, although we seem to have
acquired some regressions from a previous commit.
I'm OK with the code as-is. I'd rather have some feedback from other maintainers.
Tested-By: Luis Machado <luis.machado@arm.com>
Reviewed-By: Luis Machado <luis.machado@arm.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 1/3] Fix GDB reverse execution behavior
2023-11-22 23:33 ` [ PATCH 1/3] " Carl Love
@ 2023-11-29 11:44 ` Luis Machado
2023-11-29 16:30 ` Carl Love
0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-11-29 11:44 UTC (permalink / raw)
To: Carl Love, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 11/22/23 23:33, Carl Love wrote:
> GDB maintainers:
>
> This patch in the series is unchanged from the previously posted
> version 2 patch of patch 1 in the previous patch series.
>
> Version 2, updated the compiler check and handling for gcc version 6
> and earlier. Retested on Power 10.
>
> Per the comments on version 4 for the gdb.reverse/func-map-to-same-
> line.exp, I have added support to proc gdb_compile to enable or disable
> generating line information as part of the debug information. The two
> new options are column-info and no-column-info.
>
> This patch implements the new options for gdb_compile.
>
> These options have been tested with patch 2 and patch 3 on PowerPC with
> the GCC and clang compilers.
>
> Please let me know if the patch is acceptable for mainline. Thanks.
>
> Carl
>
> ------------------------------------------------------------
>
> Add gdb_compile options column-info and no-column-info
>
> This patch adds two new options to gdb_compile to specify if the compile
> should or should not generate the line table information. The
> options are supported on clang and gcc version 7 and newer.
>
> Patch has been tested on PowerPC with both gcc and clang.
> ---
> gdb/testsuite/lib/gdb.exp | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 63885860795..a63394d5cc0 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5150,6 +5150,8 @@ proc quote_for_host { args } {
> # debug information
> # - text_segment=addr: Tell the linker to place the text segment at ADDR.
> # - build-id: Ensure the final binary includes a build-id.
> +# - column-info/no-column-info: Enable/Disable generation of column table
> +# information.
> #
> # And here are some of the not too obscure options understood by DejaGnu that
> # influence the compilation:
> @@ -5359,6 +5361,38 @@ proc gdb_compile {source dest type options} {
> } else {
> error "Don't know how to handle text_segment option."
> }
> + } elseif { $opt == "column-info" } {
> + # If GCC or clang does not support column-info, compilation
> + # will fail and the usupported column-info option will be
> + # reported as such.
> + if {[test_compiler_info {gcc-*}]} {
> + lappend new_options "additional_flags=-gcolumn-info"
> +
> + } elseif {[test_compiler_info {clang-*}]} {
> + lappend new_options "additional_flags=-gcolumn-info"
> +
> + } else {
> + error "Option gcolumn-info option not supported by compiler."
s/Option gcolumn-info option/Option gcolumn-info
> + }
> +
> + } elseif { $opt == "no-column-info" } {
> + if {[test_compiler_info {gcc-*}]} {
> + if {[test_compiler_info {gcc-[1-6]-*}]} {
> + # In this case, don't add the compile line option and
> + # the result will be the same as using no-column-info
> + # on a version that supports the option.
> + warning "gdb_compile option no-column-info not supported, ignoring."
> + } else {
> + lappend new_options "additional_flags=-gno-column-info"
> + }
> +
> + } elseif {[test_compiler_info {clang-*}]} {
> + lappend new_options "additional_flags=-gno-column-info"
> +
> + } else {
> + error "Option gno-column-info option not supported by compiler."
s/Option gno-column-info option/Option gno-column-info
> + }
> +
> } else {
> lappend new_options $opt
> }
Other LGTM. I have tested this on aarch64-linux and arm-linux with gcc. Works fine.
Tested-By: Luis Machado <luis.machado@arm.com>
Reviewed-By: Luis Machado <luis.machado@arm.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Fix GDB reverse execution behavior
2023-11-22 23:33 ` [PATCH 3/3] " Carl Love
@ 2023-11-29 11:46 ` Luis Machado
2023-11-29 16:30 ` Carl Love
0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-11-29 11:46 UTC (permalink / raw)
To: Carl Love, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 11/22/23 23:33, Carl Love wrote:
> GDB maintainers:
>
> The following patch changes the behavior of GDB when executing the next
> command in reverse over a line that contains multiple function calls on
> the same source code line. Currently, GDB stops in the middle of the
> line not at the beginning of the line. This patch causes GDB to stop
> at the beginning of the line. The incorrect behavior was pointed out
> by Pedro Alves in
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
>
> A new testcase, func-map-to-same-line.exp, is added to verify the
> behavior of GDB.
>
> The fix requires updating two testcases, gdb/testsuite/gdb.mi/mi-
> reverse.exp and gdb.reverse/finish-reverse-next.exp. The testcases
> have to issue two reverse next instructions to reach the previous
> source code line. With this patch, they only require a single reverse
> next instruction to reach the previous line.
>
> The patch has been tested on PowerPC and X86-64 with no regression
> errors.
>
> Please let me know if the patch is acceptable for mainline. Thanks.
>
> Carl
> ----------------------------------------------------
>
> Fix GDB reverse-step and reverse-next command behavior
>
> Currently GDB when executing in reverse over multiple statements in a single
> line of source code, GDB stops in the middle of the line. Thus requiring
> multiple commands to reach the previous line. GDB should stop at the first
> instruction of the line, not in the middle of the line.
>
> The following description of the incorrect behavior was taken from an
> earlier message by Pedro Alves <pedro@palves.net>:
>
> https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html
>
> ---------------------------------
>
> The source line looks like:
>
> func1 (); func2 ();
>
> in the test case:
>
> (gdb) list 1
> 1 void func1 ()
> 2 {
> 3 }
> 4
> 5 void func2 ()
> 6 {
> 7 }
> 8
> 9 int main ()
> 10 {
> 11 func1 (); func2 ();
> 12 }
>
> compiled with:
>
> $ gcc reverse.c -o reverse -g3 -O0
> $ gcc -v
> ...
> gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04)
>
> Now let's debug it with target record, using current gdb git master
> (f3d8ae90b236),
>
> $ gdb ~/reverse
> GNU gdb (GDB) 14.0.50.20230124-git
> ...
> Reading symbols from /home/pedro/reverse...
> (gdb) start
> Temporary breakpoint 1 at 0x1147: file reverse.c, line 11.
> Starting program: /home/pedro/reverse
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Temporary breakpoint 1, main () at reverse.c:11
> 11 func1 (); func2 ();
> (gdb) record
>
> (gdb) disassemble /s
> Dump of assembler code for function main:
> reverse.c:
> 10 {
> 0x000055555555513f <+0>: endbr64
> 0x0000555555555143 <+4>: push %rbp
> 0x0000555555555144 <+5>: mov %rsp,%rbp
>
> 11 func1 (); func2 ();
> => 0x0000555555555147 <+8>: mov $0x0,%eax
> 0x000055555555514c <+13>: call 0x555555555129 <func1>
> 0x0000555555555151 <+18>: mov $0x0,%eax
> 0x0000555555555156 <+23>: call 0x555555555134 <func2>
> 0x000055555555515b <+28>: mov $0x0,%eax
>
> 12 }
> 0x0000555555555160 <+33>: pop %rbp
> 0x0000555555555161 <+34>: ret
> End of assembler dump.
>
> (gdb) n
> 12 }
>
> So far so good, a "next" stepped over the whole of line 11 and stopped at
> line 12.
>
> Let's confirm where we are now:
>
> (gdb) disassemble /s
> Dump of assembler code for function main:
> reverse.c:
> 10 {
> 0x000055555555513f <+0>: endbr64
> 0x0000555555555143 <+4>: push %rbp
> 0x0000555555555144 <+5>: mov %rsp,%rbp
>
> 11 func1 (); func2 ();
> 0x0000555555555147 <+8>: mov $0x0,%eax
> 0x000055555555514c <+13>: call 0x555555555129 <func1>
> 0x0000555555555151 <+18>: mov $0x0,%eax
> 0x0000555555555156 <+23>: call 0x555555555134 <func2>
> 0x000055555555515b <+28>: mov $0x0,%eax
>
> 12 }
> => 0x0000555555555160 <+33>: pop %rbp
> 0x0000555555555161 <+34>: ret
> End of assembler dump.
>
> Good, we're at the first instruction of line 12.
>
> Now let's undo the "next", with "reverse-next":
>
> (gdb) reverse-next
> 11 func1 (); func2 ();
>
> Seemingly stopped at line 11. Let's see exactly where:
>
> (gdb) disassemble /s
> Dump of assembler code for function main:
> reverse.c:
> 10 {
> 0x000055555555513f <+0>: endbr64
> 0x0000555555555143 <+4>: push %rbp
> 0x0000555555555144 <+5>: mov %rsp,%rbp
>
> 11 func1 (); func2 ();
> 0x0000555555555147 <+8>: mov $0x0,%eax
> 0x000055555555514c <+13>: call 0x555555555129 <func1>
> => 0x0000555555555151 <+18>: mov $0x0,%eax
> 0x0000555555555156 <+23>: call 0x555555555134 <func2>
> 0x000055555555515b <+28>: mov $0x0,%eax
>
> 12 }
> 0x0000555555555160 <+33>: pop %rbp
> 0x0000555555555161 <+34>: ret
> End of assembler dump.
> (gdb)
>
> And lo, we stopped in the middle of line 11! That is a bug, we should have
> stepped back all the way to the beginning of the line. The "reverse-next"
> should have fully undone the prior "next" command.
>
> --------------------
>
> This patch fixes the incorrect GDB behavior by ensuring that GDB stops at
> the first instruction in the line.
>
> The test case gdb.reverse/func-map-to-same-line.exp is added to testsuite
> to verify this fix when the line table information is and is not available.
> ---
> gdb/infcmd.c | 13 ++
> gdb/testsuite/gdb.mi/mi-reverse.exp | 5 +-
> .../gdb.reverse/finish-reverse-next.exp | 40 ++---
> .../gdb.reverse/func-map-to-same-line.c | 37 +++++
> .../gdb.reverse/func-map-to-same-line.exp | 140 ++++++++++++++++++
> 5 files changed, 201 insertions(+), 34 deletions(-)
> create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index cf8cd527955..1415fe55163 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -975,6 +975,19 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
> &tp->control.step_range_start,
> &tp->control.step_range_end);
>
> + if (execution_direction == EXEC_REVERSE)
> + {
> + symtab_and_line sal = find_pc_line (pc, 0);
> + symtab_and_line sal_start
> + = find_pc_line (tp->control.step_range_start, 0);
> +
> + if (sal.line == sal_start.line)
> + /* Executing in reverse, the step_range_start address is in
> + the same line. We want to stop in the previous line so
> + move step_range_start before the current line. */
> + tp->control.step_range_start--;
> + }
> +
> /* There's a problem in gcc (PR gcc/98780) that causes missing line
> table entries, which results in a too large stepping range.
> Use inlined_subroutine info to make the range more narrow. */
> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
> index baa53a495d7..0630b8b6c7f 100644
> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp
> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
> @@ -96,11 +96,8 @@ proc test_controlled_execution_reverse {} {
> "reverse finish from callme"
>
> # Test exec-reverse-next
> - # It takes two steps to get back to the previous line,
> - # as the first step moves us to the start of the current line,
> - # and the one after that moves back to the previous line.
>
> - mi_execute_to "exec-next --reverse 2" \
> + mi_execute_to "exec-next --reverse" \
> "end-stepping-range" "main" "" \
> "basics.c" $line_main_hello "" \
> "reverse next to get over the call to do_nothing"
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> index 1f53b649a7d..6488f66b107 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
> @@ -76,14 +76,10 @@ gdb_continue_to_breakpoint \
> repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
>
> # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line. A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line. Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line. Another reverse-next
> +# instruction stops at the previous source code line.
> gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
> "reverse-finish function1 LEP call from LEP "
> -gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP" \
> - "reverse next 1 LEP entry point function call from LEP"
> gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP"
>
>
> @@ -109,14 +105,10 @@ gdb_continue_to_breakpoint \
> gdb_test "step" ".*int ret = 0;.*" "step test 1"
>
> # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line. A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line. Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line. Another reverse-next
> +# instruction stops at the previous source code line.
> gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
> "reverse-finish function1 LEP call from function body"
> -gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \
> - "reverse next 1 LEP from function body"
> gdb_test "reverse-next" ".*b = 5;.*" \
> "reverse next 2 at b = 5, from function body"
>
> @@ -144,14 +136,10 @@ gdb_continue_to_breakpoint \
> repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
>
> # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line. A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line. Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line. Another reverse-next
> +# instruction stops at the previous source code line.
> gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> "function1 GEP call call from GEP"
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> - "reverse next 1 GEP entry point function call from GEP"
> gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP"
>
> gdb_test "reverse-continue" ".*" "setup for test 4"
> @@ -180,14 +168,10 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call again"
> gdb_test "stepi" "{" "stepi to between GEP and LEP"
>
> # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line. A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line. Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line. Another reverse-next
> +# instruction stops at the previous source code line.
> gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> "function1 GEP call call from GEP again"
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> - "reverse next 1 GEP entry point function call from GEP again"
> gdb_test "reverse-next" ".*b = 50;.*" \
> "reverse next 2 at b = 50, call from GEP again"
>
> @@ -212,13 +196,9 @@ gdb_continue_to_breakpoint \
> gdb_test "step" ".*int ret = 0;.*" "step test 2"
>
> # The reverse-finish command should stop on the function call instruction
> -# which is the last instruction in the source code line. A reverse-next
> -# instruction should then stop at the first instruction in the same source
> -# code line. Another revers-next instruction stops at the previous source
> -# code line.
> +# which is the last instruction in the source code line. Another reverse-next
> +# instruction stops at the previous source code line.
> gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
> "reverse-finish function1 GEP call, from function body "
> -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
> - "reverse next 1 GEP call from function body"
> gdb_test "reverse-next" ".*b = 50;.*" \
> "reverse next 2 at b = 50 from function body"
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.c b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> new file mode 100644
> index 00000000000..17fe17af267
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> @@ -0,0 +1,37 @@
> +/* Copyright 2023 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 test is used to test the reverse-step and reverse-next instruction
> + execution for a source line that contains multiple function calls. */
> +
> +void
> +func1 (void)
> +{
> +} /* END FUNC1 */
> +
> +void
> +func2 (void)
> +{
> +} /* END FUNC2 */
> +
> +int
> +main (void)
> +{
> + int a, b;
> + a = 1;
> + b = 2;
> + func1 (); func2 ();
> + a = a + b; /* START REVERSE TEST */
> +}
> diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> new file mode 100644
> index 00000000000..1b23233b43e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> @@ -0,0 +1,140 @@
> +# Copyright 2023 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 file is part of the GDB testsuite. It tests reverse stepping.
> +# Lots of code borrowed from "step-test.exp".
> +
> +# This test checks to make sure there is no regression failures for
> +# the reverse-next command when stepping back over two functions in
> +# the same line.
> +
> +require supports_reverse
> +
> +# This test uses the gcc no-column-info command which was added in gcc 7.1.
> +
> +proc run_tests {} {
> + global testfile
> +
> + clean_restart ${testfile}
> +
> + if { ![runto_main] } {
> + return
> + }
> +
> + with_test_prefix "next-test" {
> + gdb_test_no_output "record" "turn on process record"
> +
> + # This regression test verifies the reverse-step and reverse-next
> + # commands work properly when executing backwards thru a source line
> + # containing two function calls on the same source line, i.e.
> + # func1 (); func2 ();. This test is compiled so the dwarf info
> + # does not contain the line table information.
> +
> + # Test 1, reverse-next command
> + # Set breakpoint at the line after the function calls.
> + set bp_start_reverse_test [gdb_get_line_number "START REVERSE TEST"]
> +
> + gdb_breakpoint $bp_start_reverse_test temporary
> +
> + # Continue to break point for reverse-next test.
> + # Command definition: reverse-next [count]
> + # Run backward to the beginning of the previous line executed in
> + # the current (innermost) stack frame. If the line contains
> + # function calls,they will be “un-executed” without stopping.
space after comma.
> + # Starting from the first line of a function, reverse-next will
> + # take you back to the caller of that function, before the
> + # function was called, just as the normal next command would take
> + # you from the last line of a function back to its return to its
> + # caller 2.
> +
> + gdb_continue_to_breakpoint \
> + "stopped at command reverse-next test start location" \
> + ".*$bp_start_reverse_test\r\n.*"
> +
> + # The reverse-next should step all the way back to the beginning of
> + # the line, i.e. at the beginning of the func1 call.
> + gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> + " reverse-next to line with two functions"
> +
> + # We should be stopped at the first instruction of the line. A
> + # reverse-step should step back and stop at the beginning of the
> + # previous line b = 2, i.e. not in func1 ().
> + gdb_test "reverse-stepi" ".*b = 2;.*" \
> + "reverse-stepi to previous line b = 2"
> + }
> +
> + # Setup for test 2
> + clean_restart ${testfile}
> +
> + if { ![runto_main] } {
> + return
> + }
> +
> + with_test_prefix "step-test" {
> + gdb_test_no_output "record" "turn on process record"
> +
> + # Test 2, reverse-step command
> + # Set breakpoint at the line after the function calls.
> + gdb_breakpoint $bp_start_reverse_test temporary
> +
> + # Continue to the start of the reverse-step test.
> + # Command definition: reverse-step [count]
> + # Run the program backward until control reaches the start of a
> + # different source line; then stop it, and return control to gdb.
> + # Like the step command, reverse-step will only stop at the
> + # beginning of a source line. It “un-executes” the previously
> + # executed source line. If the previous source line included calls
> + # to debuggable functions, reverse-step will step (backward) into
> + # the called function, stopping at the beginning of the last
> + # statement in the called function (typically a return statement).
> + # Also, as with the step command, if non-debuggable functions are
> + # called, reverse-step will run thru them backward without
> + # stopping.
> +
> + gdb_continue_to_breakpoint \
> + "stopped at command reverse-step test start location" \
> + ".*$bp_start_reverse_test\r\n.*"
> +
> + # The first reverse step should take us call of func2 ().
> + gdb_test "reverse-step" ".*END FUNC2.*" \
> + "reverse-step into func2 "
> +
> + # The second reverse step should take us into func1 ().
> + gdb_test "reverse-step" ".*END FUNC1.*" \
> + "reverse-step into func1 "
> +
> + # The third reverse step should take us call of func1 ().
> + gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> + "reverse-step to line func1(); func2(), at call for func1 "
> + # We should be stopped at the first instruction of the line. A
> + # reversee stepi should take us to b = 2 ().
> + gdb_test "reverse-stepi" ".*b = 2;.*" \
> + "reverse-stepi to line b = 2 "
> + }
> +}
> +
> +standard_testfile .c
> +
> +# test with and without gcc column info enabled
> +foreach_with_prefix column_info_flag {column-info no-column-info} {
> + set options [list debug $column_info_flag]
> +
> + if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> + $options]} {
> + return -1
> + }
> +
> + run_tests
> +}
I haven't followed the discussions around this bug, but it looks like something that was
already there, and not a regression I suppose.
In any case, I've tested this on aarch64-linux and arm-linux. Works as expected and the
code looks OK to me.
Tested-By: Luis Machado <luis.machado@arm.com>
Reviewed-By: Luis Machado <luis.machado@arm.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 1/3] Fix GDB reverse execution behavior
2023-11-29 11:44 ` Luis Machado
@ 2023-11-29 16:30 ` Carl Love
2023-11-29 16:38 ` Luis Machado
0 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-11-29 16:30 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi, cel
Luis:
On Wed, 2023-11-29 at 11:44 +0000, Luis Machado wrote:
> > + } else {
> > + error "Option gcolumn-info option not supported by
> > compiler."
>
> s/Option gcolumn-info option/Option gcolumn-info
fixed
>
> > + }
> > +
> > + } elseif { $opt == "no-column-info" } {
> > + if {[test_compiler_info {gcc-*}]} {
> > + if {[test_compiler_info {gcc-[1-6]-*}]} {
> > + # In this case, don't add the compile line option
> > and
> > + # the result will be the same as using no-column-
> > info
> > + # on a version that supports the option.
> > + warning "gdb_compile option no-column-info not
> > supported, ignoring."
> > + } else {
> > + lappend new_options "additional_flags=-gno-
> > column-info"
> > + }
> > +
> > + } elseif {[test_compiler_info {clang-*}]} {
> > + lappend new_options "additional_flags=-gno-column-
> > info"
> > +
> > + } else {
> > + error "Option gno-column-info option not supported by
> > compiler."
>
> s/Option gno-column-info option/Option gno-column-info
fixed
I made the fixes to the patches. The changes are fairly trivial so let
s see if anyone else has any comments to add before sending out another
version.
Thanks for reviewing and testing.
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] Fix GDB reverse execution behavior
2023-11-29 11:46 ` Luis Machado
@ 2023-11-29 16:30 ` Carl Love
0 siblings, 0 replies; 17+ messages in thread
From: Carl Love @ 2023-11-29 16:30 UTC (permalink / raw)
To: Luis Machado, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi, cel
Luis:
On Wed, 2023-11-29 at 11:46 +0000, Luis Machado wrote:
> > +
> > + # Continue to break point for reverse-next test.
> > + # Command definition: reverse-next [count]
> > + # Run backward to the beginning of the previous line
> > executed in
> > + # the current (innermost) stack frame. If the line
> > contains
> > + # function calls,they will be “un-executed” without
> > stopping.
>
> space after comma.
OK, fixed.
Again, lets see if there are any other comments or changes requested
before sending out another version of the patch.
Thanks for the review and testing.
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 1/3] Fix GDB reverse execution behavior
2023-11-29 16:30 ` Carl Love
@ 2023-11-29 16:38 ` Luis Machado
0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-11-29 16:38 UTC (permalink / raw)
To: Carl Love, blarsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 11/29/23 16:30, Carl Love wrote:
> Luis:
>
> On Wed, 2023-11-29 at 11:44 +0000, Luis Machado wrote:
>>> + } else {
>>> + error "Option gcolumn-info option not supported by
>>> compiler."
>>
>> s/Option gcolumn-info option/Option gcolumn-info
> fixed
>>
>>> + }
>>> +
>>> + } elseif { $opt == "no-column-info" } {
>>> + if {[test_compiler_info {gcc-*}]} {
>>> + if {[test_compiler_info {gcc-[1-6]-*}]} {
>>> + # In this case, don't add the compile line option
>>> and
>>> + # the result will be the same as using no-column-
>>> info
>>> + # on a version that supports the option.
>>> + warning "gdb_compile option no-column-info not
>>> supported, ignoring."
>>> + } else {
>>> + lappend new_options "additional_flags=-gno-
>>> column-info"
>>> + }
>>> +
>>> + } elseif {[test_compiler_info {clang-*}]} {
>>> + lappend new_options "additional_flags=-gno-column-
>>> info"
>>> +
>>> + } else {
>>> + error "Option gno-column-info option not supported by
>>> compiler."
>>
>> s/Option gno-column-info option/Option gno-column-info
> fixed
>
> I made the fixes to the patches. The changes are fairly trivial so let
> s see if anyone else has any comments to add before sending out another
> version.
>
> Thanks for reviewing and testing.
>
> Carl
>
For what's worth, I don't think it needs another version for these small fixes.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-11-22 23:33 ` [ PATCH 0/3] Fix GDB reverse execution behavior Carl Love
@ 2023-11-30 11:36 ` Guinevere Larsen
2023-11-30 15:39 ` Carl Love
0 siblings, 1 reply; 17+ messages in thread
From: Guinevere Larsen @ 2023-11-30 11:36 UTC (permalink / raw)
To: Carl Love, Luis Machado, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 23/11/2023 00:33, Carl Love wrote:
> GDB developers:
>
> The patch "[PATCH 2/2 ver 8] Fix reverse stepping multiple contiguous
> PC ranges over the line table",
>
> https://sourceware.org/pipermail/gdb-patches/2023-August/201889.html
>
> was posted and pinged several times without any response from the
> developers. It was reviewed by Guinevere Larsen. The original patch 2
> was intended to fix failures on PowerPC and AArch64. An additional
> architecture independent change and test was added per feedback from
> the community.
>
> Based on some private discussions on how best to get the patch
> approved, it was suggested that I should break the patch in two to make
> it easier to review. Specifically, separate out the PowerPC and
> AArch64 fixes from the generic architecture independent changes. The
> original patch 2 has been split up as suggested. The hope is this will
> make it easier to review and get approval.
>
> The two new patches are functionally identical to the previously posted
> patch 2.
>
> The patch series now consists of three patches.
>
> Patch 1, adds a new option to gdb_compile to either generate or not
> generate the line table information. No change from the prior version.
>
> Patch 2, fix the specific GDB issues with reverse stepping over a line
> with multiple statements in the same line for PowerPC and AArch64.
> Behavior on X86-64 did not change.
>
> Patch 3, fix the behavior of GDB for all architectures when executing
> the next command in reverse on a line that contains multiple function
> calls.
>
> The series of three patches has been tested on PowerPC, X86 and AArch64
> with no regression errors. Please let me know if these patches are
> acceptable for mainline. Note, Luis Machado did the AArch64 testing.
> Thanks.
>
> Carl
>
>
I've taken a look at this and also see no regressions. I seem to recall
them fixing a clang regression at some point, which they don't anymore,
but these have been in the works for so long I think its better to take
them as is and check the clang regression later.
Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
On a side note, b4 didn't really like this series and I'm not sure why.
My guess is that all 3 patches have the same email subject, but it may
also be that I needed --3way when using git am. Either way, just
mentioning to gather knowledge on what b4 likes or doesn't like.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-11-30 11:36 ` Guinevere Larsen
@ 2023-11-30 15:39 ` Carl Love
2023-11-30 15:43 ` Luis Machado
2023-11-30 15:45 ` Guinevere Larsen
0 siblings, 2 replies; 17+ messages in thread
From: Carl Love @ 2023-11-30 15:39 UTC (permalink / raw)
To: Guinevere Larsen, Luis Machado, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
Guinevere:
On Thu, 2023-11-30 at 12:36 +0100, Guinevere Larsen wrote:
> On a side note, b4 didn't really like this series and I'm not sure
> why.
> My guess is that all 3 patches have the same email subject, but it
> may
> also be that I needed --3way when using git am. Either way, just
> mentioning to gather knowledge on what b4 likes or doesn't like.
I am not familiar with b4. I just did a Google search and found a
description of it at:
https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
It seems to be a tool the maintainers can use for patch management. I
will have to play around with it and see if I can figure out why b4
doesn't like the patch series.
Is this something that people have started using?
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-11-30 15:39 ` Carl Love
@ 2023-11-30 15:43 ` Luis Machado
2023-12-11 14:40 ` Luis Machado
2023-11-30 15:45 ` Guinevere Larsen
1 sibling, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-11-30 15:43 UTC (permalink / raw)
To: Carl Love, Guinevere Larsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 11/30/23 15:39, Carl Love wrote:
> Guinevere:
>
> On Thu, 2023-11-30 at 12:36 +0100, Guinevere Larsen wrote:
>> On a side note, b4 didn't really like this series and I'm not sure
>> why.
>> My guess is that all 3 patches have the same email subject, but it
>> may
>> also be that I needed --3way when using git am. Either way, just
>> mentioning to gather knowledge on what b4 likes or doesn't like.
>
>
> I am not familiar with b4. I just did a Google search and found a
> description of it at:
>
> https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
>
> It seems to be a tool the maintainers can use for patch management. I
> will have to play around with it and see if I can figure out why b4
> doesn't like the patch series.
>
> Is this something that people have started using?
I started to use it more recently. It makes it pretty easy to download a
series and apply to a tree.
For instance, take this entry: https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/T/#t
You could do this to fetch the patches:
b4 am -M https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/T/#t -o /tmp/
Looking up https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com
Grabbing thread from inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/t.mbox.gz
Analyzing 12 messages in the thread
Checking attestation on all messages, may take a moment...
---
✓ [PATCH 1/3] Fix GDB reverse execution behavior
+ Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
+ Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
✓ [PATCH 2/3] Fix GDB reverse execution behavior
+ Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
+ Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
✓ [PATCH 3/3] Fix GDB reverse execution behavior
+ Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
+ Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
---
✓ Signed: DKIM/ibm.com (From: cel@linux.ibm.com)
---
Total patches: 3
---
NOTE: Some trailers were sent to the cover letter:
Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
NOTE: Rerun with -t to apply them to all patches
---
Cover: /tmp/20231122_cel_fix_gdb_reverse_execution_behavior.cover
Link: https://lore.kernel.org/r/5483d77c72088a1e4d5dfed2eded2366643fc659.camel@linux.ibm.com
Base: not specified
git am /tmp/20231122_cel_fix_gdb_reverse_execution_behavior.maildir
And then you can easily apply the series with the above git am command.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-11-30 15:39 ` Carl Love
2023-11-30 15:43 ` Luis Machado
@ 2023-11-30 15:45 ` Guinevere Larsen
1 sibling, 0 replies; 17+ messages in thread
From: Guinevere Larsen @ 2023-11-30 15:45 UTC (permalink / raw)
To: Carl Love, Luis Machado, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
On 30/11/2023 16:39, Carl Love wrote:
> Guinevere:
>
> On Thu, 2023-11-30 at 12:36 +0100, Guinevere Larsen wrote:
>> On a side note, b4 didn't really like this series and I'm not sure
>> why.
>> My guess is that all 3 patches have the same email subject, but it
>> may
>> also be that I needed --3way when using git am. Either way, just
>> mentioning to gather knowledge on what b4 likes or doesn't like.
>
> I am not familiar with b4. I just did a Google search and found a
> description of it at:
>
> https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
>
> It seems to be a tool the maintainers can use for patch management. I
> will have to play around with it and see if I can figure out why b4
> doesn't like the patch series.
>
> Is this something that people have started using?
>
We started talking about this because it automatically collects the git
trailers and applies them, so you wouldn't need to change your local
commits, and makes it easy for reviewers. Simon wrote some tips about
using it in the wiki a little bit ago:
https://sourceware.org/gdb/wiki/DeveloperTips#Downloading_and_applying_patches_from_the_mailing_list
It's not required or anything, it's just handy.
I like to use it as "b4 shazam <message-id>" which would automatically
apply the patches, not just download them. Maybe that's where it went
wrong...
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-11-30 15:43 ` Luis Machado
@ 2023-12-11 14:40 ` Luis Machado
2023-12-14 16:10 ` Carl Love
0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-12-11 14:40 UTC (permalink / raw)
To: Carl Love, Guinevere Larsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
Hi Carl,
On 11/30/23 15:43, Luis Machado wrote:
> On 11/30/23 15:39, Carl Love wrote:
>> Guinevere:
>>
>> On Thu, 2023-11-30 at 12:36 +0100, Guinevere Larsen wrote:
>>> On a side note, b4 didn't really like this series and I'm not sure
>>> why.
>>> My guess is that all 3 patches have the same email subject, but it
>>> may
>>> also be that I needed --3way when using git am. Either way, just
>>> mentioning to gather knowledge on what b4 likes or doesn't like.
>>
>>
>> I am not familiar with b4. I just did a Google search and found a
>> description of it at:
>>
>> https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
>>
>> It seems to be a tool the maintainers can use for patch management. I
>> will have to play around with it and see if I can figure out why b4
>> doesn't like the patch series.
>>
>> Is this something that people have started using?
>
> I started to use it more recently. It makes it pretty easy to download a
> series and apply to a tree.
>
> For instance, take this entry: https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/T/#t
>
> You could do this to fetch the patches:
>
>
> b4 am -M https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/T/#t -o /tmp/
> Looking up https://inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com
> Grabbing thread from inbox.sourceware.org/gdb-patches/85a2f33d4ad12966dcb212e99e836175b69fddee.camel@linux.ibm.com/t.mbox.gz
> Analyzing 12 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
> ✓ [PATCH 1/3] Fix GDB reverse execution behavior
> + Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> + Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> ✓ [PATCH 2/3] Fix GDB reverse execution behavior
> + Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> + Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> ✓ [PATCH 3/3] Fix GDB reverse execution behavior
> + Tested-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> + Reviewed-By: Luis Machado <luis.machado@arm.com> (✓ DKIM/armh.onmicrosoft.com)
> ---
> ✓ Signed: DKIM/ibm.com (From: cel@linux.ibm.com)
> ---
> Total patches: 3
> ---
> NOTE: Some trailers were sent to the cover letter:
> Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
> NOTE: Rerun with -t to apply them to all patches
> ---
> Cover: /tmp/20231122_cel_fix_gdb_reverse_execution_behavior.cover
> Link: https://lore.kernel.org/r/5483d77c72088a1e4d5dfed2eded2366643fc659.camel@linux.ibm.com
> Base: not specified
> git am /tmp/20231122_cel_fix_gdb_reverse_execution_behavior.maildir
>
> And then you can easily apply the series with the above git am command.
Let's wait until this Friday to see if there is any other feedback regarding this series.
Otherwise, LGTM.
Approved-By: Luis Machado <luis.machado@arm.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-12-11 14:40 ` Luis Machado
@ 2023-12-14 16:10 ` Carl Love
2024-01-02 22:52 ` Carl Love
0 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2023-12-14 16:10 UTC (permalink / raw)
To: Luis Machado, Guinevere Larsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi, cel
Luis, GDB maintainers:
On Mon, 2023-12-11 at 14:40 +0000, Luis Machado wrote:
<snip>
>
> Let's wait until this Friday to see if there is any other feedback
> regarding this series.
>
> Otherwise, LGTM.
>
> Approved-By: Luis Machado <luis.machado@arm.com>
I have not seen any additional comments or objections to committing the
patch series per Luis's approval. My personal rule is to never commit
anything and then go on vacation, just in case something goes wrong. I
will be out of the office starting tomorrow Dec 15 until Jan 2.
Assuming there are no additional feedback with the patches between now
and Jan 2, I will commit the patches in January when I return.
Thanks for the approval. Have a great holiday.
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [ PATCH 0/3] Fix GDB reverse execution behavior
2023-12-14 16:10 ` Carl Love
@ 2024-01-02 22:52 ` Carl Love
0 siblings, 0 replies; 17+ messages in thread
From: Carl Love @ 2024-01-02 22:52 UTC (permalink / raw)
To: Luis Machado, Guinevere Larsen, Ulrich Weigand, gdb-patches
Cc: Pedro Alves, Tom Tromey, Simon Marchi
GDB maintainers:
On Thu, 2023-12-14 at 08:10 -0800, Carl Love wrote:
> Luis, GDB maintainers:
>
> On Mon, 2023-12-11 at 14:40 +0000, Luis Machado wrote:
>
> <snip>
>
> > Let's wait until this Friday to see if there is any other feedback
> > regarding this series.
> >
> > Otherwise, LGTM.
> >
> > Approved-By: Luis Machado <luis.machado@arm.com>
>
> I have not seen any additional comments or objections to committing
> the
> patch series per Luis's approval. My personal rule is to never
> commit
> anything and then go on vacation, just in case something goes
> wrong. I
> will be out of the office starting tomorrow Dec 15 until Jan 2.
> Assuming there are no additional feedback with the patches between
> now
> and Jan 2, I will commit the patches in January when I return.
>
> Thanks for the approval. Have a great holiday.
>
I have not seen any objections to committing the patches.
I rebased the patches on the latest mainline tree. I retested the
three patches on Power 10, Power 9, Power 8 and X86-64. No
regressions were found. The three patches have been committed to
mainline.
Carl
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-01-02 22:52 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <f624c5566d6a8a1014ecc38900ca1ba0202989ef.camel@linux.ibm.com>
[not found] ` <890101c23dd5fa60fcbf9d4b299cb2a533c260b7.camel@linux.ibm.com>
[not found] ` <1e702d8f-e5b4-4719-b1e7-42210f350305@arm.com>
[not found] ` <a01f4e5f3fd399e3a9a971421265b34d08e32388.camel@linux.ibm.com>
[not found] ` <643afce1-ab9b-4e8b-bcbb-5738dc409a28@arm.com>
[not found] ` <9e17008084c34f953f5318933436ec703250120a.camel@linux.ibm.com>
[not found] ` <92a751d1-a4b9-4c21-821e-a1dc67207516@arm.com>
[not found] ` <ee4ed81911a3196327c2aea3fa9a77f910a5e798.camel@linux.ibm.com>
2023-11-22 23:33 ` [ PATCH 0/3] Fix GDB reverse execution behavior Carl Love
2023-11-30 11:36 ` Guinevere Larsen
2023-11-30 15:39 ` Carl Love
2023-11-30 15:43 ` Luis Machado
2023-12-11 14:40 ` Luis Machado
2023-12-14 16:10 ` Carl Love
2024-01-02 22:52 ` Carl Love
2023-11-30 15:45 ` Guinevere Larsen
2023-11-22 23:33 ` [ PATCH 1/3] " Carl Love
2023-11-29 11:44 ` Luis Machado
2023-11-29 16:30 ` Carl Love
2023-11-29 16:38 ` Luis Machado
2023-11-22 23:33 ` [ PATCH 2/3] " Carl Love
2023-11-29 11:44 ` Luis Machado
2023-11-22 23:33 ` [PATCH 3/3] " Carl Love
2023-11-29 11:46 ` Luis Machado
2023-11-29 16:30 ` Carl Love
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).