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

[Simon Marchi <simark@simark.ca>]

I'd like to help reviewing this, but I don't have much time at the
moment, so just a few comments on one test to start with.

> 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..412ab180943
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> @@ -0,0 +1,36 @@
> +/* Copyright 2008-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 ()
> +{
> +} // END FUNC1

Use /* */ for comments, for consistency with the rest of the code base.

> +
> +void
> +func2 ()
> +{
> +} // END FUNC2
> +
> +int main ()
> +{
> +  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..da5ee282053
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp
> @@ -0,0 +1,156 @@
> +# Copyright 2008-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.
> +if {![test_compiler_info {gcc-*}]
> +    || [test_compiler_info {gcc-[1-6]-*}]} {
> +    return
> +}

I would prefer not to filter out by compiler explicitly like that.
It would be useful for the test to run with other compilers too.

> +
> +proc run_tests {} {
> +    global srcfile
> +    global executable
> +
> +    runto_main
> +    set target_remote [gdb_is_target_remote]
> +
> +    with_test_prefix "test1" {
> +	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 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" \
> +				   $srcfile]
> +    gdb_breakpoint $srcfile:$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 \
> +	"test1: stopped at command reverse-next test start location" \
> +	".*$srcfile:$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 \\(\\);.*" \
> +	"test1: 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;.*" \
> +	"test1: reverse-stepi to previous line b = 2"
> +
> +
> +    # Setup for test 2
> +    clean_restart $executable
> +    runto_main
> +
> +    with_test_prefix "test2" {
> +	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 $srcfile:$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 \
> +	"test2: stopped at command reverse-step test start location" \
> +	".*$srcfile:$bp_start_reverse_test\r\n.*"
> +
> +    # The first reverse step should take us call of func2 ().
> +    gdb_test "reverse-step" ".*END FUNC2.*" \
> +	"test2: reverse-step into func2 "
> +
> +    # The second reverse step should take us into func1 ().
> +    gdb_test "reverse-step" ".*END FUNC1.*" \
> +	"test2: reverse-step into func1 "
> +
> +    # The third reverse step should take us call of func1 ().
> +    gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \
> +	"test2: reverse-step to line func1(); func2(), at call for func1 "
> +
> +    # We should be stopped at the first instruction of the line. A reverse
> +    # stepi should take us to b = 2 ().
> +    gdb_test "reverse-stepi" ".*b = 2;.*" \
> +	"test2: reverse-stepi to line b = 2 "
> +}
> +
> +set srcfile  func-map-to-same-line.c
> +set executable func-map-to-same-line
> +
> +# test with gcc column info enabled
> +set options [list debug additional_flags=]
> +
> +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
> + {
> +    return -1
> +}
> +
> +clean_restart $executable
> +
> +with_test_prefix "with-column-info" {
> +    run_tests
> +}

So, the above assumes that the compiler generates column-info by
default, which has not historically been the case for GCC (it started to
emit columns by default with version 8, according to my tests).  Other
compilers may choose to not emit them by default.

I think it would make sense to make gdb_compile recognize the new
"column-info" and "no-column-info" options, which would translate to the
right flags for the given compiler.  gdb_compile already handles the
nitty gritty details of choosing compiler flags for specific compiler
versions.  This way, individual tests don't contain compiler flags that
are possibly compiler-specific.

> +
> +#test with gcc column info disabled
> +set options [list debug additional_flags=-gno-column-info]
> +
> +if {[build_executable "failed to prepare" $executable $srcfile $options] == -1}\
> + {
> +    return -1
> +}
> +
> +set $executable executable_without_column_info
> +clean_restart $executable
> +
> +with_test_prefix "no-column-info" {
> +    run_tests
> +}

This would probably be a good use for foreach_with_prefix (if you can
make it work), to make things more compact:

  foreach_with_prefix with_column_info {yes no} {
  }

... or something like that.

Simon