From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
To: Bruno Larsen <blarsen@redhat.com>, gdb-patches@sourceware.org
Cc: pedro@palves.net
Subject: Re: [PATCH v2 2/2] gdb/reverse: Fix stepping over recursive functions
Date: Wed, 24 Aug 2022 14:40:33 +0100 [thread overview]
Message-ID: <87h721ep4e.fsf@redhat.com> (raw)
In-Reply-To: <20220823142204.31659-3-blarsen@redhat.com>
Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 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.
Thanks for this. The GDB fix looks fine. I had a couple of really
minor style nits relating to the test, see inline below.
> ---
> gdb/infrun.c | 2 +-
> gdb/testsuite/gdb.reverse/step-precsave.exp | 6 ++-
> gdb/testsuite/gdb.reverse/step-reverse.c | 16 +++++-
> gdb/testsuite/gdb.reverse/step-reverse.exp | 59 +++++++++++++++++++--
> 4 files changed, 76 insertions(+), 7 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..54df9209c96 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..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)
Unless required by the test, this code should follow GNU coding
standard, so
int
recursive_callee (int val)
{
and
if (val > 1)
If the non-standard layout is required, then the comment above the
function should explain why.
> + 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
Comments should be proper sentences, so capital 'N', and end with '.'.
> +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
Missing '.'.
> +
> +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
> + }
> +}
> +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
> + }
> + }
> +}
> +
> +# 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"
You can replace the strings used with pass/fail with the magic variable
$gdb_test_name. This variable is magically set within the code blocks
of a gdb_test_multiple to the name of the test.
Thanks,
Andrew
> + }
> + }
> + -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"
> --
> 2.37.2
prev parent reply other threads:[~2022-08-24 13:41 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-23 14:22 [PATCH v2 0/2] Fix reverse nexting over recursions Bruno Larsen via Gdb-patches
2022-08-23 14:22 ` [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder Bruno Larsen via Gdb-patches
2022-08-24 13:11 ` Andrew Burgess via Gdb-patches
2022-08-24 13:42 ` Andrew Burgess via Gdb-patches
2022-08-30 11:35 ` Bruno Larsen via Gdb-patches
2022-08-24 15:38 ` Andrew Burgess via Gdb-patches
2022-08-23 14:22 ` [PATCH v2 2/2] gdb/reverse: Fix stepping over recursive functions Bruno Larsen via Gdb-patches
2022-08-24 13:40 ` Andrew Burgess via Gdb-patches [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h721ep4e.fsf@redhat.com \
--to=gdb-patches@sourceware.org \
--cc=aburgess@redhat.com \
--cc=blarsen@redhat.com \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox