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

[RFC v2][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 bdfda31dc16..b125b2e69f0 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16690,9 +16690,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.39.2

[RFC v2][PR symtab/30520 2/4] gdb/symtab: reuse last segment lookup name info

[Dmitry Neverov]

Create lookup_name_info for the last segment in a qualified name
outside the loop. The symbol_name_match_type::SEARCH_NAME is used to
avoid redundant demangling: it was already performed in
lookup_name_without_params.split_name().
---
 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 b125b2e69f0..f539eb878a3 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16638,6 +16638,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::SEARCH_NAME, completing, true);
+
       for (const cooked_index_entry *entry : table->find (last_name,
 							  completing))
 	{
@@ -16698,9 +16701,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.39.2

[RFC v2][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 f539eb878a3..338e2bfbfa7 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16632,6 +16632,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
@@ -16689,8 +16692,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.39.2

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

[Dmitry Neverov]

---
 gdb/dwarf2/read.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 338e2bfbfa7..08a834e141b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16637,14 +16637,17 @@ cooked_index_functions::expand_symtabs_matching
 
   for (enum language lang : unique_styles)
     {
-      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::SEARCH_NAME, completing, true);
+      std::vector<std::string_view> name_vec = lookup_name_without_params
+	.split_name (lang);
+      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::SEARCH_NAME, 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;
@@ -16674,13 +16677,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 ();
 	    }
@@ -16704,9 +16718,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.39.2

[RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Dmitry Neverov]

The symbol_name_match_type::SEARCH_NAME cannot be used because
demangle_for_lookup_info doesn't initialize a demangled name
for it which makes a subsequent name matching to fail.
---
 gdb/dwarf2/read.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 08a834e141b..3f1d2e9a813 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16644,7 +16644,7 @@ cooked_index_functions::expand_symtabs_matching
       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::SEARCH_NAME, completing, true);
+	  segment_name, symbol_name_match_type::FULL, completing, true);
       }
 
       for (const cooked_index_entry *entry : table->find (name_str_vec.back (),
-- 
2.39.2

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Tom Tromey]

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

Dmitry> The symbol_name_match_type::SEARCH_NAME cannot be used because
Dmitry> demangle_for_lookup_info doesn't initialize a demangled name
Dmitry> for it which makes a subsequent name matching to fail.

I don't think I understand this comment, but anyway shouldn't this patch
simply be folded into an earlier one in the series?

I read through these and they seem basically reasonable to me -- in line
with what I was hoping for.  There are some minor formatting issues.

You didn't mention what testing you did, so I am curious about that.

thanks,
Tom

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Dmitry Neverov]

Tom> Dmitry> The symbol_name_match_type::SEARCH_NAME cannot be used because
Tom> Dmitry> demangle_for_lookup_info doesn't initialize a demangled name
Tom> Dmitry> for it which makes a subsequent name matching to fail.
Tom>
Tom> I don't think I understand this comment, but anyway shouldn't this patch
Tom> simply be folded into an earlier one in the series?

Sorry, this is the first time I submit a series. I've noticed a
test failure after I sent patches and wasn't sure how to update
them. What should I do in such a situation? Create [RFC v3]?

What I was trying to say in the commit message is that
demangle_for_lookup_info::demangle_for_lookup_info has the
following logic:

if (without_params != NULL)
  {
    if (lookup_name.match_type () != symbol_name_match_type::SEARCH_NAME)
      m_demangled_name = demangle_for_lookup (without_params.get (),
      lang, storage);
    return;
  }

So when symbol_name_match_type::SEARCH_NAME was used, the
demangled name wasn't initialized which made subsequent
name_matcher calls to not match. Switching to
symbol_name_match_type::FULL fixes this.

Tom> I read through these and they seem basically reasonable to me -- in line
Tom> with what I was hoping for.  There are some minor formatting issues.

Sorry, again. I've tried to follow the code style as closely as I
can, but have missed something. I will try to be more careful
next time. If there is some formatter I can run - please let me
know.

Tom> You didn't mention what testing you did, so I am curious about that.

I've done only manual testing in the project where I can
reproduce the problem with slow symbol lookup. I'm not sure how
to write an automatic test checking that CU is not expanded
unnecessarily. Are there any existing tests checking that?

On Wed, Feb 7, 2024 at 9:25 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Dmitry" == Dmitry Neverov <dmitry.neverov@jetbrains.com> writes:
>
> Dmitry> The symbol_name_match_type::SEARCH_NAME cannot be used because
> Dmitry> demangle_for_lookup_info doesn't initialize a demangled name
> Dmitry> for it which makes a subsequent name matching to fail.
>
> I don't think I understand this comment, but anyway shouldn't this patch
> simply be folded into an earlier one in the series?
>
> I read through these and they seem basically reasonable to me -- in line
> with what I was hoping for.  There are some minor formatting issues.
>
> You didn't mention what testing you did, so I am curious about that.
>
> thanks,
> Tom

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Tom Tromey]

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

Tom> Dmitry> The symbol_name_match_type::SEARCH_NAME cannot be used because
Tom> Dmitry> demangle_for_lookup_info doesn't initialize a demangled name
Tom> Dmitry> for it which makes a subsequent name matching to fail.

Tom> I don't think I understand this comment, but anyway shouldn't this patch
Tom> simply be folded into an earlier one in the series?

Dmitry> Sorry, this is the first time I submit a series. I've noticed a
Dmitry> test failure after I sent patches and wasn't sure how to update
Dmitry> them. What should I do in such a situation? Create [RFC v3]?

In this particular case, since the patch is updating an earlier patch in
the series, what I would probably do is combine the two patches -- and
add a comment at the spot explaining why FULL is used rather than
SEARCH_NAME.

Tom> I read through these and they seem basically reasonable to me -- in line
Tom> with what I was hoping for.  There are some minor formatting issues.

Dmitry> Sorry, again. I've tried to follow the code style as closely as I
Dmitry> can, but have missed something. I will try to be more careful
Dmitry> next time. If there is some formatter I can run - please let me
Dmitry> know.

Sadly there's no formatter.  We've tried but haven't found one we can
all agree to.

The main stuff to look out for, IME, is:

* braces on their own lines (like the patch we're replying to has an
  issue with this in the context)

* operators after a newline, not before

* no trailing "("

* space before "("

Tom> You didn't mention what testing you did, so I am curious about that.

Dmitry> I've done only manual testing in the project where I can
Dmitry> reproduce the problem with slow symbol lookup. I'm not sure how
Dmitry> to write an automatic test checking that CU is not expanded
Dmitry> unnecessarily. Are there any existing tests checking that?

Hmm... I think maybe gdb.dwarf2/debug-names-missing-cu.exp does this,
after a fashion.

For a new test you probably wouldn't need the DWARF assembler stuff, you
could just translate a simple test into testsuite form.

Anyway what I was really asking is if you ran the gdb testsuite.

Thanks again for this series.  Also, did I ask about / did we get
started on copyright assignment paperwork?

thanks,
Tom

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Dmitry Neverov]

Hi Tom,

Sorry for the late reply. I was going to finish all the paperwork before
continuing, but it takes too long.

> Anyway what I was really asking is if you ran the gdb testsuite.

I've run the testsuite locally. When patch is applied I get one
additional 'KFAIL ingdb.threads/process-dies-while-handling-bp.exp'.
But this test seems to be flaky on my machine and if I rerun it in
master, it also fails.

Do you think I should fix the formatting and re-submit the
series, or is it better to wait till all papers are signed and
accepted?

Dmitry

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Tom Tromey]

> Sorry for the late reply. I was going to finish all the paperwork before
> continuing, but it takes too long.

Do you know the current status of it?
Unfortunately we can't accept the patches without it.

>> Anyway what I was really asking is if you ran the gdb testsuite.

> I've run the testsuite locally. When patch is applied I get one
> additional 'KFAIL ingdb.threads/process-dies-while-handling-bp.exp'.
> But this test seems to be flaky on my machine and if I rerun it in
> master, it also fails.

Yeah, there are some flaky tests.

Tom

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Dmitry Neverov]

> Do you know the current status of it?
> Unfortunately we can't accept the patches without it.

I've sent all the papers, but we had a typo in a disclaimer, so I'm
back to getting them fixed
and signed. Hope it will be faster than the first round.

Dmitry

Re: [RFC v2][PR symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test

[Tom Tromey]

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

>> Do you know the current status of it?
>> Unfortunately we can't accept the patches without it.

Dmitry> I've sent all the papers, but we had a typo in a disclaimer, so
Dmitry> I'm back to getting them fixed and signed. Hope it will be
Dmitry> faster than the first round.

If this is definitely happening (like it wasn't completely derailed for
some reason) then IMO it's fine to update the patches and re-send them.
That might hasten their landing once the paperwork is done.

thanks,
Tom