Re: [PATCH] gdb: Skip useless minsym scan in variable-only symbol searches

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 26/09/26 01:34, Simon Marchi wrote:
> On 9/22/26 11:51 AM, Adhemerval Zanella wrote:
>> The symbol-listing command -symbol-info-* family goes through
>> global_symbol_searcher::search, which has 3 steps:
>>
>> 1. expand_symtabs (objfile, preg): make sure every compunit that could
>>    contain a match is expanded.  Returns found_msymbol = true if it
>>    saw a matching msymbol that has no debug info.
>>
>> 2. add_matching_symbols (...): walk the compunits and collect the real
>>    matches.
>>
>> 3. After the loop, a minsym fallback: if found_msymbol was set, or
>>    unconditionally for any variable search with no file filter, re-scan
>>    msymbols and append the ones without debug info (unless the searcher
>>    set exclude_minsyms).
>>
>> Step 1 had two phases, first objfile::search, which uses the
>> quick-symbol index to expand candidate compunits; then the pre-pass
>> for every msymbol in the objfile whose name matches the regexp:
>>
>> 1.1. function search: call find_compunit_symtab_for_pc (address).  This
>>      consults the quick functions and can expand the compunit containing
>>      that address.
>>
>> 1.2. variable search: call lookup_symbol_in_objfile_from_linkage_name,
>>      which does a linear scan of objfile->compunits () per lookup.  It
>>      can not expand anything; its only output is setting found_msymbol
>>      when the lookup fails.
>>
>> So for variables the pre-pass costs 'msymbols * expanded-compunits'
>> block lookups and produces exactly one bit of information (found_msymbol)
>> and this dominates the command execution time.
>>
>> And in step 3. these MI3 commands (search_module_symbols) calls
>> spec2.set_exclude_minsyms (true), which sets !m_exclude_minsyms to false.
> 
> The phrasing "sets !m_exclude_minsyms to false" really confused me,
> could you rephrase this?

Ack, I changed to:

  In step 3, the fallback condition also requires '!m_exclude_minsyms'. The
  MI commands that go through search_module_symbols call
  spec2.set_exclude_minsyms (true), so for them the fallback never runs.
  The value of found_msymbol is then ignored and the pre-pass work is wasted.

> 
>> The fallback never runs (found_msymbol is ignored), making the pre-pass
>> calculation not required.
>>
>> The fix is make SEARCH_VAR_DOMAIN skip the pre-pass.  For variable
>> searches with no filenames the fallback condition already contains
>> '(m_kind & SEARCH_VAR_DOMAIN) != 0' as an unconditional alternative.
>>
>> Since the pre-pass now only runs for searches that include
>> SEARCH_FUNCTION_DOMAIN, the lookup_symbol_in_objfile_from_linkage_name
>> branch of the conditional inside the loop is unreachable.
>>
>> Also update the comments, which still described the variable lookup and
>> claimed it forces symtabs to be read.  That was true when
>> lookup_symbol_in_objfile_from_linkage_name was added in commit
>> 422d65e705c7, but it no longer expands any symtab.
>>
>> -symbol-info-module-variables --module <M> on the 2000-module benchmark
>> drops from 90 s to 0.56 s cold.
> 
> Awesome.  It took me a while to convince myself that the change is
> correct, and how global_symbol_searcher works in the various cases.  One
> thing that was not obvious to me was that it only supports one kind of
> search at a time (you can't search for variables _and_ functions in one
> search), despite the use of domain_search_flags that would suggest the
> opposite.  In the process I made a few patches to tweak
> global_symbol_searcher to make it more readable, I'll post them after
> your patch it merged.

Ack.

> 
>>
>> Change-Id: I6019bcf80629f21cf6dee74d0e96fa788a70e220
>> ---
>>  gdb/symtab.c | 35 ++++++++++++++---------------------
>>  1 file changed, 14 insertions(+), 21 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 93684c14777..13824587e75 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4750,22 +4750,20 @@ global_symbol_searcher::expand_symtabs
>>       SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
>>       kind);
>>  
>> -  /* Here, we search through the minimal symbol tables for functions and
>> -     variables that match, and force their symbols to be read.  This is in
>> -     particular necessary for demangled variable names, which are no longer
>> -     put into the partial symbol tables.  The symbol will then be found
>> +  /* Here, we search through the minimal symbol tables for functions that
>> +     match, and force their symbols to be read.  The symbol will then be found
>>       during the scan of symtabs later.
>>  
>> -     For functions, find_pc_symtab should succeed if we have debug info for
>> -     the function, for variables we have to call
>> -     lookup_symbol_in_objfile_from_linkage_name to determine if the
>> -     variable has debug info.  If the lookup fails, set found_msymbol so
>> -     that we will rescan to print any matching symbols without debug info.
>> -     We only search the objfile the msymbol came from, we no longer search
>> -     all objfiles.  In large programs (1000s of shared libs) searching all
>> -     objfiles is not worth the pain.  */
>> +     The find_compunit_symtab_for_pc should succeed if we have debug info for
> 
> I would remove the "The" in this last line.

Ack.

> 
>> +     the function.  If it fails, set found_msymbol so that we will rescan to
>> +     print any matching symbols without debug info.  We only search the
>> +     objfile the msymbol came from, we no longer search all objfiles.
>> +
>> +     Variables are not handled here, and looking them up does not expand any
>> +     symtab.  When no file names were given the caller unconditionally rescans
>> +     the minimal symbols for SEARCH_VAR_DOMAIN.  */
>>    if (m_filenames.empty ()
>> -      && (kind & (SEARCH_VAR_DOMAIN | SEARCH_FUNCTION_DOMAIN)) != 0)
>> +      && (kind & SEARCH_FUNCTION_DOMAIN) != 0)
>>      {
>>        for (minimal_symbol *msymbol : objfile->msymbols ())
>>  	{
>> @@ -4780,18 +4778,13 @@ global_symbol_searcher::expand_symtabs
>>  		  || preg->exec (msymbol->natural_name (), 0,
>>  				 NULL, 0) == 0)
>>  		{
>> -		  /* An important side-effect of these lookup functions is
>> +		  /* An important side-effect of this lookup function is
>>  		     to expand the symbol table if msymbol is found, later
>>  		     in the process we will add matching symbols or
>>  		     msymbols to the results list, and that requires that
>>  		     the symbols tables are expanded.  */
>> -		  if ((kind & SEARCH_FUNCTION_DOMAIN) != 0
>> -		      ? (find_compunit_symtab_for_pc
>> -			 (msymbol->value_address (objfile)) == NULL)
>> -		      : (lookup_symbol_in_objfile_from_linkage_name
>> -			 (objfile, msymbol->linkage_name (),
>> -			  SEARCH_VFT)
>> -			 .symbol == NULL))
>> +		  if (find_compunit_symtab_for_pc
>> +			(msymbol->value_address (objfile)) == nullptr)
>>  		    found_msymbol = true;
> 
> One change I had locally to help me understand what this does was to
> rename found_msymbol to found_func_msymbol_without_debug_info.  Could
> you rename it as part of this patch, and rename the variable in
> global_symbol_searcher::search too?

Ack, I replaced 'found_msymbol' with 'found_func_msymbol_without_debug_info'
on both places.

> 
> LGTM with that fixed.
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

May I assume that I could push the patch with the above fixes?

Thanks for the review.

> 
> And thanks Thiago for testing.
> 
> Simon