On 29-03-2020 23:44, Simon Marchi wrote:
> On 2020-03-29 11:56 a.m., Tom de Vries wrote:
>> diff --git a/gdb/psympriv.h b/gdb/psympriv.h
>> index 0effedc4ec..4317392149 100644
>> --- a/gdb/psympriv.h
>> +++ b/gdb/psympriv.h
>> @@ -124,16 +124,26 @@ struct partial_symtab
>>    {
>>    }
>>  
>> -  /* Read the full symbol table corresponding to this partial symbol
>> -     table.  */
>> +  /* Psymtab expansion is done in two steps:
>> +     - a call to read_symtab
>> +     - while that call is in progress, calls to expand_psymtab can be made,
>> +       both for this psymtab, and its dependencies.
>> +     This makes a distinction between a toplevel psymtab (for which both
>> +     read_symtab and expand_psymtab will be called) and a non-toplevel
>> +     psymtab (for which only expand_psymtab will be called). The
>> +     distinction can be used f.i. to do things before and after all
>> +     dependencies of a top-level psymtab have been expanded.
>> +
>> +     Read the full symbol table corresponding to this partial symbol
>> +     table.  Typically calls expand_psymtab.  */
>>    virtual void read_symtab (struct objfile *) = 0;
>>  
>> -  /* Psymtab expansion is done in two steps.  The first step is a call
>> -     to read_symtab; but while that is in progress, calls to
>> -     expand_psymtab can be made.  */
>> +  /* Expand the full symbol table for this partial symbol table.  Typically
>> +     calls read_dependencies.  */
>>    virtual void expand_psymtab (struct objfile *) = 0;
>>  
>> -  /* Ensure that all the dependencies are read in.  */
>> +  /* Ensure that all the dependencies are read in.  Typically calls
>> +     expand_psymtab for each dependency.  */
>>    void read_dependencies (struct objfile *);
> 
> I don't think the "Typically" here is right.  This is not a virtual function that can
> be implemented/overriden by sub-classes, it's a helper that sub-classes call, that calls
> expand_psymtab on all dependencies.

Ack, comment updated accordingly.

>  As you probably saw, I renamed it to
> expand_dependencies to be more accurate (since it calls expand, not read).
> 

Ack, patch rebased.

> But otherwise, I am starting to think that it's the right thing to to, to call read_symtab
> on the includer (top-level psymtab) in dwarf2_include_psymtab::read_symtab.  To read in
> an include psymtab, we read in the includer.
> 
> Following this, I also came to the conclusion that dwarf2_include_psymtab::m_readin should
> probably be removed.  An include psymtab can't be read in on its own.  It can be considered
> read in when its includer is read in.  So it doesn't make sense to maintain a "read in" state
> separate from its includer.
> 

That makes sense to me.

> Here's the version I came up with, if you want to get inspiration.
> 

I've integrated it into my patch and re-tested.

Also, added test-case.

Thanks,
- Tom