Re: [patch] Change cplus_specific to an alocated struct

[Tom Tromey <tromey@redhat.com>]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> I plan to expand the cplus_specific struct to store template
Sami> information needed, of course, only for C++. Tom suggested that I move
Sami> the substruct out of struct general_symbol_info and change its
Sami> reference to a pointer to be assigned to the struct if allocated.

Sami> Thoughts ?

I think the idea is fine.

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.

However, this takes a general_symbol_info and so, presumably, can be
used for partial symbols.  But, modifying a partial symbol is a no-no,
because they are stored in a bcache.

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.

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.

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.

Tom