Re: [PATCH] Fix gdb FilteringTypePrinter (again)

["François Dumont" <frs.dumont@gmail.com>]

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' :-)

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