[Bug gdb/25663] New: dwarf2_name caching bug

[Bug gdb/25663] New: dwarf2_name caching bug

[vcollod at nvidia dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

            Bug ID: 25663
           Summary: dwarf2_name caching bug
           Product: gdb
           Version: HEAD
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: vcollod at nvidia dot com
  Target Milestone: ---

Hello!

While working on cuda-gdb, I noticed dwarf2_name does not behave as described.

It's supposed to return the name of a dwarf DIE, as described by the following
comment:

/* Get name of a die, return NULL if not found. Anonymous namespaces are
converted to their magic string. */

It does some processing, stores the results into a cache, then strips the C++
namespaces of the name. The last step should probably be done before storing
the result into the cache.

The following code sample should help reproduce the issue.

I did not fix it myself as I am concerned some code relies on this odd
behavior.

cat > dwarf2_name_repro.gdb <<EOF
b dwarf2_name
run
while 1
      fin
      printf "logged dwarf2_name(%p) = %s\n", (size_t)die, $
      continue
end
EOF

cat > dwarf2_name_repro.cc <<EOF
namespace TestSpace {
        typedef struct {
                int a;
        } type;
}

int main() {
        TestSpace::type test{42};
        return test.a;
}
EOF

make CFLAGS='-O0 -g' dwarf2_name_repro

gdb -q -batch -x dwarf2_name_repro.gdb -arg PATH_TO_GDB_WITH_DEBUG_INFO -ex
'quit' ./dwarf2_name_repro | grep logged | grep type

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[vcollod at nvidia dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

Victor Collod <vcollod at nvidia dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vcollod at nvidia dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
That code is relatively new, I think it's fine to switch
the order here.
I'll send a patch.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #2 from Tom Tromey <tromey at sourceware dot org> ---
Actually, I'm not completely sure it's a win to do this.
Code elsewhere also demangles and uses the bcache.
Maybe we get a memory savings by sharing the same strings.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Tom Tromey from comment #2)
> Actually, I'm not completely sure it's a win to do this.
> Code elsewhere also demangles and uses the bcache.
> Maybe we get a memory savings by sharing the same strings.

Ok, I looked again and I think it's fine.
partial_die_info::fixup does the same stuff but
before interning.
dwarf2_physname demangles but uses different parameters.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[vcollod at nvidia dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #4 from Victor Collod <vcollod at nvidia dot com> ---
> I'm not completely sure it's a win to do this.

What do you mean?

My concern is that calling the function two times with the same parameters can
return different results.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Victor Collod from comment #4)
> > I'm not completely sure it's a win to do this.
> 
> What do you mean?

In that comment I meant that we might save more memory
with the current approach.  But I no longer think that's true.

> My concern is that calling the function two times with the same parameters
> can return different results.

Yeah, I see what you mean.
That does seem bad.

The current approach turns out to be relied on :-(
See anonymous_struct_prefix.  Making a change to the order causes
gdb.cp/anon-struct.exp to fail.

But maybe we can separate the caching from the return value,
so that the call is idempotent.

This code is very messy.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #6 from Tom Tromey <tromey at sourceware dot org> ---
Created attachment 12369
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12369&action=edit
patch

This seems to work ok.
It just would be better if it cached the stripped name,
like partial_die_info::fixup does.
Also, canonicalizing the demangled name on the linkage
name attribute just seems bad.  Like, what if we want
to examine the actual linkage name for some reason?
Anyway this seems like a mild improvement, WDYT?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[vcollod at nvidia dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #7 from Victor Collod <vcollod at nvidia dot com> ---
(In reply to Tom Tromey from comment #6)
> It just would be better if it cached the stripped name,
> like partial_die_info::fixup does.

I'm not sure I understand why that couldn't be done, it some code relying on
the content of the cache?

> Also, canonicalizing the demangled name on the linkage
> name attribute just seems bad.  Like, what if we want
> to examine the actual linkage name for some reason?

That's the way the code currently works ¯\_(ツ)_/¯

> Anyway this seems like a mild improvement, WDYT?

It's indeed much better!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #8 from Tom Tromey <tromey at sourceware dot org> ---
> I'm not sure I understand why that couldn't be done, it some code relying on
> the content of the cache?

anonymous_struct_prefix examines the DW_STRING and wants to find
the entire demangled name there.  So, it isn't the cache exactly.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #9 from Tom Tromey <tromey at sourceware dot org> ---
https://sourceware.org/pipermail/gdb-patches/2020-March/166609.html

Once this goes in, I may repurpose this bug to be about
cleaning up the remaining weirdness in this area.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

--- Comment #10 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=67430cd00afcc270a27e44b10f9ef4249d554e66

commit 67430cd00afcc270a27e44b10f9ef4249d554e66
Author: Tom Tromey <tromey@adacore.com>
Date:   Mon Mar 16 15:00:52 2020 -0600

    Fix dwarf2_name caching bug

    PR gdb/25663 points out that dwarf2_name will cache a value in the
    bcache and then return a substring.  However, this substring return is
    only done on the branch that caches the value -- so if the function is
    called twice with the same arguments, it will return different values.

    This patch fixes this problem.

    This area is strange.  We cache the entire demangled string, but only
    return the suffix.  I looked at caching just the suffix, but it turns
    out that anonymous_struct_prefix assumes that the entire string is
    stored.  Also weird is that this code is demangling the linkage name
    and then storing the demangled form back into the linkage name
    attribute -- that seems bad, because what if some code wants to find
    the actual linkage name?

    Fixing these issues was non-trivial, though; and in the meantime this
    patch seems like an improvement.  Regression tested on x86-64
    Fedora 30.

    gdb/ChangeLog
    2020-03-16  Tom Tromey  <tromey@adacore.com>

            PR gdb/25663:
            * dwarf2/read.c (dwarf2_name): Strip leading namespaces after
            putting value into bcache.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[ssbssa at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

Hannes Domani <ssbssa at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ssbssa at sourceware dot org

--- Comment #11 from Hannes Domani <ssbssa at sourceware dot org> ---
(In reply to Sourceware Commits from comment #10)
> The master branch has been updated by Tom Tromey <tromey@sourceware.org>:
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;
> h=67430cd00afcc270a27e44b10f9ef4249d554e66
> 
> commit 67430cd00afcc270a27e44b10f9ef4249d554e66
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Mon Mar 16 15:00:52 2020 -0600
> 
>     Fix dwarf2_name caching bug
>     
>     PR gdb/25663 points out that dwarf2_name will cache a value in the
>     bcache and then return a substring.  However, this substring return is
>     only done on the branch that caches the value -- so if the function is
>     called twice with the same arguments, it will return different values.
>     
>     This patch fixes this problem.
>     
>     This area is strange.  We cache the entire demangled string, but only
>     return the suffix.  I looked at caching just the suffix, but it turns
>     out that anonymous_struct_prefix assumes that the entire string is
>     stored.  Also weird is that this code is demangling the linkage name
>     and then storing the demangled form back into the linkage name
>     attribute -- that seems bad, because what if some code wants to find
>     the actual linkage name?
>     
>     Fixing these issues was non-trivial, though; and in the meantime this
>     patch seems like an improvement.  Regression tested on x86-64
>     Fedora 30.
>     
>     gdb/ChangeLog
>     2020-03-16  Tom Tromey  <tromey@adacore.com>
>     
>             PR gdb/25663:
>             * dwarf2/read.c (dwarf2_name): Strip leading namespaces after
>             putting value into bcache.

Can this be closed?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25663] dwarf2_name caching bug

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25663

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Tom Tromey <tromey at sourceware dot org> ---
I think I meant to reuse this to clean up some other caching
weirdness -- but it's probably not really that important.

-- 
You are receiving this mail because:
You are on the CC list for the bug.