From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12500 invoked by alias); 6 May 2008 17:43:04 -0000 Received: (qmail 12489 invoked by uid 22791); 6 May 2008 17:43:02 -0000 X-Spam-Check-By: sourceware.org Received: from qnxmail.qnx.com (HELO nimbus.ott.qnx.com) (209.226.137.76) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 06 May 2008 17:42:45 +0000 Received: from [10.42.100.129] (min.ott.qnx.com [10.42.100.129]) by nimbus.ott.qnx.com with SMTP (Microsoft Exchange Internet Mail Service Version 5.5.2653.13) id J6QQVKT6; Tue, 6 May 2008 13:42:42 -0400 Message-ID: <48209891.3000400@qnx.com> Date: Tue, 06 May 2008 18:39:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.14 (Windows/20080421) MIME-Version: 1.0 Newsgroups: gmane.comp.gdb.patches To: Aleksandar Ristovski , gdb-patches@sourceware.org Subject: Re: [patch] Do not add partial_symbol again and again to the list References: <20080211210935.GA31767@caradoc.them.org> <47B0C0F4.4090302@qnx.com> <20080211214750.GA1953@caradoc.them.org> <47B0C7C6.9090605@qnx.com> <20080211223056.GA3833@caradoc.them.org> <47B0CF8A.6080306@qnx.com> <20080211225314.GA5832@caradoc.them.org> <47B27EB1.6030606@qnx.com> <20080503205413.GA22704@caradoc.them.org> <481F58ED.5070207@qnx.com> <20080506141754.GA17275@caradoc.them.org> In-Reply-To: <20080506141754.GA17275@caradoc.them.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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 X-SW-Source: 2008-05/txt/msg00243.txt.bz2 Message-ID: <20080506183900.Bksj-DJ8ZY7ZsYTWXTCnHavNvt9ic2dDel02usfEcOU@z> Daniel Jacobowitz wrote: > On Mon, May 05, 2008 at 02:58:53PM -0400, Aleksandar Ristovski wrote: >>>> +const void * >>>> +bcache (const void *addr, int length, struct bcache *bcache) >>>> +{ >>>> + return bcache_data (addr, length, bcache); >>>> +} >>>> + >>>> +void * >>>> +bcache_added (const void *addr, int length, struct bcache *bcache, >>>> + int *added) >>> It should return a const pointer, like bcache. Also the indentation >>> on the second line is too deep. >> bcache_added is more like bcache_data which returns void*. It would make >> sense to return void const* but then I would have to change const-ness in >> many places (too many: I would leave that for some other patch). > > This really is not OK. Take a look at the explanation of a bcache: > > A bcache is a data structure for factoring out duplication in > read-only structures. Yes, but only while loading partial symbols; I am not aware it is being used after psymbols have been loaded. > > If you modify the pointed-to object then things go wrong: we put > duplicates in the cache which is what it's supposed to avoid. And you > modify the other copies which share the same bcache entry, corrupting > them. > > If you don't want to fix the constification issues right now, then > just call this deprecated_bcache_added and we can fix it later; you're > not making it any worse. But this is something you have to understand > about the bcache. Understood and agreed. As you said, this patch does not make things any worse than what it is now. >> >> The attached is revised patch with the changes above; additionally, it >> calls new add_partial_symbol_to_global_list for all partial symbols that >> are being added to the global list. I don't see how. Each reader treats psymbols in its own way (unless I am missing something here). > > This is not symbol-reader specific, right? There's nothing that makes > it safe for DWARF2 but unsafe for stabs? > > If so, a smaller patch would be do just do this in add_psymbol_to_list > if list == &objfile->global_symbols. > Yes it could be done that way, I thought what I proposed was 'cleaner'... but either way it would work. I am not, however, happy with the patch completely - I think we can optimize it a bit more. Both struct partial_symbol and struct symbol are space critical and we should save everything we (reasonably) can. My "feeling" is that we could use struct symbol for both, populate only the "partial_symbol" portion and then, as we load full symbols, "promote" the existing allocated structures to full symbols; some operations would not repeat (like name construction or fixup_[p]?symbol_section); initial memory allocation would be greater, but in the long run I would expect that gdb would use less memory than it does now. Savings I am hoping to see from such approach are: overall lower memory consumption, much lower number of memory alloc's, and saving some redundant operations we do for both psyms and syms. Or, maybe, we could go step by step: commit this patch (providing you find it good enough) and then continue with more extensive changes? Either way, let me know what you think. Thanks, Aleksandar