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 5E54A382CF23 for ; Tue, 7 Jun 2022 17:11:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5E54A382CF23 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 257G67qt016125; Tue, 7 Jun 2022 17:11:46 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3gj9jbhw91-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Jun 2022 17:11:46 +0000 Received: from m0098409.ppops.net (m0098409.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 257G7RQ3020991; Tue, 7 Jun 2022 17:11:46 GMT Received: from ppma03wdc.us.ibm.com (ba.79.3fa9.ip4.static.sl-reverse.com [169.63.121.186]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3gj9jbhw8q-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Jun 2022 17:11:45 +0000 Received: from pps.filterd (ppma03wdc.us.ibm.com [127.0.0.1]) by ppma03wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 257H6J2Y002803; Tue, 7 Jun 2022 17:11:44 GMT Received: from b01cxnp22036.gho.pok.ibm.com (b01cxnp22036.gho.pok.ibm.com [9.57.198.26]) by ppma03wdc.us.ibm.com with ESMTP id 3gfy19npxh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Jun 2022 17:11:44 +0000 Received: from b01ledav003.gho.pok.ibm.com (b01ledav003.gho.pok.ibm.com [9.57.199.108]) by b01cxnp22036.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 257HBiXK12058982 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 7 Jun 2022 17:11:44 GMT Received: from b01ledav003.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3746FB205F; Tue, 7 Jun 2022 17:11:44 +0000 (GMT) Received: from b01ledav003.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A1273B2065; Tue, 7 Jun 2022 17:11:43 +0000 (GMT) Received: from lexx (unknown [9.160.81.62]) by b01ledav003.gho.pok.ibm.com (Postfix) with ESMTP; Tue, 7 Jun 2022 17:11:43 +0000 (GMT) Message-ID: <971526aef8d957f678d9a8b10bfa2e41c4ea41c1.camel@vnet.ibm.com> Subject: Re: [PATCH,v5] Fix reverse stepping multiple contiguous PC ranges over the line table From: will schmidt To: Carl Love , Bruno Larsen , Luis Machado , gdb-patches@sourceware.org Cc: rogealve@br.ibm.com Date: Tue, 07 Jun 2022 12:11:43 -0500 In-Reply-To: <65abc453edc9d73df97a8630503420ebf8c5747b.camel@us.ibm.com> References: <20220506085506.9184-1-luis.machado@arm.com> <9e420536-01e0-7192-d585-747c52fdf4d5@redhat.com> <65abc453edc9d73df97a8630503420ebf8c5747b.camel@us.ibm.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: SaXFXJ1h4fy6ke1zhKn50wK_PSANd8kU X-Proofpoint-ORIG-GUID: bRWWZyf_qvW0_nMMTTPmu_ccV7TIgvVh Content-Transfer-Encoding: 7bit X-Proofpoint-UnRewURL: 2 URL's were un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.874,Hydra:6.0.517,FMLib:17.11.64.514 definitions=2022-06-07_07,2022-06-07_02,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 bulkscore=0 phishscore=0 malwarescore=0 adultscore=0 mlxscore=0 priorityscore=1501 clxscore=1015 mlxlogscore=999 impostorscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000 definitions=main-2206070070 X-Spam-Status: No, score=-11.7 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_PASS, 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 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, 07 Jun 2022 17:11:53 -0000 On Fri, 2022-05-06 at 09:48 -0700, Carl Love wrote: > Bruno, GDB maintainers: > > The patch has been updated per the comments on the testcase from > Bruno. > > The patch was retested on Power 10 to ensure the test still works > correctly. > > Please let us know if there are any additional comments or if the > patch is ready to commit. We thank you for your help with this patch. > > Assorted nits below, but unless my comments reveal something, LGTM. > > Carl Love > -------------------------------------------------------- > Fix reverse stepping multiple contiguous PC ranges over the line > table > > v5: > - Updated test case comments on the purpose of the test. > - Add test to check system supports record-replay. > - Removed now unnecessary reord test when activating record. s/reord/record/ :-) > v4: > - Updated testcase to make it a bit longer so it can exercise > reverse-stepping > multiple times. > - Cleaned up debugging prints. > > v3: > - Updated testcase. The format for writing the DWARF program body in > the > testcase expect file changed. > See commit gdb/testsuite/dwarf: simplify line number program syntax > (commit d4c4a2298cad06ca71cfef725f5248f68205f0be) > > v2: > - Check if both the line and symtab match for a particular line table > entry. > > -- > > 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] Curious, what incantation is used to collect the PC range info? > The two distinct ranges are generated because GCC started outputting > source > column information, which GDB doesn't take into account at the > moment. Not critical for this, but is there a general sense for when GCC started generating the source column info? > 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. > > In any case, the following patch addresses this problem. > > Validated on aarch64-linux and x86_64/Ubuntu 20.04/18.04. Carl Love > has > verified that it does fix a similar issue on ppc. > > Ubuntu 18.04 doesn't actually run into these failures because the > compiler > doesn't generate distinct PC ranges for the same line. > > I see similar failures on x86_64 in the gdb.reverse tests > (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). > Those are > also fixed by this patch. > > The included testcase (based on a test Carl wrote) exercises this > problem for > Arm, ppc and x86. It shows full passes with the patch applied. > > Co-authored-by: Carl Love < > cel@us.ibm.com > > > --- > gdb/infrun.c | 22 ++- > gdb/symtab.c | 49 ++++++ > gdb/symtab.h | 16 ++ > gdb/testsuite/gdb.reverse/map-to-same-line.c | 55 +++++++ > .../gdb.reverse/map-to-same-line.exp | 141 > ++++++++++++++++++ > 5 files changed, 282 insertions(+), 1 deletion(-) > 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 6e5853ef42a..82c28961aeb 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -6955,11 +6955,31 @@ 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, Nit: Could be rephrased as "When we are reverse-stepping inside a particular line range" > + 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->stop_pc ()); > + > + 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 > && execution_direction == EXEC_REVERSE) > end_stepping_range (ecs); ok > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 4b33d6c91af..de4cb5dd0eb 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -3433,6 +3433,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); > +} ok > + > +/* 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 {}; I assume our lines start at 1 so this will never false positive? > + > + 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; ok > + > + /* 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; ok > +} > + > /* See symtab.h. */ > > struct symtab * > diff --git a/gdb/symtab.h b/gdb/symtab.h > index b1cf84f756f..226fe8803db 100644 > --- a/gdb/symtab.h > +++ b/gdb/symtab.h > @@ -2285,6 +2285,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. > +*/ ok > + > +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/map-to-same-line.c > b/gdb/testsuite/gdb.reverse/map-to-same-line.c > new file mode 100644 > index 00000000000..dd9f9f8a400 > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.c > @@ -0,0 +1,55 @@ > +/* Copyright 2022 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 URL will need to be fixed up. Likely a side effect of the IBM email infrastructure that does not exist in the patch itself. :-) > +/* 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"); > + 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..c3fb859be55 > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp > @@ -0,0 +1,141 @@ > +# Copyright 2022 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/ > >. > + Same. > +# 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 > +} > + > +if [get_compiler_info] { > + return -1 > +} > + > +# The DWARF assembler requires the gcc compiler. > +if {!$gcc_compiled} { > + 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_end_sequence > + } > + } > +} > + > +if { [prepare_for_testing "failed to prepare" ${testfile} \ > + [list $srcfile $asm_file] {nodebug} ] } { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# Activate process record/replay > +gdb_test_no_output "record" "turn on process record" > + > +gdb_test "tbreak main_return" "Temporary breakpoint .*" "breakpoint > at return" > +gdb_test "continue" "Temporary breakpoint .*" "run to end of main" > + > +# 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" > +} > -- > 2.31.1 > ok.