Re: [PATCH][gdb] Fix missing symtab includes

[Tom de Vries <tdevries@suse.de>]

On 30-03-2020 19:42, Tom de Vries wrote:
> 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.

And committed.

Thanks,
- Tom