Re: [PATCH] gdb: cooked_index_functions::expand_symtabs_matching: require psymtab

[Lancelot SIX <Lancelot.Six@amd.com>]

Hi,

Thanks for the comments.

On 18/05/2022 21:09, Tom Tromey wrote:
> [CAUTION: External Email]
> 
>>>>>> Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index 37e9587e43e..489b857459b 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -18559,6 +18559,9 @@ cooked_index_functions::expand_symtabs_matching
>>         domain_enum domain,
>>         enum search_domain kind)
>>   {
>> +  /* Make sure that the psymtab have been read for objfile.  */
>> +  objfile->require_partial_symbols (false);
> 
> Why 'false' here, when psymtab.c uses 'true'?

This is because otherwise I see a regression in the testsuite (but this 
is probably not a good enough reason, see below).

> 
> Also, can this bug affect the other methods in cooked_index_functions?

I did not initially find other problems, at least not as clear as this 
one.  However the regression mentioned above is a tell I should have 
picked up.

In the regression, we see:

     set variable $x = 32
     Reading symbols from 
.../gdb/testsuite/outputs/gdb.python/py-events/py-events-shlib.so...
     (gdb) FAIL: gdb.python/py-events.exp: do something

This is because the symbol reading from py-events-shlib.so happens quite 
late.  If I compare with how this was done before the new indexer is 
activated (I used 600f5f702728f66ced24f8497c75c58ff442aeb6 as a 
reference), the psymbols for py-events-shlib.so are read from the 
find_pc_sect_compunit_symtab methad, which happens to be called much 
earlier.

So if I also add a objfile->require_partial_symbols call in 
cooked_index_functions::find_pc_sect_compunit_symtab, everything "looks 
good".

The various methods in psymbol_functions consistently uses 
require_partial_symbols.  This could be what should be done in various 
subclasses of quick_symbol_functions used in the new indexer.

However, I feel that it could be clearer to have this done in objfile 
before it actually calls the quick_symbol_functions. This makes it so 
that only the objfile implementation needs to take care of this (This is 
what was initially done with ALL_OBJFILE_PSYMTABS_REQUIRED when lazy 
loading was introduced).

WDYT?

I'll prepare a patch to do this shortly, including changes to address 
Pedro's comments.

Best,
Lancelot.
> 
> thanks,
> Tom