Re: [PATCH] gdb/record: print frame information when exiting a recursive call

[Guinevere Larsen via Gdb-patches <gdb-patches@sourceware.org>]

On 23/09/2023 23:56, Kevin Buettner wrote:
> Hi Guinevere,
>
> Just a few nits.  See below...

Hi Kevin!

Thanks for taking a look, I'll fix all the nits for v2.

>
> On Sat, 23 Sep 2023 12:34:58 +0200
> Guinevere Larsen via Gdb-patches <gdb-patches@sourceware.org> wrote:
>
>> Currently,  when GDB is reverse stepping out of a function into the same
>> function due to a recursive call, it doesn't print frame information, as
>> reported by PR record/29178. This happens because when the inferior
>> leaves the current frame, GDB decides to refresh the step information,
>> clobbering the original step_frame_id, making it impossible to figure
>> out later on that the frame has been changed.
>>
>> This commit changes GDB so that, if we notice we're in this exact
>> situation, we won't refresh the step information.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29178
>> ---
>>   gdb/infrun.c                            | 18 +++++++++
>>   gdb/testsuite/gdb.reverse/recursion.c   | 38 +++++++++++++++++++
>>   gdb/testsuite/gdb.reverse/recursion.exp | 49 +++++++++++++++++++++++++
>>   3 files changed, 105 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.reverse/recursion.c
>>   create mode 100644 gdb/testsuite/gdb.reverse/recursion.exp
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 4730d290442..00e6215ebc8 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -7679,6 +7679,11 @@ process_event_stop_test (struct execution_control_state *ecs)
>>       }
>>   
>>     bool refresh_step_info = true;
>> +
>> +  /* shorthand to make if statements smaller.  */
> Capitalize "shorthand".
>
>> +  struct frame_id original_frame_id
>> +    = ecs->event_thread->control.step_frame_id;
>> +  struct frame_id curr_frame_id = get_frame_id (get_current_frame ());
> I think these could be used to simplify at least one other, already
> existing, if-statement too.  Perhaps post another patch with that
> change?  (Or make it a two-part series with the above addition w/
> updates to existing code as part 1.)
Yeah, the if statement right above this one can be simplified quite a 
bit, but I was worried about ballooning too much. I'll send it 
separately for v2.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
>>     if ((ecs->event_thread->stop_pc () == stop_pc_sal.pc)
>>         && (ecs->event_thread->current_line != stop_pc_sal.line
>>   	  || ecs->event_thread->current_symtab != stop_pc_sal.symtab))
>> @@ -7722,6 +7727,19 @@ process_event_stop_test (struct execution_control_state *ecs)
>>   			       "it's not the start of a statement");
>>   	}
>>       }
>> +  else if (execution_direction == EXEC_REVERSE
>> +	  && curr_frame_id != original_frame_id
>> +	  && original_frame_id.code_addr_p && curr_frame_id.code_addr_p
>> +	  && original_frame_id.code_addr == curr_frame_id.code_addr)
>> +    {
>> +      /* If we enter here, we're leaving a recursive function call.  In this
>> +	 situation, we shouldn't refresh the step information, because if we
>> +	 do, we'll lose the frame_id of when we started stepping, and this
>> +	 will make GDB not know we need to print frame information.  */
>> +      refresh_step_info = false;
>> +      infrun_debug_printf ("reverse stepping, left a recursive call, don't "
>> +			   "update step info so we remember we left a frame");
>> +    }
>>   
>>     /* We aren't done stepping.
>>   
>> diff --git a/gdb/testsuite/gdb.reverse/recursion.c b/gdb/testsuite/gdb.reverse/recursion.c
>> new file mode 100644
>> index 00000000000..747404ce22c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/recursion.c
>> @@ -0,0 +1,38 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   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/>.  */
>> +
>> +/* Test GDB's ability to handle recursive functions when executing
>> +   in reverse.  */
>> +
>> +int
>> +foo (int x) {
>> +    if (x) return foo(x-1);
>> +    return 0;
>> +}
>> +
>> +int
>> +bar(int x){
>> +    int r = foo(x);
>> +    return 2*r;
>> +}
>> +
>> +int
>> +main() {
>> +    int i = bar(5);
>> +    int j = foo(5);
>> +    return 0;			/* END OF MAIN */
>> +}
> Unless there's a good reason not to do so, I'd like to see the above
> C code follow the GNU coding standard.
>
>> diff --git a/gdb/testsuite/gdb.reverse/recursion.exp b/gdb/testsuite/gdb.reverse/recursion.exp
>> new file mode 100644
>> index 00000000000..331113bee0a
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.reverse/recursion.exp
>> @@ -0,0 +1,49 @@
>> +# 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".
>> +
>> +#
>> +# Test step and next in reverse
>> +#
>> +
>> +require supports_reverse
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +    return -1
>> +}
>> +
>> +runto_main
>> +
>> +if [supports_process_record] {
>> +    # Activate process record/replay
>> +    gdb_test_no_output "record" "turn on process record"
>> +}
>> +
>> +set end_of_program [gdb_get_line_number "END OF MAIN" "$srcfile"]
>> +gdb_breakpoint $end_of_program
>> +gdb_continue_to_breakpoint ".*$srcfile/$end_of_program.*"
>> +
>> +## test if GDB can reverse over a recursive program
>> +gdb_test "reverse-next" ".*int j = foo.*" "Skipping recursion from outside"
>> +## setup and next over a recursion for inside a recursive call
>> +repeat_cmd_until "reverse-step" ".*" ".*foo .x=4.*"
>> +gdb_test "reverse-next" ".*return foo.*" "Skipping recursion from inside"
>> +gdb_test "reverse-next" ".*foo .x=5.*" "print frame when stepping out"
>> +gdb_test "reverse-next" ".*bar .x=5.*" "stepping into a different function"
>> +gdb_test "reverse-next" "main .. at .*" "stepping back to main"
>> -- 
>> 2.41.0
>>