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

[Bruno Larsen <blarsen@redhat.com>]

On 30/09/2022 20:28, Pedro Alves wrote:
> 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.
Oops, sorry, old habits die hard.
>
>> +
>>   /* 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.

Thanks for catching that, it is a problem, but for a different reason.

On line 147 on this file we set the execution direction as reverse, so 
all "next" commands are "reverse-next". The problem is that using an 
explicit reverse-next makes GDB throw an error and doesn't step, which 
will give us an infinite loop for sure.

>
>> +	}
>> +    }
>> +}
>> +
>> +# 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"
I just noticed this test name is incorrect.
>> +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.
This, on the other hand, is not a problem. The execution is set as 
reverse, so "next" here is equivalent to "reverse-next" implicitly.
>
>> +	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.

I think you attached the before log, not the after, not sure if that is 
what you meant to do...

Regardless, what it looks to me is that this version of gcc in ubuntu 
20.04 is making it so we need more than one step to go through the line 
`a[5] = a[3] - a[4];`, so the inferior's actual state gets de-synced 
from the test's expected state. As supporting evidence, after my patch , 
we get PASSes after my re-syncing code, which were FAILs before.

You can probably easily fix this with a gcc check and an extra step at 
the right spot.

Cheers,
Bruno

>
> @@ -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
>