From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110178 invoked by alias); 29 Aug 2015 21:26:28 -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 110166 invoked by uid 89); 29 Aug 2015 21:26:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-ob0-f175.google.com Received: from mail-ob0-f175.google.com (HELO mail-ob0-f175.google.com) (209.85.214.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sat, 29 Aug 2015 21:26:27 +0000 Received: by obbwr7 with SMTP id wr7so66285154obb.2 for ; Sat, 29 Aug 2015 14:26:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=2ANpqKuWblQgER0ULJiTrR/Wfa73WRxLNCj0Hc8+AMM=; b=AVvUAvkxAFiczNGmv/GtwIRduIPGfn216bj+6VT8b3+rplRDxyjKqFmQJuUAvoDyNA XyoFgVB/zOUpo1LgYXLdozFqKiOIRZOP3oXsrNqToB27u2iWYXNH0Udni78QNiP047gL wGYMM+Dw/viQf88GxAgcKLL71GqwcC/IbvEUjegb09ViRqdMTzhAPRMZC1GCRW39GCma FB6zvETQiJGGBPxpF9Ui8iEf6H/rrpFecoEbTYoQZBVsWgnzKu+lr/i2pkmzMunEtCG1 6e98hTBh+vBAUXlQgS5cRDffmPZYDO0MvFncj9rugBvtsf7RRrzfy6hQomM8kslNvzyW Uh4g== X-Gm-Message-State: ALoCoQm9wSfT1kxnF7oiJRX2fUCcrvrFG+BfwhjQsDOkbSzClMcGbXhA5ChmHET5MxI0V46zkF6A X-Received: by 10.182.142.39 with SMTP id rt7mr9208260obb.3.1440883584967; Sat, 29 Aug 2015 14:26:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.115.105 with HTTP; Sat, 29 Aug 2015 14:26:05 -0700 (PDT) In-Reply-To: References: <1435631281-31970-1-git-send-email-patrick@parcs.ath.cx> From: Patrick Palka Date: Sat, 29 Aug 2015 21:26:00 -0000 Message-ID: Subject: Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch To: Doug Evans Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-08/txt/msg00842.txt.bz2 On Sat, Aug 29, 2015 at 2:05 PM, Doug Evans wrote: > Patrick Palka writes: >> For the command "gdb gdb" valgrind currently reports 100s of individual >> memory leaks, 500 of which originate solely out of the function >> alloc_type_arch. This function allocates a "struct type" associated >> with the given gdbarch using malloc but apparently the types allocated >> by this function are never freed. >> >> This patch fixes these leaks by making the function alloc_type_arch >> allocate these gdbarch-associated types on the gdbarch obstack instead >> of on the general heap. Since, from what I can tell, the types >> allocated by this function are all fundamental "wired-in" types, such >> types would not benefit from more granular memory management anyway. >> They would likely live as long as the gdbarch is alive so allocating >> them on the gdbarch obstack makes sense. >> >> With this patch, the number of individual vargrind warnings emitted for >> the command "gdb gdb" drops from ~800 to ~300. >> >> Tested on x86_64-unknown-linux-gnu. Does this fix make sense? It may >> not be ideal (more disciplined memory management may be?), but for the >> time being it helps reduce the noise coming from valgrind. Or maybe >> there is a good reason that these types are allocated on the heap... > > OOC, Are these real leaks? > I wasn't aware of arches ever being freed. > I'm all for improving the S/N ratio of valgrind though. Yeah this is just to reduce the number of warnings emitted by valgrind. They aren't real leaks (and don't amount to much memory-wise). > > How are you running valgrind? > I'd like to recreate this for myself. I'm doing "valgrind --leak-check=full --log-file=valgrind.log $GDB $GDB" and then exiting via "quit" at the prompt. > > btw, while looking into this I was reading copy_type_recursive. > The first thing I noticed was this: > > if (! TYPE_OBJFILE_OWNED (type)) > return type; > ... > new_type = alloc_type_arch (get_type_arch (type)); > > and my first thought was "Eh? We're copying an objfile's type but we're > storing it with the objfile's arch???" There's nothing in the name > "copy_type_recursive" that conveys this subtlety. > Then I realized this function is for, for example, saving the value > history when an objfile goes away (grep for it in value.c). > The copied type can't live with the objfile, it's going away, and there's > only one other place that it can live with: the objfile's arch (arches > never go away). I see. > > Then I read this comment for copy_type_recursive: > > /* Recursively copy (deep copy) TYPE, if it is associated with > OBJFILE. Return a new type allocated using malloc, a saved type if > we have already visited TYPE (using COPIED_TYPES), or TYPE if it is > not associated with OBJFILE. */ > > Note the "Return a new type using malloc" > > This comment is lacking: it would be really nice if it explained > *why* the new type is saved with malloc. This critical feature of this > routine is a bit subtle given the name is just "copy_type_recursive". > Or better yet change the visible (exported) name of the function to > "preserve_type", or some such just as its callers are named preserve_foo, > rename copy_type_recursive to preserve_type_recursive, make it static, > and have the former call the latter. [The _recursive in the name isn't really > something callers care about. If one feels it's important to include > this aspect in the name how about something like preserve_type_deep?] > > To make a long story short, I'm guessing that's the history behind why > alloc_type_arch used malloc (I know there's discussion of this > in the thread). > > At the least, we'll need to update copy_type_recursive's function > comment and change the malloc reference. Good points... I'll post a patch that removes the malloc reference in the documentation for copy_type_recursive. Some background for this change: The TYPE_OBJFILE_OWNED macro tells us who owns a given type, and according to the macro's documentation a type is always owned either by an objfile or by a gdbarch. Given this binary encoding of ownership it doesn't seem to make much sense for _any_ type to be allocated by malloc. All types should be allocated on an objfile obstack or on a gdbarch obstack. To support allocating a type by malloc, I think the type ownership scheme should have three possible cases: that a type is owned by an objfile, or that it's owned by a gdbarch, or that it's owned by the caller. This new last case could correspond to a type that's allocated by malloc instead of on an obstack. Does this make sense, or maybe I am misunderstanding what "owning" means here?