Re: [patch] Do not add partial_symbol again and again to the list

[Aleksandar Ristovski <aristovski at qnx dot com>]

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