From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45879 invoked by alias); 29 Aug 2015 18:06:33 -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 45858 invoked by uid 89); 29 Aug 2015 18:06:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f49.google.com Received: from mail-pa0-f49.google.com (HELO mail-pa0-f49.google.com) (209.85.220.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sat, 29 Aug 2015 18:06:30 +0000 Received: by padhm10 with SMTP id hm10so38826360pad.3 for ; Sat, 29 Aug 2015 11:06:29 -0700 (PDT) X-Received: by 10.68.192.225 with SMTP id hj1mr24884694pbc.67.1440871589150; Sat, 29 Aug 2015 11:06:29 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by smtp.gmail.com with ESMTPSA id cy10sm9308579pdb.13.2015.08.29.11.06.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 29 Aug 2015 11:06:28 -0700 (PDT) From: Doug Evans To: Patrick Palka Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Use gdbarch obstack to allocate types in alloc_type_arch References: <1435631281-31970-1-git-send-email-patrick@parcs.ath.cx> Date: Sat, 29 Aug 2015 18:06:00 -0000 In-Reply-To: <1435631281-31970-1-git-send-email-patrick@parcs.ath.cx> (Patrick Palka's message of "Mon, 29 Jun 2015 22:28:00 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00834.txt.bz2 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. How are you running valgrind? I'd like to recreate this for myself. 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). 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.