Re: [PATCH] gdb/reverse: Fix stepping over recursive functions

[Pedro Alves <pedro@palves.net>]

On 2022-05-25 19:02, Bruno Larsen via Gdb-patches wrote:
> Currently, when using GDB to do reverse debugging, if we try to use the
> command "reverse next" to skip a recursive function, instead of skipping
> all of the recursive calls and stopping in the previous line, we stop at
> the second to last recursive call, and need to manually step backwards
> until we leave the first call.  This is well documented in PR gdb/16678.
> 
> This bug happens because when GDB notices that a reverse step has
> entered into a function, GDB will add a step_resume_breakpoint at the
> start of the function, then single step out of the prologue once that
> breakpoint is hit.  Recursion poses a problem because that breakpoint will
> be hit many times before GDB should actually stop the inferior.  To fix
> this issue, when the step_resume_breakpoint is hit (and GDB is executing
> backwards), we analyze if the caller of the frame is the original frame
> where we started the "reverse next", and if it is, GDB will stop the
> inferior, otherwise GDB will just ignore the breakpoint.
> 
> This commit also changes gdb.reverse/step-reverse.exp to contain a
> recursive function and attempt to both, skip it altogether, and to skip
> the second call from inside the first call, as this setup broke a
> previous version of the patch.
> ---
>  gdb/infrun.c                               | 24 +++++++++
>  gdb/testsuite/gdb.reverse/step-reverse.c   | 16 +++++-
>  gdb/testsuite/gdb.reverse/step-reverse.exp | 59 ++++++++++++++++++++--
>  3 files changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 02c98b50c8c..95fb3227aa3 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6761,6 +6761,30 @@ process_event_stop_test (struct execution_control_state *ecs)
>      case BPSTAT_WHAT_STEP_RESUME:
>        infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME");
>  
> +      /* If we are executing backwards, we are doing a "next" and the
> +	 current frame is not the same as where we started, the
> +	 step_resume_breakpoint we have just hit has been added to the start
> +	 of a function so we can skip the whle function.  However, if we are

whle -> whole

> +	 skipping a recursive call, we only want to act as if we hit the
> +	 breakpoint only when the caller of the current frame is the original
> +	 frame we were single stepping from.  */

This wording confused me, took me a while to understand what is meant.  I suggest:

      /* If we are executing a reverse next, and going backwards entered
         a different frame, we've set a step_resume_breakpoint at the start
	 of the function so we can skip the whole function.  However, if we are
	 skipping a recursive call, we want to ignore the breakpoint hit until
         the caller of the current frame is the original frame we were nexting.  */

However, see further below.

> +      if (execution_direction == EXEC_REVERSE
> +	  && ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
> +	  && !frame_id_eq (get_stack_frame_id (get_current_frame ()),
> +			   ecs->event_thread->control.step_stack_frame_id))
> +	{
> +	  infrun_debug_printf ("We are not done with recursing yet");
> +	  /* If the caller's ID is not the same as the starting frame, we
> +	     can ignore this breakpoint.  */
> +	  if (!frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
> +			    ecs->event_thread->control.step_stack_frame_id))
> +	    {
> +	      keep_going (ecs);
> +	      return;
> +	    }
> +	} else
> +	    infrun_debug_printf ("are we not recursing?");

This last else is odd -- we will print it if execution_direction != EXEC_REVERSE, for
example.

I also don't understand why do we need all these different frame checks as above.  Breakpoints
can have a frame associated, so that when a breakpoint hits, the breakpoint.c stuff checks
whether the hit happened in the right frame, and if not, the breakpoint is ignored / set to
be auto-stepped-over, and BPSTAT_WHAT_STEP_RESUME isn't reached.  

The actual problem seems to me that we create the step-resume breakpoint in
question without giving it a frame.  If I do that instead of your fix, like so:

diff --git c/gdb/infrun.c w/gdb/infrun.c
index 02c98b50c8c..1360b8da70e 100644
--- c/gdb/infrun.c
+++ w/gdb/infrun.c
@@ -7118,8 +7118,8 @@ process_event_stop_test (struct execution_control_state *ecs)
                  symtab_and_line sr_sal;
                  sr_sal.pc = ecs->stop_func_start;
                  sr_sal.pspace = get_frame_program_space (frame);
-                 insert_step_resume_breakpoint_at_sal (gdbarch,
-                                                       sr_sal, null_frame_id);
+                 insert_step_resume_breakpoint_at_sal
+                   (gdbarch, sr_sal, get_stack_frame_id (frame));
                }
            }
          else

and then run your updated testcase, it passes.  I also see no regressions in gdb.reverse/ tests.

WDYT?  Are you seeing something that would break with this approach?

> +
>        delete_step_resume_breakpoint (ecs->event_thread);
>        if (ecs->event_thread->control.proceed_to_finish
>  	  && execution_direction == EXEC_REVERSE)
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index aea2a98541d..a390ac2580c 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,17 @@ int callee() {		/* ENTER CALLEE */
>    return myglob++;	/* ARRIVED IN CALLEE */
>  }			/* RETURN FROM CALLEE */
>  
> +/* We need to make this function take more than a single instruction
> +   to run, otherwise it could hide PR gdb/16678, as reverse execution can
> +   step over a single-instruction function.  */
> +int recursive_callee (int val){
> +    if (val == 0) return 0;
> +    val /= 2;
> +    if (val>1)
> +	val++;
> +    return recursive_callee (val);	/* RECURSIVE CALL */
> +} /* EXIT RECURSIVE FUNCTION */
> +
>  /* A structure which, we hope, will need to be passed using memcpy.  */
>  struct rhomboidal {
>    int rather_large[100];
> @@ -51,6 +62,9 @@ int main () {
>     y = y + 4;
>     z = z + 5;	/* STEP TEST 2 */
>  
> +   /* Test that next goes over recursive calls too */
> +   recursive_callee (32); /* NEXT OVER THIS RECURSION */
> +
>     /* Test that "next" goes over a call */
>     callee();	/* NEXT OVER THIS CALL */
>  
> @@ -60,7 +74,7 @@ int main () {
>     /* Test "stepi" */
>     a[5] = a[3] - a[4]; /* FINISH TEST */
>     callee();	/* STEPI TEST */
> -   
> +
>     /* Test "nexti" */
>     callee();	/* NEXTI TEST */
>  
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index 997b62604d5..4f56b4785ca 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -47,9 +47,11 @@ gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>  gdb_test "next 2" ".*NEXT TEST 2.*" "next test 2"
>  gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>  
> +# next through a recursive function call
> +gdb_test "next 2" "NEXT OVER THIS CALL.*" "next over recursion"
> +
>  # step over call
>  
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
>  gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>  
>  # step into call
> @@ -118,7 +120,7 @@ gdb_test_multiple "stepi" "$test_message" {
>  
>  set test_message "stepi back from function call"
>  gdb_test_multiple "stepi" "$test_message" {
> -    -re "NEXTI TEST.*$gdb_prompt $" {
> +    -re -wrap "NEXTI TEST.*" {
>  	pass "$test_message"
>      }
>      -re "ARRIVED IN CALLEE.*$gdb_prompt $" {
> @@ -143,7 +145,6 @@ gdb_test_multiple "stepi" "$test_message" {
>  ###
>  
>  # Set reverse execution direction
> -
>  gdb_test_no_output "set exec-dir reverse" "set reverse execution"
>  
>  # stepi backward thru return and into a function
> @@ -247,6 +248,58 @@ gdb_test_multiple "step" "$test_message" {
>  
>  gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>  
> +# Dont reverse the execution direction yet, as we will need another
> +# forward step after this test

- Dont -> Don't

- Add missing period.


> +
> +set step_out 0
> +gdb_test_multiple "next" "reverse next over recursion" {
> +    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> +	pass "reverse next over recursion"
> +    }
> +    -re -wrap ".*RECURSIVE CALL.*" {
> +	fail "reverse next over recursion"
> +	set step_out 1
> +    }
> +}

Please use $gdb_test_name.

> +if { "$step_out" == 1 } {
> +    gdb_test_multiple "next" "stepping out of recursion" {

"stepping out" -> "step out"

> +	-re -wrap "NEXT OVER THIS RECURSION.*" {
> +	    set step_out 0
> +	}
> +	-re -wrap ".*" {
> +	    send_gdb "reverse-next\n"
> +	    exp_continue
> +	}
> +    }
> +}

No pass call?  I guess I see -- this whole $step_out thing is to kind of
handle the previous test's failure?  Why do we have this at all?

> +
> +# Step forward over recursion again so we can test stepping over calls
> +# inside the recursion itself.
> +gdb_test_no_output "set exec-dir forward" "forward again to test recursion"
> +gdb_test "next" "NEXT OVER THIS CALL.*" "reverse next over recursion again"

This "next" wasn't a reverse next.

> +gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion"
> +
> +gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function"
> +set step_pass 1
> +gdb_test_multiple "next" "step over recursion inside the recursion" {
> +    -re -wrap ".*EXIT RECURSIVE FUNCTION.*" {
> +	set step_pass 0
> +	send_gdb "next\n"
> +	exp_continue
> +    }
> +    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> +	if {$step_pass} {
> +	    pass "step over recursion inside the recursion"
> +	} else {
> +	    fail "step over recursion inside the recursion"
> +	}

        gdb_assert $step_pass $gdb_test_name

> +    }
> +    -re -wrap ".*" {
> +	send_gdb "next\n"
> +	exp_continue
> +    }
> +}
> +
>  # step/next backward with count
>  


>  gdb_test "step 3" ".*REVERSE STEP TEST 1.*" "reverse step test 1"

I actually have to leave now, and haven't had a chance to finish looking at the testcase
in much detail.  But figured it's best to press send anyhow.  So...  <send>.

Pedro Alves