Smart pointer pretty printers

Smart pointer pretty printers

[Juraj]

Hello everyone. I am using an IDE (CLion) which uses gdb for
debugging. An annoying issue is that the pretty printer for smart
pointers (shared_ptr and unique_ptr) does not serve the derefenced
pointer as a child, so the object tree cannot be expanded in the IDE
further than the smart pointer.

I have found this discussion from 2013, where Michael Marte suggested a patch:

https://gcc.gnu.org/ml/libstdc++/2013-04/msg00133.html

The last message in the thread was an amended patch by him, so I am
wondering why this didn't go in? It makes debugging a complex
application with many smart pointers in an IDE much easier.

Thanks,
Juraj

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 23/02/17 10:36 +0100, Juraj wrote:
>Hello everyone. I am using an IDE (CLion) which uses gdb for
>debugging. An annoying issue is that the pretty printer for smart
>pointers (shared_ptr and unique_ptr) does not serve the derefenced
>pointer as a child, so the object tree cannot be expanded in the IDE
>further than the smart pointer.
>
>I have found this discussion from 2013, where Michael Marte suggested a patch:
>
>https://gcc.gnu.org/ml/libstdc++/2013-04/msg00133.html
>
>The last message in the thread was an amended patch by him, so I am
>wondering why this didn't go in? It makes debugging a complex
>application with many smart pointers in an IDE much easier.

I wasn't entirely convinced it was an improvement, and I think the
patch is significant enough to require a copyright assignment.

See https://gcc.gnu.org/ml/libstdc++/2013-04/msg00102.html

Re: Smart pointer pretty printers

[Juraj Oršulić]

On Thu, Feb 23, 2017 at 7:05 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> I wasn't entirely convinced it was an improvement, and I think the
> patch is significant enough to require a copyright assignment.

Hi Jonathan,

I hope I can convince you that addressing this will remove a major
inconvenience for IDE users. For example, take a look at this page on
the CLion issue tracker, where the issue has been opened in 2015 and
has accumulated 40 votes:
https://youtrack.jetbrains.com/issue/CPP-2496

> See https://gcc.gnu.org/ml/libstdc++/2013-04/msg00102.html

At first, I haven't read your message in great detail and assumed that
Michael, in his final message that I have linked, addressed the issues
that you raised (simply because the message came after your message).
Now I see that he didn't. I agree with you, printing a variable
containing a smart shouldn't duplicate information (the pointer
address).

I took a closer look and I think I have come up with a solution.  It
seems that the problem that Tom Tromey has mentioned, that { .... }
replaces the to_string result in case when the printer has children,
is no longer an issue. This means that we can remove the pointer
address from the to_string result, and expose the pointer as a child
like Michael suggested. This ensures that the pointer address is
printed when the pretty printer is recursively invoked on the child.
However, we should (I think?) keep the name of the child synchronized
with how we would access it in regular code, so the member path,
generated by stitching together the printer children names, is valid.
(Introducing a child node named "Managed object" is not valid in this
sense.) Therefore, I propose that we name the child "get()" or
"operator->()".

I have included the patch for the unique pointer printer:

--- libstdcxx/v6/printers.py
+++ libstdcxx/v6/printers.py
@@ -124,11 +124,13 @@ class UniquePointerPrinter:

     def __init__ (self, typename, val):
         self.val = val
+        self.pointer = val['_M_t']['_M_head_impl']
+
+    def children (self):
+        return [('get()', self.pointer)]

     def to_string (self):
-        v = self.val['_M_t']['_M_head_impl']
-        return ('std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                       str(v)))
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))

 def get_value_from_list_node(node):
     """Returns the value held in an _List_node<_Val>"""



This is how it looks from the gdb shell, almost like you wanted:

(gdb) print up
$1 = std::unique_ptr<int> = {get() = 0x61bc20}

...and it works from an IDE such as CLion. If you think this is okay,
perhaps this can be expanded to the shared pointer printer, and even
iterator printers, so that the pointed objects can all be explored in
graphical debuggers by expanding the pointers.

Regards,
Juraj

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 24/02/17 16:08 +0100, Juraj Oršulić wrote:
>On Thu, Feb 23, 2017 at 7:05 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> I wasn't entirely convinced it was an improvement, and I think the
>> patch is significant enough to require a copyright assignment.
>
>Hi Jonathan,
>
>I hope I can convince you that addressing this will remove a major
>inconvenience for IDE users. For example, take a look at this page on
>the CLion issue tracker, where the issue has been opened in 2015 and
>has accumulated 40 votes:
>https://youtrack.jetbrains.com/issue/CPP-2496
>
>> See https://gcc.gnu.org/ml/libstdc++/2013-04/msg00102.html
>
>At first, I haven't read your message in great detail and assumed that
>Michael, in his final message that I have linked, addressed the issues
>that you raised (simply because the message came after your message).
>Now I see that he didn't. I agree with you, printing a variable
>containing a smart shouldn't duplicate information (the pointer
>address).
>
>I took a closer look and I think I have come up with a solution.  It
>seems that the problem that Tom Tromey has mentioned, that { .... }
>replaces the to_string result in case when the printer has children,
>is no longer an issue. This means that we can remove the pointer
>address from the to_string result, and expose the pointer as a child
>like Michael suggested. This ensures that the pointer address is
>printed when the pretty printer is recursively invoked on the child.
>However, we should (I think?) keep the name of the child synchronized
>with how we would access it in regular code, so the member path,
>generated by stitching together the printer children names, is valid.
>(Introducing a child node named "Managed object" is not valid in this
>sense.) Therefore, I propose that we name the child "get()" or
>"operator->()".
>
>I have included the patch for the unique pointer printer:
>
>--- libstdcxx/v6/printers.py
>+++ libstdcxx/v6/printers.py
>@@ -124,11 +124,13 @@ class UniquePointerPrinter:
>
>     def __init__ (self, typename, val):
>         self.val = val
>+        self.pointer = val['_M_t']['_M_head_impl']
>+
>+    def children (self):
>+        return [('get()', self.pointer)]
>
>     def to_string (self):
>-        v = self.val['_M_t']['_M_head_impl']
>-        return ('std::unique_ptr<%s> containing %s' % (str(v.type.target()),
>-                                                       str(v)))
>+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))
>
> def get_value_from_list_node(node):
>     """Returns the value held in an _List_node<_Val>"""
>
>
>
>This is how it looks from the gdb shell, almost like you wanted:
>
>(gdb) print up
>$1 = std::unique_ptr<int> = {get() = 0x61bc20}
>
>...and it works from an IDE such as CLion. If you think this is okay,
>perhaps this can be expanded to the shared pointer printer, and even
>iterator printers, so that the pointed objects can all be explored in
>graphical debuggers by expanding the pointers.

This looks promising, thanks. I'll look into making that change, and
something similar to the shared_ptr printer too.

Re: Smart pointer pretty printers

[Juraj Oršulić]

On Fri, Feb 24, 2017 at 4:11 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> This looks promising, thanks. I'll look into making that change, and
> something similar to the shared_ptr printer too.

Great, thanks! Well, you can have the diff for the shared_ptr printer
too, it's almost the same thing :). I also added printing of the
pointed type analogously to the unique pointer version.

--- libstdcxx/v6/printers_min.py
+++ libstdcxx/v6/printers.py
@@ -106,6 +106,10 @@ class SharedPointerPrinter:
     def __init__ (self, typename, val):
         self.typename = typename
         self.val = val
+ self.pointer = val['_M_ptr']
+
+    def children (self):
+        return [('get()', self.pointer)]

     def to_string (self):
         state = 'empty'
@@ -116,8 +120,8 @@ class SharedPointerPrinter:
             if usecount == 0:
                 state = 'expired, weak %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count = %d, weak count = %d' %
(usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename,
str(self.pointer.type.target()), state)

 class UniquePointerPrinter:
     "Print a unique_ptr"


Regards, Juraj

Re: Smart pointer pretty printers

[Juraj Oršulić]

I now realized that I worked on an older version of the printers.py
file. I have produced a patch against a recent version, which had some
changes to the unique pointer printer regarding a newer implementation
of the unique pointer. The patch includes this for both the unique and
shared pointers. Also, I have added the same for the vector iterator
just for your consideration. Do you think the same could be applied
for other STL container iterators?

Also, let me know if you need a copyright assignment.

Regards, Juraj

--- libstdcxx/v6/printers.py
+++ libstdcxx/v6/printers.py
@@ -121,6 +121,10 @@ class SharedPointerPrinter:
     def __init__ (self, typename, val):
         self.typename = strip_versioned_namespace(typename)
         self.val = val
+        self.pointer = val['_M_ptr']
+
+    def children (self):
+        return [('get()', self.pointer)]

     def to_string (self):
         state = 'empty'
@@ -131,25 +135,27 @@ class SharedPointerPrinter:
             if usecount == 0:
                 state = 'expired, weak %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count = %d, weak count = %d' %
(usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename,
str(self.pointer.type.target()), state)

 class UniquePointerPrinter:
     "Print a unique_ptr"

     def __init__ (self, typename, val):
         self.val = val
-
-    def to_string (self):
         impl_type = self.val.type.fields()[0].type.tag
         if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New
implementation
-            v = self.val['_M_t']['_M_t']['_M_head_impl']
+            self.pointer = self.val['_M_t']['_M_t']['_M_head_impl']
         elif is_specialization_of(impl_type, 'tuple'):
-            v = self.val['_M_t']['_M_head_impl']
+            self.pointer = self.val['_M_t']['_M_head_impl']
         else:
             raise ValueError("Unsupported implementation for
unique_ptr: %s" % self.val.type.fields()[0].type.tag)
-        return 'std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                      str(v))
+
+    def children (self):
+        return [('get()', self.pointer)]
+
+    def to_string (self):
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))

 def get_value_from_aligned_membuf(buf, valtype):
     """Returns the value held in a __gnu_cxx::__aligned_membuf."""
@@ -350,11 +356,17 @@ class StdVectorIteratorPrinter:

     def __init__(self, typename, val):
         self.val = val
+        self.pointer = self.val['_M_current']
+
+    def children(self):
+        if not self.pointer:
+            return []
+        return [('operator->()', self.pointer)]

     def to_string(self):
-        if not self.val['_M_current']:
+        if not self.pointer:
             return 'non-dereferenceable iterator for std::vector'
-        return str(self.val['_M_current'].dereference())
+        return ('std::vector<%s>::iterator' %
(str(self.pointer.type.target())))

 class StdTuplePrinter:
     "Print a std::tuple"

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 24/02/17 17:11 +0100, Juraj Oršulić wrote:
>Also, let me know if you need a copyright assignment.

For a patch this size we do, so I'm not going to look at your patch.
It will probably be simpler to just do it myself, and not worry about
paperwork.

If you want to contribue in future then please do complete the
necessary paperwork anyway:
https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html#contrib.list

Re: Smart pointer pretty printers

[Juraj Oršulić]

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

On Fri, Feb 24, 2017 at 5:36 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> For a patch this size we do, so I'm not going to look at your patch.
> It will probably be simpler to just do it myself, and not worry about
> paperwork.
>
> If you want to contribue in future then please do complete the
> necessary paperwork anyway:
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html#contrib.list

Hi Jonathan,

I have completed the assignment.

I am resubmitting the patch from the last time, this time as an
attachment (since the previous was broken by newlines). As before,
there are two versions of the patch: the one that I obtained by editing
printers.py on Ubuntu 16.04, which ships with gcc 5.4.0; and the
other, for the current printers.py from the gcc repo (I assume it's gcc
6?), which needs a slightly different patch because the unique_ptr
printer has significantly changed.

I have been using the gcc 5 patch without problems.  I have not tested
the gcc 6 patch, but since it is very similar, I don't expect any
problems.

Let me know if you want me to expand this for other iterators.

Regards,
Juraj

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

--- printers.py
+++ printers.py
@@ -106,6 +106,10 @@ class SharedPointerPrinter:
     def __init__ (self, typename, val):
         self.typename = typename
         self.val = val
+        self.pointer = val['_M_ptr']
+
+    def children (self):
+        return [('get()', self.pointer)]
 
     def to_string (self):
         state = 'empty'
@@ -116,20 +120,22 @@ class SharedPointerPrinter:
             if usecount == 0:
                 state = 'expired, weak %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count = %d, weak count = %d' % (usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename, str(self.pointer.type.target()), state)
 
 class UniquePointerPrinter:
     "Print a unique_ptr"
 
     def __init__ (self, typename, val):
         self.val = val
+        self.pointer = self.val['_M_t']['_M_head_impl']
 
-    def to_string (self):
-        v = self.val['_M_t']['_M_head_impl']
-        return ('std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                       str(v)))
+    def children (self):
+        return [('get()', self.pointer)]
+
+    def to_string (self):
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))

 def get_value_from_list_node(node):
     """Returns the value held in an _List_node<_Val>"""
     try:
@@ -322,9 +328,17 @@ class StdVectorIteratorPrinter:
 
     def __init__(self, typename, val):
         self.val = val
+        self.pointer = self.val['_M_current']
+
+    def children(self):
+        if not self.pointer:
+            return []
+        return [('operator->()', self.pointer)]
 
     def to_string(self):
-        return self.val['_M_current'].dereference()
+        if not self.pointer:
+             return 'non-dereferenceable iterator for std::vector'
+        return ('std::vector<%s>::iterator' % (str(self.pointer.type.target())))
 
 class StdTuplePrinter:
     "Print a std::tuple"

[-- Attachment #3: printers_py_gcc6.patch --]
[-- Type: text/x-patch, Size: 2654 bytes --]

--- printers.py
+++ printers.py
@@ -121,6 +121,10 @@ class SharedPointerPrinter:
     def __init__ (self, typename, val):
         self.typename = strip_versioned_namespace(typename)
         self.val = val
+        self.pointer = val['_M_ptr']
+
+    def children (self):
+        return [('get()', self.pointer)]
 
     def to_string (self):
         state = 'empty'
@@ -131,25 +135,27 @@ class SharedPointerPrinter:
             if usecount == 0:
                 state = 'expired, weak %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count = %d, weak count = %d' % (usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename, str(self.pointer.type.target()), state)
 
 class UniquePointerPrinter:
     "Print a unique_ptr"
 
     def __init__ (self, typename, val):
         self.val = val
-
-    def to_string (self):
         impl_type = self.val.type.fields()[0].type.tag
         if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New implementation
-            v = self.val['_M_t']['_M_t']['_M_head_impl']
+            self.pointer = self.val['_M_t']['_M_t']['_M_head_impl']
         elif is_specialization_of(impl_type, 'tuple'):
-            v = self.val['_M_t']['_M_head_impl']
+            self.pointer = self.val['_M_t']['_M_head_impl']
         else:
             raise ValueError("Unsupported implementation for unique_ptr: %s" % self.val.type.fields()[0].type.tag)
-        return 'std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                      str(v))
+
+    def children (self):
+        return [('get()', self.pointer)]
+
+    def to_string (self):
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))
 
 def get_value_from_aligned_membuf(buf, valtype):
     """Returns the value held in a __gnu_cxx::__aligned_membuf."""
@@ -350,11 +356,17 @@ class StdVectorIteratorPrinter:
 
     def __init__(self, typename, val):
         self.val = val
+        self.pointer = self.val['_M_current']
+
+    def children(self):
+        if not self.pointer:
+            return []
+        return [('operator->()', self.pointer)]
 
     def to_string(self):
-        if not self.val['_M_current']:
+        if not self.pointer:
             return 'non-dereferenceable iterator for std::vector'
-        return str(self.val['_M_current'].dereference())
+        return ('std::vector<%s>::iterator' % (str(self.pointer.type.target())))
 
 class StdTuplePrinter:
     "Print a std::tuple"

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 02/03/17 19:10 +0100, Juraj Oršulić wrote:
>On Fri, Feb 24, 2017 at 5:36 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> For a patch this size we do, so I'm not going to look at your patch.
>> It will probably be simpler to just do it myself, and not worry about
>> paperwork.
>>
>> If you want to contribue in future then please do complete the
>> necessary paperwork anyway:
>> https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html#contrib.list
>
>Hi Jonathan,
>
>I have completed the assignment.
>
>I am resubmitting the patch from the last time, this time as an
>attachment (since the previous was broken by newlines). As before,
>there are two versions of the patch: the one that I obtained by editing
>printers.py on Ubuntu 16.04, which ships with gcc 5.4.0; and the
>other, for the current printers.py from the gcc repo (I assume it's gcc
>6?), which needs a slightly different patch because the unique_ptr
>printer has significantly changed.
>
>I have been using the gcc 5 patch without problems.  I have not tested
>the gcc 6 patch, but since it is very similar, I don't expect any
>problems.

So I assume you haven't tried with gcc 7 either :-)

I think we're probably too close to the gcc7 release to make this
change now, sorry. If we make this change early in Stage 1 for gcc8
(which should mean committing it in 6-7 weeks) then there will be time
for IDEs or other tools to identify and resolve any problems or
integration niggles that this causes.

And if it works fine on the subversion trunk and nobody finds issues
then we can probably backport it to gcc 7 and maybe gcc 6 too.

Re: Smart pointer pretty printers

[Juraj Oršulić]

On Mon, Mar 27, 2017 at 5:15 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> I think we're probably too close to the gcc7 release to make this
> change now, sorry.

No problem, I think it's most important that it entered the pipeline,
since it's been completely dead for 4 years :-)

Just a reminder, I added this only for those smart pointers that I
needed at the moment - the unique and shared pointer, and for the
vector iterator. When you (or someone else) will be doing a review of
the patch, let me know if you think this should be expanded for other
iterators as well.

Thanks,
Juraj

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 27/03/17 17:34 +0200, Juraj Oršulić wrote:
>On Mon, Mar 27, 2017 at 5:15 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> I think we're probably too close to the gcc7 release to make this
>> change now, sorry.
>
>No problem, I think it's most important that it entered the pipeline,
>since it's been completely dead for 4 years :-)
>
>Just a reminder, I added this only for those smart pointers that I
>needed at the moment - the unique and shared pointer, and for the
>vector iterator. When you (or someone else) will be doing a review of
>the patch, let me know if you think this should be expanded for other
>iterators as well.

Will do, thanks.

Re: Smart pointer pretty printers

[Juraj Oršulić]

Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
patches for improving the smart pointer pretty printers in March. They
haven't been reviewed.

Regards, and happy 2018,
Juraj

On Mon, Mar 27, 2017 at 6:05 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 27/03/17 17:34 +0200, Juraj Oršulić wrote:
>>On Mon, Mar 27, 2017 at 5:15 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> I think we're probably too close to the gcc7 release to make this
>>> change now, sorry.
>>
>>No problem, I think it's most important that it entered the pipeline,
>>since it's been completely dead for 4 years :-)
>>
>>Just a reminder, I added this only for those smart pointers that I
>>needed at the moment - the unique and shared pointer, and for the
>>vector iterator. When you (or someone else) will be doing a review of
>>the patch, let me know if you think this should be expanded for other
>>iterators as well.
>
> Will do, thanks.
>

Re: Smart pointer pretty printers

[Jonathan Wakely]

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

On 04/01/18 11:22 +0100, Juraj Oršulić wrote:
>Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
>patches for improving the smart pointer pretty printers in March. They
>haven't been reviewed.

Thanks for the reminder. I'm testing the attached patch, which has
been rebased on trunk, but I'm getting test failures for the
shared_ptr printers.

I can't see any difference between the expected output and what GDB
prints, so I think it's some DejaGnu or Tcl/Tk oddity. The actual
printer seems to work OK.


>Regards, and happy 2018,
>Juraj
>
>On Mon, Mar 27, 2017 at 6:05 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 27/03/17 17:34 +0200, Juraj Oršulić wrote:
>>>On Mon, Mar 27, 2017 at 5:15 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>> I think we're probably too close to the gcc7 release to make this
>>>> change now, sorry.
>>>
>>>No problem, I think it's most important that it entered the pipeline,
>>>since it's been completely dead for 4 years :-)
>>>
>>>Just a reminder, I added this only for those smart pointers that I
>>>needed at the moment - the unique and shared pointer, and for the
>>>vector iterator. When you (or someone else) will be doing a review of
>>>the patch, let me know if you think this should be expanded for other
>>>iterators as well.
>>
>> Will do, thanks.
>>

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

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 6da8508d944..7b999ee6db4 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -120,6 +120,10 @@ class SharedPointerPrinter:
     def __init__ (self, typename, val):
         self.typename = strip_versioned_namespace(typename)
         self.val = val
+        self.pointer = val['_M_ptr']
+
+    def children (self):
+        return [('get()', self.pointer)]
 
     def to_string (self):
         state = 'empty'
@@ -130,25 +134,29 @@ class SharedPointerPrinter:
             if usecount == 0:
                 state = 'expired, weak %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count %d, weak count %d' % (usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename, str(self.pointer.type.target().strip_typedefs()), state)
 
 class UniquePointerPrinter:
     "Print a unique_ptr"
 
     def __init__ (self, typename, val):
         self.val = val
-
-    def to_string (self):
-        impl_type = self.val.type.fields()[0].type.tag
+        impl_type = val.type.fields()[0].type.tag
         if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New implementation
-            v = self.val['_M_t']['_M_t']['_M_head_impl']
+            self.pointer = val['_M_t']['_M_t']['_M_head_impl']
         elif is_specialization_of(impl_type, 'tuple'):
-            v = self.val['_M_t']['_M_head_impl']
+            self.pointer = val['_M_t']['_M_head_impl']
         else:
+            self.pointer = None
+
+    def children (self):
+        return [('get()', self.pointer)]
+
+    def to_string (self):
+        if not self.pointer:
             raise ValueError("Unsupported implementation for unique_ptr: %s" % self.val.type.fields()[0].type.tag)
-        return 'std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                      str(v))
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))
 
 def get_value_from_aligned_membuf(buf, valtype):
     """Returns the value held in a __gnu_cxx::__aligned_membuf."""

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 09/01/18 14:59 +0000, Jonathan Wakely wrote:
>On 04/01/18 11:22 +0100, Juraj Oršulić wrote:
>>Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
>>patches for improving the smart pointer pretty printers in March. They
>>haven't been reviewed.
>
>Thanks for the reminder. I'm testing the attached patch, which has
>been rebased on trunk, but I'm getting test failures for the
>shared_ptr printers.
>
>I can't see any difference between the expected output and what GDB
>prints, so I think it's some DejaGnu or Tcl/Tk oddity. The actual
>printer seems to work OK.

No, the problem is that I'm just misreading the results.

I'll finish testing and commit this soon.

Re: Smart pointer pretty printers

[Jonathan Wakely]

On 02/03/17 19:10 +0100, Juraj Oršulić wrote:
>On Fri, Feb 24, 2017 at 5:36 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> For a patch this size we do, so I'm not going to look at your patch.
>> It will probably be simpler to just do it myself, and not worry about
>> paperwork.
>>
>> If you want to contribue in future then please do complete the
>> necessary paperwork anyway:
>> https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html#contrib.list
>
>Hi Jonathan,
>
>I have completed the assignment.
>
>I am resubmitting the patch from the last time, this time as an
>attachment (since the previous was broken by newlines). As before,
>there are two versions of the patch: the one that I obtained by editing
>printers.py on Ubuntu 16.04, which ships with gcc 5.4.0; and the
>other, for the current printers.py from the gcc repo (I assume it's gcc
>6?), which needs a slightly different patch because the unique_ptr
>printer has significantly changed.
>
>I have been using the gcc 5 patch without problems.  I have not tested
>the gcc 6 patch, but since it is very similar, I don't expect any
>problems.
>
>Let me know if you want me to expand this for other iterators.

I don't like the change for std::vector iterators, because it makes it
appear as though the iterator stores a pointer. Although that's true
internally, conceptually it's not how we should think about iterators.

An iterator refers to a value, not a pointer. However, the current
code has a serious flaw that it always dereferences the pointer, even
for the end iterator (which is https://gcc.gnu.org/PR59170 and is
worse than displaying the value as a pointer!)

There's certainly scope for improvement, but I'm not sure this is the
right solution.


>@@ -322,9 +328,17 @@ class StdVectorIteratorPrinter:
> 
>     def __init__(self, typename, val):
>         self.val = val
>+        self.pointer = self.val['_M_current']
>+
>+    def children(self):
>+        if not self.pointer:
>+            return []
>+        return [('operator->()', self.pointer)]
> 
>     def to_string(self):
>-        return self.val['_M_current'].dereference()
>+        if not self.pointer:
>+             return 'non-dereferenceable iterator for std::vector'
>+        return ('std::vector<%s>::iterator' % (str(self.pointer.type.target())))
> 
> class StdTuplePrinter:
>     "Print a std::tuple"

Re: Smart pointer pretty printers

[Jonathan Wakely]

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

On 09/01/18 15:02 +0000, Jonathan Wakely wrote:
>On 09/01/18 14:59 +0000, Jonathan Wakely wrote:
>>On 04/01/18 11:22 +0100, Juraj Oršulić wrote:
>>>Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
>>>patches for improving the smart pointer pretty printers in March. They
>>>haven't been reviewed.
>>
>>Thanks for the reminder. I'm testing the attached patch, which has
>>been rebased on trunk, but I'm getting test failures for the
>>shared_ptr printers.
>>
>>I can't see any difference between the expected output and what GDB
>>prints, so I think it's some DejaGnu or Tcl/Tk oddity. The actual
>>printer seems to work OK.
>
>No, the problem is that I'm just misreading the results.
>
>I'll finish testing and commit this soon.

I changed the code slightly, so it still works for GDB versions older
than 7.5, which don't have the fix to allow children() to return a
list. There's no easy way to tell if the GDB interpreting the code has
the fix or not.

Thanks very much for the patches, and sorry for the delay reviewing
them.

Tested x86_64-linux, committed to trunk.



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

commit 4c65ef4dae877443c5372ec504e3881a3922dc18
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jan 9 15:36:13 2018 +0000

    PR libstdc++/59253 Improve pretty printers for smart pointers
    
            PR libstdc++/59253 (partial)
            * python/libstdcxx/v6/printers.py (SmartPtrIterator): Common iterator
            type for pointer stored by shared_ptr, weak_ptr and unique_ptr.
            (SharedPointerPrinter, UniquePointerPrinter): Treat stored values as
            children.
            * testsuite/libstdc++-prettyprinters/cxx11.cc: Update expected output
            of unique_ptr printer.
            * testsuite/libstdc++-prettyprinters/shared_ptr.cc: Update expected
            output of shared_ptr printer.

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 6da8508d944..e9f7359d63f 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -114,12 +114,31 @@ def strip_versioned_namespace(typename):
         return typename.replace(_versioned_namespace, '')
     return typename
 
+class SmartPtrIterator(Iterator):
+    "An iterator for smart pointer types with a single 'child' value"
+
+    def __init__(self, val):
+        self.val = val
+
+    def __iter__(self):
+        return self
+
+    def __next__(self):
+        if self.val is None:
+            raise StopIteration
+        self.val, val = None, self.val
+        return ('get()', val)
+
 class SharedPointerPrinter:
     "Print a shared_ptr or weak_ptr"
 
     def __init__ (self, typename, val):
         self.typename = strip_versioned_namespace(typename)
         self.val = val
+        self.pointer = val['_M_ptr']
+
+    def children (self):
+        return SmartPtrIterator(self.pointer)
 
     def to_string (self):
         state = 'empty'
@@ -128,27 +147,29 @@ class SharedPointerPrinter:
             usecount = refcounts['_M_use_count']
             weakcount = refcounts['_M_weak_count']
             if usecount == 0:
-                state = 'expired, weak %d' % weakcount
+                state = 'expired, weak count %d' % weakcount
             else:
-                state = 'count %d, weak %d' % (usecount, weakcount - 1)
-        return '%s (%s) %s' % (self.typename, state, self.val['_M_ptr'])
+                state = 'use count %d, weak count %d' % (usecount, weakcount - 1)
+        return '%s<%s> (%s)' % (self.typename, str(self.pointer.type.target().strip_typedefs()), state)
 
 class UniquePointerPrinter:
     "Print a unique_ptr"
 
     def __init__ (self, typename, val):
         self.val = val
+        impl_type = val.type.fields()[0].type.tag
+        if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New implementation
+            self.pointer = val['_M_t']['_M_t']['_M_head_impl']
+        elif is_specialization_of(impl_type, 'tuple'):
+            self.pointer = val['_M_t']['_M_head_impl']
+        else:
+            raise ValueError("Unsupported implementation for unique_ptr: %s" % impl_type)
+
+    def children (self):
+        return SmartPtrIterator(self.pointer)
 
     def to_string (self):
-        impl_type = self.val.type.fields()[0].type.tag
-        if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New implementation
-            v = self.val['_M_t']['_M_t']['_M_head_impl']
-        elif is_specialization_of(impl_type, 'tuple'):
-            v = self.val['_M_t']['_M_head_impl']
-        else:
-            raise ValueError("Unsupported implementation for unique_ptr: %s" % self.val.type.fields()[0].type.tag)
-        return 'std::unique_ptr<%s> containing %s' % (str(v.type.target()),
-                                                      str(v))
+        return ('std::unique_ptr<%s>' % (str(self.pointer.type.target())))
 
 def get_value_from_aligned_membuf(buf, valtype):
     """Returns the value held in a __gnu_cxx::__aligned_membuf."""
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx11.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx11.cc
index 64119155282..3af564f2bf7 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx11.cc
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx11.cc
@@ -127,14 +127,14 @@ main()
   std::unique_ptr<datum> uptr (new datum);
   uptr->s = "hi bob";
   uptr->i = 23;
-// { dg-final { regexp-test uptr {std::unique_ptr.datum. containing 0x.*} } }
+// { dg-final { regexp-test uptr {std::unique_ptr.datum. = {get\(\) = 0x.*}} } }
   std::unique_ptr<datum> &ruptr = uptr;
-// { dg-final { regexp-test ruptr {std::unique_ptr.datum. containing 0x.*} } }
+// { dg-final { regexp-test ruptr {std::unique_ptr.datum. = {get\(\) = 0x.*}} } }
 
   ExTuple tpl(6,7);
-// { dg-final { note-test tpl {std::tuple containing = {[1] = 6, [2] = 7}} } }  
+// { dg-final { note-test tpl {std::tuple containing = {[1] = 6, [2] = 7}} } }
   ExTuple &rtpl = tpl;
-// { dg-final { note-test rtpl {std::tuple containing = {[1] = 6, [2] = 7}} } }   
+// { dg-final { note-test rtpl {std::tuple containing = {[1] = 6, [2] = 7}} } }
   placeholder(""); // Mark SPOT
   use(efl);
   use(fl);
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/shared_ptr.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/shared_ptr.cc
index 556ecd1cc35..27c2e405fa4 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/shared_ptr.cc
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/shared_ptr.cc
@@ -49,25 +49,25 @@ main()
   typedef std::weak_ptr<int> weak;
 
   shared esp;
-// { dg-final { note-test esp "std::shared_ptr (empty) 0x0" } }
+// { dg-final { note-test esp "std::shared_ptr<int> (empty) = {get() = 0x0}" } }
   weak ewp1;
-// { dg-final { note-test ewp1 "std::weak_ptr (empty) 0x0" } }
+// { dg-final { note-test ewp1 "std::weak_ptr<int> (empty) = {get() = 0x0}" } }
   weak ewp2 = esp;
-// { dg-final { note-test ewp2 "std::weak_ptr (empty) 0x0" } }
+// { dg-final { note-test ewp2 "std::weak_ptr<int> (empty) = {get() = 0x0}" } }
 
   shared sp1 = make(0x12345678);
   shared sp2 = sp1;
-// { dg-final { note-test sp1 "std::shared_ptr (count 2, weak 0) 0x12345678" } }
+// { dg-final { note-test sp1 "std::shared_ptr<int> (use count 2, weak count 0) = {get() = 0x12345678}" } }
 
   shared sp3 = make(0x12344321);
   weak sp4 = sp3;
   weak wp1 = sp3;
-// { dg-final { note-test wp1 "std::weak_ptr (count 1, weak 2) 0x12344321" } }
+// { dg-final { note-test wp1 "std::weak_ptr<int> (use count 1, weak count 2) = {get() = 0x12344321}" } }
 
   shared sp5 = make(0x56788765);
   weak wp2 = sp5;
   sp5.reset();
-// { dg-final { note-test wp2 "std::weak_ptr (expired, weak 1) 0x56788765" } }
+// { dg-final { note-test wp2 "std::weak_ptr<int> (expired, weak count 1) = {get() = 0x56788765}" } }
 
   placeholder(""); // Mark SPOT
   use(esp);

Re: Smart pointer pretty printers

[Jonathan Wakely]

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

On 09/01/18 18:50 +0000, Jonathan Wakely wrote:
>On 09/01/18 15:02 +0000, Jonathan Wakely wrote:
>>On 09/01/18 14:59 +0000, Jonathan Wakely wrote:
>>>On 04/01/18 11:22 +0100, Juraj Oršulić wrote:
>>>>Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
>>>>patches for improving the smart pointer pretty printers in March. They
>>>>haven't been reviewed.
>>>
>>>Thanks for the reminder. I'm testing the attached patch, which has
>>>been rebased on trunk, but I'm getting test failures for the
>>>shared_ptr printers.
>>>
>>>I can't see any difference between the expected output and what GDB
>>>prints, so I think it's some DejaGnu or Tcl/Tk oddity. The actual
>>>printer seems to work OK.
>>
>>No, the problem is that I'm just misreading the results.
>>
>>I'll finish testing and commit this soon.
>
>I changed the code slightly, so it still works for GDB versions older
>than 7.5, which don't have the fix to allow children() to return a
>list. There's no easy way to tell if the GDB interpreting the code has
>the fix or not.
>
>Thanks very much for the patches, and sorry for the delay reviewing
>them.
>
>Tested x86_64-linux, committed to trunk.

I've just realised I didn't put your name in the ChangeLog, sorry!
(I semi-automate my changelog entries, so have to remember to do an
extra step when I'm not the author, or not the sole author).

Fixed with this patch, committed to trunk.




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

commit b4e59f03b68387c5b556f3d8430ece8927cdca9f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jan 9 22:01:20 2018 +0000

    Correct earlier ChangeLog entry
    
    Add Juraj Oršulić as original patch author.

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 6c5a2741ba8..88af8ecf37c 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -32,7 +32,8 @@
 	* testsuite/23_containers/unordered_map/insert/83709.cc: New.
 	* testsuite/23_containers/unordered_set/insert/83709.cc: New.
 
-2018-01-09  Jonathan Wakely  <jwakely@redhat.com>
+2018-01-09  Juraj Oršulić  <juraj.orsulic@fer.hr>
+	    Jonathan Wakely  <jwakely@redhat.com>
 
 	PR libstdc++/59253 (partial)
 	* python/libstdcxx/v6/printers.py (SmartPtrIterator): Common iterator

Re: Smart pointer pretty printers

[Juraj Oršulić]

Hi Jonathan,

Thanks for looking into this! It’s unfortunate that we can't know if the
pointer in the iterator is legal to dereference. Nevertheless, inspecting
smart pointers is a far more common use case, it's good that it has finally
been sorted out.

Thanks, Juraj


On Tue, 9 Jan 2018 at 23:10, Jonathan Wakely <jwakely@redhat.com> wrote:

> On 09/01/18 18:50 +0000, Jonathan Wakely wrote:
> >On 09/01/18 15:02 +0000, Jonathan Wakely wrote:
> >>On 09/01/18 14:59 +0000, Jonathan Wakely wrote:
> >>>On 04/01/18 11:22 +0100, Juraj Oršulić wrote:
> >>>>Hi Jonathan (and the libstdc++ list). Can we revive this? I sent the
> >>>>patches for improving the smart pointer pretty printers in March. They
> >>>>haven't been reviewed.
> >>>
> >>>Thanks for the reminder. I'm testing the attached patch, which has
> >>>been rebased on trunk, but I'm getting test failures for the
> >>>shared_ptr printers.
> >>>
> >>>I can't see any difference between the expected output and what GDB
> >>>prints, so I think it's some DejaGnu or Tcl/Tk oddity. The actual
> >>>printer seems to work OK.
> >>
> >>No, the problem is that I'm just misreading the results.
> >>
> >>I'll finish testing and commit this soon.
> >
> >I changed the code slightly, so it still works for GDB versions older
> >than 7.5, which don't have the fix to allow children() to return a
> >list. There's no easy way to tell if the GDB interpreting the code has
> >the fix or not.
> >
> >Thanks very much for the patches, and sorry for the delay reviewing
> >them.
> >
> >Tested x86_64-linux, committed to trunk.
>
> I've just realised I didn't put your name in the ChangeLog, sorry!
> (I semi-automate my changelog entries, so have to remember to do an
> extra step when I'm not the author, or not the sole author).
>
> Fixed with this patch, committed to trunk.
>
>
>
>