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

[Bruno Larsen <blarsen@redhat.com>]

On 04/05/2023 04:55, Carl Love wrote:
> Bruno:
>
> On Wed, 2023-05-03 at 11:53 +0200, Bruno Larsen wrote:
>> On 27/04/2023 22:59, Carl Love wrote:
> <snip>
<snip>
>>> +
>>> +# This test uses the gcc no-column-info command.
>> Correct me if I'm wrong, but it seems to me that the other test is a
>> more generic version of this one, so this test could check for a gcc
>> recent enough to support this feature, instead of just generically
>> gcc.
>> That said, gcc added it on version
>> 7(
>> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0029b929c9719a
>>   ), is it
>> old enough that we don't care?
> GCC supports line tables, I don't know that clang or other compilers
> do.  So all we really need is to check for gcc.  The line table stuff
> was added a long time ago so not sure that we really need to check for
> version 7 at this point.  So just checked that we are using gcc.  The
> "other test" func-map-to-same-line.exp expects the line table so it
> should probably also be checking that we are using gcc.

GCC 7.1 (first gcc 7 release) was on May 2nd 2017, almost exactly 6 
years ago, and there was a gcc 6.8 release in october 2018. I don't know 
if 5 years is long enough to assume that everyone has abandoned the old 
version (especially seeing as we sometimes test for gcc 4 or 3, but that 
might just be old cruft). That said, it's not a blocker for me, so /shrug

Also, clang will have line tables - otherwise almost nothing on our test 
suite would work. It doesn't have column info, though, which is why I'm 
fine with it being ignored in the test that uses -gno-column-info.

>
>>> +if ![is_c_compiler_gcc] {
>>> +    unsupported "gcc is required for this test"
>>> +    return 0
>>> +}
>>> +
>>> +set srcfile  func-map-to-same-line.c
>>> +set executable func-map-to-same-line
>>> +
>>> +set options [list debug additional_flags=-gno-column-info]
>>> +
>>> +if {[build_executable "failed to prepare" $executable $srcfile
>>> $options] == -1}\
>>> + {
>>> +    return -1
>>> +}
>>> +
>>> +clean_restart $executable
>>> +
>>> +runto_main
>>> +set target_remote [gdb_is_target_remote]
>>> +
>>> +if [supports_process_record] {
>>> +    # Activate process record/replay.
>>> +    gdb_test_no_output "record" "turn on process record for test1"
>>> +}
>>> +
>>> +# 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 \
>>> +    "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 \\(\\);.*" \
>>> +    "reverse-next to line with two functions"
>>> +
>>> +# 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-step" ".*b = 2;.*" \
>>> +    "reverse-step to previous line b = 2"
>> The point of this test is to confirm that we are at the very first
>> instruction of the line, right? So I would think it is better to do
>> a
>> reverse-stepi here, to make sure that even walking a single
>> instruction
>> we reach a different line.
> Yes, we should be on the first instruction of the line so stepi would
> be a better test to prove we are really on the first instruction.
> Changed the reverse-step to reverse-stepi.
>
>> Either that or doing what Pedro did in his
>> email: save the PC before executing the line, then do and undo the
>> line
>> and confirm that PCs match exactly.
>>> +
>>> +
>>> +# Setup for test 2
>>> +# Go back to the start of the function
>>> +gdb_test "reverse-continue" "a = 1;" "At start of main, setup for
>>> test 2"
>>> +
>>> +# Turn off record to clear logs and turn on again
>>> +gdb_test "record stop"  "Process record is stopped.*" \
>>> +    "turn off process record for test1"
>>> +gdb_test_no_output "record" "turn on process record for test2"
>> Since you don't require process record for this test, you can't
>> assume
>> these to work. I think its better to clean restart and record if the
>> process supports recording, this way you're sure to reset history no
>> matter the inferior.
> No, the test requires process record.  If record is not supported, we
> can't do reverse execution.  That said, doing a "clean restart" and
> then record would be another way, probably better way, of clearing the
> history.  Put in a clean restart rather than turning off/on the
> recording to clear the log file.

 From my understanding, you could have architectures or inferiors that 
support reverse execution without needing to record, that's why 
"supports_reverse" and "supports_process_record" are different.

However, if you want to restrict this to record-only, that's fine, I 
just think it should be a requirement at the top of the test, not in the 
middle of the execution.

-- 
Cheers,
Bruno