From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 67EFE3858D33 for ; Mon, 16 Jan 2023 12:31:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 67EFE3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673872301; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4R5Gvp1jQxNXirpGMXqQD1zvNjj3YNqrM9G0ar6Qh0g=; b=ZbP923ssju8UwI2NKLZXuCFzmcUcA1u/g8S0r4T8zcNhQp/QGqQx6v4wSkzQgWzm0jI3uu Iab+Fcan2+HnnO5Ky6xHtyiYQAHlO8W79ScWbjLL9h2tw43+DZU4LR3WisEZ0CI0hE0bRX WH/FM6L0X1WMraNwkL0Zw6305AZipGc= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-374-CpPUiFWaPsK4SG4LFaaFnw-1; Mon, 16 Jan 2023 07:31:40 -0500 X-MC-Unique: CpPUiFWaPsK4SG4LFaaFnw-1 Received: by mail-ed1-f70.google.com with SMTP id e6-20020a056402190600b0048ee2e45daaso18844574edz.4 for ; Mon, 16 Jan 2023 04:31:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=4R5Gvp1jQxNXirpGMXqQD1zvNjj3YNqrM9G0ar6Qh0g=; b=7huXVaQAdufQ/Rau+c4azdmKYm75h5tHmmzl95+F3r3eR8EZre0oB3eEm4iLV46IzG NB9PYfsfDUAkwn/mz0Dv8DYFux4NkWgxD6rvXj0PNYcGQ0bLldTzHHDTU7An66HspHLL Stqw2hNPxqDcNhMzUIQZGvcTSCySkQxijQtMjtpM7oAZrmXuNt1LztWywhCtWJbYqZr/ QxdNyUABbJ/xp/4lzq2lkHSotubzjM+HkuMSa44m7F+DfPDlACh/AVzVt5QrWpc7d3FY TZq4LK6BNk0Qy6Fx7tZow5msZxxr3tYubMqIvfECr4uEZ37TUjYvG/BX/oKFo5X/DR7U Dc1Q== X-Gm-Message-State: AFqh2krO+yx1KEJk+AJdh1UaQSlO+nvnXEly2Mvulh+CuO2fnqUECNCo OqtQjKNqMwZ57tepzxl1jIUlLtBsf73tdOKF60EUQsYU+kIDHVkMIh9Nv+EV4LF8+1De1IsbHkg Fo9oSpRJM1ZH1FSCQvnqq2w== X-Received: by 2002:a05:6402:1cce:b0:472:1b3b:35f1 with SMTP id ds14-20020a0564021cce00b004721b3b35f1mr99539411edb.21.1673872298354; Mon, 16 Jan 2023 04:31:38 -0800 (PST) X-Google-Smtp-Source: AMrXdXtdnYBnRU/ErH7a/RS4p2Or+Ponx2Uu2WVYDqJQsJktoQ/px2kAJWGO91Oi+XfzT48YWPAS5A== X-Received: by 2002:a05:6402:1cce:b0:472:1b3b:35f1 with SMTP id ds14-20020a0564021cce00b004721b3b35f1mr99539380edb.21.1673872297803; Mon, 16 Jan 2023 04:31:37 -0800 (PST) Received: from [10.43.2.105] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id bt16-20020a0564020a5000b00482e0c55e2bsm11314475edb.93.2023.01.16.04.31.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Jan 2023 04:31:37 -0800 (PST) Message-ID: <011768e8-2b76-f8ed-1174-fbaa020b15e7@redhat.com> Date: Mon, 16 Jan 2023 13:31:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [PATCH 1/2] fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp To: Carl Love , Ulrich Weigand , "will_schmidt@vnet.ibm.com" , gdb-patches@sourceware.org References: <51c4bfc82ac72e475e10577dc60e4d75fa48767e.camel@de.ibm.com> <3ea97a8aa9cccb39299adde682f92055d1986ab3.camel@us.ibm.com> <53878e37c6e57de1d04d9c9960c5d0a74324ee6e.camel@us.ibm.com> <50474aa92ba82eff05cdc8f49001eae56be29670.camel@us.ibm.com> <89331c26795e3f7743e1e068dce43b3c2dd53008.camel@us.ibm.com> <071f24ecf9b3a2bbbe8fee7db77492eb55c5f3ff.camel@us.ibm.com> <1d9b21914354bef6a290ac30673741e722e11757.camel@de.ibm.com> <3e3c9c40f07ab01c79fe10915e76ffa187c42ad9.camel@us.ibm.com> <122f5d2d3db9ef1979b0f8da927d005f32bba82c.camel@us.ibm.com> From: Bruno Larsen In-Reply-To: <122f5d2d3db9ef1979b0f8da927d005f32bba82c.camel@us.ibm.com> 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: 8bit X-Spam-Status: No, score=-12.1 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_H2,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 List-Id: On 14/01/2023 19:08, Carl Love wrote: > On Fri, 2023-01-13 at 14:33 +0100, Bruno Larsen wrote: >> On 11/01/2023 19:27, Carl Love via Gdb-patches wrote: >>> GDB maintainers: >>> >>> This patch fixes the issues with the reverse-finish command on X86. >>> The reverse-finish command now correctly stops at the first >>> instruction >>> in the source code line of the caller. It now only requires a >>> single >>> reverse-step or reverse-next instruction to get back to the >>> previous >>> source code line. >>> >>> It also adds a new testcase, gdb.reverse/finish-reverse-next.exp, >>> and >>> updates several existing testcases. >>> >>> Please let me know if you have any comments on the patch. Thanks. >> Thanks for looking at this, this is a nice change. I just have a >> couple >> of comments, mostly related to the testsuite side. >>> Carl >>> >>> -------------------------------------------------------------- >>> X86: reverse-finish fix >>> >>> Currently on X86, when executing the finish command in reverse, gdb >>> does a >>> single step from the first instruction in the callee to get back to >>> the >>> caller. GDB stops on the last instruction in the source code line >>> where >>> the call was made. When stopped at the last instruction of the >>> source code >>> line, a reverse next or step command will stop at the first >>> instruction >>> of the same source code line thus requiring two step/next commands >>> to >>> reach the previous source code line. It should only require one >>> step/next >>> command to reach the previous source code line. >>> >>> By contrast, a reverse next or step command from the first line in >>> a >>> function stops at the first instruction in the source code line >>> where the >>> call was made. >>> >>> This patch fixes the reverse finish command so it will stop at the >>> first >>> instruction of the source line where the function call was >>> made. The >>> behavior on X86 for the reverse-finish command now matches doing a >>> reverse-next from the beginning of the function. >>> >>> The proceed_to_finish flag in struct thread_control_state is no >>> longer >>> used. This patch removes the declaration, initialization and >>> setting of >>> the flag. >>> >>> This patch requires a number of regression tests to be >>> updated. Test >>> gdb.mi/mi-reverse.exp no longer needs to execute two steps to get >>> to the >>> previous line. The gdb output for tests gdb.reverse/until- >>> precsave.exp >>> and gdb.reverse/until-reverse.exp changed slightly. The expected >>> result in >>> tests gdb.reverse/amd64-ailcall-reverse.exp and >> s/ailcall/tailcall > Fixed > >>> gdb.reverse/singlejmp-reverse.exp are updated to the correct >>> expected >>> result. >>> >>> This patch adds a new test gdb.reverse/finish-reverse-next.exp to >>> test the >>> reverse-finish command when returning from the entry point and from >>> the >>> body of the function. >>> >>> The step_until proceedure in test gdb.reverse/step-indirect-call- >>> thunk.exp >>> was moved to lib/gdb.exp and renamed cmd_until. >> I'm not a big fan of the name cmd_until, because it sounded to me >> like >> you were testing the GDB command until. I think repeat_cmd_until or >> repeat_until would avoid this possible confusion. > Changed cmd_until to repeat_cmd_until. > >>> The patch has been tested on X86 and PowerPC to verify no >>> additional >>> regression failures occured. >>> >>> Bug: >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927 >>> >> If you add record/29927 somewhere along the text of your commit >> message, >> there is some automation that will comment on the bugzilla bug >> specifying this commit. Might be worth doing for future reference. > Added. I realized I had forgotten to do that after I sent the email. > I added it to both patches. > >>> --- >>> gdb/gdbthread.h | 4 - >>> gdb/infcall.c | 3 - >>> gdb/infcmd.c | 32 +++--- >>> gdb/infrun.c | 41 +++---- >>> gdb/testsuite/gdb.mi/mi-reverse.exp | 9 +- >>> .../gdb.reverse/amd64-tailcall-reverse.exp | 5 +- >>> .../gdb.reverse/finish-reverse-next.c | 48 ++++++++ >>> .../gdb.reverse/finish-reverse-next.exp | 108 >>> ++++++++++++++++++ >>> gdb/testsuite/gdb.reverse/finish-reverse.exp | 5 + >>> .../gdb.reverse/singlejmp-reverse.exp | 5 +- >>> .../gdb.reverse/step-indirect-call-thunk.exp | 49 ++------ >>> gdb/testsuite/gdb.reverse/until-precsave.exp | 2 +- >>> gdb/testsuite/gdb.reverse/until-reverse.exp | 2 +- >>> gdb/testsuite/lib/gdb.exp | 33 ++++++ >>> 14 files changed, 240 insertions(+), 106 deletions(-) >>> create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse- >>> next.c >>> create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse- >>> next.exp >>> >>> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h >>> index 11d69fceab0..e4edff2d621 100644 >>> --- a/gdb/gdbthread.h >>> +++ b/gdb/gdbthread.h >>> @@ -150,10 +150,6 @@ struct thread_control_state >>> the finished single step. */ >>> int trap_expected = 0; >>> >>> - /* Nonzero if the thread is being proceeded for a "finish" >>> command >>> - or a similar situation when return value should be >>> printed. */ >>> - int proceed_to_finish = 0; >>> - >>> /* Nonzero if the thread is being proceeded for an inferior >>> function >>> call. */ >>> int in_infcall = 0; >>> diff --git a/gdb/infcall.c b/gdb/infcall.c >>> index e09904f9a35..116605c43ef 100644 >>> --- a/gdb/infcall.c >>> +++ b/gdb/infcall.c >>> @@ -625,9 +625,6 @@ run_inferior_call >>> (std::unique_ptr sm, >>> >>> disable_watchpoints_before_interactive_call_start (); >>> >>> - /* We want to print return value, please... */ >>> - call_thread->control.proceed_to_finish = 1; >>> - >>> try >>> { >>> /* Infcalls run synchronously, in the foreground. */ >>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c >>> index 0497ad05091..9c42efeae8d 100644 >>> --- a/gdb/infcmd.c >>> +++ b/gdb/infcmd.c >>> @@ -1721,19 +1721,10 @@ finish_backward (struct finish_command_fsm >>> *sm) >>> >>> sal = find_pc_line (func_addr, 0); >>> >>> - tp->control.proceed_to_finish = 1; >>> - /* Special case: if we're sitting at the function entry point, >>> - then all we need to do is take a reverse singlestep. We >>> - don't need to set a breakpoint, and indeed it would do us >>> - no good to do so. >>> - >>> - Note that this can only happen at frame #0, since there's >>> - no way that a function up the stack can have a return address >>> - that's equal to its entry point. */ >>> + frame_info_ptr frame = get_selected_frame (nullptr); >>> >>> if (sal.pc != pc) >>> { >>> - frame_info_ptr frame = get_selected_frame (nullptr); >>> struct gdbarch *gdbarch = get_frame_arch (frame); >>> >>> /* Set a step-resume at the function's entry point. Once >>> that's >>> @@ -1743,16 +1734,22 @@ finish_backward (struct finish_command_fsm >>> *sm) >>> sr_sal.pspace = get_frame_program_space (frame); >>> insert_step_resume_breakpoint_at_sal (gdbarch, >>> sr_sal, null_frame_id); >>> - >>> - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); >>> } >>> else >>> { >>> - /* We're almost there -- we just need to back up by one more >>> - single-step. */ >>> - tp->control.step_range_start = tp->control.step_range_end = >>> 1; >>> - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); >>> + /* We are exactly at the function entry point. Note that >>> this >>> + can only happen at frame #0. >>> + >>> + When setting a step range, need to call set_step_info >>> + to setup the current_line/symtab fields as well. */ >>> + set_step_info (tp, frame, find_pc_line (pc, 0)); >>> + >>> + /* Return using a step range so we will keep stepping back >>> + to the first instruction in the source code line. */ >>> + tp->control.step_range_start = sal.pc; >>> + tp->control.step_range_end = sal.pc; >>> } >>> + proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); >>> } >>> >>> /* finish_forward -- helper function for finish_command. FRAME >>> is the >>> @@ -1778,9 +1775,6 @@ finish_forward (struct finish_command_fsm >>> *sm, frame_info_ptr frame) >>> >>> set_longjmp_breakpoint (tp, frame_id); >>> >>> - /* We want to print return value, please... */ >>> - tp->control.proceed_to_finish = 1; >>> - >>> proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); >>> } >>> >>> diff --git a/gdb/infrun.c b/gdb/infrun.c >>> index 181d961d80d..8ed538ea9ec 100644 >>> --- a/gdb/infrun.c >>> +++ b/gdb/infrun.c >>> @@ -2748,8 +2748,6 @@ clear_proceed_status_thread (struct >>> thread_info *tp) >>> >>> tp->control.stop_step = 0; >>> >>> - tp->control.proceed_to_finish = 0; >>> - >>> tp->control.stepping_command = 0; >>> >>> /* Discard any remaining commands or status from previous >>> stop. */ >>> @@ -6737,31 +6735,28 @@ process_event_stop_test (struct >>> execution_control_state *ecs) >>> >>> case BPSTAT_WHAT_STEP_RESUME: >>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME"); >>> - >>> delete_step_resume_breakpoint (ecs->event_thread); >>> - if (ecs->event_thread->control.proceed_to_finish >>> - && execution_direction == EXEC_REVERSE) >>> + fill_in_stop_func (gdbarch, ecs); >>> + >>> + if (execution_direction == EXEC_REVERSE >>> + && ecs->event_thread->stop_pc () == ecs->stop_func_start) >> Is there any reason to invert the order of checks here? The second >> if >> clause is the same and keeping that would make the changes easier to >> parse. > No, must have inadvertently swizzled it as part of the patch > development. Per comments for the second patch, PowerPC, the "cs- >> event_thread->stop_pc () == ecs->stop_func_start" check should be > removed in this patch not the PowerPC patch. Probably got missed when > I switched the order of the patches. > > Fixed, removed the "ecs->event_thread->stop_pc () == ecs- >> stop_func_start" test here. >>> { >>> struct thread_info *tp = ecs->event_thread; >>> + stop_pc_sal = find_pc_line (ecs->event_thread->stop_pc (), >>> 0); >>> >>> - /* We are finishing a function in reverse, and just hit the >>> - step-resume breakpoint at the start address of the >>> - function, and we're almost there -- just need to back up >>> - by one more single-step, which should take us back to the >>> - function call. */ >>> - tp->control.step_range_start = tp->control.step_range_end = >>> 1; >>> - keep_going (ecs); >>> - return; >>> - } >>> - fill_in_stop_func (gdbarch, ecs); >>> - if (ecs->event_thread->stop_pc () == ecs->stop_func_start >>> - && execution_direction == EXEC_REVERSE) >>> - { >>> - /* We are stepping over a function call in reverse, and just >>> - hit the step-resume breakpoint at the start address of >>> - the function. Go back to single-stepping, which should >>> - take us back to the function call. */ >>> - ecs->event_thread->stepping_over_breakpoint = 1; > The following comment was from the second email. > > > case BPSTAT_WHAT_STEP_RESUME:> Something else that I failed to > notice. Since you removed both >> comments >> that mention that this case is here for reverse finishing, there is >> no >> good way to figure out what GDB wants to do when this part of the >> code >> is reached. Adding a comment here mentioning it would fix that. >>> infrun_debug_printf ("BPSTAT_WHAT_STEP_RESUME"); > There were two separate if statements, each with a comment about what > they were for. Those comments were removed and a new, similar, comment > was added in the single new if statement. Admittedly, the new comment > is a bit farther into the function and thus easy to miss. So, I moved > the initial comment about what is going on "We are finishing a function > in reverse or..." up to the beginning of the if statement. Hopefully > that helps make it quicker/easier for the reader to see what the > purpose of the case statement/if statement. Please let me know if that > helps address your concerns. Yeah, this works. It is mostly so that we don't end up with a comment kinda far away or in a situation where it's hard to understand the point of this case statement. > >>> + /* When setting a step range, need to call set_step_info >>> + to setup the current_line/symtab fields as well. */ >>> + set_step_info (tp, frame, stop_pc_sal); >>> + >>> + /* We are finishing a function in reverse or stepping over a >>> function >>> + call in reverse, and just hit the step-resume breakpoint >>> at the >>> + start address of the function, and we're almost there -- >>> just need >>> + to back up to the function call. >>> + >>> + Return using a step range so we will keep stepping back to >>> the >>> + first instruction in the source code line. */ >>> + tp->control.step_range_start = ecs->stop_func_start; >>> + tp->control.step_range_end = ecs->stop_func_start; >>> keep_going (ecs); >>> return; >>> } >>> diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp >>> b/gdb/testsuite/gdb.mi/mi-reverse.exp >>> index d631beb17c8..30635ab1754 100644 >>> --- a/gdb/testsuite/gdb.mi/mi-reverse.exp >>> +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp >>> @@ -97,15 +97,10 @@ proc test_controlled_execution_reverse {} { >>> "basics.c" $line_main_callme_1 "" \ >>> "reverse finish from callme" >>> >>> - # Test exec-reverse-next >>> - # It takes two steps to get back to the previous line, >>> - # as the first step moves us to the start of the current >>> line, >>> - # and the one after that moves back to the previous line. >>> - >>> - mi_execute_to "exec-next --reverse 2" \ >>> + mi_execute_to "exec-next --reverse" \ >>> "end-stepping-range" "main" "" \ >>> "basics.c" $line_main_hello "" \ >>> - "reverse next to get over the call to do_nothing" >>> + "reverse next to get over the call to do_nothing" >>> >>> # Test exec-reverse-step >>> >>> diff --git a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp >>> b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp >>> index 52a87faabf7..9964b4f8e4b 100644 >>> --- a/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp >>> +++ b/gdb/testsuite/gdb.reverse/amd64-tailcall-reverse.exp >>> @@ -44,6 +44,5 @@ if [supports_process_record] { >>> gdb_test "next" {f \(\);} "next to f" >>> gdb_test "next" {v = 3;} "next to v = 3" >>> >>> -# FAIL was: >>> -# 29 g (); >>> -gdb_test "reverse-next" {f \(\);} >>> +# Reverse step back into f (). Puts us at call to g () in >>> function f (). >>> +gdb_test "reverse-next" {g \(\);} >>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.c >>> b/gdb/testsuite/gdb.reverse/finish-reverse-next.c >>> new file mode 100644 >>> index 00000000000..42e41b5a2e0 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.c >>> @@ -0,0 +1,48 @@ >>> +/* This testcase is part of GDB, the GNU debugger. >>> + >>> + Copyright 2012-2022 Free Software Foundation, Inc. >> copyright year should be 2023. >>> + >>> + 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 < >>> http://www.gnu.org/licenses/ >>> >. */ >>> + >>> +/* The reverse finish command should return from a function and >>> stop on >>> + the first instruction of the source line where the function >>> call is made. >>> + Specifically, the behavior should match doing a reverse next >>> from the >>> + first instruction in the function. GDB should only require one >>> reverse >>> + step or next statement to reach the previous source code line. >>> + >>> + This test verifies the fix for gdb bugzilla: >>> + >>> + >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927 >>> >>> +*/ >>> + >>> +int >>> +function1 (int a, int b) // FUNCTION1 >>> +{ >>> + int ret = 0; >>> + >>> + ret = a + b; >>> + return ret; >>> +} >>> + >>> +int >>> +main(int argc, char* argv[]) >>> +{ >>> + int a, b; >>> + >>> + a = 1; >>> + b = 5; >>> + >>> + function1 (a, b); // CALL FUNCTION >>> + return 0; >>> +} >>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp >>> b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp >>> new file mode 100644 >>> index 00000000000..7880de10ffc >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp >>> @@ -0,0 +1,108 @@ >>> +# Copyright 2008-2022 Free Software Foundation, Inc. > Fixed copyright so it reads 2008-2023. Fixed in finish-reverse- > next.exp and finish-reverse-next.c. > >>> + >>> +# 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 < >>> http://www.gnu.org/licenses/ >>> >. */ >>> + >>> +# This file is part of the GDB testsuite. It tests reverse >>> stepping. >>> +# Lots of code borrowed from "step-test.exp". >>> + >>> +# The reverse finish command should return from a function and >>> stop on >>> +# the first instruction of the source line where the function call >>> is made. >>> +# Specifically, the behavior should match doing a reverse next >>> from the >>> +# first instruction in the function. GDB should only take one >>> reverse step >>> +# or next statement to reach the previous source code line. >>> + >>> +# This testcase verifies the reverse-finish command stops at the >>> first >>> +# instruction in the source code line where the function was >>> called. There >>> +# are two scenarios that must be checked: >>> +# 1) gdb is at the entry point instruction for the function >>> +# 2) gdb is in the body of the function. >> While testing locally, I ran into a bug with reverse finish at the >> epilogue of the function, that your patch also fixed. It would be >> nice >> if the test extended that. And since the bug was that GDB stopped >> responding and even ctrl+c did nothing, I would suggest adding it as >> the >> last test. > Discussed this additional bug in earlier emails. Waiting to hear if > the new test I sent reliably exposes the gdb hang that Bruno reported. > If it does, I will add the new test to the new test case before posting > the updated patch set. Per the discussions, I have not been able to > reproduce the issue on my X86 or PowerPC machines. I just tried reproducing it again on my end and failed, even my original test. It must have been a fluke, maybe I forgot to compile something after pulling from upstream.  Thanks for all the thought you put into it, though! > >>> + >>> +# This test verifies the fix for gdb bugzilla: >>> +# >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927 >>> >>> + >>> +if ![supports_reverse] { >>> + return >>> +} >>> + >>> +standard_testfile >>> + >>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] >>> } { >>> + return -1 >>> +} >>> + >>> +runto_main >>> +set target_remote [gdb_is_target_remote] >>> + >>> +if [supports_process_record] { >>> + # Activate process record/replay. >>> + gdb_test_no_output "record" "turn on process record for test1" >>> +} >>> + >>> + >>> +### TEST 1: reverse finish from the entry point instruction in >>> +### function1. >>> + >>> +# Set breakpoint at call to function1 in main. >>> +set FUNCTION_test [gdb_get_line_number "CALL FUNCTION" $srcfile] >>> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at >>> .*" \his >>> + "set breakpoint on function1 call to stepi into function" >> There is a proc in lib/gdb.exp called gdb_breakpoint which >> couldsimplify >> this gdb_test to >> >> gdb_breakpoint $srcfile:$FUNCTION_test temporary >> >> And would remove the need for the delete_breakpoints call later. >> > OK, made the change in both tests. Made the same change in the PowerPC > patch that adds additional tests. > > >>> + >>> +# Continue to break point at function1 call in main. >>> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*" >>> \ >>> + "stopped at function1 entry point instruction to stepi into >>> function" >> You can use gdb_continue_to_breakpoint here instead. > OK, made the change in both tests. Made the same change in the PowerPC > patch that adds additional tests. > >>> + >>> +# stepi until we see "{" indicating we entered function1 >>> +cmd_until "stepi" "CALL FUNCTION" "{" "stepi into function1 call" >>> + >>> +delete_breakpoints >>> + >>> +gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL >>> FUNCTION.*" \ >>> + "reverse-finish function1 " >>> + >>> +# Check to make sure we stopped at the first instruction in the >>> source code >>> +# line. It should only take one reverse next command to get to >>> the previous >>> +# source line. If GDB stops at the last instruction in the >>> source code line >>> +# it will take two reverse next instructions to get to the >>> previous source >>> +# line. >>> +gdb_test "reverse-next" ".*b = 5;.*" "reverse next at b = 5, call >>> from function" >>> + >>> +# Clear the recorded log. >>> +gdb_test "record stop" "Process record is stopped.*" \ >>> + "turn off process record for test1" >>> +gdb_test_no_output "record" "turn on process record for test2" >>> + >>> + >>> +### TEST 2: reverse finish from the body of function1. >>> + >>> +# Set breakpoint at call to functiojust dont get it confused with maftn1 in main. >>> +gdb_test "break $srcfile:$FUNCTION_test" "Breakpoint $decimal at >>> .*" \ >>> + "set breakpoint on function1 call to step into body of >>> function" >>> + >>> +# Continue to break point at function1 call in main. >>> +gdb_test "continue" "Breakpoint $decimal,.*function1 \\(a, b\\).*" >>> \ >>> + "stopped at function1 entry point instruction to step to body >>> of function" >>> + >>> +delete_breakpoints >>> + >>> +# do a step instruction to get to the body of the function >>> +gdb_test "step" ".*int ret = 0;.*" "step test 1" >>> + >>> +gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL >>> FUNCTION.*" \ >>> + "reverse-finish function1 call from function body" >>> + >>> +# Check to make sure we stopped at the first instruction in the >>> source code >>> +# line. It should only take one reverse next command to get to >>> the previous >>> +# source line. >>> +gdb_test "reverse-next" ".*b = 5;.*" \ >>> + "reverse next at b = 5, from function body" >>> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse.exp >>> b/gdb/testsuite/gdb.reverse/finish-reverse.exp >>> index 01ba309420c..a05cb81892a 100644 >>> --- a/gdb/testsuite/gdb.reverse/finish-reverse.exp >>> +++ b/gdb/testsuite/gdb.reverse/finish-reverse.exp >>> @@ -16,6 +16,11 @@ >>> # This file is part of the GDB testsuite. It tests 'finish' with >>> # reverse debugging. >>> >>> +# This test verifies the fix for gdb bugzilla: >>> + >>> +# >>> https://sourceware.org/bugzilla/show_bug.cgi?id=29927 >>> >>> + >>> + >> Is this comment a left over from an earlier version? >> >> I actually wonder if the whole new test is needed, or if you can >> just >> add a couple of new tests to finish-reverse.exp; is there any reason >> you >> went with the new test instead > Yes, it is left over. Initially I just added an additional test to > finish-reverse.exp for the PowerPC fix. But as work progressed, I kept > adding more tests for PowerPC then for X86. I felt that it was better > to have a new test file that was tied to the Bugzilla. The existing > test file has a different focus from the new tests. The bugzilla > change didn't get removed from finish-reverse.exp when the tests were > moved to the new file. We can combine the tests again if that is > preferable? My preference would be to have separate test files. > Please let me know if you would prefer a single file and I will merge > them before re-posting the patches. Oh, ok, the separation makes sense now. I looked only at this patch first and asked before looking at patch 2. I'd say its fine, no need to merge them. > >>> if ![supports_reverse] { >>> return >>> } >>> diff --git a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp >>> b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp >>> index 1ca7c2ce559..eb03051625a 100644 >>> --- a/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp >>> +++ b/gdb/testsuite/gdb.reverse/singlejmp-reverse.exp >>> @@ -56,7 +56,4 @@ gdb_test "next" {v = 3;} "next to v = 3" >>> # { >>> gdb_test "reverse-step" {nodebug \(\);} >>> >>> -# FAIL was: >>> -# No more reverse-execution history. >>> -# { >>> -gdb_test "reverse-next" {f \(\);} >>> +gdb_test "reverse-next" {g \(\);} >>> diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp >>> b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp >>> index ad637899e5b..1928cdda217 100644 >>> --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp >>> +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp >>> @@ -39,39 +39,6 @@ if { ![runto_main] } { >>> return -1 >>> } >>> >>> -# Do repeated stepping COMMANDs in order to reach TARGET from >>> CURRENT >>> -# >>> -# COMMAND is a stepping command >>> -# CURRENT is a string matching the current location >>> -# TARGET is a string matching the target location >>> -# TEST is the test name >>> -# >>> -# The function issues repeated COMMANDs as long as the location >>> matches >>> -# CURRENT up to a maximum of 100 steps. >>> -# >>> -# TEST passes if the resulting location matches TARGET and fails >>> -# otherwise. >>> -# >>> -proc step_until { command current target test } { >>> - global gdb_prompt >>> - >>> - set count 0 >>> - gdb_test_multiple "$command" "$test" { >>> - -re "$current.*$gdb_prompt $" { >>> - incr count >>> - if { $count < 100 } { >>> - send_gdb "$command\n" >>> - exp_continue >>> - } else { >>> - fail "$test" >>> - } >>> - } >>> - -re "$target.*$gdb_prompt $" { >>> - pass "$test" >>> - } >>> - } >>> -} >>> - >>> gdb_test_no_output "record" >>> gdb_test "next" ".*" "record trace" >>> >>> @@ -91,20 +58,20 @@ gdb_test "reverse-next" "apply\.2.*" \ >>> "reverse-step through thunks and over inc" >>> >>> # We can use instruction stepping to step into thunks. >>> -step_until "stepi" "apply\.2" "indirect_thunk" "stepi into call >>> thunk" >>> -step_until "stepi" "indirect_thunk" "inc" \ >>> +cmd_until "stepi" "apply\.2" "indirect_thunk" "stepi into call >>> thunk" >>> +cmd_until "stepi" "indirect_thunk" "inc" \ >>> "stepi out of call thunk into inc" >>> set alphanum_re "\[a-zA-Z0-9\]" >>> set pic_thunk_re "__$alphanum_re*\\.get_pc_thunk\\.$alphanum_re* >>> \\(\\)" >>> -step_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi >>> into return thunk" >>> -step_until "stepi" "return_thunk" "apply" \ >>> +cmd_until "stepi" "(inc|$pic_thunk_re)" "return_thunk" "stepi into >>> return thunk" >>> +cmd_until "stepi" "return_thunk" "apply" \ >>> "stepi out of return thunk back into apply" >>> >>> -step_until "reverse-stepi" "apply" "return_thunk" \ >>> +cmd_until "reverse-stepi" "apply" "return_thunk" \ >>> "reverse-stepi into return thunk" >>> -step_until "reverse-stepi" "return_thunk" "inc" \ >>> +cmd_until "reverse-stepi" "return_thunk" "inc" \ >>> "reverse-stepi out of return thunk into inc" >>> -step_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" >>> \ >>> +cmd_until "reverse-stepi" "(inc|$pic_thunk_re)" "indirect_thunk" \ >>> "reverse-stepi into call thunk" >>> -step_until "reverse-stepi" "indirect_thunk" "apply" \ >>> +cmd_until "reverse-stepi" "indirect_thunk" "apply" \ >>> "reverse-stepi out of call thunk into apply" >>> diff --git a/gdb/testsuite/gdb.reverse/until-precsave.exp >>> b/gdb/testsuite/gdb.reverse/until-precsave.exp >>> index 0c2d7537cd6..777aec94ac1 100644 >>> --- a/gdb/testsuite/gdb.reverse/until-precsave.exp >>> +++ b/gdb/testsuite/gdb.reverse/until-precsave.exp >>> @@ -142,7 +142,7 @@ gdb_test "advance marker2" \ >>> # Finish out to main scope (backward) >>> >>> gdb_test "finish" \ >>> - " in main .*$srcfile:$bp_location20.*" \ >>> + "main .*$srcfile:$bp_location20.*" \ >> This change doesn't seem connected to anything in this patch, is >> this >> just a cosmetic change or was there some problem? >>> "reverse-finish from marker2" >>> > The output changes due to the functional changes in infrun.c. Instead > of stepping back one instruction i.e. ecs->event_thread- >> stepping_over_breakpoint = 1 we step back using a range. Apparently > this causes the gdb output message to change. > > Without the patch the output looks like: > Run back to call of #0 marker2 (a=43) at.../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30 > > 0x0000000010000838 in main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at /.../gdb/testsuite/gdb.reverse/until-reverse.c:48^ > > With the patch the output looks like: > > Run back to call of #0 marker2 (a=43) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/ur1.c:30 > > main (argc=1, argv=0x7fffffffcc58, envp=0x7fffffffcc68) at .../binutils-gdb-finish-precsave/gdb/testsuite/gdb.reverse/until-reverse.c:48 > > Basically, you lose the hex value and "in" with the patch applied. > This is true in the until-reverse.exp tes, below, as well. The output > change was mentioned in the commit message as well. Oh right, I should have checked the output before asking. Thank you for explaining! -- Cheers, Bruno > >>> # Advance backward to last line of factorial (outer invocation) >>> diff --git a/gdb/testsuite/gdb.reverse/until-reverse.exp >>> b/gdb/testsuite/gdb.reverse/until-reverse.exp >>> index 23fc881dbf2..3a05953329f 100644 >>> --- a/gdb/testsuite/gdb.reverse/until-reverse.exp >>> +++ b/gdb/testsuite/gdb.reverse/until-reverse.exp >>> @@ -113,7 +113,7 @@ gdb_test "advance marker2" \ >>> # Finish out to main scope (backward) >>> >>> gdb_test "finish" \ >>> - " in main .*$srcfile:$bp_location20.*" \ >>> + "main .*$srcfile:$bp_location20.*" \ >> same here. > Yup, see above. > >>> "reverse-finish from marker2" >>> >>> # Advance backward to last line of factorial (outer invocation) >>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp >>> index c41d4698d66..25f42eb5510 100644 >>> --- a/gdb/testsuite/lib/gdb.exp >>> +++ b/gdb/testsuite/lib/gdb.exp >>> @@ -9301,6 +9301,39 @@ proc gdb_step_until { regexp {test_name ""} >>> {max_steps 10} } { >>> } >>> } >>> >>> +# Do repeated stepping COMMANDs in order to reach TARGET from >>> CURRENT >>> +# >>> +# COMMAND is a stepping command >>> +# CURRENT is a string matching the current location >>> +# TARGET is a string matching the target location >>> +# TEST is the test name >>> +# >>> +# The function issues repeated COMMANDs as long as the location >>> matches >>> +# CURRENT up to a maximum of 100 steps. >>> +# >>> +# TEST passes if the resulting location matches TARGET and fails >>> +# otherwise. >>> + >>> +proc cmd_until { command current target test } { >>> + global gdb_prompt >>> + >>> + set count 0 >>> + gdb_test_multiple "$command" "$test" { >>> + -re "$current.*$gdb_prompt $" { >>> + incr count >>> + if { $count < 100 } { >>> + send_gdb "$command\n" >>> + exp_continue >>> + } else { >>> + fail "$test" >>> + } >>> + } >>> + -re "$target.*$gdb_prompt $" { >>> + pass "$test" >>> + } >>> + } >>> +} >>> + >>> # Check if the compiler emits epilogue information associated >>> # with the closing brace or with the last statement line. >>> # >>