[PATCH v3][PR symtab/30520 1/4] gdb/symtab: check name matches before expanding a CU

[PATCH v3][PR symtab/30520 1/4] 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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 049ee4d52ff..e2f010b7849 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16694,9 +16694,25 @@ 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;
+
+		  if (entry->lang != language_unknown)
+		    {
+		      const language_defn *lang_def = language_def (entry->lang);
+		      lookup_name_info last_segment_lookup_name (
+			last_name.data (), symbol_name_match_type::FULL,
+			false, true);
+		      symbol_name_matcher_ftype *name_matcher
+			= lang_def->get_symbol_name_matcher
+			  (last_segment_lookup_name);
+		      if (!name_matcher (entry->canonical,
+					 last_segment_lookup_name, nullptr))
+			continue;
+		    }
+	      }
 	    }
 	  else
 	    {
-- 
2.34.1

[PATCH v3][PR symtab/30520 2/4] gdb/symtab: reuse last segment lookup name info by creating it outside the loop

[Dmitry Neverov]

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

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e2f010b7849..a49ade9dc31 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16643,6 +16643,9 @@ cooked_index_functions::expand_symtabs_matching
 	= lookup_name_without_params.split_name (lang);
       std::string last_name (name_vec.back ());
 
+      lookup_name_info last_segment_lookup_name (
+	last_name, symbol_name_match_type::FULL, completing, true);
+
       for (const cooked_index_entry *entry : table->find (last_name,
 							  completing))
 	{
@@ -16702,9 +16705,6 @@ cooked_index_functions::expand_symtabs_matching
 		  if (entry->lang != language_unknown)
 		    {
 		      const language_defn *lang_def = language_def (entry->lang);
-		      lookup_name_info last_segment_lookup_name (
-			last_name.data (), symbol_name_match_type::FULL,
-			false, true);
 		      symbol_name_matcher_ftype *name_matcher
 			= lang_def->get_symbol_name_matcher
 			  (last_segment_lookup_name);
-- 
2.34.1

[PATCH v3][PR symtab/30520 3/4] gdb/symtab: compute match_type outside the loop

[Dmitry Neverov]

It will be used for all segments in a qualified name,
not only the last one.
---
 gdb/dwarf2/read.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a49ade9dc31..f2842f0f581 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16637,6 +16637,9 @@ cooked_index_functions::expand_symtabs_matching
     language_ada
   };
 
+  symbol_name_match_type match_type
+    = lookup_name_without_params.match_type ();
+
   for (enum language lang : unique_styles)
     {
       std::vector<std::string_view> name_vec
@@ -16693,8 +16696,6 @@ cooked_index_functions::expand_symtabs_matching
 	     "x::a::b".  */
 	  if (symbol_matcher == nullptr)
 	    {
-	      symbol_name_match_type match_type
-		= lookup_name_without_params.match_type ();
 	      if ((match_type == symbol_name_match_type::FULL
 		   || (lang != language_ada
 		       && match_type == symbol_name_match_type::EXPRESSION)))
-- 
2.34.1

[PATCH v3][PR symtab/30520 4/4] gdb/symtab: use symbol name matcher for all segments in a qualified name

[Dmitry Neverov]

---
 gdb/dwarf2/read.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index f2842f0f581..e8416905163 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16644,12 +16644,16 @@ cooked_index_functions::expand_symtabs_matching
     {
       std::vector<std::string_view> name_vec
 	= lookup_name_without_params.split_name (lang);
-      std::string last_name (name_vec.back ());
-
-      lookup_name_info last_segment_lookup_name (
-	last_name, symbol_name_match_type::FULL, completing, true);
+      std::vector<std::string> name_str_vec (name_vec.begin (), name_vec.end ());
+      std::vector<lookup_name_info> segment_lookup_names;
+      segment_lookup_names.reserve (name_vec.size ());
+      for (auto &segment_name : name_str_vec)
+	{
+	  segment_lookup_names.emplace_back (segment_name,
+	    symbol_name_match_type::FULL, completing, true);
+	}
 
-      for (const cooked_index_entry *entry : table->find (last_name,
+      for (const cooked_index_entry *entry : table->find (name_str_vec.back (),
 							  completing))
 	{
 	  QUIT;
@@ -16678,13 +16682,24 @@ cooked_index_functions::expand_symtabs_matching
 	    {
 	      /* If we ran out of entries, or if this segment doesn't
 		 match, this did not match.  */
-	      if (parent == nullptr
-		  || strncmp (parent->name, name_vec[i - 1].data (),
-			      name_vec[i - 1].length ()) != 0)
+	      if (parent == nullptr)
 		{
 		  found = false;
 		  break;
 		}
+	      if (parent->lang != language_unknown)
+		{
+		  const language_defn *lang_def = language_def (parent->lang);
+		  symbol_name_matcher_ftype *name_matcher
+		    = lang_def->get_symbol_name_matcher
+		      (segment_lookup_names[i-1]);
+		  if (!name_matcher (parent->canonical,
+				     segment_lookup_names[i-1], nullptr))
+		    {
+		      found = false;
+		      break;
+		    }
+		}
 
 	      parent = parent->get_parent ();
 	    }
@@ -16708,9 +16723,9 @@ cooked_index_functions::expand_symtabs_matching
 		      const language_defn *lang_def = language_def (entry->lang);
 		      symbol_name_matcher_ftype *name_matcher
 			= lang_def->get_symbol_name_matcher
-			  (last_segment_lookup_name);
+			  (segment_lookup_names.back ());
 		      if (!name_matcher (entry->canonical,
-					 last_segment_lookup_name, nullptr))
+					 segment_lookup_names.back (), nullptr))
 			continue;
 		    }
 	      }
-- 
2.34.1

Re: [PATCH v3][PR symtab/30520 4/4] gdb/symtab: use symbol name matcher for all segments in a qualified name

[Dmitry Neverov]

I've run tests on x86-64 linux. The only unexpected failure was

FAIL: gdb.base/empty-host-env-vars.exp: env_var_name=HOME: show
index-cache directory

But it doesn't seem to be related to this patch.

--
Dmitry

Re: [PATCH v3][PR symtab/30520 1/4] gdb/symtab: check name matches before expanding a CU

[Tom Tromey]

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

Thank you for your continued work on this.

Dmitry> +		      const language_defn *lang_def = language_def (entry->lang);
Dmitry> +		      lookup_name_info last_segment_lookup_name (
Dmitry> +			last_name.data (), symbol_name_match_type::FULL,
Dmitry> +			false, true);

Normally in gdb the open paren would go on the next line, like

> +		      lookup_name_info last_segment_lookup_name
> +			(last_name.data (), symbol_name_match_type::FULL,
> +			 false, true);

... depending on how long the lines are -- it would be split after some
',' if each line would then fit in 80 columns.

Dmitry> +		      symbol_name_matcher_ftype *name_matcher
Dmitry> +			= lang_def->get_symbol_name_matcher
Dmitry> +			  (last_segment_lookup_name);

This also looks weird by gdb rules.

Tom

Re: [PATCH v3][PR symtab/30520 2/4] gdb/symtab: reuse last segment lookup name info by creating it outside the loop

[Tom Tromey]

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

Dmitry> +      lookup_name_info last_segment_lookup_name (
Dmitry> +	last_name, symbol_name_match_type::FULL, completing, true);

Formatting should also be changed here.  Thanks.

Tom

Re: [PATCH v3][PR symtab/30520 3/4] gdb/symtab: compute match_type outside the loop

[Tom Tromey]

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

Dmitry> It will be used for all segments in a qualified name,
Dmitry> not only the last one.

Looks good, thank you.
Approved-By: Tom Tromey <tom@tromey.com>


Tom

Re: [PATCH v3][PR symtab/30520 4/4] gdb/symtab: use symbol name matcher for all segments in a qualified name

[Tom Tromey]

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

Thanks for the patch.

Dmitry>        std::vector<std::string_view> name_vec
Dmitry>  	= lookup_name_without_params.split_name (lang);
Dmitry> -      std::string last_name (name_vec.back ());
Dmitry> -
Dmitry> -      lookup_name_info last_segment_lookup_name (
Dmitry> -	last_name, symbol_name_match_type::FULL, completing, true);
Dmitry> +      std::vector<std::string> name_str_vec (name_vec.begin (), name_vec.end ());

I was going to suggest changing split_name to simply return this vector,
but I'd forgotten that cooked-index.c also uses it.

Dmitry> +      std::vector<lookup_name_info> segment_lookup_names;
Dmitry> +      segment_lookup_names.reserve (name_vec.size ());
Dmitry> +      for (auto &segment_name : name_str_vec)
Dmitry> +	{
Dmitry> +	  segment_lookup_names.emplace_back (segment_name,
Dmitry> +	    symbol_name_match_type::FULL, completing, true);

This should be indented more to line up with "segment_name" from the
previous line.

Otherwise this looks good to me.

Tom

Re: [PATCH v3][PR symtab/30520 1/4] gdb/symtab: check name matches before expanding a CU

[Dmitry.Neverov]

Tom Tromey <tom@tromey.com> writes:
> Dmitry> +		      symbol_name_matcher_ftype *name_matcher
> Dmitry> +			= lang_def->get_symbol_name_matcher
> Dmitry> +			  (last_segment_lookup_name);
>
> This also looks weird by gdb rules.

Do I understand correctly that it should look like this:

		      symbol_name_matcher_ftype *name_matcher = lang_def
			->get_symbol_name_matcher (last_segment_lookup_name);

?

How do I send an updated patch? I've read the Contribution Checklist,
but didn't find the answer. Do I amend the commit and 'git send-mail' it
again with proper '--in-reply-to'?

--
Dmitry

Re: [PATCH v3][PR symtab/30520 1/4] gdb/symtab: check name matches before expanding a CU

[Tom Tromey]

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

Dmitry> Tom Tromey <tom@tromey.com> writes:
Dmitry> +		      symbol_name_matcher_ftype *name_matcher
Dmitry> +			= lang_def->get_symbol_name_matcher
Dmitry> +			  (last_segment_lookup_name);
>> 
>> This also looks weird by gdb rules.

Dmitry> Do I understand correctly that it should look like this:

I'd write:

		      symbol_name_matcher_ftype *name_matcher
			= lang_def->get_symbol_name_matcher (last_segment_lookup_name);

Sometimes wrapping is unavoidable but there's also

		      symbol_name_matcher_ftype *name_matcher
			= (lang_def->get_symbol_name_matcher
                             (last_segment_lookup_name));

Dmitry> How do I send an updated patch? I've read the Contribution Checklist,
Dmitry> but didn't find the answer. Do I amend the commit and 'git send-mail' it
Dmitry> again with proper '--in-reply-to'?

I just re-send it with --reroll-count=...

I don't normally worry about in-reply-to.  Not sure, maybe I should?
For series I often use b4 these days, though I've also been bitten by
its bizarre behavior, so I'm not sure I can really recommend it.

Tom

Re: [PATCH v3][PR symtab/30520 1/4] gdb/symtab: check name matches before expanding a CU

[Dmitry.Neverov]

Tom Tromey <tom@tromey.com> writes:

> I just re-send it with --reroll-count=...

Thanks, I fixed formatting and re-sent all pathces:

https://sourceware.org/pipermail/gdb-patches/2024-May/209010.html

-- 
Dmitry