From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-ve1eur02olkn080e.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe06::80e]) by sourceware.org (Postfix) with ESMTPS id 60555385B834 for ; Tue, 24 Mar 2020 10:20:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 60555385B834 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=mNzVmNxrj9D6t0rLc3Gl8MaL1eP8hX0MuE02rqxvhGcjXqrV90bL9qFVJYGZ4SZIkqSIrEo5h0pbYmYhktuzYeiNr3kBq07yph0lQ6HyQZBfbHYOVBa4pPY4CwSPB7hOwKXKmlJ0w6IuhR57StfEfJJMaily8RLDAq5LqkkX4vp/IRfy2HIo5WVLeST1CAlu6yOso3Ve8/FHNkftru7joyXJQia7A6upMZ+SHO5m8QFZP+ec3DuvcgVQKhIz8+GcoBbTczSsK4b84rG0KMICs+QqNTGGW173mdKStkyY0TnRlZkusTAW27eWxpnxMWDDKoXhrZF/wpr8ueJGZMhHtg== 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=SH1dVuhptg0m4qrd3Kow7RmzI7T0SF0ZRS5MOE5Ihyw=; b=Lkn7ZmBBWCSBUH6ZMLKJcfiIdytqdyzVwbFnWn+6D7WcVhrtY4bbvCUr5y9ZzXrwr340SlFS4VaLrvkRegWCVJCzN1Q0GwQq36z2GbaklA3xy1rJdq4T0Y+nwyE8lwe1cdN0Co3/H1EE4P11+NrRLI1mV2Dj3bdNKuJlqxg/Ws0pUdvKcjHw9S2MivHhNAGvYRlHz7vzuFeCwia2+EfZT8t9HCNuOlG5mbr+Mi4rIyeiRZ8gtOfnmKC/GpHYimgTmNYH770epARpaYmsHNDq+y+PbcwieLZE8A7TJP+HAr8yVR+712E0Xgh7oFsV7svhlAV7T8YuYex7jCVW7mCw7g== 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 HE1EUR02FT027.eop-EUR02.prod.protection.outlook.com (2a01:111:e400:7e1d::39) by HE1EUR02HT173.eop-EUR02.prod.protection.outlook.com (2a01:111:e400:7e1d::193) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2814.13; Tue, 24 Mar 2020 10:20:26 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com (10.152.10.60) by HE1EUR02FT027.mail.protection.outlook.com (10.152.10.72) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2814.13 via Frontend Transport; Tue, 24 Mar 2020 10:20:26 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:9AE575241E2EAD24588ABA2AB317237F739B424A168F663E7BB9821109DF8A37; UpperCasedChecksum:7DA847439B5BA2E91129B70EE97CCB346D5506898FF51D12DC0810253AB48540; SizeAsReceived:8206; 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; Tue, 24 Mar 2020 10:20:26 +0000 Subject: Re: [PATCHv2] Fix an undefined behavior in record_line To: Andrew Burgess Cc: "gdb-patches@sourceware.org" References: <20200324091013.GT3317@embecosm.com> From: Bernd Edlinger Message-ID: Date: Tue, 24 Mar 2020 11:20:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 In-Reply-To: <20200324091013.GT3317@embecosm.com> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-ClientProxiedBy: AM4PR0902CA0017.eurprd09.prod.outlook.com (2603:10a6:200:9b::27) To AM6PR03MB5170.eurprd03.prod.outlook.com (2603:10a6:20b:ca::23) X-Microsoft-Original-Message-ID: <11b34bca-c9c1-0149-81ee-03be8c264d5e@hotmail.de> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.1.101] (92.77.140.102) by AM4PR0902CA0017.eurprd09.prod.outlook.com (2603:10a6:200:9b::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2835.19 via Frontend Transport; Tue, 24 Mar 2020 10:20:26 +0000 X-Microsoft-Original-Message-ID: <11b34bca-c9c1-0149-81ee-03be8c264d5e@hotmail.de> X-TMN: [U7OJBnBVN7XKLQ3cB17FViknjJd7zWsp] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 50 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: 2f568590-528b-4976-0daa-08d7cfdcf854 X-MS-TrafficTypeDiagnostic: HE1EUR02HT173: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 0X8FwJd1BZln9Pf8V2SSvebC4bEYIcsv9L6SBu92QibD7eBXBYMbj8nsRw0lTda9RB0bA4kkFCxh5LGKw0M06SLCq2p/EQMG2pyszfXzQH2xqLO4EUkubvXU8ocUhb/h6x0g7MSmjAgGMOpqKS+hTuItl3DyItF9/Q2FMn1jw3jPhw9gHIwBb4Drnz3mUEFD X-MS-Exchange-AntiSpam-MessageData: xwRttWvxlbUAZ3+hKE8AaIIknbFXC0eK6fVOBTl+agFL8siKn/qS/JU+n4P5Hm3BgJ9dCU8CfZBqMeMTtCjtYK1T7KMLbggB2bEYDQfldEyGJw07G8jDItj+NgPwV4egQ1oMZ8MqxWaON1zalasFTQ== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2f568590-528b-4976-0daa-08d7cfdcf854 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Mar 2020 10:20:26.6364 (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: HE1EUR02HT173 X-Spam-Status: No, score=-24.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: Tue, 24 Mar 2020 10:20:30 -0000 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. 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.