From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 74A403858D1E for ; Wed, 2 Aug 2023 09:30:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 74A403858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1690968607; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rKbqyNZ5OgPKjrpLvXzaqCcX5eqV3oIZjJPKBONCB4Q=; b=LUx6g3pP/mVP01GpOXUlji2PjnXUZclBmT7/lEahldsQdcawcVqBWQewJOdmvPbO4O1LPd eQlbfPyNt4+Jq4+55/J4JIvd2d+PffRQBFRkHrrwMUAEqO814/evKPwGi5ivs30tcruI9E V80ZRdmcgyqKfRalMrDsTJxLd7XzAZ4= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-166-70h34akxOq2maMz7CzX7IQ-1; Wed, 02 Aug 2023 05:30:03 -0400 X-MC-Unique: 70h34akxOq2maMz7CzX7IQ-1 Received: by mail-qk1-f197.google.com with SMTP id af79cd13be357-7656c94fc4eso777446685a.2 for ; Wed, 02 Aug 2023 02:30:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690968603; x=1691573403; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=rKbqyNZ5OgPKjrpLvXzaqCcX5eqV3oIZjJPKBONCB4Q=; b=k2j+hjEZj4lR/NOVCIpbV/JhbWp1Q3rI1ZO56qjMikjnVpaMyGbdWYI1zrnUUcRjHy pVrvJrD63SJmKQIyJMDOLijntTGWEmUs7C4mUCe86x0hJpR46+9kSHLVbDe6dIXntlmN 6p7tH87oUojRWvRAQ1TDZYtAw07fRWyeBufLs9brihL+xezxDl3u+Rz8XNPYqsVq+uJh +FVGHGWaU8Cb4agcvRlJvAQw6rh84sLuIchX1HsHcFPVYehYTf+SaQXy5HQbFU8A36dh GGZSWJKlNeDK/mUYXOAqYCmen12qX6JhMPIjIJb4PSW4fYpw2V9t4iZUyJkHHh+4Lbx6 +4LQ== X-Gm-Message-State: ABy/qLZtko9RtmmYp9x3htEAddX/sL4vxqUei8cxb6t3Pwx648Z1O5KU bdjQ79WY77cLHfz+9e8CJSnkUfRzSNiJu/Ixnwj0XczLVwtTKMmFY+tuuiT1RyPycnB5ob5+yrn nw9X7updCCeHPKWVMlws++A== X-Received: by 2002:a05:620a:9d5:b0:76c:97a9:8ff0 with SMTP id y21-20020a05620a09d500b0076c97a98ff0mr12592210qky.77.1690968603074; Wed, 02 Aug 2023 02:30:03 -0700 (PDT) X-Google-Smtp-Source: APBJJlEgD4n2ps3W3A/aV/pbWYW2jDy7eQDeETNqI7VM9H9nnL3kZ6zASfo1h06oh1g+hiXadMPOSg== X-Received: by 2002:a05:620a:9d5:b0:76c:97a9:8ff0 with SMTP id y21-20020a05620a09d500b0076c97a98ff0mr12592184qky.77.1690968602579; Wed, 02 Aug 2023 02:30:02 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id bn50-20020a05620a2af200b00767db6f47bbsm4863610qkb.73.2023.08.02.02.30.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Aug 2023 02:30:02 -0700 (PDT) Message-ID: Date: Wed, 2 Aug 2023 11:29:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH 2/2 ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp To: Carl Love , Simon Marchi , Tom de Vries , Ulrich Weigand , "gdb-patches@sourceware.org" , "pedro@palves.net" References: <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> From: Bruno Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_MANYTO,KAM_SHORT,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,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: On 01/08/2023 00:59, Carl Love wrote: > 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. Hi Carl! Thanks for the thorough explanation and test. You did understood what I meant perfectly :) This is a little besides the point, but I don't see any other patches that touch this area and haven't been committed yet. Am I missing something obvious? Regardless, I feel like this one change can propagate enough that it should be its own commit anyway. > > 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. The patch didn't apply on current master, but I added the changes manually and tested it. It does fix the issue, and adds no regressions. I feel like it could be worth sending an official version this patch as a new thread, instead of a reply to this one, to get people who might not be interested in reverse debugging but may have thoughts on this patch, but maybe wait a bit to see if someone in here says anything :) -- Cheers, Bruno > > 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"