From patchwork Fri Feb 14 20:05:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bernd Edlinger X-Patchwork-Id: 38085 Received: (qmail 31156 invoked by alias); 14 Feb 2020 20:05:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 31147 invoked by uid 89); 14 Feb 2020 20:05:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=refuse, Hx-clientproxiedby:2603, gccs, UD:frame.h X-HELO: EUR05-VI1-obe.outbound.protection.outlook.com Received: from mail-vi1eur05olkn2043.outbound.protection.outlook.com (HELO EUR05-VI1-obe.outbound.protection.outlook.com) (40.92.90.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Feb 2020 20:05:53 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PMs/7m/u627bATo+7qnusH77KspLkrS5/cAASCumvvoz9iDXzmFW5K3s8OrpVe16z5fd/ZroeKlVUQpp/tROVJQ+0eXK3zrKR3nK1B/7Dak2SuBmdtZ6FVqXZ2jK+kO3hYdt6c9Ym/STETgOlYSFTzCbcCjK8225l7BVGcAfiXD2jlwe52sWadLi398bAWrzy9Pm5W0YYUgm9D6Zbmrpl/z6SoZx0WmU8rU8+/IIOR2BpIHJ+lhFt52+uajVjUAUT3qkUN64a6hNOiG/POYdLbcruCXbUFrQjsehPdExAbvm4MkwlBIYttGQkTfi1Z2N4owgAJKrx9MJuyHxKl5HVQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=oc8JgeQ0/lbAQbCej+O6bARjAH69iJ1S3IyjjsrIW+E=; b=E8AX0K1KDxa58uUvCmVTugiaKHMkz3f9epd8eGC7cmGxATXoMUwCC8sDCCv7xXyF/NSqwNPtKVX/7jTY6rXEmfTXEVAlorxwt2Z4CcCztTTB/awtnSM+PHzxXLujU2EEOXGFiUEKPB4St/3aqtTTG5IWAyPWM3MmVpFiz7ehiLdFjaUe6o6pZVbqyXNKiXmDJEBdoiatJ1/NFLzzbqnra2x3v08B0gRPbZlsbnRPnZzLb0jtB0omZ9etwahdhx6aPHQiKH9t66Wp3d3O9s3EhaSOd7XMqQmutOFmiD2YsQNRlaYAYVJO7qv+AZat2/UQWIi0Lpvclq+TPpGMPNaCiQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none Received: from AM6EUR05FT004.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc11::37) by AM6EUR05HT188.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc11::295) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2729.22; Fri, 14 Feb 2020 20:05:51 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com (10.233.240.60) by AM6EUR05FT004.mail.protection.outlook.com (10.233.240.227) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2729.22 via Frontend Transport; Fri, 14 Feb 2020 20:05:50 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com ([fe80::1956:d274:cab3:b4dd]) by AM6PR03MB5170.eurprd03.prod.outlook.com ([fe80::1956:d274:cab3:b4dd%6]) with mapi id 15.20.2729.025; Fri, 14 Feb 2020 20:05:50 +0000 Received: from [192.168.1.101] (92.77.140.102) by FR2P281CA0012.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:a::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2729.23 via Frontend Transport; Fri, 14 Feb 2020 20:05:50 +0000 From: Bernd Edlinger To: Andrew Burgess CC: "gdb-patches@sourceware.org" Subject: Re: [PATCH 2/2] gdb: Add support for tracking the DWARF line table is-stmt field Date: Fri, 14 Feb 2020 20:05:50 +0000 Message-ID: References: <94e33268f64060fc887670f4ee5ed524050cbcc7.1580902412.git.andrew.burgess@embecosm.com> <20200211135710.GR4020@embecosm.com> In-Reply-To: <20200211135710.GR4020@embecosm.com> x-microsoft-original-message-id: <492e0f12-3fd6-a18f-0427-cab0061d2c93@hotmail.de> x-ms-exchange-antispam-messagedata: XGkxHa/EPIWy8NF31ha16OznV32VRo0DEXc8Donu9Zz7YuCDAFeWuSD29TtQiPjun/AbhuH+3U7sz5nr5AKIHfFKfjI8Fr43BcvxYwn5CpgFo2vHW51X9ptqV2oR3t0cWajnNfkiegLDhZ+C2crETg== x-ms-exchange-transport-forked: True Content-ID: <455D1824C1AAB94CB654985671389E3A@eurprd03.prod.outlook.com> MIME-Version: 1.0 On 2/11/20 2:57 PM, Andrew Burgess wrote: > * Bernd Edlinger [2020-02-05 17:55:37 +0000]: > >> On 2/5/20 12:37 PM, Andrew Burgess wrote: >> >> did you mean, when we don't land in the middle of a line ? > > No, in this case I did write the correct thing. > > So GDB had/has some code designed to "improve" the handling of looping > constructs. I think the idea is to handle cases like this: > > 0x100 LN=5 > 0x104 <-\ > 0x108 | > 0x10c | LN=6 > 0x110 | > 0x114 --/ > > So when line 6 branches back to the middle of line 5, we don't stop > (because we are in the middle of line 5), but we do want to stop at > the start of line 6, so we "reset" the starting point back to line 5. > > This is done by calling set_step_info in process_event_stop_test, in > infrun.c. > > What we actually did though was whenever we were at an address that > didn't exactly match a SAL (in the example above, marked LN=5, LN=6) > we called set_step_info. This worked fine, at address 0x104 we reset > back to LN=5, same at address 0x108. > > However, in the new world things can look different, for example, like > this: > > 0x100 LN=5,STMT=1 > 0x104 > 0x108 LN=6,STMT=0 > 0x10c LN=6,STMT=1 > 0x110 > 0x114 > > In this world, when we move forward to 0x100 we don't have a matching > SAL, so we get the SAL for address 0x100. We can then safely call > set_step_info like we did before, that's fine. But when we step to > 0x108 we do now have a matching SAL, but we don't want to stop yet as > this address is 'STMT=0'. However, if we call set_step_info then GDB > is going to think we are stepping away from LN=6, and so will refuse > to stop at address 0x10c. > > The proposal in this commit is that if we land at an address which > doesn't specifically match a SAL, 0x104, 0x110, 0x114 in the above > example, then we do call set_step_info, but otherwise we don't. > > There are going to be situations where the behaviour of GDB changes > after this patch, but I don't think we can avoid that. What we had > previously was just a heuristic. I think enabling this new > information in GDB will allow us to do better overall, so I think we > should make this change and fix any issues as they show up. > I agree with that, thanks for this explanation. However, I think I found a small problem in this patch. You can see it with the next-inline test case. When you use just step the execution does not stop between the inline functions, because not calling current behaviour is this: Breakpoint 1, main () at next-inline.cc:63 63 get_alias_set (&xx); (gdb) s get_alias_set (t=0x404040 ) at next-inline.cc:50 50 if (t != NULL (gdb) s 51 && TREE_TYPE (t).z != 1 (gdb) s 0x0000000000401169 in tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s 0x000000000040117d in tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s 0x000000000040118f in tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s main () at next-inline.cc:64 64 return 0; (gdb) I debugged a bit because I was not sure why that happens, and it looks like set_step_info does also set the current inline frame, but you only want to suppress the line number not the currently executing inline function. With this small patch the step works as expected. commit 193d81765d88b11e68111a8856a84cdf084d0b22 Author: Bernd Edlinger Date: Fri Feb 14 20:41:51 2020 +0100 Fix next-inline test case with step Should stop between inline function invocations. I'm not sure if that is the cleanest solution, but it works. With that adjustment, the stepping out of the inline and back into makes the step call stop, which is the correct behavior: Breakpoint 1, main () at next-inline.cc:63 63 get_alias_set (&xx); (gdb) s get_alias_set (t=0x404040 ) at next-inline.cc:50 50 if (t != NULL (gdb) s 51 && TREE_TYPE (t).z != 1 (gdb) s 0x0000000000401169 in tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s 52 && TREE_TYPE (t).z != 2 (gdb) s tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s 53 && TREE_TYPE (t).z != 3) (gdb) s tree_check (i=0, t=0x404040 ) at next-inline.h:34 34 if (t->x != i) (gdb) s main () at next-inline.cc:64 64 return 0; (gdb) If you like you can integrate that into your patch. You will probably want to add that to the test case. >> >>> --- a/gdb/stack.c >>> +++ b/gdb/stack.c >>> @@ -330,7 +330,7 @@ frame_show_address (struct frame_info *frame, >>> return false; >>> } >>> >>> - return get_frame_pc (frame) != sal.pc; >>> + return get_frame_pc (frame) != sal.pc || !sal.is_stmt; >>> } >>> >>> /* See frame.h. */ >>> @@ -1036,6 +1036,7 @@ print_frame_info (const frame_print_options &fp_opts, >>> int location_print; >>> struct ui_out *uiout = current_uiout; >>> >>> + >>> if (!current_uiout->is_mi_like_p () >>> && fp_opts.print_frame_info != print_frame_info_auto) >>> { >> >> Is this white space change intentional? > > Ooops. Fixed. > > I also fixed the space before tab issue you pointed out in another > mail. > > I see you have a patch that depends on this one, but I'd like to leave > this on the mailing list for a little longer before I merge it to give > others a chance to comment. > Thanks, regarding my other patch... I used gprof to get performance numbers, because I was not sure if that simple approach adds too much execution time, and it turns out that it spent 30% of the complete execution time in the record_inline_range_end function, when I was debugging GCC's cc1 executable. So I decided that this needs to be optimized, I will send a new version that does the line table patching with a binary search, after the weekend. Thanks Bernd. > I'll probably merge this over the weekend if there's no other > feedback. > > Thanks, > Andrew > diff --git a/gdb/infrun.c b/gdb/infrun.c index 3919de8..89e6654 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -7190,6 +7190,8 @@ process_event_stop_test (struct execution_control_state *e ecs->event_thread->control.step_range_start = stop_pc_sal.pc; ecs->event_thread->control.step_range_end = stop_pc_sal.end; ecs->event_thread->control.may_range_step = 1; + ecs->event_thread->control.step_frame_id = get_frame_id (frame); + ecs->event_thread->control.step_stack_frame_id = get_stack_frame_id (frame); if (refresh_step_info) set_step_info (frame, stop_pc_sal);