Re: [PATCH] Fix gdb FilteringTypePrinter (again)

[Jonathan Wakely <jwakely@redhat.com>]

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




> So I removed it as useless
> and redundant with this function default value.
>
> >
> >
> >
> >> +        self.template = template
> >>         self.name = name
> >> +        self.targ1 = targ1
> >>         self.enabled = True
> >>
> >>     class _recognizer(object):
> >>         "The recognizer class for FilteringTypePrinter."
> >>
> >> -        def __init__(self, match, name):
> >> -            self.match = match
> >> +        def __init__(self, template, name, targ1):
> >> +            self.template = template
> >>             self.name = name
> >> +            self.targ1 = targ1
> >>             self.type_obj = None
> >>
> >>         def recognize(self, type_obj):
> >>             """
> >> -            If type_obj starts with self.match and is the same type as
> >> +            If type_obj starts with self.template and is the same
> >> type as
> >>             self.name then return self.name, otherwise None.
> >>             """
> >>             if type_obj.tag is None:
> >>                 return None
> >>
> >>             if self.type_obj is None:
> >> -                if not type_obj.tag.startswith(self.match):
> >> +                if self.targ1 is not None:
> >> +                    if not
> >> type_obj.tag.startswith('{}<{}'.format(self.template, self.targ1)):
> >
> > I wonder if we should make targ1 a gdb.Type object, not just a string.
> > That seems like it would be better. The add_on_type_printer function
> > could still accept a string, and then call gdb.lookup_type(targ1) to
> > get a gdb.Type. We can change that later though.
> >
> > OK for trunk with the comments fixed to say "specialization" again.
> >
> Now committed.
>