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

Aleksandar Ristovski
2008-02-12T01:08:16Z

> On Mon, Feb 11, 2008 at 05:43:22PM -0500, Aleksandar Ristovski wrote:
>> If one compilation unit has a list of symbols and they appear more than
>> once... do we really need to have all duplicate records in partial symbols
>> list? The partial symbol lookup (by symbol name) will find only the first
>> one matching and probably cause loading the full symbols at which point
>> all works as before.
>
> Sure. But your patches aren't checking one compilation unit, they're
> checking every compilation unit in the objfile at once. That's why
> they found duplicates. I don't think any one compilation unit will
> have a duplicate.
Then I am definitely missing something big. Please correct me where I'm wrong.

struct dwarf2_cu has a pointer to struct objfile, which (if I'm not mistaken)
get's allocated per compilation unit.

Further, struct objfile contains pointer to struct bcache: psymbol_cache which
gets initialized in objfile initialization (allocate_objfile function).
Therefore, there should be exactly one psymbol_cache per objfile.

bcache_data hashes objects in psymbol_cache, that is, it will hash only partial
symbols of that particular compilation unit.

There are two pointers to struct psymbol_allocation_list, both declared in
struct objfile: global_psymbols and static_psymbols. Therefore, there should be
exactly one of each per each compilation unit.

In dwarf2read.c we determine whether a symbol belongs to the global or static
psymbol list and we pass this list along with the objfile pointer to
add_psymbol_to_list function.

Now we get to the point where I made the change:
The first patch I submitted allows for having a psymbol listed in each list, but
only once (as opposed to many times before). The second patch I submitted will
prevent adding a partial symbol to both lists.

What am I missing?

>
>> Not sure if it is a valid indicator but I didn't have any regressions in
>> 'make check'.
>
> Yeah, I think I could write some testcases that were affected by this,
> but I'm not sure. It's tricky because if something else causes the
> full symtab to load, the problem won't appear.

Yes, that is the tricky part. We want lookup by symbol name to occur in order to
test this. Maybe adding a maintenance command that would explicitly lookup a
partial symbol by name?

>
> Maybe there's some way we can avoid needing psymbols for types at all.
> But I think we need to have a real design and some documentation for
> it instead of just accidentally omitting them.
>

Not sure how would that work. But I think it would make sense to have only one
global_psymbols list.