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

Message ID 20240125084404.3536456-1-dmitry.neverov@jetbrains.com
State New
Headers
Series [RFC,v2,PR,symtab/30520] gdb/symtab: fix the gdb.cp/anon-struct test |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Dmitry.Neverov Jan. 25, 2024, 8:44 a.m. UTC
  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(-)
  

Comments

Tom Tromey Feb. 7, 2024, 8:25 p.m. UTC | #1
>>>>> "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
  
Dmitry.Neverov Feb. 9, 2024, 4:32 p.m. UTC | #2
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
  
Tom Tromey Feb. 9, 2024, 7:38 p.m. UTC | #3
>>>>> "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
  
Dmitry.Neverov March 18, 2024, 4:35 p.m. UTC | #4
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
  
Tom Tromey March 18, 2024, 5:58 p.m. UTC | #5
> 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
  
Dmitry.Neverov March 19, 2024, 12:16 p.m. UTC | #6
> 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
  
Tom Tromey March 22, 2024, 5:29 p.m. UTC | #7
>>>>> "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
  
Joel Brobecker May 4, 2024, 1:20 a.m. UTC | #8
Hi Dmitry (hey Tom),

Any news on the copyright assignment side? We're getting closer to
being ready for GDB 15 branching, and soon after that for GDB 15.1
release. If we haven't heard from them in a while, I think it would
be worth explaining to them that we're block on this, and that
if we can't get the assignment papers through soon, it'll me we'll
have to ship 15.1 without this enhancement, which would be a real
shame.

On Fri, Mar 22, 2024 at 11:29:02AM -0600, Tom Tromey wrote:
> >>>>> "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.

FWIW, that's an excellent idea. Any chance we could have those
submitted, and hopefully approved soon after?

Thank you!
  
Dmitry.Neverov May 6, 2024, 2:32 p.m. UTC | #9
> Any news on the copyright assignment side?

I've sent an updated assignment version and it seems to be ok. It is
sent to be countersigned by the FSF deputy director. I didn't get
updates after that.

> . Any chance we could have those submitted, and hopefully approved soon after?

I will re-submit the patches shortly.

--
Dmitry
  

Patch

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 (),