Re: [RFC][PR symtab/30520] gdb/symtab: check name matches before expanding a CU

[Tom Tromey <tom@tromey.com>]

>>>>> Dmitry Neverov <dmitry.neverov@jetbrains.com> writes:

> From: "Dmitry.Neverov" <dmitry.neverov@jetbrains.com>
> The added check fixes the case when an unqualified lookup
> name without template arguments causes expansion of many CUs
> which contain the name with template arguments.

> This is similar to what dw2_expand_symtabs_matching_symbol does
> before expanding the CU.

Thank you for doing this.  It's been on my to-do list, but unfortunately
hadn't made it to the top yet.

> In the referenced issue the lookup name was wxObjectDataPtr and many
> CUs had names like wxObjectDataPtr<wxBitmapBundleImpl>. This caused
> their expansion and the lookup took around a minute. The added check
> helps to avoid the expansion and makes the symbol lookup to return in
> a second or so.

I think a regression test would be good.

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30520

I wonder if this also fixes

    https://sourceware.org/bugzilla/show_bug.cgi?id=31010

and maybe even

    https://sourceware.org/bugzilla/show_bug.cgi?id=31192

> ---
>  gdb/dwarf2/read.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 1b5e566bb01..ccb32f152b0 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -16882,9 +16882,24 @@ cooked_index_functions::expand_symtabs_matching
>  		= lookup_name_without_params.match_type ();
>  	      if ((match_type == symbol_name_match_type::FULL
>  		   || (lang != language_ada
> -		       && match_type == symbol_name_match_type::EXPRESSION))
> -		  && parent != nullptr)
> -		continue;
> +		       && match_type == symbol_name_match_type::EXPRESSION)))
> +		{
> +		  if (parent != nullptr)
> +		    continue;
> +
> +		  enum language cu_lang = entry->per_cu->lang (false);

After some recent changes you should just use 'entry->lang' now.

> +		  if (cu_lang != language_unknown)
> +		  {
> +		    const language_defn *lang_def = language_def (cu_lang);
> +		    symbol_name_matcher_ftype *name_matcher
> +		      = lang_def->get_symbol_name_matcher
> +		      (lookup_name_without_params);
> +		    auto_obstack temp_storage;
> +		    if (!name_matcher (entry->full_name(&temp_storage),
> +				       lookup_name_without_params, nullptr))
> +		      continue;

I get what this is doing, but I was hoping there is a better way.

Like, in your case:

> In the referenced issue the lookup name was wxObjectDataPtr and many
> CUs had names like wxObjectDataPtr<wxBitmapBundleImpl>. This caused

I wonder if the name matcher can be used on just the entry's canonical
name, without constructing the full name here.

Also, earlier there is a loop that checks the entry's context:

	  /* We've found the base name of the symbol; now walk its
	     parentage chain, ensuring that each component
	     matches.  */
[...]
	      if (parent == nullptr
		  || strncmp (parent->name, name_vec[i - 1].data (),
			      name_vec[i - 1].length ()) != 0)

This seems like another spot that should be using a language-specific
name matcher.

If this can't be done, and we do need to construct the full name, then I
wonder if the hunk you've changed can / should be merged with the 'else'
clause:

	  else
	    {
	      auto_obstack temp_storage;
	      const char *full_name = entry->full_name (&temp_storage);
	      if (!symbol_matcher (full_name))
		continue;
	    }

... because presumably this could lead to some extra filtering -- and
the fewer CUs that are expanded, the better.

Tom