Re: [patch] Change cplus_specific to an alocated struct

[sami wagiaalla <swagiaal at redhat dot com>]

> 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.