public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix gdb FilteringTypePrinter (again)
@ 2022-10-06 17:38 François Dumont
  2022-11-14 17:57 ` François Dumont
  2022-11-15 16:17 ` Jonathan Wakely
  0 siblings, 2 replies; 8+ messages in thread
From: François Dumont @ 2022-10-06 17:38 UTC (permalink / raw)
  To: libstdc++; +Cc: gcc-patches

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

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


[-- Attachment #2: gdb_printers.patch --]
[-- Type: text/x-patch, Size: 8406 bytes --]

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.
     """
 
-    def __init__(self, match, name):
-        self.match = match
+    def __init__(self, template, name, targ1):
+        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)):
+                        # Filter didn't match.
+                        return None
+                elif not type_obj.tag.startswith(self.template):
                     # Filter didn't match.
                     return None
+
                 try:
                     self.type_obj = gdb.lookup_type(self.name).strip_typedefs()
                 except:
                     pass
-            if self.type_obj == type_obj:
-                return strip_inline_namespaces(self.name)
 
             if self.type_obj is None:
                 return None
 
-            # Workaround ambiguous typedefs matching both std:: and std::__cxx11:: symbols.
-            ambiguous = False
-            for ch in ('', 'w', 'u8', 'u16', 'u32'):
-                if self.name == 'std::' + ch + 'string':
-                    ambiguous = True
-                    break
+            if gdb.types.get_basic_type(self.type_obj) == gdb.types.get_basic_type(type_obj):
+                return strip_inline_namespaces(self.name)
 
-            if ambiguous:
+            # Workaround ambiguous typedefs matching both std:: and std::__cxx11:: symbols.
+            if self.template.split('::')[-1] == 'basic_string':
                 if self.type_obj.tag.replace('__cxx11::', '') == type_obj.tag.replace('__cxx11::', ''):
                     return strip_inline_namespaces(self.name)
 
@@ -2103,14 +2113,14 @@ class FilteringTypePrinter(object):
 
     def instantiate(self):
         "Return a recognizer object for this type printer."
-        return self._recognizer(self.match, self.name)
+        return self._recognizer(self.template, self.name, self.targ1)
 
-def add_one_type_printer(obj, match, name):
-    printer = FilteringTypePrinter('std::' + match, 'std::' + name)
+def add_one_type_printer(obj, template, name, targ1 = None):
+    printer = FilteringTypePrinter('std::' + template, 'std::' + name, targ1)
     gdb.types.register_type_printer(obj, printer)
-    if _versioned_namespace and not '__cxx11' in match:
+    if _versioned_namespace and not '__cxx11' in template:
         ns = 'std::' + _versioned_namespace
-        printer = FilteringTypePrinter(ns + match, ns + name)
+        printer = FilteringTypePrinter(ns + template, ns + name, targ1)
         gdb.types.register_type_printer(obj, printer)
 
 def register_type_printers(obj):
@@ -2120,29 +2130,33 @@ def register_type_printers(obj):
         return
 
     # Add type printers for typedefs std::string, std::wstring etc.
-    for ch in ('', 'w', 'u8', 'u16', 'u32'):
-        add_one_type_printer(obj, 'basic_string', ch + 'string')
-        add_one_type_printer(obj, '__cxx11::basic_string', ch + 'string')
+    for ch in (('', 'char'),
+               ('w', 'wchar_t'),
+               ('u8', 'char8_t'),
+               ('u16', 'char16_t'),
+               ('u32', 'char32_t')):
+        add_one_type_printer(obj, 'basic_string', ch[0] + 'string', ch[1])
+        add_one_type_printer(obj, '__cxx11::basic_string', ch[0] + 'string', ch[1])
         # Typedefs for __cxx11::basic_string used to be in namespace __cxx11:
         add_one_type_printer(obj, '__cxx11::basic_string',
-                             '__cxx11::' + ch + 'string')
-        add_one_type_printer(obj, 'basic_string_view', ch + 'string_view')
+                             '__cxx11::' + ch[0] + 'string', ch[1])
+        add_one_type_printer(obj, 'basic_string_view', ch[0] + 'string_view', ch[1])
 
     # Add type printers for typedefs std::istream, std::wistream etc.
-    for ch in ('', 'w'):
+    for ch in (('', 'char'), ('w', 'wchar_t')):
         for x in ('ios', 'streambuf', 'istream', 'ostream', 'iostream',
                   'filebuf', 'ifstream', 'ofstream', 'fstream'):
-            add_one_type_printer(obj, 'basic_' + x, ch + x)
+            add_one_type_printer(obj, 'basic_' + x, ch[0] + x, ch[1])
         for x in ('stringbuf', 'istringstream', 'ostringstream',
                   'stringstream'):
-            add_one_type_printer(obj, 'basic_' + x, ch + x)
+            add_one_type_printer(obj, 'basic_' + x, ch[0] + x, ch[1])
             # <sstream> types are in __cxx11 namespace, but typedefs aren't:
-            add_one_type_printer(obj, '__cxx11::basic_' + x, ch + x)
+            add_one_type_printer(obj, '__cxx11::basic_' + x, ch[0] + x, ch[1])
 
     # Add type printers for typedefs regex, wregex, cmatch, wcmatch etc.
     for abi in ('', '__cxx11::'):
-        for ch in ('', 'w'):
-            add_one_type_printer(obj, abi + 'basic_regex', abi + ch + 'regex')
+        for ch in (('', 'char'), ('w', 'wchar_t')):
+            add_one_type_printer(obj, abi + 'basic_regex', abi + ch[0] + 'regex', ch[1])
         for ch in ('c', 's', 'wc', 'ws'):
             add_one_type_printer(obj, abi + 'match_results', abi + ch + 'match')
             for x in ('sub_match', 'regex_iterator', 'regex_token_iterator'):
@@ -2170,9 +2184,13 @@ def register_type_printers(obj):
 
     # Add type printers for experimental::basic_string_view typedefs.
     ns = 'experimental::fundamentals_v1::'
-    for ch in ('', 'w', 'u8', 'u16', 'u32'):
+    for ch in (('', 'char'),
+               ('w', 'wchar_t'),
+               ('u8', 'char8_t'),
+               ('u16', 'char16_t'),
+               ('u32', 'char32_t')):
         add_one_type_printer(obj, ns + 'basic_string_view',
-                             ns + ch + 'string_view')
+                             ns + ch[0] + 'string_view', ch[1])
 
     # Do not show defaulted template arguments in class templates.
     add_one_template_type_printer(obj, 'unique_ptr',

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-10-06 17:38 [PATCH] Fix gdb FilteringTypePrinter (again) François Dumont
@ 2022-11-14 17:57 ` François Dumont
  2022-11-15 16:17 ` Jonathan Wakely
  1 sibling, 0 replies; 8+ messages in thread
From: François Dumont @ 2022-11-14 17:57 UTC (permalink / raw)
  To: libstdc++; +Cc: gcc-patches

Any chance to review this one ?

On 06/10/22 19:38, 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
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-10-06 17:38 [PATCH] Fix gdb FilteringTypePrinter (again) François Dumont
  2022-11-14 17:57 ` François Dumont
@ 2022-11-15 16:17 ` Jonathan Wakely
  2022-11-16  6:04   ` François Dumont
  1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2022-11-15 16:17 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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.

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



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-11-15 16:17 ` Jonathan Wakely
@ 2022-11-16  6:04   ` François Dumont
  2022-11-16 11:35     ` Jonathan Wakely
  0 siblings, 1 reply; 8+ messages in thread
From: François Dumont @ 2022-11-16  6:04 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-11-16  6:04   ` François Dumont
@ 2022-11-16 11:35     ` Jonathan Wakely
  2022-11-16 11:54       ` Jonathan Wakely
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2022-11-16 11:35 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-11-16 11:35     ` Jonathan Wakely
@ 2022-11-16 11:54       ` Jonathan Wakely
  2022-11-16 12:29         ` Jonathan Wakely
  2022-11-17  5:28         ` François Dumont
  0 siblings, 2 replies; 8+ messages in thread
From: Jonathan Wakely @ 2022-11-16 11:54 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-11-16 11:54       ` Jonathan Wakely
@ 2022-11-16 12:29         ` Jonathan Wakely
  2022-11-17  5:28         ` François Dumont
  1 sibling, 0 replies; 8+ messages in thread
From: Jonathan Wakely @ 2022-11-16 12:29 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

[-- 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:

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix gdb FilteringTypePrinter (again)
  2022-11-16 11:54       ` Jonathan Wakely
  2022-11-16 12:29         ` Jonathan Wakely
@ 2022-11-17  5:28         ` François Dumont
  1 sibling, 0 replies; 8+ messages in thread
From: François Dumont @ 2022-11-17  5:28 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

On 16/11/22 12:54, Jonathan Wakely 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)
Indeed, I guess I mess it up myself when doing all my tests.
>>
>>
>> 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,...>.
>
I had noticed this usage of std::string but surprisingly had never 
managed to have a test failure because of that so I left it untouch.

Note that I had not re-tested the patch after your approval, maybe 
something changed in the month gap.

Thanks for having sorted that out.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-17  5:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 17:38 [PATCH] Fix gdb FilteringTypePrinter (again) François Dumont
2022-11-14 17:57 ` François Dumont
2022-11-15 16:17 ` Jonathan Wakely
2022-11-16  6:04   ` François Dumont
2022-11-16 11:35     ` Jonathan Wakely
2022-11-16 11:54       ` Jonathan Wakely
2022-11-16 12:29         ` Jonathan Wakely
2022-11-17  5:28         ` François Dumont

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).