From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4525 invoked by alias); 14 Jun 2010 19:29:35 -0000 Received: (qmail 4515 invoked by uid 22791); 14 Jun 2010 19:29:34 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Jun 2010 19:29:28 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5EJTQQG018960 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 14 Jun 2010 15:29:26 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5EJTP3R016412; Mon, 14 Jun 2010 15:29:26 -0400 Received: from [10.15.16.55] (toner.yyz.redhat.com [10.15.16.55]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o5EJTPB6007478; Mon, 14 Jun 2010 15:29:25 -0400 Message-ID: <4C167FCA.7030300@redhat.com> Date: Mon, 14 Jun 2010 19:29:00 -0000 From: sami wagiaalla User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc12 Thunderbird/3.0.4 MIME-Version: 1.0 To: tromey@redhat.com CC: gdb-patches@sourceware.org Subject: Re: [patch] Change cplus_specific to an alocated struct References: <4BFD4230.3030600@redhat.com> In-Reply-To: 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: 2010-06/txt/msg00326.txt.bz2 > Sami> char * > Sami> ada_decode_symbol (const struct general_symbol_info *gsymbol) > Sami> { > Sami> - char **resultp = > Sami> - (char **)&gsymbol->language_specific.cplus_specific.demangled_name; > Sami> - if (*resultp == NULL) > Sami> + char *result = symbol_get_cplus_demangled_name (gsymbol); > > I don't think this change is correct -- but FWIW I don't think the > existing code is correct, either. > > The code is written this way because this function caches the demangled > name in the cplus_specific struct. Your change removes this caching. > Totally missed this :) Badness of the original code aside my patch can be corrected by using the setter function. > All this reminds me -- you should look at bcache utilization and memory > use before and after your changes to make sure we aren't hitting a > memory use regression here. There is some command that will show you > bcache statistics, though I forget what it is offhand. > I used "maint print statistics" there are no changes in the "Total memory used for *" fields. But that differ seem to differ even between runs of the same version. Here are the results of a couple of diffs: $ diff before after 790c790 < Hash table population: 43% --- > Hash table population: 44% 793c793 < Maximum hash chain length: 5 --- > Maximum hash chain length: 6 [swagiaal@toner build]$ diff before after 237c237 < Hash table population: 45% --- > Hash table population: 44% 632c632 < Hash table population: 55% --- > Hash table population: 53% 793c793 < Maximum hash chain length: 4 --- > Maximum hash chain length: 6 > Sami> +char * > Sami> +symbol_get_cplus_demangled_name (const struct general_symbol_info *gsymbol) > Sami> +{ > Sami> + if (gsymbol->language_specific.cplus_specific != NULL) > Sami> + return gsymbol->language_specific.cplus_specific->demangled_name; > Sami> + > Sami> + return NULL; > Sami> +} > > Perhaps we should have different types of structs in the > language_specific union, and have functions like this examine the > language field. > > It seems to me that soon we're going to want to add a bunch of > C++-specific fields, and we don't want to unnecessarily penalize the > other languages with our baggage. > > If you were planning that for a follow-on patch, that is fine -- but > also the sort of thing that it is handy to note in your submissions. > I wasn't really planing one :D, but what do you think of this: We leave the current struct as is and rename cplus_specific to mangled_lang_specific (or just mangled_lang). And the the union we add a cplus_specific that managed as things are in this patch, and is actually cplus_specific ? > Sami> +/* Initialize the cplus_specific structure. 'cplus_specific' is intended to > Sami> + be allocated lazily. So this should only be called if the structure is > Sami> + needed. */ > Sami> +static void > Sami> +symbol_init_cplus_specific (struct general_symbol_info *gsymbol, > Sami> + struct objfile *objfile) > > Initializing lazily is usually good, but it breaks the bcache. > However perhaps this can be fixed by having the partial symbol code > force the issue. > I think that is just a wrong use of lazy here. I meant to say initialize it /if/ it is going to be used rather than when... symbol_init_cplus_specific is called from symbol_set_names where the bcache is updated.