From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id dYWiNhFO0mS8fTsAWB0awg (envelope-from ) for ; Tue, 08 Aug 2023 10:15:45 -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=A4zIrP2y; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D62001E0BB; Tue, 8 Aug 2023 10:15:45 -0400 (EDT) Received: from server2.sourceware.org (ip-8-43-85-97.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 B3E621E028 for ; Tue, 8 Aug 2023 10:15:43 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C94933857700 for ; Tue, 8 Aug 2023 14:15:42 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C94933857700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1691504142; bh=9uYpna20wNSjMgebW5hePUtIbg9YQXSjUt0cDaRVVCk=; h=Date:Subject:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=A4zIrP2yCJcSjBiauBFc/8gRCd6mUUNE+BbXmJMYkJl/nHKTtHTZskZZQdt2BbqSz oD/XcFAhh1sjU7x6YOGeeEPYkFYEi9JKH6fEY1+ws57ovnbzpbvaC4+qf26S03p7ez XHMI2yWgVKx/55FcOcGhRLVwLuVDkznRXqojT1mw= 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 73E843857438 for ; Tue, 8 Aug 2023 14:14:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 73E843857438 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-137-AfHdbzAwNgmDpYOaeW2Y4g-1; Tue, 08 Aug 2023 10:14:35 -0400 X-MC-Unique: AfHdbzAwNgmDpYOaeW2Y4g-1 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-40feb0c08c0so45964441cf.3 for ; Tue, 08 Aug 2023 07:14:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691504074; x=1692108874; h=content-transfer-encoding:in-reply-to:from:references:cc: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=9uYpna20wNSjMgebW5hePUtIbg9YQXSjUt0cDaRVVCk=; b=Xnog2jhc8YwOJjDx8LxGMkTEZWQF71tvy+ZdQHph/qP9D+lI1nSqp8CVJZiZN7I9A0 1Cm/WaIjG8wScZGNEVSy5gTEe2utx2OK78sIsI9McC9IRBG+PBAfC4NhfxpENlcE9cwT /wrZ1qlfkANU7lK3YmHCZ5cUfjmZdl127966KKGo7hH7CGNtX71+K9strRoUahcFfo0d L/NQ0dnwim/xzAXATrbC8ZfRRBBFAlBfF4Ja0ZVFAnh+yHUs36ioUW8+3e1Jy0EZ8zk+ pSn56SkpepdpGjPsqz0z25ebseAh92H3vaDYwuD7MA6LsksD781I5MqKPYbKNmpD0anT Twwg== X-Gm-Message-State: AOJu0Yxkwxx35L/QnMvmPwwFAIhRkV3mZZ+Rk9y6uiJv8N31zm0I/TUz HdbAfUe4pPTezcPuVnlpk35GsghcVW7umxt9TlXyrctd5+q6U0fTDcoKmUboZSTWmdUip9h62iO GE+LGlm2pimIz7gquI7s87w== X-Received: by 2002:a05:622a:1043:b0:405:4eec:6365 with SMTP id f3-20020a05622a104300b004054eec6365mr13319974qte.53.1691504074198; Tue, 08 Aug 2023 07:14:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHRmavjHFRNBNdFramX+GuvZYHhhCvOync1MSsJTn2o/7SnRpPX6jXncSFCgYaGgU2bBnTJ7g== X-Received: by 2002:a05:622a:1043:b0:405:4eec:6365 with SMTP id f3-20020a05622a104300b004054eec6365mr13319918qte.53.1691504073345; Tue, 08 Aug 2023 07:14:33 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id t10-20020ac8760a000000b0040331f93ee0sm3392305qtq.77.2023.08.08.07.14.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 08 Aug 2023 07:14:33 -0700 (PDT) Message-ID: <4771b172-902b-5995-d32d-494e6d6f5341@redhat.com> Date: Tue, 8 Aug 2023 16:14:30 +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 2/2 ver 7] Fix reverse stepping multiple contiguous PC ranges over the line table. To: Carl Love , Simon Marchi , gdb-patches@sourceware.org, UlrichWeigand , pedro@palves.net Cc: luis.machado@arm.com References: <3a46ac42391707325ee2aab900d13bce05c8a0b8.camel@us.ibm.com> In-Reply-To: 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.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, 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.29 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 07/08/2023 21:03, Carl Love wrote: > > Bruno, Simon, GDB maintainers: Heads up, I go by Guinevere now, no longer Bruno :) Other than that, I have a very minor nit inlined, but  you can take or leave that one, honestly. Reviewed-By: Guinevere Larsen > > Version 7, addressed behavior of GDB when stepping backward thru a > function call as mentioned by Bruno. GDB would stop at the function > call and then a reverse step/next would cause GDB to stop at the first > instruction of the same line as the function call instead of stopping > in the previous line. The behavior was fixed and the various test > programs were updated to remove one of the two reverse step or next > instructions required to reach the previous line. > > Version 6, fixed various code style issues in the GDB source. The > testcases were updated to use with_test_prefix for each gdb test in the > step and next test cases, switch using the standard_testfile, use > foreach_with_prefix to combine otherwise identical tests. Retested on > Power 10. > > Version 5, changed comments in test case func-map-to-same-line.c. > Patch 1/2 implemented the new options for gdb_compile. Updated the > call to proc run_tests to use the new gdb_compile options in a > foreach_with_prefix loop. > > Version 4, additional fixes for gcc version check, wrap function calls > using "with_test_prefix", move load_lib dwarf.exe. Fixed typo noted by > Luis. > > Version 3, added the gcc version check as discussed further from > version 2 of the patch. Also updated the tests to check for supporting > reverse execution rather than requiring recording. I also noticed > there were a couple more instances of a requirement check, i.e. if [] > which I changed to "require" per the current style for checking on the > test requirements. > > > The following patch fixes issues on PowerPC with the reverse-step and > reverse-next instructions when there are multiple assignment statements > on the same line and when there are multiple function calls on the same > line. The commit log below discusses these issues in further depth. > The discussion included what the correct operation should be for these > commands based on the GDB documentation. The proposed patch at that > time changed how the commands worked on other platforms such as X86 in > a way they no longer matched the documentation. > > The issue is the line table contains multiple entries for the same > source line. The patch adds a function to search the line table to > find the address of the first instruction of a line. When setup up the > reverse stepping range, the function is called to make sure the start > of the range corresponds to the address of the first instruction for > the line. This approach was used. When Luis initially developed the > patch, he considered merging the contiguous ranges in the line table > when reading the line tables. He decided it was better to work with the > data directly in the line table rather than creating and using a > modified version of the line table. > > The following patch fixes the execution of the reveres-step and > reverse-next commands for both senarios of multiple statements on the > same line for PowerPC and aarch64-linux. Unlike the previous patch, it > does not change the operation of the commands on other platforms, i.e. > X86. The patch adds new test cases for both scenarios to verify they > work correctly. > > The patch has been tested on PowerPC, Intel X86 and aarch64-linux with > no new regression failures. > > Please let me know if the patch is acceptable for mainline. Thanks. > > Carl > > > > -------------------------------- > Fix reverse stepping multiple contiguous PC ranges over the line table. > > There are a couple of scenarios where the GDB reverse-step and reverse-next > commands do not work correctly. > > Scenario 1 issue description by Luis Machado: > > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on > the ppc backend), I noticed some failures in gdb.reverse/solib-precsave.exp > and gdb.reverse/solib-reverse.exp. > > The failure happens around the following code: > > 38 b[1] = shr2(17); /* middle part two */ > 40 b[0] = 6; b[1] = 9; /* generic statement, end part two */ > 42 shr1 ("message 1\n"); /* shr1 one */ > > Normal execution: > > - step from line 38 will land on line 40. > - step from line 40 will land on line 42. > > Reverse execution: > - step from line 42 will land on line 40. > - step from line 40 will land on line 40. > - step from line 40 will land on line 38. > > V > The problem here is that line 40 contains two contiguous but distinct > PC ranges in the line table, like so: > > Line 40 - [0x7ec ~ 0x7f4] > Line 40 - [0x7f4 ~ 0x7fc] > > The two distinct ranges are generated because GCC started outputting source > column information, which GDB doesn't take into account at the moment. > > When stepping forward from line 40, we skip both of these ranges and land on > line 42. When stepping backward from line 42, we stop at the start PC of the > second (or first, going backwards) range of line 40. > > Since we've reached ecs->event_thread->control.step_range_start, we stop > stepping backwards. > > --------------------------------------------------------- > > Scenario 2 issue described by Pedro Alves: > > The following explanation of the issue was taken from the gdb mailing list > discussion of the withdrawn patch to change the behavior of the reverse-step > and reverse-next commands. Specifically, message from Pedro Alves > where he demonstrates the issue where you have multiple > function calls on the same source code line: > > https://sourceware.org/pipermail/gdb-patches/2023-January/196110.html > > The source line looks like: > > func1 (); func2 (); > > so stepping backwards over that line should always stop at the first > instruction of the line, not in the middle. Let's simplify this. > > Here's the full source code of my example: > > (gdb) list 1 > 1 void func1 () > 2 { > 3 } > 4 > 5 void func2 () > 6 { > 7 } > 8 > 9 int main () > 10 { > 11 func1 (); func2 (); > 12 } > > Compiled with: > > $ gcc reverse.c -o reverse -g3 -O0 > $ gcc -v > ... > gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04) > > Now let's debug it with target record, using current gdb git master (f3d8ae90b236), > without your patch: > > $ gdb ~/reverse > GNU gdb (GDB) 14.0.50.20230124-git > ... > Reading symbols from /home/pedro/reverse... > (gdb) start > Temporary breakpoint 1 at 0x1147: file reverse.c, line 11. > Starting program: /home/pedro/reverse > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > > Temporary breakpoint 1, main () at reverse.c:11 > 11 func1 (); func2 (); > (gdb) record > > (gdb) disassemble /s > Dump of assembler code for function main: > reverse.c: > 10 { > 0x000055555555513f <+0>: endbr64 > 0x0000555555555143 <+4>: push %rbp > 0x0000555555555144 <+5>: mov %rsp,%rbp > > 11 func1 (); func2 (); > => 0x0000555555555147 <+8>: mov $0x0,%eax > 0x000055555555514c <+13>: call 0x555555555129 > 0x0000555555555151 <+18>: mov $0x0,%eax > 0x0000555555555156 <+23>: call 0x555555555134 > 0x000055555555515b <+28>: mov $0x0,%eax > > 12 } > 0x0000555555555160 <+33>: pop %rbp > 0x0000555555555161 <+34>: ret > End of assembler dump. > > (gdb) n > 12 } > > So far so good, a "next" stepped over the whole of line 11 and stopped at line 12. > > Let's confirm where we are now: > > (gdb) disassemble /s > Dump of assembler code for function main: > reverse.c: > 10 { > 0x000055555555513f <+0>: endbr64 > 0x0000555555555143 <+4>: push %rbp > 0x0000555555555144 <+5>: mov %rsp,%rbp > > 11 func1 (); func2 (); > 0x0000555555555147 <+8>: mov $0x0,%eax > 0x000055555555514c <+13>: call 0x555555555129 > 0x0000555555555151 <+18>: mov $0x0,%eax > 0x0000555555555156 <+23>: call 0x555555555134 > 0x000055555555515b <+28>: mov $0x0,%eax > > 12 } > => 0x0000555555555160 <+33>: pop %rbp > 0x0000555555555161 <+34>: ret > End of assembler dump. > > Good, we're at the first instruction of line 12. > > Now let's undo the "next", with "reverse-next": > > (gdb) reverse-next > 11 func1 (); func2 (); > > Seemingly stopped at line 11. Let's see exactly where: > > (gdb) disassemble /s > Dump of assembler code for function main: > reverse.c: > 10 { > 0x000055555555513f <+0>: endbr64 > 0x0000555555555143 <+4>: push %rbp > 0x0000555555555144 <+5>: mov %rsp,%rbp > > 11 func1 (); func2 (); > 0x0000555555555147 <+8>: mov $0x0,%eax > 0x000055555555514c <+13>: call 0x555555555129 > => 0x0000555555555151 <+18>: mov $0x0,%eax > 0x0000555555555156 <+23>: call 0x555555555134 > 0x000055555555515b <+28>: mov $0x0,%eax > > 12 } > 0x0000555555555160 <+33>: pop %rbp > 0x0000555555555161 <+34>: ret > End of assembler dump. > (gdb) > > And lo, we stopped in the middle of line 11! That is a bug, we should have > stepped back all the way to the beginning of the line. The "reverse-next" > should have fully undone the prior "next" command. > > The above issues were fixed by introducing a new function that looks for > adjacent PC ranges for the same line, until we notice a line change. Then > we take that as the start PC of the range. The new start PC for the range > is used for the control.step_range_start when setting up a step range. > > The test case gdb.reverse/map-to-same-line.exp is added to test the fix > for the issues in scenario 1. > > The test case gdb.reverse/func-map-to-same-line.exp was added to test the > fix for scenario 2 when the binary was compiled with and without line > table information. > > bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28426 > > Co-authored-by: Luis Machado > Co-authored-by: Carl Love > --- > gdb/infcmd.c | 13 ++ > gdb/infrun.c | 59 +++++++ > gdb/symtab.c | 49 ++++++ > gdb/symtab.h | 16 ++ > gdb/testsuite/gdb.mi/mi-reverse.exp | 5 +- > .../gdb.reverse/finish-reverse-next.exp | 42 ++--- > .../gdb.reverse/func-map-to-same-line.c | 37 +++++ > .../gdb.reverse/func-map-to-same-line.exp | 139 ++++++++++++++++ > gdb/testsuite/gdb.reverse/map-to-same-line.c | 58 +++++++ > .../gdb.reverse/map-to-same-line.exp | 153 ++++++++++++++++++ > 10 files changed, 537 insertions(+), 34 deletions(-) > create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.c > create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same-line.exp > create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.c > create mode 100644 gdb/testsuite/gdb.reverse/map-to-same-line.exp > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 15702f84894..add0eadd8c1 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -982,6 +982,19 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm) > &tp->control.step_range_start, > &tp->control.step_range_end); > > + if (execution_direction == EXEC_REVERSE) > + { > + symtab_and_line sal = find_pc_line (pc, 0); > + symtab_and_line sal_start > + = find_pc_line (tp->control.step_range_start, 0); > + > + if (sal.line == sal_start.line) > + /* Executing in reverse, the step_range_start address is in > + the same line. We want to stop in the previous line so > + move step_range_start before the current line. */ > + tp->control.step_range_start--; > + } > + > /* There's a problem in gcc (PR gcc/98780) that causes missing line > table entries, which results in a too large stepping range. > Use inlined_subroutine info to make the range more narrow. */ > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 8286026e6c6..32ba852f227 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -115,6 +115,9 @@ static struct async_event_handler *infrun_async_inferior_event_token; > Starts off as -1, indicating "never enabled/disabled". */ > static int infrun_is_async = -1; > > +static CORE_ADDR update_line_range_start (CORE_ADDR pc, > + struct execution_control_state *ecs); > + > /* See infrun.h. */ > > void > @@ -6884,6 +6887,27 @@ handle_signal_stop (struct execution_control_state *ecs) > process_event_stop_test (ecs); > } > > +/* Return the address for the beginning of the line. */ > + > +CORE_ADDR > +update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs) > +{ > + /* The line table may have multiple entries for the same source code line. > + Given the PC, check the line table and return the PC that corresponds > + to the line table entry for the source line that PC is in. */ > + CORE_ADDR start_line_pc = ecs->event_thread->control.step_range_start; > + gdb::optional real_range_start; > + > + /* Call find_line_range_start to get the smallest address in the > + linetable for multiple Line X entries in the line table. */ > + real_range_start = find_line_range_start (pc); > + > + if (real_range_start.has_value ()) > + start_line_pc = *real_range_start; > + > + return start_line_pc; > +} > + > /* Come here when we've got some debug event / signal we can explain > (IOW, not a random signal), and test whether it should cause a > stop, or whether we should resume the inferior (transparently). > @@ -7685,6 +7709,28 @@ process_event_stop_test (struct execution_control_state *ecs) > > if (stop_pc_sal.is_stmt) > { > + if (execution_direction == EXEC_REVERSE) > + { > + /* We are stepping backwards make sure we have reached the > + beginning of the line. */ > + CORE_ADDR stop_pc = ecs->event_thread->stop_pc (); > + CORE_ADDR start_line_pc > + = update_line_range_start (stop_pc, ecs); > + > + if (stop_pc != start_line_pc) > + { > + /* Have not reached the beginning of the source code line. > + Set a step range. Execution should stop in any function > + calls we execute back into before reaching the beginning > + of the line. */ > + ecs->event_thread->control.step_range_start = start_line_pc; > + ecs->event_thread->control.step_range_end = stop_pc; > + set_step_info (ecs->event_thread, frame, stop_pc_sal); > + keep_going (ecs); > + return; > + } > + } > + > /* We are at the start of a statement. > > So stop. Note that we don't stop if we step into the middle of a > @@ -7747,6 +7793,19 @@ process_event_stop_test (struct execution_control_state *ecs) > set_step_info (ecs->event_thread, frame, stop_pc_sal); > > infrun_debug_printf ("keep going"); > + > + if (execution_direction == EXEC_REVERSE) > + { > + CORE_ADDR stop_pc = ecs->event_thread->stop_pc (); > + > + /* Make sure the stop_pc is set to the beginning of the line. */ > + if (stop_pc != ecs->event_thread->control.step_range_start) > + { > + stop_pc = update_line_range_start (stop_pc, ecs); > + ecs->event_thread->control.step_range_start = stop_pc; pretty small nit, but I think it is better to just use ecs->event_thread->control.step_range_start     = update_line_range_start (stop_pc, ecs); Just because it is kind of weird, to see the step_range_start being set to stop pc, and made me do a double take :) -- Cheers, Guinevere Larsen She/Her/Hers