On 2022-08-31 1:07 p.m., Bruno Larsen 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.  The problem was happening because GDB wouldn't give
> that step_resume_breakpoint a frame-id, so the first time the breakpoint
> was hit, the inferior would be stopped.  This is fixed by giving the
> current frame-id to the breakpoint.
> 
> This commit also changes gdb.reverse/step-reverse.c 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                                |  2 +-
>  gdb/testsuite/gdb.reverse/step-precsave.exp |  6 ++-
>  gdb/testsuite/gdb.reverse/step-reverse.c    | 18 ++++++-
>  gdb/testsuite/gdb.reverse/step-reverse.exp  | 58 +++++++++++++++++++--
>  4 files changed, 76 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 033699bc3f7..679a0c83ece 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7133,7 +7133,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  		  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);
> +							sr_sal, get_stack_frame_id (frame));
>  		}
>  	    }
>  	  else
> diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
> index 0836ed2629f..3279b6ce879 100644
> --- a/gdb/testsuite/gdb.reverse/step-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
> @@ -86,7 +86,8 @@ gdb_test "step 3" ".*STEP TEST 2.*" "step test 2"
>  
>  # step over call
>  
> -gdb_test "step" ".*NEXT OVER THIS CALL.*" "step up to call"
> +gdb_test "step" ".*NEXT OVER THIS RECURSION.*" "step up to call"
> +gdb_test "next" ".*NEXT OVER THIS CALL.*" "skip recursive call"
>  gdb_test "next" ".*STEP INTO THIS CALL.*" "next over call"
>  
>  # step into call
> @@ -280,9 +281,10 @@ gdb_test_multiple "step" "$test_message" {
>      }
>  }
>  
> -# next backward over call
> +# Next backward over calls.
>  
>  gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
> +gdb_test "next" ".*NEXT OVER THIS RECURSION.*" "reverse next over recursive call"
>  
>  # step/next backward with count
>  
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index aea2a98541d..3d647b9b29d 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -26,6 +26,19 @@ 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 */

Could you make the function follow GNU formatting?  I know that the file
has other bits that don't follow the style, but we can just not add more cases.

> +
>  /* A structure which, we hope, will need to be passed using memcpy.  */
>  struct rhomboidal {
>    int rather_large[100];
> @@ -51,6 +64,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 +76,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..a540b1f88ce 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
> @@ -243,10 +244,59 @@ gdb_test_multiple "step" "$test_message" {
>      }
>  }
>  
> -# next backward over call
> +# Next backward over call.
>  
>  gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call"
>  
> +set step_out 0
> +gdb_test_multiple "next" "reverse next over recursion" {
> +    -re -wrap ".*NEXT OVER THIS RECURSION.*" {
> +	pass "$gdb_test_name"
> +    }
> +    -re -wrap ".*RECURSIVE CALL.*" {
> +	fail "$gdb_test_name"
> +	set step_out 1
> +    }
> +}
> +if { "$step_out" == 1 } {
> +    gdb_test_multiple "next" "stepping out of recursion" {
> +	-re -wrap "NEXT OVER THIS RECURSION.*" {
> +	    set step_out 0
> +	}
> +	-re -wrap ".*" {
> +	    send_gdb "reverse-next\n"
> +	    exp_continue

The command passed to gdb_test_multiple was "next", but here you
do "reverse-next".  Seems best to be consistent.  After, this can
potentially infinite loop of the reverse-next never takes you to the
expected line, which seems a bit dangerous.

> +	}
> +    }
> +}
> +
> +# 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"
> +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

> +    }
> +    -re -wrap ".*" {
> +	send_gdb "next\n"

Ditto, re. infinite loop.

> +	exp_continue
> +    }
> +}
> +

Other than that, it looks fine to me.  

I still see this failing on Ubuntu 20.04, though.  I haven't investigated why.  I've attached
the gdb.log, in case something jumps out as obvious to you.

The before/after gdb.sum diff shows that some other tests FAIL on this machine, so maybe
it's just more of the same.

@@ -13,7 +13,7 @@ PASS: gdb.reverse/step-reverse.exp: next
 PASS: gdb.reverse/step-reverse.exp: step test 1
 PASS: gdb.reverse/step-reverse.exp: next test 2
 PASS: gdb.reverse/step-reverse.exp: step test 2
-PASS: gdb.reverse/step-reverse.exp: step up to call 
+PASS: gdb.reverse/step-reverse.exp: next over recursion 
 PASS: gdb.reverse/step-reverse.exp: next over call
 PASS: gdb.reverse/step-reverse.exp: step into call
 PASS: gdb.reverse/step-reverse.exp: finish out of fn call
@@ -27,14 +27,20 @@ PASS: gdb.reverse/step-reverse.exp: simp
 FAIL: gdb.reverse/step-reverse.exp: reverse step into fn call
 FAIL: gdb.reverse/step-reverse.exp: reverse step out of called fn
 FAIL: gdb.reverse/step-reverse.exp: reverse next over call
-FAIL: gdb.reverse/step-reverse.exp: reverse step test 1 
-FAIL: gdb.reverse/step-reverse.exp: reverse next test 1 
-FAIL: gdb.reverse/step-reverse.exp: reverse step test 2 
-FAIL: gdb.reverse/step-reverse.exp: reverse next test 2 
+FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion 
+PASS: gdb.reverse/step-reverse.exp: forward again to test recursion 
+FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion again 
+PASS: gdb.reverse/step-reverse.exp: reverse again to test recursion 
+FAIL: gdb.reverse/step-reverse.exp: enter recursive function 
+PASS: gdb.reverse/step-reverse.exp: step over recursion inside the recursion 
+PASS: gdb.reverse/step-reverse.exp: reverse step test 1 
+PASS: gdb.reverse/step-reverse.exp: reverse next test 1 
+PASS: gdb.reverse/step-reverse.exp: reverse step test 2