From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id oe3EOL8HEGUvARwAWB0awg (envelope-from ) for ; Sun, 24 Sep 2023 05:56:15 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=D2wR2UT2; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id DDAFD1E0C3; Sun, 24 Sep 2023 05:56:15 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id CB5291E092 for ; Sun, 24 Sep 2023 05:56:13 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DC36C3858C2A for ; Sun, 24 Sep 2023 09:56:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DC36C3858C2A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1695549372; bh=AriieJfWplp4G8uw0cPG03H8tRTqFz7v/KfliS8ibvk=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=D2wR2UT2qEEnVBLdMSDsuHI2a6+88XuEMCG5chx+PWqsf/ZF+zAzM3Thqs2KdwSmT 4w0e925akxe/KtaXJNucl1sJW+vkHKsDMUybg+71xioeVJO2JhUeXMqZTLQjPxLtA8 DddEEE1SmEdhQwBfswy4cYhYYAZ9e7lRCnac3PJo= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id DFAE73858C50 for ; Sun, 24 Sep 2023 09:55:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DFAE73858C50 Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-37-2ICUutGrOu-wNwz_eU72cw-1; Sun, 24 Sep 2023 05:55:48 -0400 X-MC-Unique: 2ICUutGrOu-wNwz_eU72cw-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-32006e08483so3540972f8f.0 for ; Sun, 24 Sep 2023 02:55:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695549347; x=1696154147; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AriieJfWplp4G8uw0cPG03H8tRTqFz7v/KfliS8ibvk=; b=BFQqW6ICDBFPBCvhjCkl6M/h1oJ5P3UF4LaaMNYj7Vj05WoqNMaMxwy/J6YKMo+fZ5 Pnz9w23ODdgTvXCH/uWU/JpnFoJZEl0gWYVwCS2HSYyx7N/ALO/fmCLAH8Cog/e8Uwcl 7+d3Ti8kwn6xYvkNlfxWF/0PLnLkvU5wgmKTSE41iiLAjm8g4GpgOvASkmnWr+6ygWGx jrPjlJSd6JglWErBznULn07J4hU62IyzE2cgh4Py+wVWB8Va90PqdzRreRE6EM0YitvR 07Njnonb5gIrL1C9FYLKVdgvVJtfsFz1r6iYrU7Rc1+9o5bMUkGwMmBpWn1zM34sDy2r HD2A== X-Gm-Message-State: AOJu0YxDaaQ8Z9pDg7CGXGQInPGaMaZ2rJhMrEJDsDxS4GzPtgPPbBR8 Q6U4x/sAdWg8O/U81xWYl1THGEUeS7v71IgXqy5oQaFeh5cEoH7nodoq6qb8k6vECRS1JfOK+2A b5l0wiMtGeH+ZuFKe2SGrTApL1xDIbY0I X-Received: by 2002:adf:ca10:0:b0:323:1629:eeba with SMTP id o16-20020adfca10000000b003231629eebamr3414768wrh.1.1695549347220; Sun, 24 Sep 2023 02:55:47 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFq8VLihtYuXq0sBXRwQEtGzjeNGDBEf8ZaGady6wTyqjPd/SoeQmhTv/cjA34iX/ihrRDc5A== X-Received: by 2002:adf:ca10:0:b0:323:1629:eeba with SMTP id o16-20020adfca10000000b003231629eebamr3414761wrh.1.1695549346773; Sun, 24 Sep 2023 02:55:46 -0700 (PDT) Received: from [10.228.5.150] ([195.89.33.213]) by smtp.gmail.com with ESMTPSA id t10-20020adff60a000000b0031c5e9c2ed7sm8867316wrp.92.2023.09.24.02.55.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 24 Sep 2023 02:55:46 -0700 (PDT) Message-ID: Date: Sun, 24 Sep 2023 11:55:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] gdb/record: print frame information when exiting a recursive call To: Kevin Buettner , Guinevere Larsen via Gdb-patches References: <20230923103457.29768-2-blarsen@redhat.com> <20230923145630.322bf1d5@f37-zws-nv> In-Reply-To: <20230923145630.322bf1d5@f37-zws-nv> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Guinevere Larsen via Gdb-patches Reply-To: Guinevere Larsen Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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 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 . */ >> + >> +/* 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 . */ >> + >> +# 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 >>