From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 756673858D28 for ; Tue, 2 May 2023 15:40:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 756673858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=us.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=us.ibm.com Received: from pps.filterd (m0353728.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 342FeAcX018766; Tue, 2 May 2023 15:40:21 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : from : to : cc : date : in-reply-to : references : content-type : content-transfer-encoding : mime-version : subject; s=pp1; bh=zJoy4zhHPdsbhmUMcrfHRJX8khKh6S0c4E0/2EUc85M=; b=OwHXNHakZyc9JMmtx0u1nznVRcTK5SuhqaSogLNnv0Hk2PnUKWz4jfRGmBGvznZJ4rtS s1irOMp+/uyV0DWeBj0mEA2fDmsyfEvcNZM0fnGLxuvh5wSNtj2fcesVEUujiV8qCM5U lFBJak9v9vGx5FU1PP8oq769iiptmN75GA39hgWcw8sY6SezQQFKlb7fDtDrlTMt+3kM Dm+VM5pIMtwfbKvWqe6548VloEkJ1l1V0hI1rBctmYra0wx8xHsGMsifijKyINRtm4hn yULjdjLxzm+ikmunETq1LcZXzuJW+X6ojF6PCx2qYLoVb31Zll1RT4u0vmFLvtPOH0Z6 Qg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3qb54ar9jx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 02 May 2023 15:40:20 +0000 Received: from m0353728.ppops.net (m0353728.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 342FeJ1n019238; Tue, 2 May 2023 15:40:19 GMT Received: from ppma01wdc.us.ibm.com (fd.55.37a9.ip4.static.sl-reverse.com [169.55.85.253]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3qb54ar9hy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 02 May 2023 15:40:19 +0000 Received: from pps.filterd (ppma01wdc.us.ibm.com [127.0.0.1]) by ppma01wdc.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 342FHqI3011904; Tue, 2 May 2023 15:40:18 GMT Received: from smtprelay02.dal12v.mail.ibm.com ([9.208.130.97]) by ppma01wdc.us.ibm.com (PPS) with ESMTPS id 3q8tv7r18y-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 02 May 2023 15:40:18 +0000 Received: from smtpav01.wdc07v.mail.ibm.com (smtpav01.wdc07v.mail.ibm.com [10.39.53.228]) by smtprelay02.dal12v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 342FeHhS25887148 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 2 May 2023 15:40:17 GMT Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8457058066; Tue, 2 May 2023 15:40:17 +0000 (GMT) Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id DD0C55804B; Tue, 2 May 2023 15:40:16 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.163.52.55]) by smtpav01.wdc07v.mail.ibm.com (Postfix) with ESMTP; Tue, 2 May 2023 15:40:16 +0000 (GMT) Message-ID: <4a03c3208d2a6505273753fa47ee1aefc5536408.camel@us.ibm.com> From: Carl Love To: Bruno Larsen , gdb-patches@sourceware.org, Ulrich Weigand , pedro@palves.net Cc: luis.machado@arm.com Date: Tue, 02 May 2023 08:40:16 -0700 In-Reply-To: <0a4e5ac7-7c2c-2d9c-813e-bdc5ba46bbf1@redhat.com> References: <74630f1ccb6e9258ae60682105ee5490726fb255.camel@us.ibm.com> <0a4e5ac7-7c2c-2d9c-813e-bdc5ba46bbf1@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-18.el8) X-TM-AS-GCONF: 00 X-Proofpoint-GUID: weYWBHzzPkSWmZaRFMvwLTj0EPlWtdN8 X-Proofpoint-ORIG-GUID: JLX_wnEDM6SKtz-vLMMn_XYv7qXLSRvj Content-Transfer-Encoding: 7bit X-Proofpoint-UnRewURL: 5 URL's were un-rewritten MIME-Version: 1.0 Subject: RE: [PATCH] Fix reverse stepping multiple contiguous PC ranges over the line table. X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-05-02_10,2023-04-27_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 priorityscore=1501 bulkscore=0 mlxscore=0 impostorscore=0 phishscore=0 adultscore=0 lowpriorityscore=0 malwarescore=0 clxscore=1015 mlxlogscore=999 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2305020133 X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,KAM_ASCII_DIVIDERS,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Bruno: On Tue, 2023-05-02 at 16:15 +0200, Bruno Larsen wrote: I had intended everything from here down as part of the commit message. It is admittedly a lot. It could be cut down my making the first part could be moved to the mailing list context. The commit message would then start with the two scenarios the patch fixes. > > > --------------------------------------------------------------- > > 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. The first scenario consists of > > multiple > > assignment statements on the same line. A patch was proposed to > > address the > > issue by Luis Machado and briefly discussed on the mailing list in > > Feb 2021. > > > > https://sourceware.org/pipermail/gdb-patches/2021-February/175678.html > > > > The discussion was revived by Carl Love with regards to fixing the > > same > > issue on PowerPC. > > > > https://sourceware.org/pipermail/gdb-patches/2022-March/186463.html > > > > The patch was not completed and has been on Carl's to do list for > > some time. > > > > Discussion of a patch to change how the reverse-step and reverse- > > next > > commands submitted by Carl Love was started in thread: > > > > https://sourceware.org/pipermail/gdb-patches/2023-January/195563.html > > > > The patch was withdrawn as it was pointed out the proposed patch > > would > > change the intended behavior of the commands as documented in the > > GDB > > manual. However, it was pointed out by Pedro Alves < > > pedro@palves.net> > > that the reverse-step and reverse-next commands do not work when > > there > > are multiple function calls on the same line. This is a second > > scenario > > where the commands do not work correctly. > > > > The following patch is an extended version of the original patch by > > Luis Machado to fix the issues in scenario 1 to also address the > > issues in > > scenario 2. > > Move the above to the mailing list Start here with the commit message > > -------------------------------------------------------- > > > > 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. > > > > 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. > > > > This happens because we have this check in > > infrun.c:process_event_stop_test: > > > > /* When stepping backward, stop at beginning of line range > > (unless it's the function entry point, in which case > > keep going back to the call point). */ > > CORE_ADDR stop_pc = ecs->event_thread->stop_pc (); > > if (stop_pc == ecs->event_thread->control.step_range_start > > && stop_pc != ecs->stop_func_start > > && execution_direction == EXEC_REVERSE) > > end_stepping_range (ecs); > > else > > keep_going (ecs); > > > > Since we've reached ecs->event_thread->control.step_range_start, we > > stop > > stepping backwards. > > > > The right thing to do is to look 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. > > > > Another solution I thought about is to merge the contiguous ranges > > when > > we are reading the line tables. Though I'm not sure if we really > > want to > > process that data as opposed to keeping it as the compiler created, > > and > > then working around that. > > > > The test case gdb.reverse/map-to-same-line.exp is added to test the > > fix > > for the issues in scenario 1. > > > > --------------------------------------------------------- > > > > 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 test cases gdb.reverse/func-map-to-same-line-no-colum-info.exp > > and > > gdb.reverse/func-map-to-same-line.exp were 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 End of commit message > > Hey Carl, > > Thanks for working on this. I'm wondering which parts will be part > of > the final commit messages and which is just context for the mailing > list, so some clarity would be nice, but that is not a huge deal. > > I wanted to test this change, but it doesn't apply anymore on > master, > and `git apply --3way` can't figure out how to do it. Which commit > did > you use as base (or alternatively, can you rebase it)? OK, let me rebase it and repost. Let me know if you think the suggested commit message is still too much. Thanks. Carl >