From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05olkn2034.outbound.protection.outlook.com [40.92.90.34]) by sourceware.org (Postfix) with ESMTPS id C7384385E008 for ; Wed, 25 Mar 2020 11:50:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C7384385E008 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=hotmail.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=bernd.edlinger@hotmail.de ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RFuMTlcPd8VZneEyYMWo15Yz8numhlkBB1iG/6nuYI9/ZYEyorzTOz7J9inbc+cv3+sWWt0QabCtdAUL7QggqpgOyD1//OrijsfQOoSDhi0t5Xk/zOS9LgMt3a4ll0tgj+S1EZzzQySaLxyZVYRMRgfAKZCh7tMg1zghHOLKz5/AGPkxnY4x72MQVdTW5YHQQUkkIXSeMEQl9lYJx7TZYJPH61ppKZAHP64hXSg/zSLJBiCr/WjNdCcdcAwBZ4d2WB30Dp6WaJr3gJJ7IP06l2omogK8Yd8WEmnCDmOYa/2TpbefmELhITwEcPGpWQLQz5HYVx9ytJRRTOYutmXByw== 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=cgadgypCpY0Bup2cm2eIgXzzK1/K2PuRnKDtbF3RSuE=; b=ZlyQDBbghUTlcmIDTCYjfMq9p0/UP7OOICaV7Cm3M2cUjAY3ICeVR3OWae+P3H+JwgxY4mLYVd7Z1tgYH0cmoZRp1afhvzmhjpZgSqsHnVZ7FhV3AG9olZNgdeCFgdkv2NhFhm+FSi8+tszJR5cv2cLXVfmwF6+rPFcK0hy022MN1Es19OhnGASB6YarOZj52gvVQIoHXJe/5QH1XTifB0S48K+qJOyFofIpMPpRt8RoZcxXjr3ta2fuiboMYgYqrSrTXRDCSDejp5G8RgFr70OS2O5T3uYgGy4UvcEGkMWyBuVhO/HOuO2VLXhVi1E0kiIlb/tgTbyWkUhIMM6kuA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=hotmail.de; dmarc=pass action=none header.from=hotmail.de; dkim=pass header.d=hotmail.de; arc=none Received: from AM6EUR05FT006.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc11::34) by AM6EUR05HT118.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc11::344) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2856.17; Wed, 25 Mar 2020 11:50:04 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com (10.233.240.55) by AM6EUR05FT006.mail.protection.outlook.com (10.233.241.54) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2856.17 via Frontend Transport; Wed, 25 Mar 2020 11:50:04 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:F312F06631CC2C3493D8DB0C36FD384F95D67363E33ECF9308B5A5A0F1A3D863; UpperCasedChecksum:CE77E93CFB531A66A0B2C47895C8B89CEA4AC9F53818099DDC9A41D9CC95027B; SizeAsReceived:8326; Count:50 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.2835.023; Wed, 25 Mar 2020 11:50:04 +0000 Subject: Re: [PATCHv2] Fix an undefined behavior in record_line To: Andrew Burgess Cc: "gdb-patches@sourceware.org" References: <20200324091013.GT3317@embecosm.com> <20200325110845.GV3317@embecosm.com> From: Bernd Edlinger Message-ID: Date: Wed, 25 Mar 2020 12:50:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 In-Reply-To: <20200325110845.GV3317@embecosm.com> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-ClientProxiedBy: AM0PR02CA0100.eurprd02.prod.outlook.com (2603:10a6:208:154::41) To AM6PR03MB5170.eurprd03.prod.outlook.com (2603:10a6:20b:ca::23) X-Microsoft-Original-Message-ID: <4e729e1b-f7c5-3bc9-630c-28919e0c2b9e@hotmail.de> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.1.101] (92.77.140.102) by AM0PR02CA0100.eurprd02.prod.outlook.com (2603:10a6:208:154::41) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2835.18 via Frontend Transport; Wed, 25 Mar 2020 11:50:03 +0000 X-Microsoft-Original-Message-ID: <4e729e1b-f7c5-3bc9-630c-28919e0c2b9e@hotmail.de> X-TMN: [wGEa6+eVqheV00ukk14+Z+/IthywjB24] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 50 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: 063c1342-9c80-4ad4-cf87-08d7d0b2a83d X-MS-TrafficTypeDiagnostic: AM6EUR05HT118: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: JPX3eeBCh10yDNuMyXtuF9A+MBt29W24gBwLEynWvfQbSNA5dmtiehDgRuraKT7m7VuGZ8GNzoA55txGuE2RPSAIWrxHm/9z4XMf97Bew7W5uM+v1CS0V+9GX7ZimRxD7HxWWZJ8+e0oLReWeFBGxw7CX8/loSJRkE+FtwCCLHm++DguSaRN0gnXxXvwK+h/ X-MS-Exchange-AntiSpam-MessageData: PW7KX2DCpkSjVeSTy4UY5krMJ2+A2A1J6iZhzDBp37a4K8lZdLQI1GepdsgIjYQKyYMCy32q42k3zCDfW6jnnIv/uU5Ivbe+JMMWgQcBArcRjsS4qXBhm76cEsJtQabPo1tJUfnnrxTWNfmXtknl0g== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 063c1342-9c80-4ad4-cf87-08d7d0b2a83d X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Mar 2020 11:50:04.5291 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6EUR05HT118 X-Spam-Status: No, score=-21.8 required=5.0 tests=BAYES_00, FORGED_MUA_MOZILLA, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, MSGID_FROM_MTA_HEADER, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 25 Mar 2020 11:50:08 -0000 On 3/25/20 12:08 PM, Andrew Burgess wrote: > * Bernd Edlinger [2020-03-24 11:20:25 +0100]: > >> >> >> On 3/24/20 10:10 AM, Andrew Burgess wrote: >>> * Bernd Edlinger [2020-03-23 22:25:42 +0100]: >>> >>>> On 3/22/20 4:25 AM, Bernd Edlinger wrote: >>>>> On 3/13/20 12:55 PM, Bernd Edlinger wrote: >>>>>> Additionally do not completely remove symbols >>>>>> at the same PC than the end marker, instead >>>>>> make them non-is-stmt breakpoints. >>>>>> >>>>>> Also fix the condition when the line table need to be resized, >>>>>> that was wasting one element. >>> >>> I suspect this commit message has evolved overtime - having the first >>> word be "additionally" seems a little strange. >>> >> >> I'll re-think the commit message, thanks. >> >>>>>> >>>>>> 2020-03-10 Bernd Edlinger >>>>>> * buildsym.c (record_line): Fix ub and preserve lines at eof. >>> >>> Typo: ub -> up >>> >>>>>> --- >>>>>> gdb/buildsym.c | 28 +++++++++++----------------- >>>>>> 1 file changed, 11 insertions(+), 17 deletions(-) >>>>>> >>>>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c >>>>>> index 7155db3..960a36c 100644 >>>>>> --- a/gdb/buildsym.c >>>>>> +++ b/gdb/buildsym.c >>>>>> @@ -695,7 +695,7 @@ struct blockvector * >>>>>> } >>>>>> } >>>>>> >>>>>> - if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length) >>>>>> + if (subfile->line_vector->nitems >= subfile->line_vector_length) >>>>>> { >>>>>> subfile->line_vector_length *= 2; >>>>>> subfile->line_vector = (struct linetable *) >>>>>> @@ -705,27 +705,21 @@ struct blockvector * >>>>>> * sizeof (struct linetable_entry)))); >>>>>> } >>> >>> This part seems separate to what comes below I think. This should be >>> a separate commit. >>> >> >> Okay, good point. That should be easy. >> >>>>>> >>>>>> - /* Normally, we treat lines as unsorted. But the end of sequence >>>>>> - marker is special. We sort line markers at the same PC by line >>>>>> - number, so end of sequence markers (which have line == 0) appear >>>>>> - first. This is right if the marker ends the previous function, >>>>>> - and there is no padding before the next function. But it is >>>>>> - wrong if the previous line was empty and we are now marking a >>>>>> - switch to a different subfile. We must leave the end of sequence >>>>>> - marker at the end of this group of lines, not sort the empty line >>>>>> - to after the marker. The easiest way to accomplish this is to >>>>>> - delete any empty lines from our table, if they are followed by >>>>>> - end of sequence markers. All we lose is the ability to set >>>>>> - breakpoints at some lines which contain no instructions >>>>>> - anyway. */ >>>>>> + /* The end of sequence marker is special. We need to reset the >>>>>> + is_stmt flag on previous lines at the same PC, otherwise these >>>>>> + lines may cause problems. All we lose is the ability to set >>>>>> + breakpoints at some lines which contain no instructions >>>>>> - anyway. */ >>> >>> You need to expand on what "problems" means here. Someone coming back >>> to this code in the future will have no idea why we're making this >>> change, and with no tests for this commit they can't even try to >>> figure out the "problems" by looking at a test. >>> >> >> I will try to explain that better, yes. >> >>>>>> if (line == 0 && subfile->line_vector->nitems > 0) >>>>>> { >>>>>> - e = subfile->line_vector->item + subfile->line_vector->nitems - 1; >>>>>> - while (subfile->line_vector->nitems > 0 && e->pc == pc) >>>>>> + e = subfile->line_vector->item + subfile->line_vector->nitems; >>>>>> + do >>>>>> { >>>>>> e--; >>>>>> - subfile->line_vector->nitems--; >>>>>> + if (e->pc != pc || e->line == 0) >>>>>> + break; >>>>>> + e->is_stmt = 0; >>>>>> } >>>>>> + while (e > subfile->line_vector->item); >>>>>> } >>>>>> >>>>>> e = subfile->line_vector->item + subfile->line_vectoms++; >>>>>> >>>> >>>> Andrew, this is the place where currently the is-stmt entries >>>> are deleted. With your is-stmt patch this code is executed in more >>>> cases than before. Therefore I would suggest to convert them >>>> to !is_stmt lines for now, but maybe in the long run add a new flag >>>> that allows them to be used in the file:line case, but make these >>>> lines behave differently when stepping, I am only trying to fix >>>> the case where you step out of the subroutine. >>> >>> I'm super uncomfortable with any code that changes is-stmt to >>> !is-stmt, as I worry about what we might be giving up. You say "All >>> we lose is the ability to set breakpoints at some lines which contain >>> no instructions anyway.", but I'll need to work through some examples >>> to see what this actually means in practice before I can be happy with >>> this change. >>> >> >> There is no pressure from my side to do anything about it. >> I am just saying is-stmt -> !is-stmt is better than removing >> is-stmt lines that are at the same PC by chance. > > You're absolutely right, I miss-understood what was going on here. I > think if you split the two parts of the patch, and could expand on the > description a bit then this should be fine. > > My understanding of the "problem" here is that lines appear > within one subfile at the same address that we switch to some other > subfile. As such I think, the address will be attributed to the > second subfile, and we shouldn't be reporting lines for the first > subfile. > > Hopefully you can expand that more with your understanding. > I am just a bit tired in the moment :) as I did not get much sleep lately. I think just be patient with me, it is on my TODO list, but I better take the time I need. Thanks Bernd. > Thanks, > Andrew > > > >> >> I will come up with an updated patch, eventually, but will need >> to spend more time on the openssl project now, to meet the schedule for the >> next release. >> >> >> Bernd.