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.129.124]) by sourceware.org (Postfix) with ESMTPS id 987F93858D28 for ; Tue, 2 May 2023 15:42:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 987F93858D28 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=1683042149; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=k5U32mhIU74wG9i+FEniP0/GuDzebaE8OHaDxrwVplc=; b=PbH4dgiQ6uXMr9fW5yddmme4D1rcg/zQ8UVDVCFnKtBULQ9eBkCMI5kfYgSbtsVDLFlOPP ccIIyXVhhJ8AcAMD7kNVerXCRL+L9dURoHmPP9ezGfjWLC7ALYb8A0TRqzT70pswoDNban 8e8WfGwmppLWAufgEg3SY7k5/vbcQCk= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-517-womvGu8iOASwI0aBSf139Q-1; Tue, 02 May 2023 11:42:28 -0400 X-MC-Unique: womvGu8iOASwI0aBSf139Q-1 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-3ef33ed8843so21258581cf.1 for ; Tue, 02 May 2023 08:42:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683042147; x=1685634147; h=content-transfer-encoding:in-reply-to:from:references:cc: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=k5U32mhIU74wG9i+FEniP0/GuDzebaE8OHaDxrwVplc=; b=eBozqIp4aQizCBkBXbxMVtmEgseBugR2Rd4s7aLP/suqlDymjvDwxSYmVdVlUsUwgG AYzCEjLyjfdWhDvgMEPIz5FVodIPTbUuMguGxwT2DdFrMOHeJXYzVIR9eysWskr6HRkM JzQaWY/D2xMW84JBLZFJXnkBI89+xZ2kIHf/Sq7GNSSTRzG+mLGdUgy8nGg+mpVGATg2 qpeDy/di/SEeI4LejvILaKwJNsN5ToKDnOi2u6dfxzKp7Mw8YoyZl1l+pcKYU12JD98s IQStvXLqeTz+FNaSgpQkQWblXk3rGSTePIY1D7bo7O0lfYkkxxjxso2SLKMwLhnAFI2Z IKYw== X-Gm-Message-State: AC+VfDyOP4udAQJuBFp94tXkvEug8tVPHRlT/qNnhV0H8KpIeXvAN0po HWDIkQxATEZ14+h7m3vtMPH+8wWi8YpEPqs4BtcAwfU4Wx/6u9gMycODHnX+oBQYa7bwa87yh0b 4AZ+NSlq1fKluc9nIfQhtNg== X-Received: by 2002:ac8:5795:0:b0:3f1:f410:e173 with SMTP id v21-20020ac85795000000b003f1f410e173mr28947709qta.13.1683042147605; Tue, 02 May 2023 08:42:27 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7GQlP81JqpEYb65TXUGH+wDLLG+zCrhSO4qzGSwDXOqy5N96Qzej14naFoDttnhthKwB/zWQ== X-Received: by 2002:ac8:5795:0:b0:3f1:f410:e173 with SMTP id v21-20020ac85795000000b003f1f410e173mr28947668qta.13.1683042147233; Tue, 02 May 2023 08:42:27 -0700 (PDT) Received: from [192.168.0.45] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id w12-20020ac857cc000000b003f20ec69b1asm3140382qta.17.2023.05.02.08.42.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 May 2023 08:42:26 -0700 (PDT) Message-ID: <4b3e3f8d-4fec-6dd6-ec7d-5f77b9aef2c7@redhat.com> Date: Tue, 2 May 2023 17:42:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH] Fix reverse stepping multiple contiguous PC ranges over the line table. To: Carl Love , gdb-patches@sourceware.org, Ulrich Weigand , pedro@palves.net Cc: luis.machado@arm.com References: <74630f1ccb6e9258ae60682105ee5490726fb255.camel@us.ibm.com> <0a4e5ac7-7c2c-2d9c-813e-bdc5ba46bbf1@redhat.com> <4a03c3208d2a6505273753fa47ee1aefc5536408.camel@us.ibm.com> From: Bruno Larsen In-Reply-To: <4a03c3208d2a6505273753fa47ee1aefc5536408.camel@us.ibm.com> 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=-4.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_ASCII_DIVIDERS,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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 02/05/2023 17:40, Carl Love wrote: > Bruno: > > On Tue, 2023-05-02 at 16:15 +0200, Bruno Larsen wrote: > > I had intended everything from here down as part of the commit message. > It is admittedly a lot. It could be cut down my making the first part > could be moved to the mailing list context. The commit message would > then start with the two scenarios the patch fixes. >>> --------------------------------------------------------------- >>> Fix reverse stepping multiple contiguous PC ranges over the line >>> table. >>> >>> There are a couple of scenarios where the GDB reverse-step and >>> reverse-next >>> commands do not work correctly. The first scenario consists of >>> multiple >>> assignment statements on the same line. A patch was proposed to >>> address the >>> issue by Luis Machado and briefly discussed on the mailing list in >>> Feb 2021. >>> >>> https://sourceware.org/pipermail/gdb-patches/2021-February/175678.html >>> >>> The discussion was revived by Carl Love with regards to fixing the >>> same >>> issue on PowerPC. >>> >>> https://sourceware.org/pipermail/gdb-patches/2022-March/186463.html >>> >>> The patch was not completed and has been on Carl's to do list for >>> some time. >>> >>> Discussion of a patch to change how the reverse-step and reverse- >>> next >>> commands submitted by Carl Love was started in thread: >>> >>> https://sourceware.org/pipermail/gdb-patches/2023-January/195563.html >>> >>> The patch was withdrawn as it was pointed out the proposed patch >>> would >>> change the intended behavior of the commands as documented in the >>> GDB >>> manual. However, it was pointed out by Pedro Alves < >>> pedro@palves.net> >>> that the reverse-step and reverse-next commands do not work when >>> there >>> are multiple function calls on the same line. This is a second >>> scenario >>> where the commands do not work correctly. >>> >>> The following patch is an extended version of the original patch by >>> Luis Machado to fix the issues in scenario 1 to also address the >>> issues in >>> scenario 2. >>> > Move the above to the mailing list > > Start here with the commit message > >>> -------------------------------------------------------- >>> >>> 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. >>> >>> 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. >>> >>> 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 > End of commit message >> Hey Carl, >> >> Thanks for working on this. I'm wondering which parts will be part >> of >> the final commit messages and which is just context for the mailing >> list, so some clarity would be nice, but that is not a huge deal. >> >> I wanted to test this change, but it doesn't apply anymore on >> master, >> and `git apply --3way` can't figure out how to do it. Which commit >> did >> you use as base (or alternatively, can you rebase it)? > OK, let me rebase it and repost. Let me know if you think the > suggested commit message is still too much. Thanks. Hey sorry, this part was my bad, no need to rebase. I'll look at the commit message in a bit -- Cheers, Bruno > > Carl