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

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

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

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

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
  

> 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
  

> 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
  

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