Re: [PATCH] Fix gdb FilteringTypePrinter (again)

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8993 bytes --]

On Wed, 16 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Wed, 16 Nov 2022 at 11:35, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Wed, 16 Nov 2022 at 06:04, François Dumont <frs.dumont@gmail.com> wrote:
> > >
> > > On 15/11/22 17:17, Jonathan Wakely wrote:
> > > > On 06/10/22 19:38 +0200, François Dumont wrote:
> > > >> Hi
> > > >>
> > > >> Looks like the previous patch was not enough. When using it in the
> > > >> context of a build without dual abi and versioned namespace I started
> > > >> having failures again. I guess I hadn't rebuild everything properly.
> > > >>
> > > >> This time I think the problem was in those lines:
> > > >>
> > > >>             if self.type_obj == type_obj:
> > > >>                 return strip_inline_namespaces(self.name)
> > > >>
> > > >> I've added a call to gdb.types.get_basic_type so that we do not compare
> > > >> a type with its typedef.
> > > >>
> > > >> Thanks for the pointer to the doc !
> > > >>
> > > >> Doing so I eventually use your code Jonathan to make FilteringTypeFilter
> > > >> more specific to a given instantiation.
> > > >>
> > > >>     libstdc++: Fix gdb FilteringTypePrinter
> > > >>
> > > >>     Once we found a matching FilteringTypePrinter instance we look for
> > > >> the associated
> > > >>     typedef and check that the returned Python Type is equal to the
> > > >> Type to recognize.
> > > >>     But gdb Python Type includes properties to distinguish a typedef
> > > >> from the actual
> > > >>     type. So use gdb.types.get_basic_type to check if we are indeed on
> > > >> the same type.
> > > >>
> > > >>     Additionnaly enhance FilteringTypePrinter matching mecanism by
> > > >> introducing targ1 that,
> > > >>     if not None, will be used as the 1st template parameter.
> > > >>
> > > >>     libstdc++-v3/ChangeLog:
> > > >>
> > > >>             * python/libstdcxx/v6/printers.py (FilteringTypePrinter):
> > > >> Rename 'match' field
> > > >>             'template'. Add self.targ1 to specify the first template
> > > >> parameter of the instantiation
> > > >>             to match.
> > > >>             (add_one_type_printer): Add targ1 optional parameter,
> > > >> default to None.
> > > >>             Use gdb.types.get_basic_type to compare the type to
> > > >> recognize and the type
> > > >>             returned from the typedef lookup.
> > > >>             (register_type_printers): Adapt calls to
> > > >> add_one_type_printers.
> > > >>
> > > >> Tested under Linux x86_64 normal, version namespace with or without dual
> > > >> abi.
> > > >>
> > > >> François
> > > >>
> > > >> diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py
> > > >> b/libstdc++-v3/python/libstdcxx/v6/printers.py
> > > >> index 0fa7805183e..52339b247d8 100644
> > > >> --- a/libstdc++-v3/python/libstdcxx/v6/printers.py
> > > >> +++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
> > > >> @@ -2040,62 +2040,72 @@ def add_one_template_type_printer(obj, name,
> > > >> defargs):
> > > >>
> > > >> class FilteringTypePrinter(object):
> > > >>     r"""
> > > >> -    A type printer that uses typedef names for common template
> > > >> specializations.
> > > >> +    A type printer that uses typedef names for common template
> > > >> instantiations.
> > > >>
> > > >>     Args:
> > > >> -        match (str): The class template to recognize.
> > > >> +        template (str): The class template to recognize.
> > > >>         name (str): The typedef-name that will be used instead.
> > > >> +        targ1 (str): The first template argument.
> > > >> +            If arg1 is provided (not None), only template
> > > >> instantiations with this type
> > > >> +            as the first template argument, e.g. if
> > > >> template='basic_string<targ1'
> > > >>
> > > >> -    Checks if a specialization of the class template 'match' is the
> > > >> same type
> > > >> +    Checks if an instantiation of the class template 'template' is
> > > >> the same type
> > > >>     as the typedef 'name', and prints it as 'name' instead.
> > > >>
> > > >> -    e.g. if an instantiation of std::basic_istream<C, T> is the same
> > > >> type as
> > > >> +    e.g. for template='basic_istream', name='istream', if any
> > > >> instantiation of
> > > >> +    std::basic_istream<C, T> is the same type as std::istream then
> > > >> print it as
> > > >> +    std::istream.
> > > >> +
> > > >> +    e.g. for template='basic_istream', name='istream', targ1='char',
> > > >> if any
> > > >> +    instantiation of std::basic_istream<char, T> is the same type as
> > > >>     std::istream then print it as std::istream.
> > > >>     """
> > > >
> > > > These are template specializations, not instantiations. Please undo
> > > > the changes to the comments, because the comments are 100% correct
> > > > now, and would become wrong with this patch.
> > > >
> > > > template<class T, class U> struct foo { };
> > > > using F = foo<int, int>; // #1
> > > > template<class T> struct foo<T, void> { }; // #2
> > > > template<> struct foo<void, void> { }; // #3
> > > >
> > > > #1 is a *specialization* of the class template foo. It is
> > > > *instantiated* when you construct one or depend on its size, or its
> > > > members.
> > > > #2 is a *partial specialization* and #3 is an explicit specialization.
> > > > But #1 is a speclialization, not an instantiation.
> > > >
> > > > Instantiation is a process that happens during compilation. A
> > > > specialization is a type (or function, or variable) generated from a
> > > > template by substituting arguments for the template parameters. The
> > > > python type printer matches specializations.
> > >
> > > Lesson learned, thanks.
> > >
> > > Maybe comment on line 169 is wrong then. I think there is a clue in the
> > > function name 'is_specialization_of' :-)
> >
> > Good point! Thanks, I'll fix it.
> >
> > >
> > > >
> > > >>
> > > >> -    def __init__(self, match, name):
> > > >> -        self.match = match
> > > >> +    def __init__(self, template, name, targ1):
> > > >
> > > > Is there a reason to require targ1 here, instead of making it
> > > > optional, by using =None as the default?
> > >
> > > In your original, and I know untested, proposal it was not working.
> > >
> > > The function add_one_type_printer was missing to pass its targ1
> > > parameter to the FilteringTypePrinter ctor but thanks to the default
> > > value it was un-noticed by the interpreter.
> >
> > My untested patch had this, which adds it, doesn't it?
> >
> > -def add_one_type_printer(obj, match, name):
> > -    printer = FilteringTypePrinter('std::' + match, 'std::' + name)
> > +def add_one_type_printer(obj, match, name, targ1 = None):
> > +    printer = FilteringTypePrinter('std::' + match, 'std::' + name, targ1)
> >      gdb.types.register_type_printer(obj, printer)
> >      if _versioned_namespace:
> >          ns = 'std::' + _versioned_namespace
> > -        printer = FilteringTypePrinter(ns + match, ns + name)
> > +        printer = FilteringTypePrinter(ns + match, ns + name, targ1)
> >          gdb.types.register_type_printer(obj, printer)
> >
> >
> > I think FilteringTypePrinter should be usable without specifying None
> > explicitly as the argument. Even if we don't actually use it that way
> > today, it seems like a better API. If the argument is optional, then
> > the idiomatic way to express that is to give it a default, not require
> > None to be passed.
> >
> > I'll add that default argument, but first I need to figure out why I'm
> > seeing new failures for libfundts.cc with -D_GLIBCXX_USE_CXX11_ABI=0.
> > Your patch has introduced this new error:
> >
> > $12 = Python Exception <class 'gdb.error'>: No type named
> > std::experimental::fundamentals_v1::any::_Manager_internal<std::__cxx11::basic_string<char,
> > std::char_traits<char>, std::allocator<char> >>.
> > got: $12 = Python Exception <class 'gdb.error'>: No type named
> > std::experimental::fundamentals_v1::any::_Manager_internal<std::__cxx11::basic_string<char,
> > std::char_traits<char>, std::allocator<char> >>.
> > FAIL: libstdc++-prettyprinters/libfundts.cc print as
>
> The problem happens here in StdExpAnyPrinter:
>
>             mgrname = m.group(1)
>             # FIXME need to expand 'std::string' so that gdb.lookup_type works
>             if 'std::string' in mgrname:
>                 mgrname = re.sub("std::string(?!\w)",
> str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
>
>             mgrtype = gdb.lookup_type(mgrname)
>
> After your patch, gdb.lookup_type('std::string').strip_typedefs() is
> returning std::__cxx11::basic_string<char,...> which is not the
> correct type for this specialization of the any manager function. It
> contains a std::basic_string<char,...>.

And here's the rather disgusting "fix". I'll push to trunk.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3671 bytes --]

commit 04291b4a677b569caecfc0231e9ddac3041b1cb5
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 16 12:22:04 2022

    libstdc++: Fix std::any pretty printer
    
    The recent changes to FilteringTypePrinter affect the result of
    gdb.lookup_type('std::string') in StdExpAnyPrinter, causing it to always
    return the std::__cxx11::basic_string specialization. This then causes a
    gdb.error exception when trying to lookup the std::any manager type for
    a specliaization using that string, but that manager was never
    instantiated in the program. This causes FAILs when running the tests
    with -D_GLIBCXX_USE_CXX11_ABI=0:
    
    FAIL: libstdc++-prettyprinters/libfundts.cc print as
    FAIL: libstdc++-prettyprinters/libfundts.cc print as
    
    The ugly solution used in this patch is to repeat the lookup for every
    type that std::string could be a typedef for, and hope it only works for
    one of them.
    
    libstdc++-v3/ChangeLog:
    
            * python/libstdcxx/v6/printers.py (StdExpAnyPrinter): Make
            expansion of std::string in manager name more robust.

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 19c70d12035..1abf0a4bce3 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -1272,9 +1272,34 @@ class StdExpAnyPrinter(SingleObjContainerPrinter):
             mgrname = m.group(1)
             # FIXME need to expand 'std::string' so that gdb.lookup_type works
             if 'std::string' in mgrname:
-                mgrname = re.sub("std::string(?!\w)", str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
-
-            mgrtype = gdb.lookup_type(mgrname)
+                # This lookup for std::string might return the __cxx11 version,
+                # but that's not necessarily the one used by the std::any
+                # manager function we're trying to find.
+                strings = {str(gdb.lookup_type('std::string').strip_typedefs())}
+                # So also consider all the other possible std::string types!
+                s = 'basic_string<char, std::char_traits<char>, std::allocator<char> >'
+                quals = ['std::', 'std::__cxx11::', 'std::' + _versioned_namespace]
+                strings |= {q+s for q in quals} # set of unique strings
+                mgrtypes = []
+                for s in strings:
+                    try:
+                        x = re.sub("std::string(?!\w)", s, m.group(1))
+                        # The following lookup might raise gdb.error if the
+                        # manager function was never instantiated for 's' in the
+                        # program, because there will be no such type.
+                        mgrtypes.append(gdb.lookup_type(x))
+                    except gdb.error:
+                        pass
+                if len(mgrtypes) != 1:
+                    # FIXME: this is unlikely in practice, but possible for
+                    # programs that use both old and new string types with
+                    # std::any in a single program. Can we do better?
+                    # Maybe find the address of each type's _S_manage and
+                    # compare to the address stored in _M_manager?
+                    raise ValueError('Cannot uniquely determine std::string type used in std::any')
+                mgrtype = mgrtypes[0]
+            else:
+                mgrtype = gdb.lookup_type(mgrname)
             self.contained_type = mgrtype.template_argument(0)
             valptr = None
             if '::_Manager_internal' in mgrname: