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 6E7B43858D28 for ; Mon, 31 Jul 2023 22:59:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6E7B43858D28 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 (m0356516.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 36VMiS3T005131; Mon, 31 Jul 2023 22:59:17 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=OeY00OOedqDGv5DICHd+tDyDyCPExpzxS3eTRqwIePo=; b=eetpwZW4mpzX9wKh+cox4+ScbQs1jCY5hJVaVFaewdJfiFVjoa35EgGKwoOAcpSJ7ouV F22bBt5nNeEdDLqAAPD1k0WG9v33aX9eC/0zrqTfOndfMJ4hIrcMpiccMS4MUbLvmTRZ NES3TDDtlNNs1hG+Ysd//Td+X+d3xsA+E9a3cgIoHwjvRxt4tpCs6XnfM+DFv44LSlAF RN77c4WCazbS6Sl5EJgR9YDTTkHKMrDrhXJfoHStYUQtWBO0Yt+mSTiXHvFx00WBikOS Aif9PG+X/6jE9mbRZMyvFF1FMzdWMaHi9+9PumWi/ZtuheUXvDp43fWP02e7Bq59HkpD bA== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3s6p0t8aac-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jul 2023 22:59:16 +0000 Received: from m0356516.ppops.net (m0356516.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 36VMjqOC010180; Mon, 31 Jul 2023 22:59:16 GMT Received: from ppma22.wdc07v.mail.ibm.com (5c.69.3da9.ip4.static.sl-reverse.com [169.61.105.92]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3s6p0t8aa1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jul 2023 22:59:16 +0000 Received: from pps.filterd (ppma22.wdc07v.mail.ibm.com [127.0.0.1]) by ppma22.wdc07v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 36VM8lPk017005; Mon, 31 Jul 2023 22:59:15 GMT Received: from smtprelay06.wdc07v.mail.ibm.com ([172.16.1.73]) by ppma22.wdc07v.mail.ibm.com (PPS) with ESMTPS id 3s5dfxy96p-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 Jul 2023 22:59:15 +0000 Received: from smtpav01.dal12v.mail.ibm.com (smtpav01.dal12v.mail.ibm.com [10.241.53.100]) by smtprelay06.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 36VMxE2Y64553282 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 31 Jul 2023 22:59:15 GMT Received: from smtpav01.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B860858065; Mon, 31 Jul 2023 22:59:14 +0000 (GMT) Received: from smtpav01.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2569258059; Mon, 31 Jul 2023 22:59:14 +0000 (GMT) Received: from li-e362e14c-2378-11b2-a85c-87d605f3c641.ibm.com (unknown [9.61.185.149]) by smtpav01.dal12v.mail.ibm.com (Postfix) with ESMTP; Mon, 31 Jul 2023 22:59:14 +0000 (GMT) Message-ID: From: Carl Love To: Bruno Larsen , Simon Marchi , Tom de Vries , Ulrich Weigand , "gdb-patches@sourceware.org" , "pedro@palves.net" Cc: cel@us.ibm.com Date: Mon, 31 Jul 2023 15:59:13 -0700 In-Reply-To: <63d4dcba-923a-9a66-23f3-9135bb1b4521@redhat.com> References: <71aa635593df0677811afb85409aa190bcfa4f6a.camel@us.ibm.com> <15864a6b87b25c93e99a28149f23138267735f2a.camel@us.ibm.com> <041f62e9f26fd4a536bc90c34f072985582e6237.camel@de.ibm.com> <46c2c756475ba5923d7eed97996632a08285dd42.camel@us.ibm.com> <65861786-069e-53a1-ca17-a525b6629c95@suse.de> <5be0c849abeef84d34a6ff255fb2705ca5dcb035.camel@us.ibm.com> <5e60a837-b21c-011f-c94e-e8bbf7645c5d@simark.ca> <7639de48695d52a806627b0a91979ad2e5fd9b42.camel@us.ibm.com> <9cf51eb9-c731-6f42-ab2b-a37048f25d12@simark.ca> <60c362e6dadd05754907af5f10e6f3c0423e1901.camel@us.ibm.com> <2a1d55cb-118b-d942-b315-a5f2348894f5@simark.ca> <0ab7af40f2f8d1edbceea851c37282a03c830567.camel@us.ibm.com> <90c98190-f8eb-1d64-43ce-8bab67d5caef@simark.ca> <6c58d4ce-d9ff-e1d9-398b-724859394f22@redhat.com> <38ab07a7a217bcc05be78c82d351aef73b411fb9.camel@us.ibm.com> <63d4dcba-923a-9a66-23f3-9135bb1b4521@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: xmlwMfdmg-nhQ3FVSUcL7NFaKvpVadUv X-Proofpoint-ORIG-GUID: NmGCS9j6SHfesPItl24AxfeZED9RMJsW Content-Transfer-Encoding: 7bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 Subject: RE: [PATCH 2/2 ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26 definitions=2023-07-31_15,2023-07-31_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 mlxscore=0 phishscore=0 malwarescore=0 priorityscore=1501 impostorscore=0 spamscore=0 bulkscore=0 adultscore=0 suspectscore=0 lowpriorityscore=0 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000 definitions=main-2307310205 X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,KAM_MANYTO,KAM_SHORT,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.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Bruno: On Fri, 2023-07-21 at 09:24 +0200, Bruno Larsen wrote: > > I think what Bruno is again asking is to have > > control.step_range_start > > set to the real start range initially, i.e. what Simon asked > > about. I > > think to do that, we would need to make significant changes to how > > reverse execution works to allow us to make that change. It didn't > > appear to be a straight forward fix to me. I may be wrong. Maybe > > someone sees a good way to make that work that I am missing. So it > > looks like this patch fixes most issues but not all of the problems > > with reverse-step and reverse-next. It might be good to try and > > fix > > this additional scenario in a separate patch, not sure??? > > I just tested with the newest version of the patch relating to > reverse > stepping over contiguous lines, but that didn't make a difference. > GCC > continued broken. I think the problem is that GCC doesn't have > contiguous ranges, it has a single range that doesn't contain the PC > > My (probably naive) thinking is to set step_range_end to the starting > PC > of the following line, and step_range_start to be the real start of > the > current line at the moment the command is parsed. This sounds like > it > would solve the GCC problem, but I assume I'm missing something. > Unfortunately, I don't have the time to test this theory myself, but > I'll happily test any patches you submit :). If you want to repeat > my > test, I want the behavior of gdb.reverse/finish-reverse-next from gcc > to > match clang's (on x86 machines), that is, if you reverse finish from > function1, a single "rn" command would land you on the b=5 line. So, I took the example of the error from your previous patch where you demonstrated the scenario where you have stopped at the call to the function, then do the reverse-next which stops at the beginning of the same line and put it into a new test case for discussion and for debugging the issue. Nice to have a simple example of the issue. The new test is bruno_7_20_2023.exp. So, in this test, when we are stopped at the call, as you showed on Intel (gdb) maint info line-table > (snip) > 15 81 0x0000000000401185 0x0000000000401185 Y > 16 83 0x000000000040118c 0x000000000040118c Y > 17 86 0x000000000040119b 0x000000000040119b Y > (gdb) disas /s > Dump of assembler code for function main: > (snip) > 83 function1 (a, b); // CALL VIA LEP > 0x000000000040118c <+37>: mov -0x10(%rbp),%edx > 0x000000000040118f <+40>: mov -0xc(%rbp),%eax > 0x0000000000401192 <+43>: mov %edx,%esi > 0x0000000000401194 <+45>: mov %eax,%edi > => 0x0000000000401196 <+47>: call 0x40112c > > We can see that we have stopped at the right instruction, but it > isn't > mapped to any line number directly. If we reverse-next from there: the tp->control.step_range_start is set to 0x40118c in function prepare_one_step() in gdb/infcmd.c. The PC in your case is at 0x401196. Of course both of the addresses are in the same line. We are executing backwards so we need step_range_start to be set to a value in the previous line. In general, we have no idea what the previous line was as we may have arrived on this line via a jump or branch in theory. Also, note, when setting the range we need to have the PC in the range or GDB will give us an assert error. So really the issue is that we want the step_range_start to be in the line where we came from. So, if we see the line number for PC and step_range_start are the same, we can set step_range_start to one less then the address of the beginning of the current line when we parse the instruction and setup the stepping range. The goal is to have GDB stop in the previous line and not at the beginning of the line and thus match the behavior of gdb on clang. I implemented the check and adjustment for step_range_start as just described and tested it with your example, bruno_7_20_2023.exp and it does seem to work as desired. The existing testcases expect to have to do two reverse step/next instructions to reach the previous line. So, we need to fix the existing tests gdb.reverse/finish-reverse-next.exp and gdb.mi/mi- reverse.exp to remove the extra reverse step/next command. Anyway, I put the gdb code fix, the bruno_7_20_2023.exp and test fixes into the patch below for discussion purposes. If everyone is happy with the gdb change and test changes (we can drop the Bruno test) I can merge these changes into the patch. Hopefully keeping these changes in a separate patch for the moment will make it easier to see what is being changed. If you were to apply the following patch on top of the currently proposed patch, does gdb then execute as you expect on your system? I have tested the change on IBM Power 10. It would be good to make sure it works on your X86 system as well. I also ran the full gdb regression tests on Power 10 with no additional regression failures. Thanks for looking at the patch, proposed fix and the additional testing. Carl --------------------------------------------------------------- [PATCH 2/2] finish-reverse-next new test case from bruno. GDB fix for Bruno test, updated other tests for the gdb fix. --- gdb/infcmd.c | 9 ++ gdb/testsuite/gdb.mi/mi-reverse.exp | 6 +- gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp | 85 +++++++++++++++++++ .../gdb.reverse/finish-reverse-next.exp | 44 +++++++--- 4 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp diff --git a/gdb/infcmd.c b/gdb/infcmd.c index b8134665f3f..c09b3edad3d 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -981,6 +981,15 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm) find_pc_line_pc_range (pc, &tp->control.step_range_start, &tp->control.step_range_end); + symtab_and_line sal = find_pc_line (pc, 0); + symtab_and_line sal_start + = find_pc_line (tp->control.step_range_start, 0); + + if (sal.line == sal_start.line) + /* The step_range_start address is in the same line. We want to + stop in the previous line so move step_range_start one + instruction earlier. */ + tp->control.step_range_start--; /* There's a problem in gcc (PR gcc/98780) that causes missing line table entries, which results in a too large stepping range. diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp index baa53a495d7..997309cfb71 100644 --- a/gdb/testsuite/gdb.mi/mi-reverse.exp +++ b/gdb/testsuite/gdb.mi/mi-reverse.exp @@ -99,8 +99,12 @@ proc test_controlled_execution_reverse {} { # It takes two steps to get back to the previous line, # as the first step moves us to the start of the current line, # and the one after that moves back to the previous line. +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. - mi_execute_to "exec-next --reverse 2" \ + mi_execute_to "exec-next --reverse" \ "end-stepping-range" "main" "" \ "basics.c" $line_main_hello "" \ "reverse next to get over the call to do_nothing" diff --git a/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp new file mode 100644 index 00000000000..0f19a959043 --- /dev/null +++ b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp @@ -0,0 +1,85 @@ +# 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 . */ + +# This file is part of the GDB testsuite. It tests reverse stepping. +# Lots of code borrowed from "step-test.exp". + +# The reverse finish command should return from a function and stop on +# the first instruction of the source line where the function call is made. +# Specifically, the behavior should match doing a reverse next from the +# first instruction in the function. GDB should only take one reverse step +# or next statement to reach the previous source code line. + +# This testcase verifies the reverse-finish command stops at the first +# instruction in the source code line where the function was called. There +# are two scenarios that must be checked: +# 1) gdb is at the entry point instruction for the function +# 2) gdb is in the body of the function. + +# This test verifies the fix for gdb bugzilla: +# https://sourceware.org/bugzilla/show_bug.cgi?id=29927 + +# PowerPC supports two entry points to a function. The normal entry point +# is called the local entry point (LEP). The alternate entry point is called +# the global entry point (GEP). A function call via a function pointer +# will entry via the GEP. A normal function call will enter via the LEP. +# +# This test has been expanded to include tests to verify the reverse-finish +# command works properly if the function is called via the GEP. The original +# test only verified the reverse-finish command for a normal call that used +# the LEP. + +if ![supports_reverse] { + return +} + +standard_testfile finish-reverse-next.c + +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" +} + + +# until 83 +# Set breakpoint at call to function1 in main. +set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile] +gdb_breakpoint $srcfile:$bp_LEP_test temporary + +# Continue to break point at function1 call in main. +gdb_continue_to_breakpoint \ + "stopped at function1 entry point instruction to stepi into function" \ + ".*$srcfile:$bp_LEP_test\r\n.*" + +gdb_test "next" ".*a = 10;.*" \ + "next from function body function1 LEP to a = 10;" + +gdb_test "reverse-step" ".*70.*}.*" \ + "reverse step from a = 10, stops at last inst in function1 call" + +gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \ + "reverse-finish should stop at last inst in line for function1" + +gdb_test "reverse-next" ".*b = 5;.*" \ + "reverse-finish should stop at b = 5, not first inst in line for function1" + + diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp index 1f53b649a7d..921d0051233 100644 --- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp +++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp @@ -80,10 +80,15 @@ repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100" # instruction should then stop at the first instruction in the same source # code line. Another revers-next instruction stops at the previous source # code line. + +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \ "reverse-finish function1 LEP call from LEP " -gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP" \ - "reverse next 1 LEP entry point function call from LEP" +##gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP" \ +## "reverse next 1 LEP entry point function call from LEP" gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP" @@ -113,10 +118,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 1" # instruction should then stop at the first instruction in the same source # code line. Another revers-next instruction stops at the previous source # code line. +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. gdb_test "reverse-finish" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \ "reverse-finish function1 LEP call from function body" -gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \ - "reverse next 1 LEP from function body" +## gdb_test "reverse-next" ".*function1 \\(a, b\\); // CALL VIA LEP.*" \ +## "reverse next 1 LEP from function body" gdb_test "reverse-next" ".*b = 5;.*" \ "reverse next 2 at b = 5, from function body" @@ -148,10 +157,15 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call" # instruction should then stop at the first instruction in the same source # code line. Another revers-next instruction stops at the previous source # code line. +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. + gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \ "function1 GEP call call from GEP" -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ - "reverse next 1 GEP entry point function call from GEP" +## gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ +## "reverse next 1 GEP entry point function call from GEP" gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP" gdb_test "reverse-continue" ".*" "setup for test 4" @@ -184,10 +198,15 @@ gdb_test "stepi" "{" "stepi to between GEP and LEP" # instruction should then stop at the first instruction in the same source # code line. Another revers-next instruction stops at the previous source # code line. +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. + gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \ "function1 GEP call call from GEP again" -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ - "reverse next 1 GEP entry point function call from GEP again" +##gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ +## "reverse next 1 GEP entry point function call from GEP again" gdb_test "reverse-next" ".*b = 50;.*" \ "reverse next 2 at b = 50, call from GEP again" @@ -216,9 +235,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 2" # instruction should then stop at the first instruction in the same source # code line. Another revers-next instruction stops at the previous source # code line. +## I BELIEVE Bruno is arguing this part of the test is wrong. When we are +## stopped at the call, which is the last instruction in the line, the +## reverse next should take us directly to the previous line, not stop at +## the first instruction in the same line. + gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \ "reverse-finish function1 GEP call, from function body " -gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ - "reverse next 1 GEP call from function body" +## gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \ +## "reverse next 1 GEP call from function body" gdb_test "reverse-next" ".*b = 50;.*" \ "reverse next 2 at b = 50 from function body" -- 2.37.2