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

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

[Dmitry Neverov]

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.

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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30520
---
 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);
+		  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;
+		  }
+	      }
 	    }
 	  else
 	    {
-- 
2.39.2

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

[Tom Tromey]

>>>>> 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