From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51976 invoked by alias); 11 Sep 2018 20:38:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 51953 invoked by uid 89); 11 Sep 2018 20:38:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=dangerous X-HELO: gateway32.websitewelcome.com Received: from gateway32.websitewelcome.com (HELO gateway32.websitewelcome.com) (192.185.145.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Sep 2018 20:38:47 +0000 Received: from cm10.websitewelcome.com (cm10.websitewelcome.com [100.42.49.4]) by gateway32.websitewelcome.com (Postfix) with ESMTP id 414371B2EA for ; Tue, 11 Sep 2018 15:38:46 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id zpQsfboGTBcCXzpQsf2TMJ; Tue, 11 Sep 2018 15:38:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=6aVSf2kaEBJLp7/dbIbG0JLipFVKNR91NF9MnJh0guo=; b=bl5gMRya946AoF6b2n2jRkhcVW QL5vMb0mU9M0m8Ibw2/Kf9QqmspKdRYyOg3YwkkTqOlRkmY+wYGPjnHfSmYt68nlWnmDOlyVtvUdI aZplNIHuXG/GHfhdxyGe62867; Received: from 97-122-189-157.hlrn.qwest.net ([97.122.189.157]:60536 helo=pokyo) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1fzpQs-000e3D-0P; Tue, 11 Sep 2018 15:38:46 -0500 From: Tom Tromey To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC References: <20180911141147.14107-1-andrew.burgess@embecosm.com> Date: Tue, 11 Sep 2018 20:38:00 -0000 In-Reply-To: <20180911141147.14107-1-andrew.burgess@embecosm.com> (Andrew Burgess's message of "Tue, 11 Sep 2018 15:11:47 +0100") Message-ID: <87lg87rcq3.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-09/txt/msg00344.txt.bz2 >>>>> "Andrew" == Andrew Burgess writes: Andrew> I ran into the following issue after runnig valgrind on GDB. Andrew> Calling TYPE_ALLOC or TYPE_ZALLOC currently allocates memory from Andrew> either the objfile obstack or by using malloc. The problem with this Andrew> is that types are allocated either on the objfile obstack, or on the Andrew> gdbarch obstack. Thank you for the patch. I found the analysis very clear, so thanks for that too. Andrew> This commit ensures that auxiliary type data is allocated from the Andrew> same obstack as the type itself, which should reduce leaked memory. I'm curious whether this is something you intend to do. gdb has many things that are allocated for the lifetime of the process... Andrew> The one problem case that I found with this change was in eval.c, Andrew> where in one place we allocate a local type structure, and then used Andrew> TYPE_ZALLOC to allocate some space for the type. This local type is Andrew> neither object file owned, nor gdbarch owned, and so the updated Andrew> TYPE_ALLOC code is unable to find an objstack to allocate space on. Andrew> My proposed solution for this issue is that the space should be Andrew> allocated with a direct call to xzalloc. We could extend TYPE_ALLOC Andrew> to check for type->gdbarch being null, and then fall back to a direct Andrew> call to xzalloc, however, I think making the rare case of a local type Andrew> requiring special handling is not a bad thing, this serves to Andrew> highlight that clearing up the memory will require special handling Andrew> too. I went and refreshed my memory of this area. My first thought was that this would be dangerous because -- surely -- there was code allocating a type on the heap and manually managing it. I was remembering Python, but I think we didn't do this because we bailed on the whole "type GC" idea. So, there no problems that I could find. I suppose if the type from fake_method ever ends up somewhere it shouldn't -- passed to something that uses TYPE_ALLOC -- gdb will crash. I wonder if get_type_arch should assert that the result is not NULL. Andrew> * gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile Andrew> obstack, or the gdbarch obstack. Andrew> (TYPE_ZALLOC): Rewrite using TYPE_ALLOC. Andrew> * eval.c (fake_method::fake_method): Call xzalloc directly for a Andrew> type that is neither object file owned, nor gdbarch owned. Andrew> +/* See comment on TYPE_ALLOC. */ Andrew> + Andrew> +#define TYPE_ZALLOC(t,size) (memset (TYPE_ALLOC (t, size), 0, size)) Would you mind seeing what happens if you use obstack_zalloc here instead? It seems like that would give us some extra compile-time safety. If it fails, that would be interesting. I think the eval.c change should have a comment explaining why TYPE_ZALLOC is not used. This is ok with that nit changed. If obstack_zalloc works the I think that variant would be better. thanks, Tom