This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Change cplus_specific to an alocated struct



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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]