From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 340103858C74 for ; Tue, 8 Mar 2022 22:01:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 340103858C74 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 228Ju1Eb012793 for ; Tue, 8 Mar 2022 22:01:58 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3enweheutx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 08 Mar 2022 22:01:58 +0000 Received: from m0098420.ppops.net (m0098420.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 228Lw3Lt027460 for ; Tue, 8 Mar 2022 22:01:58 GMT Received: from ppma03dal.us.ibm.com (b.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.11]) by mx0b-001b2d01.pphosted.com with ESMTP id 3enweheutn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 08 Mar 2022 22:01:58 +0000 Received: from pps.filterd (ppma03dal.us.ibm.com [127.0.0.1]) by ppma03dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 228Lme25023571; Tue, 8 Mar 2022 22:01:57 GMT Received: from b03cxnp07029.gho.boulder.ibm.com (b03cxnp07029.gho.boulder.ibm.com [9.17.130.16]) by ppma03dal.us.ibm.com with ESMTP id 3emy8h3np5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 08 Mar 2022 22:01:57 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 228M1tRm25362694 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 8 Mar 2022 22:01:55 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 7D0FA7805C; Tue, 8 Mar 2022 22:01:55 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3414478060; Tue, 8 Mar 2022 22:01:55 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.211.79.233]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Tue, 8 Mar 2022 22:01:55 +0000 (GMT) Message-ID: <0fbaf3783401f8aa8e76a4d3928efff46485ab8d.camel@us.ibm.com> From: Carl Love To: Bruno Larsen , gdb-patches@sourceware.org Cc: Rogerio Alves , cel@us.ibm.com Date: Tue, 08 Mar 2022 14:01:54 -0800 In-Reply-To: References: <442684e3f81aa1df073960bd45918106acefa2b9.camel@us.ibm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-18.el8) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: VTAC6k1LOkv6pmnUwI07vkhBsDxLbkcj X-Proofpoint-ORIG-GUID: 267rY82v-KTmEikJGwZj_SNE2p3HZdbi Subject: RE: [PATCH] Updated, fix reverse stepping multiple contiguous PC ranges X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.64.514 definitions=2022-03-08_09,2022-03-04_01,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 mlxlogscore=999 spamscore=0 mlxscore=0 impostorscore=0 lowpriorityscore=0 malwarescore=0 phishscore=0 priorityscore=1501 suspectscore=0 clxscore=1015 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000 definitions=main-2203080111 X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: , X-List-Received-Date: Tue, 08 Mar 2022 22:02:01 -0000 On Tue, 2022-03-08 at 17:21 -0300, Bruno Larsen wrote: > > > Thanks for looking at this. Since I don't test on aarch64 often, I am > not sure if I see regressions or racy testcases, but it does fix the > issue you mentioned, and there doesn't seem to be regressions on > x86_64 hardware. I have a few nits, but the main feedback is: could > you add a testcase for this, using the dwarf assembler and manually > creating contiguous PC ranges, so we can confirm that this is not > regressed in the future on any hardware? > > Also, I can't approve a patch, but with the testcase this patch is > mostly ok by me > I have not writen anything in dwarf assembler in the past. Off the top of my head, don't know how to create such a test case. It does seem that there are the two testcases gdb.reverse/solib-precsave.exp and gdb.reverse/solib-reverse.exp which the patch fixes. I guess the dwarf assembly test would be similar to the C level code. Do you have an example of how to write dwarf assembly or a pointer to a tutorial on writting dwarf? I will work on the two comments you had on the patch. Thanks for your time reviewing the patch. Carl > > ----------------------------------------------------------- > > From: Luis Machado > > Date: Mon, 21 Feb 2022 23:11:23 +0000 > > Subject: [PATCH] Fix reverse stepping multiple contiguous PC ranges > > > > When running GDB's testsuite on aarch64-linux/Ubuntu 20.04, 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 1 will land on line 2. > > - step from line 2 will land on line 3. > > > > Reverse execution: > > > > - step from line 3 will land on line 2. > > - step from line 2 will land on line 2. > > - step from line 2 will land on line 1. > > > > The problem here is that line 40 contains two contiguous but > > distinct > > PC ranges, like so: > > > > Line 40 - [0x7ec ~ 0x7f4] > > Line 40 - [0x7f4 ~ 0x7fc] > > > > When stepping forward from line 2, we skip both of these ranges and > > land on > > line 42. When stepping backward from line 3, 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->suspend.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. > > > > In any case, the following patch addresses this problem. > > > > I'm not particularly happy with how we go back in the ranges (using > > "pc - 1"). > > Feedback would be welcome. > > > > Validated on aarch64-linux/Ubuntu 20.04/18.04. > > > > Ubuntu 18.04 doesn't actually run into these failures because the > > compiler > > doesn't generate distinct PC ranges for the same line. > > > > gdb/ChangeLog: > > > > YYYY-MM-DD Luis Machado > > > > * infrun.c (process_event_stop_test): Handle backward > > stepping > > across multiple ranges for the same line. > > * symtab.c (find_line_range_start): New function. > > * symtab.h (find_line_range_start): New prototype. > > > > > > Co-authored-by: Carl Love > > --- > > gdb/infrun.c | 24 +++++++++++++++++++++++- > > gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++ > > gdb/symtab.h | 16 ++++++++++++++++ > > 3 files changed, 74 insertions(+), 1 deletion(-) > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > index 376a541faf6..997042d3e45 100644 > > --- a/gdb/infrun.c > > +++ b/gdb/infrun.c > > @@ -6782,11 +6782,33 @@ if (ecs->event_thread- > > >control.proceed_to_finish > > have software watchpoints). */ > > ecs->event_thread->control.may_range_step = 1; > > > > + /* When we are stepping inside a particular line range, in > > reverse, > > + and we are sitting at the first address of that range, we need > > to > > + check if this address also shows up in another line range as > > the > > + end address. > > + > > + If so, we need to check what line such a step range points to. > > + If it points to the same line as the current step range, that > > + means we need to keep going in order to reach the first > > address > > + of the line range. We repeat this until we eventually get to > > the > > + first address of a particular line we're stepping through. */ > > + CORE_ADDR range_start = ecs->event_thread- > > >control.step_range_start; > > + if (execution_direction == EXEC_REVERSE) > > + { > > + gdb::optional real_range_start > > + // = find_line_range_start (ecs->event_thread- > > >suspend.stop_pc); > > + = find_line_range_start (ecs->event_thread- > > >stop_pc()); //carll fi> + > > + > > + if (real_range_start.has_value ()) > > + range_start = *real_range_start; > > + } > > + > > /* 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 > > + if (stop_pc == range_start > > && stop_pc != ecs->stop_func_start > > I think this could be moved to the line above. > > > && execution_direction == EXEC_REVERSE) > > end_stepping_range (ecs); > > diff --git a/gdb/symtab.c b/gdb/symtab.c > > index 1a39372aad0..c40739919d1 100644 > > --- a/gdb/symtab.c > > +++ b/gdb/symtab.c > > @@ -3425,6 +3425,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent) > > return sal; > > } > > > > +/* 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 (prev_sal.line != current_sal.line) > > + 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 (prev_sal.line != current_sal.line) > > + done = true; > > Shouldn't prev_sal.line also be checked here and return an empty > optional? I am not sure when that happens, so please enlighten me if > there is no need to check. > > > + } > > + > > + return prev_pc; > > +} > > + > > /* See symtab.h. */ > > > > struct symtab * > > diff --git a/gdb/symtab.h b/gdb/symtab.h > > index d12eee6e9d8..4d893a8a3b8 100644 > > --- a/gdb/symtab.h > > +++ b/gdb/symtab.h > > @@ -2149,6 +2149,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); > >