From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id DF1A63858D28 for ; Thu, 4 May 2023 02:55:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DF1A63858D28 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 (m0353724.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3442e99X000776; Thu, 4 May 2023 02:55:31 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=7R6B87oPziY3suypU46c8wF44xFEEA3r63JX09P7f8s=; b=TBajfPs4D6ld9irjRuasnlDeV9qUaKt7bpEH4olmG8pksPY3RloxC7ZXVXYA4ecN7MTX gIEXu+Eg29726a3Jm+KcrT1xm8FFNIwxrW2skM/DU3tmkMlLrJGy39UaQJcPgHaHli+9 CrXnWE6P4yPdtfIicUVVy+82b/xPjllMjecrWfopefYZRybk928R/nIhXUD7toHiJDHv Pn4B9w61pj2KWG9cOHad5dx5seXZ79LFVH9iypnpEs53Ock2S6DFfjw9CuRkVrxgf+A6 X3lHCpdyY3AkMLRWFLKy8Vsc7JjhlFKfjvVJlLTAzK0WICIv4i7hN8+vEUOtxig+JJ3i Xw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3qc3u70gje-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 04 May 2023 02:55:30 +0000 Received: from m0353724.ppops.net (m0353724.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 3442pYah031355; Thu, 4 May 2023 02:55:29 GMT Received: from ppma04dal.us.ibm.com (7a.29.35a9.ip4.static.sl-reverse.com [169.53.41.122]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3qc3u70gj4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 04 May 2023 02:55:29 +0000 Received: from pps.filterd (ppma04dal.us.ibm.com [127.0.0.1]) by ppma04dal.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 3442OtD7008631; Thu, 4 May 2023 02:55:29 GMT Received: from smtprelay06.wdc07v.mail.ibm.com ([9.208.129.118]) by ppma04dal.us.ibm.com (PPS) with ESMTPS id 3q8tv8d882-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 04 May 2023 02:55:28 +0000 Received: from smtpav06.wdc07v.mail.ibm.com (smtpav06.wdc07v.mail.ibm.com [10.39.53.233]) by smtprelay06.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 3442tRVG7340598 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 4 May 2023 02:55:27 GMT Received: from smtpav06.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5497A5804E; Thu, 4 May 2023 02:55:27 +0000 (GMT) Received: from smtpav06.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 865B95803F; Thu, 4 May 2023 02:55:26 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.163.52.55]) by smtpav06.wdc07v.mail.ibm.com (Postfix) with ESMTP; Thu, 4 May 2023 02:55:26 +0000 (GMT) Message-ID: <7c596ccfc69b237a6094ead018f4a0b38b82a632.camel@us.ibm.com> From: Carl Love To: Bruno Larsen , gdb-patches@sourceware.org, Ulrich Weigand , pedro@palves.net Cc: luis.machado@arm.com, cel@us.ibm.com Date: Wed, 03 May 2023 19:55:25 -0700 In-Reply-To: <46d73c69-9168-44c6-b515-23dd893fc0eb@redhat.com> References: <74630f1ccb6e9258ae60682105ee5490726fb255.camel@us.ibm.com> <46d73c69-9168-44c6-b515-23dd893fc0eb@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-ORIG-GUID: amtoLl7XDLucwQ96LGDzt4if_oJ-S_7A X-Proofpoint-GUID: o8zH4fKeOkZkvUijljLylqvh3JG09WQy Content-Transfer-Encoding: 8bit X-Proofpoint-UnRewURL: 8 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-03_16,2023-05-03_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 mlxlogscore=999 adultscore=0 malwarescore=0 bulkscore=0 phishscore=0 lowpriorityscore=0 impostorscore=0 mlxscore=0 priorityscore=1501 spamscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2305040019 X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Bruno: On Wed, 2023-05-03 at 11:53 +0200, Bruno Larsen wrote: > On 27/04/2023 22:59, Carl Love wrote: > > Hi Carl, thanks for clarifying the intended commit message. I'm > reacting > to it here because I also have some thoughts on the code, now that I > managed to apply it locally. > > Starting on the commit message, it would be nice to have a 1-line > description of the problem before describing the scenarios in depth. > Taking the first line of the previous block is enough IMO. Yes, agreed. Kept the first line before the discussion of the different failure scenarios. > > > 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. > I think these last 3 paragraphs should be moved. I like to finish > commits with a description of the solution, rather than having it in > the > middle of the text. Also, I think we like to avoid mentioning > explicit > code in the commit text (though I might be mistaken). OK, I moved the fix discussion to the end. I also dropped the explicit reference to infrun.c:process_event_stop_test. > > 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. > This paragraph doesn't need to be here in the final commit message > IMO. > It was nice context for the mailing list but is not necessary for > future OK, removed it from the commit log and need to update the mailing list message with this. > reference, I don't think. > > 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 > > --- > > gdb/infrun.c | 57 +++++++ > > gdb/symtab.c | 49 ++++++ > > gdb/symtab.h | 16 ++ > > .../func-map-to-same-line-no-column-info.exp | 135 > > ++++++++++++++++ > > .../gdb.reverse/func-map-to-same-line.c | 36 +++++ > > .../gdb.reverse/func-map-to-same-line.exp | 123 > > ++++++++++++++ > > gdb/testsuite/gdb.reverse/map-to-same-line.c | 58 +++++++ > > .../gdb.reverse/map-to-same-line.exp | 153 > > ++++++++++++++++++ > > 8 files changed, 627 insertions(+) > > create mode 100644 gdb/testsuite/gdb.reverse/func-map-to-same- > > line-no-column-info.exp > > 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/infrun.c b/gdb/infrun.c > > index 2f1c6cd694b..59374a05471 100644 > > --- a/gdb/infrun.c > > +++ b/gdb/infrun.c > > @@ -113,6 +113,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 > > @@ -6768,6 +6771,25 @@ handle_signal_stop (struct > > execution_control_state *ecs) > > process_event_stop_test (ecs); > > } > > > > +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 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). > > @@ -7569,6 +7591,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 > > @@ -7631,6 +7675,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; > > + } > > + } > > + > > keep_going (ecs); > > } > > > > diff --git a/gdb/symtab.c b/gdb/symtab.c > > index 27611a34ec4..91d35616eb9 100644 > > --- a/gdb/symtab.c > > +++ b/gdb/symtab.c > > @@ -3282,6 +3282,55 @@ find_pc_line (CORE_ADDR pc, int notcurrent) > > return sal; > > } > > > > +/* Compare two symtab_and_line entries. Return true if both have > > + the same line number and the same symtab pointer. That means > > we > > + are dealing with two entries from the same line and from the > > same > > + source file. > > + > > + Return false otherwise. */ > > + > > +static bool > > +sal_line_symtab_matches_p (const symtab_and_line &sal1, > > + const symtab_and_line &sal2) > > +{ > > + return (sal1.line == sal2.line && sal1.symtab == sal2.symtab); > > +} > > + > > +/* See symtah.h. */ > > + > > +gdb::optional > > +find_line_range_start (CORE_ADDR pc) > > +{ > > + struct symtab_and_line current_sal = find_pc_line (pc, 0); > > + > > + if (current_sal.line == 0) > > + return {}; > > + > > + struct symtab_and_line prev_sal = find_pc_line (current_sal.pc - > > 1, 0); > > + > > + /* If the previous entry is for a different line, that means we > > are already > > + at the entry with the start PC for this line. */ > > + if (!sal_line_symtab_matches_p (prev_sal, current_sal)) > > + return current_sal.pc; > > + > > + /* Otherwise, keep looking for entries for the same line but > > with > > + smaller PC's. */ > > + bool done = false; > > + CORE_ADDR prev_pc; > > + while (!done) > > + { > > + prev_pc = prev_sal.pc; > > + > > + prev_sal = find_pc_line (prev_pc - 1, 0); > > + > > + /* Did we notice a line change? If so, we are done with the > > search. */ > > + if (!sal_line_symtab_matches_p (prev_sal, current_sal)) > > + done = true; > > + } > > + > > + return prev_pc; > > +} > > + > > /* See symtab.h. */ > > > > struct symtab * > > diff --git a/gdb/symtab.h b/gdb/symtab.h > > index 404d0ab30a8..f54305636da 100644 > > --- a/gdb/symtab.h > > +++ b/gdb/symtab.h > > @@ -2346,6 +2346,22 @@ extern struct symtab_and_line find_pc_line > > (CORE_ADDR, int); > > extern struct symtab_and_line find_pc_sect_line (CORE_ADDR, > > struct obj_section *, > > int); > > > > +/* Given PC, and assuming it is part of a range of addresses that > > is part of a > > + line, go back through the linetable and find the starting PC of > > that > > + line. > > + > > + For example, suppose we have 3 PC ranges for line X: > > + > > + Line X - [0x0 - 0x8] > > + Line X - [0x8 - 0x10] > > + Line X - [0x10 - 0x18] > > + > > + If we call the function with PC == 0x14, we want to return 0x0, > > as that is > > + the starting PC of line X, and the ranges are contiguous. > > +*/ > > + > > +extern gdb::optional find_line_range_start (CORE_ADDR > > pc); > > + > > /* Wrapper around find_pc_line to just return the symtab. */ > > > > extern struct symtab *find_pc_line_symtab (CORE_ADDR); > > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line-no- > > column-info.exp b/gdb/testsuite/gdb.reverse/func-map-to-same-line- > > no-column-info.exp > > new file mode 100644 > > index 00000000000..20529c90fc2 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line-no-column- > > info.exp > > @@ -0,0 +1,135 @@ > > +# 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 < > > 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". > > + > > +# This test checks to make sure there is no regression failures > > for > > +# the reverse-next command when stepping back over two functions > > in > > +# the same line. > > + > > +if ![supports_reverse] { > > + return > > +} > Nowadays you should use require instead of the if clause, like in > gdb.reverse/break-reverse.exp OK, changed that > > + > > +# This test uses the gcc no-column-info command. > Correct me if I'm wrong, but it seems to me that the other test is a > more generic version of this one, so this test could check for a gcc > recent enough to support this feature, instead of just generically > gcc. > That said, gcc added it on version > 7( > https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0029b929c9719a > ), is it > old enough that we don't care? GCC supports line tables, I don't know that clang or other compilers do. So all we really need is to check for gcc. The line table stuff was added a long time ago so not sure that we really need to check for version 7 at this point. So just checked that we are using gcc. The "other test" func-map-to-same-line.exp expects the line table so it should probably also be checking that we are using gcc. > > +if ![is_c_compiler_gcc] { > > + unsupported "gcc is required for this test" > > + return 0 > > +} > > + > > +set srcfile func-map-to-same-line.c > > +set executable func-map-to-same-line > > + > > +set options [list debug additional_flags=-gno-column-info] > > + > > +if {[build_executable "failed to prepare" $executable $srcfile > > $options] == -1}\ > > + { > > + return -1 > > +} > > + > > +clean_restart $executable > > + > > +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" > > +} > > + > > +# This regression test verifies the reverse-step and reverse-next > > commands > > +# work properly when executing backwards thru a source line > > containing > > +# two function calls on the same source line, i.e. func1 (); func2 > > (); > > +# This test is compiled so the dwarf info not contain the line > > table > > +# information. > > + > > +# Test 1, reverse-next command > > +# Set breakpoint at the line after the function calls. > > +set bp_start_reverse_test [gdb_get_line_number "START REVERSE > > TEST" $srcfile] > > +gdb_breakpoint $srcfile:$bp_start_reverse_test temporary > > + > > +# Continue to break point for reverse-next test. > > +# Command definition: reverse-next [count] > > +# Run backward to the beginning of the previous line executed in > > the current > > +# (innermost) stack frame. If the line contains function calls, > > they will be > > +# “un-executed” without stopping. Starting from the first line > > of a function, > > +# reverse-next will take you back to the caller of that > > function, before the > > +# function was called, just as the normal next command would > > take you from > > +# the last line of a function back to its return to its caller 2 > > . > > + > > +gdb_continue_to_breakpoint \ > > + "stopped at command reverse-next test start location" \ > > + ".*$srcfile:$bp_start_reverse_test\r\n.*" > > + > > +# The reverse-next should step all the way back to the beginning > > of the line, > > +# i.e. at the beginning of the func1 call. > > +gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \ > > + "reverse-next to line with two functions" > > + > > +# A reverse-step should step back and stop at the beginning > > +# of the previous line b = 2, i.e. not in func1 (). > > +gdb_test "reverse-step" ".*b = 2;.*" \ > > + "reverse-step to previous line b = 2" > The point of this test is to confirm that we are at the very first > instruction of the line, right? So I would think it is better to do > a > reverse-stepi here, to make sure that even walking a single > instruction > we reach a different line. Yes, we should be on the first instruction of the line so stepi would be a better test to prove we are really on the first instruction. Changed the reverse-step to reverse-stepi. > Either that or doing what Pedro did in his > email: save the PC before executing the line, then do and undo the > line > and confirm that PCs match exactly. > > + > > + > > +# Setup for test 2 > > +# Go back to the start of the function > > +gdb_test "reverse-continue" "a = 1;" "At start of main, setup for > > test 2" > > + > > +# Turn off record to clear logs and turn on again > > +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" > Since you don't require process record for this test, you can't > assume > these to work. I think its better to clean restart and record if the > process supports recording, this way you're sure to reset history no > matter the inferior. No, the test requires process record. If record is not supported, we can't do reverse execution. That said, doing a "clean restart" and then record would be another way, probably better way, of clearing the history. Put in a clean restart rather than turning off/on the recording to clear the log file. > > + > > +# Delete all breakpoints and catchpoints. > > +delete_breakpoints > > + > > + > > +# Test 2, reverse-step command > > +# Set breakpoint at the line after the function calls. > > +gdb_breakpoint $srcfile:$bp_start_reverse_test temporary > > + > > +# Continue to the start of the reverse-step test. > > +# Command definition: reverse-step [count] > > +# Run the program backward until control reaches the start of a > > +# different source line; then stop it, and return control to > > gdb. > > +# Like the step command, reverse-step will only stop at the > > beginning of a > > +# source line. It “un-executes” the previously executed source > > line. If the > > +# previous source line included calls to debuggable functions, > > reverse-step > > +# will step (backward) into the called function, stopping at > > the beginning > > +# of the last statement in the called function (typically a > > return > > +# statement). Also, as with the step command, if non- > > debuggable functions > > +# are called, reverse-step will run thru them backward without > > stopping. > > + > > +gdb_continue_to_breakpoint \ > > + "stopped at command reverse-step test start location" \ > > + ".*$srcfile:$bp_start_reverse_test\r\n.*" > > + > > +# The first reverse step should take us call of func2 (). > > +gdb_test "reverse-step" ".*}.*" \ > > + "reverse-step into func2 " > > + > > +# The second reverse step should take us into func1 (). > > +gdb_test "reverse-step" ".*}.*" \ > > + "reverse-step into func1 " > > + > > +# The third reverse step should take us call of func1 (). > > +gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \ > > + "reverse-step to line func1(); func2(), at call for func1 " > > + > > +# The fourth reverse step should take us to b = 2 (). > > +gdb_test "reverse-step" ".*b = 2;.*" \ > > + "reverse-step to line b = 2 " > Ditto from the other test like this. Yes, stepi is a better test to prove you were on the first instruction in the line. Changed. > Also, I feel that, while the test > name for the last 2 gdb_test are different, they don't meaningfully > communicate which part of the test is failing. I think it would be > better if you differentiated them by adding "for step test" or "for > test > 2" at the end of the name would make it easier to understand where > things went wrong when looking at the sum file. OK, agreed. For the above tests I added "test#," where # is either 1 or 2 to the test names. For example: +gdb_test "reverse-step" ".*b = 2;.*" \ + "reverse-step to line b = 2 " was changed to +gdb_test "reverse-stepi" ".*b = 2;.*" \ + "test2, reverse-step to line b = 2 " I also updated the test to identify the closing } for func1 and func2 to make it clearer in the test which function we just steped back into. > > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.c > > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c > > new file mode 100644 > > index 00000000000..e9787ef9ff5 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c > > @@ -0,0 +1,36 @@ > > +/* 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 < > > http://www.gnu.org/licenses/ > > >. > > + > > + This test is used to test the reverse-step and reverse-next > > instruction > > + execution for a source line that contains multiple function > > calls. */ > > + > > +void > > +func1 () > > +{ > > +} > > + > > +void > > +func2 () > > +{ > > +} > > + > > +int main () > > +{ > > + int a, b; > > + a = 1; > > + b = 2; > > + func1 (); func2 (); > > + a = a + b; // START REVERSE TEST > > +} > > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp > > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp > > new file mode 100644 > > index 00000000000..b632a236bbe > > --- /dev/null > > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.exp > > @@ -0,0 +1,123 @@ > > +# 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 < > > 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". > > + > > +# This test checks to make sure there is no regression failures > > for > > +# the reverse-next command when stepping back over two functions > > in > > +# the same line. > > + > > +if ![supports_reverse] { > > + return > > +} > > I'm not sure it's worth separating these 2 tests into separate > files. > You could instead just have most of the test defined as a proc, and > call > it twice, once after compiling the inferior with column info, the > other > compiling without if gcc is used. This way it's less likely that the > tests will diverge over time. OK, good point. I created a test proc which is then called with the binary compiled with and without line information. The extra exp test file was removed. > > > + > > +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" > > +} > > + > > +# This regression test verifies the reverse-step and reverse-next > > commands > > +# work properly when executing backwards thru a source line > > containing > > +# two function calls on the same source line, i.e. func1 (); func2 > > (); > > +# The assumption for this test is the dwarf info contain the > > column > > +# information. > > + > > +# Test 1, reverse-next command > > +# Set breakpoint at the line after the function calls. > > +set bp_start_reverse_test [gdb_get_line_number "START REVERSE > > TEST" $srcfile] > > +gdb_breakpoint $srcfile:$bp_start_reverse_test temporary > > + > > +# Continue to break point for reverse-next test. > > +# Command definition: reverse-next [count] > > +# Run backward to the beginning of the previous line executed in > > the current > > +# (innermost) stack frame. If the line contains function calls, > > they will be > > +# “un-executed” without stopping. Starting from the first line > > of a function, > > +# reverse-next will take you back to the caller of that > > function, before the > > +# function was called, just as the normal next command would > > take you from > > +# the last line of a function back to its return to its caller 2 > > . > > + > > +gdb_continue_to_breakpoint \ > > + "stopped at command reverse-next test start location" \ > > + ".*$srcfile:$bp_start_reverse_test\r\n.*" > > + > > +# The reverse-next should step all the way back to the beginning > > of the line, > > +# i.e. at the beginning of the func1 call. > > +gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \ > > + "reverse-next to line with two functions" > > + > > +# A reverse-step should step back and stop at the beginning > > +# of the previous line b = 2, i.e. not in func1 (). > > +gdb_test "reverse-step" ".*b = 2;.*" \ > > + "reverse-step to previous line b = 2" > > + > > + > > +# Setup for test 2 > > +# Go back to the start of the function > > +gdb_test "reverse-continue" "a = 1;" "At start of main, setup for > > test 2" > > + > > +# Turn off record to clear logs and turn on again > > +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" > > + > > +# Delete all breakpoints and catchpoints. > > +delete_breakpoints > > + > > + > > +# Test 2, reverse-step command > > +# Set breakpoint at the line after the function calls. > > +gdb_breakpoint $srcfile:$bp_start_reverse_test temporary > > + > > +# Continue to the start of the reverse-step test. > > +# Command definition: reverse-step [count] > > +# Run the program backward until control reaches the start of a > > +# different source line; then stop it, and return control to > > gdb. > > +# Like the step command, reverse-step will only stop at the > > beginning of a > > +# source line. It “un-executes” the previously executed source > > line. If the > > +# previous source line included calls to debuggable functions, > > reverse-step > > +# will step (backward) into the called function, stopping at > > the beginning > > +# of the last statement in the called function (typically a > > return > > +# statement). Also, as with the step command, if non- > > debuggable functions > > +# are called, reverse-step will run thru them backward without > > stopping. > > + > > +gdb_continue_to_breakpoint \ > > + "stopped at command reverse-step test start location" \ > > + ".*$srcfile:$bp_start_reverse_test\r\n.*" > > + > > +# The first reverse step should take us call of func2 (). > > +gdb_test "reverse-step" ".*}.*" \ > > + "reverse-step into func2 " > > + > > +# The second reverse step should take us into func1 (). > > +gdb_test "reverse-step" ".*}.*" \ > > + "reverse-step into func1 " > > + > > +# The third reverse step should take us call of func1 (). > > +gdb_test "reverse-step" ".*func1 \\(\\); func2 \\(\\);.*" \ > > + "reverse-step to line func1(); func2(), at call for func1 " > > + > > +# The fourth reverse step should take us to b = 2 (). > > +gdb_test "reverse-step" ".*b = 2;.*" \ > > + "reverse-step to line b = 2 " > > diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.c > > b/gdb/testsuite/gdb.reverse/map-to-same-line.c > > new file mode 100644 > > index 00000000000..f20d778f40e > > --- /dev/null > > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c > > @@ -0,0 +1,58 @@ > > +/* 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 < > > http://www.gnu.org/licenses/ > > >. */ > > + > > +/* The purpose of this test is to create a DWARF line table that > > contains two > > + or more entries for the same line. When stepping (forwards or > > backwards), > > + GDB should step over the entire line and not just a particular > > entry in the > > + line table. */ > > + > > +int > > +main () > > +{ /* TAG: main prologue */ > > + asm ("main_label: .globl main_label"); > > + int i = 1, j = 2, k; > > + float f1 = 2.0, f2 = 4.1, f3; > > + const char *str_1 = "foo", *str_2 = "bar", *str_3; > > + > > + asm ("line1: .globl line1"); > > + k = i; f3 = f1; str_3 = str_1; /* TAG: line 1 */ > > + > > + asm ("line2: .globl line2"); > > + k = j; f3 = f2; str_3 = str_2; /* TAG: line 2 */ > > + > > + asm ("line3: .globl line3"); > > + k = i; f3 = f1; str_3 = str_1; /* TAG: line 3 */ > > + > > + asm ("line4: .globl line4"); > > + k = j; f3 = f2; str_3 = str_2; /* TAG: line 4 */ > > + > > + asm ("line5: .globl line5"); > > + k = i; f3 = f1; str_3 = str_1; /* TAG: line 5 */ > > + > > + asm ("line6: .globl line6"); > > + k = j; f3 = f2; str_3 = str_2; /* TAG: line 6 */ > > + > > + asm ("line7: .globl line7"); > > + k = i; f3 = f1; str_3 = str_1; /* TAG: line 7 */ > > + > > + asm ("line8: .globl line8"); > > + k = j; f3 = f2; str_3 = str_2; /* TAG: line 8 */ > > + > > + asm ("main_return: .globl main_return"); > > + k = j; f3 = f2; str_3 = str_2; /* TAG: main return */ > > + > > + asm ("end_of_sequence: .globl end_of_sequence"); > > + return 0; /* TAG: main return */ > > +} > > diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp > > b/gdb/testsuite/gdb.reverse/map-to-same-line.exp > > new file mode 100644 > > index 00000000000..a01579c2a8d > > --- /dev/null > > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp > > @@ -0,0 +1,153 @@ > > +# 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 < > > http://www.gnu.org/licenses/ > > = >. > > + > > +# When stepping (forwards or backwards), GDB should step over the > > entire line > > +# and not just a particular entry in the line table. This test was > > added to > > +# verify the find_line_range_start function properly sets the step > > range for a > > +# line that consists of multiple statements, i.e. multiple entries > > in the line > > +# table. This test creates a DWARF line table that contains two > > entries for > > +# the same line to do the needed testing. > > + > > +load_lib dwarf.exp > > + > > +# This test can only be run on targets which support DWARF-2 and > > use gas. > > +if {![dwarf2_support]} { > > + unsupported "dwarf2 support required for this test" > > + return 0 > > +} > Again, the new way to check for these is "required". And IIUC, you > can > add multiple requirements into a singe require call. OK, updated. > > + > > +if [get_compiler_info] { > > + return -1 > > +} > > + > > +# The DWARF assembler requires the gcc compiler. > > +if ![is_c_compiler_gcc] { > > + unsupported "gcc is required for this test" > > + return 0 > > +} > > + > > +# This test suitable only for process record-replay > > +if ![supports_process_record] { > > + return > > +} > > + > > +standard_testfile .c .S > > + > > +if { [prepare_for_testing "failed to prepare" ${testfile} > > ${srcfile}] } { > > + return -1 > > +} > > + > > +set asm_file [standard_output_file $srcfile2] > > +Dwarf::assemble $asm_file { > > + global srcdir subdir srcfile > > + declare_labels integer_label L > > + > > + # Find start address and length of program > > + lassign [function_range main [list > > ${srcdir}/${subdir}/$srcfile]] \ > > + main_start main_len > > + set main_end "$main_start + $main_len" > > + > > + cu {} { > > + compile_unit { > > + {language @DW_LANG_C} > > + {name map-to-same-line.c} > > + {stmt_list $L DW_FORM_sec_offset} > > + {low_pc 0 addr} > > + } { > > + subprogram { > > + {external 1 flag} > > + {name main} > > + {low_pc $main_start addr} > > + {high_pc $main_len DW_FORM_data4} > > + } > > + } > > + } > > + > > + lines {version 2 default_is_stmt 1} L { > > + include_dir "${srcdir}/${subdir}" > > + file_name "$srcfile" 1 > > + > > + # Generate the line table program with distinct source lines > > being > > + # mapped to the same line entry. Line 1, 5 and 8 contain 1 > > statement > > + # each. Line 2 contains 2 statements. Line 3 contains 3 > > statements. > > + program { > > + DW_LNE_set_address $main_start > > + line [gdb_get_line_number "TAG: main prologue"] > > + DW_LNS_copy > > + DW_LNE_set_address line1 > > + line [gdb_get_line_number "TAG: line 1" ] > > + DW_LNS_copy > > + DW_LNE_set_address line2 > > + line [gdb_get_line_number "TAG: line 2" ] > > + DW_LNS_copy > > + DW_LNE_set_address line3 > > + line [gdb_get_line_number "TAG: line 2" ] > > + DW_LNS_copy > > + DW_LNE_set_address line4 > > + line [gdb_get_line_number "TAG: line 3" ] > > + DW_LNS_copy > > + DW_LNE_set_address line5 > > + line [gdb_get_line_number "TAG: line 3" ] > > + DW_LNS_copy > > + DW_LNE_set_address line6 > > + line [gdb_get_line_number "TAG: line 3" ] > > + DW_LNS_copy > > + DW_LNE_set_address line7 > > + line [gdb_get_line_number "TAG: line 5" ] > > + DW_LNS_copy > > + DW_LNE_set_address line8 > > + line [gdb_get_line_number "TAG: line 8" ] > > + DW_LNS_copy > > + DW_LNE_set_address main_return > > + line [gdb_get_line_number "TAG: main return"] > > + DW_LNS_copy > > + DW_LNE_set_address end_of_sequence > > + DW_LNE_end_sequence > > + } > > + } > > +} > > + > > +if { [prepare_for_testing "failed to prepare" ${testfile} \ > > + [list $srcfile $asm_file] {nodebug} ] } { > > + return -1 > > +} > > + > > +if ![runto_main] { > > + return -1 > > +} > runto_main already errors out and leaves, I think, so no need for the > if > clause. OK, updated. > > + > > +# Print the line table > > +gdb_test_multiple "maint info line-table ${testfile}" "" { > > + -re "\r\n$decimal\[ \t\]+$decimal\[ \t\]+($hex)\[ > > \t\]+Y\[^\r\n\]*" { > > + lappend is_stmt $expect_out(1,string) > > + exp_continue > > + } > > + -re -wrap "" { > > + } > > +} > > + > > +# Activate process record/replay > > +gdb_test_no_output "record" "turn on process record" > > + > > +gdb_test "tbreak main_return" "Temporary breakpoint .*" > > "breakpoint at return" > you can set a temporary breakpoint using gdb_breakpoint "..." > temporary, > no need to manually call tbreak. > > +gdb_test "continue" "Temporary breakpoint .*" "run to end of main" > gdb_continue_to_breakpoint can handle temporary breakpoints as well. OK, updated the break and continue statements. > > +gdb_test "display \$pc" ".*pc =.*" "display pc" > > + > > +# At this point, GDB has already recorded the execution up until > > the return > > +# statement. Reverse-step and test if GDB transitions between > > lines in the > > +# expected order. It should reverse-step across lines 8, 5, 3, 2 > > and 1. > > +foreach line {8 5 3 2 1} { > > + gdb_test "reverse-step" ".*TAG: line $line.*" "reverse step to > > line $line" > > +} > > I'm not sure if it is needed, but I don't think it would hurt to > also > test reverse-next in a separate foreach right after this one. OK, added a clean restart, run to the end of main then do reverse next using a foreach line. Carl