Pretty printers for versioned namespace

[François Dumont]

Hi

Here is a patch to fix pretty printers when versioned namespace is
activated.

You will see that I have hesitated in making the fix independant of
the version being used. In source files you will find (__7::)? patterns
while in xmethods.py I chose (__\d+::)? making it ready for __8 and
forward. Do you want to generalize one option ? If so which one ?

At the moment version namespace is visible within gdb, it displays
for instance 'std::__7::string'. I am pretty sure we could hide it, is
it preferable ? I would need some time to do so as I am neither a python
nor regex expert.

I am not fully happy with the replication in printers.py of
StdRbtreeIteratorPrinter and StdExpAnyPrinter(SingleObjContainerPrinter
in respectively StdVersionedRbtreeIteratorPrinter and
StdExpVerAnyPrinter(SingleObjContainerPrinter just to adapt 2 lines
where regex is not an option. We could surely keep only one and pass it
'' or '__7'. But as I said I am not a python expert so any help would be
appreciated.

I still need to run it with versioned namespace disabled to make
sure there is no regression.

François

[Jonathan Wakely]

Post by François Dumont
Hi
Here is a patch to fix pretty printers when versioned namespace is
activated.
You will see that I have hesitated in making the fix independant
of the version being used. In source files you will find (__7::)?
patterns while in xmethods.py I chose (__\d+::)? making it ready for
__8 and forward. Do you want to generalize one option ? If so which
one ?

I don't really mind, but I note that the point of the path
libstdcxx/v6/printers.py was that we'd have different printers for v7,
v8 etc. ... I think it's simpler to keep everything in one place
though.

Post by François Dumont
At the moment version namespace is visible within gdb, it displays
for instance 'std::__7::string'. I am pretty sure we could hide it, is
it preferable ? I would need some time to do so as I am neither a
python nor regex expert.

It's fine to display it.

Post by François Dumont
I am not fully happy with the replication in printers.py of
StdRbtreeIteratorPrinter and
StdExpAnyPrinter(SingleObjContainerPrinter in respectively
StdVersionedRbtreeIteratorPrinter and
StdExpVerAnyPrinter(SingleObjContainerPrinter just to adapt 2 lines
where regex is not an option. We could surely keep only one and pass
it '' or '__7'. But as I said I am not a python expert so any help
would be appreciated.

We definitely want to avoid that duplication. For
StdRbtreeIteratorPrinter you can just look at 'typename' and see
whether it starts with "std::__7" or not. If it does, you need to lookup
std::__7::_Rb_tree_node<...>, otherwise you need to lookup
std::_Rb_tree_node<...> instead.

For StdExpAnyPrinter just do two replacements: first replace
std::string with the result of gdb.lookup_type('std::string') and then
replace std::__7::string with the result of looking that up. Are you
sure that's even needed though? Does std::__7::string actually appear
in the manager function's name? I would expect it to appear as
std::__7::basic_string<char, std::__7::char_traits<char>, std::__7::allocator<char> > >
which doesn't need to be expanded anyway. So I think you can just
remove your StdExpVerAnyPrinter.

Post by François Dumont
--- a/libstdc++-v3/testsuite/lib/gdb-test.exp
+++ b/libstdc++-v3/testsuite/lib/gdb-test.exp
@@ -74,6 +74,14 @@ proc whatis-test {var result} {
lappend gdb_tests $var $result whatis
}
+# A test of 'whatis'. This tests a type rather than a variable through a
+# regexp.

Please use "regular expression" here rather than "regexp".

Post by François Dumont
+proc whatis-regexp-test {var result} {
+ global gdb_tests
+
+ lappend gdb_tests $var $result whatisrexp
+}
+

And something other than "whatisrexp" e.g. "whatis_regexp" would be
OK, but "rexp" is not a conventional abbreviation.

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
Hi
Here is a patch to fix pretty printers when versioned namespace is
activated.
You will see that I have hesitated in making the fix independant
of the version being used. In source files you will find (__7::)?
patterns while in xmethods.py I chose (__\d+::)? making it ready for
__8 and forward. Do you want to generalize one option ? If so which
one ?

I don't really mind, but I note that the point of the path
libstdcxx/v6/printers.py was that we'd have different printers for v7,
v8 etc. ... I think it's simpler to keep everything in one place
though.

Ok, I think the folder v6 depends more on a potential gdb api change.

Post by Jonathan Wakely

Post by François Dumont
At the moment version namespace is visible within gdb, it displays
for instance 'std::__7::string'. I am pretty sure we could hide it,
is it preferable ? I would need some time to do so as I am neither a
python nor regex expert.

It's fine to display it.

Post by François Dumont
I am not fully happy with the replication in printers.py of
StdRbtreeIteratorPrinter and
StdExpAnyPrinter(SingleObjContainerPrinter in respectively
StdVersionedRbtreeIteratorPrinter and
StdExpVerAnyPrinter(SingleObjContainerPrinter just to adapt 2 lines
where regex is not an option. We could surely keep only one and pass
it '' or '__7'. But as I said I am not a python expert so any help
would be appreciated.

We definitely want to avoid that duplication. For
StdRbtreeIteratorPrinter you can just look at 'typename' and see
whether it starts with "std::__7" or not. If it does, you need to lookup
std::__7::_Rb_tree_node<...>, otherwise you need to lookup
std::_Rb_tree_node<...> instead.
For StdExpAnyPrinter just do two replacements: first replace
std::string with the result of gdb.lookup_type('std::string') and then
replace std::__7::string with the result of looking that up. Are you
sure that's even needed though? Does std::__7::string actually appear
in the manager function's name? I would expect it to appear as
std::__7::basic_string<char, std::__7::char_traits<char>,
std::__7::allocator<char> > >
which doesn't need to be expanded anyway. So I think you can just
remove your StdExpVerAnyPrinter.

We needed the StdExpVerAnyPrinter just because of the loopkup for
'std::string' which has to be 'std::__7::string'. But I used similar
technique exposed previously to get rid of it.

So here is the simplified version I plan to test without versioned
namespace.

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely

Post by François Dumont
I am not fully happy with the replication in printers.py of
StdRbtreeIteratorPrinter and
StdExpAnyPrinter(SingleObjContainerPrinter in respectively
StdVersionedRbtreeIteratorPrinter and
StdExpVerAnyPrinter(SingleObjContainerPrinter just to adapt 2
lines where regex is not an option. We could surely keep only one
and pass it '' or '__7'. But as I said I am not a python expert so
any help would be appreciated.

We definitely want to avoid that duplication. For
StdRbtreeIteratorPrinter you can just look at 'typename' and see
whether it starts with "std::__7" or not. If it does, you need to lookup
std::__7::_Rb_tree_node<...>, otherwise you need to lookup
std::_Rb_tree_node<...> instead.
For StdExpAnyPrinter just do two replacements: first replace
std::string with the result of gdb.lookup_type('std::string') and then
replace std::__7::string with the result of looking that up. Are you
sure that's even needed though? Does std::__7::string actually appear
in the manager function's name? I would expect it to appear as
std::__7::basic_string<char, std::__7::char_traits<char>,
std::__7::allocator<char> > >
which doesn't need to be expanded anyway. So I think you can just
remove your StdExpVerAnyPrinter.

We needed the StdExpVerAnyPrinter just because of the loopkup for
'std::string' which has to be 'std::__7::string'. But I used similar
technique exposed previously to get rid of it.

But I don't see any std::__7::string in the relevant symbols. Here's
the manager function for an std:experimental::__7::any storing a
std::__7::string:

std::experimental::fundamentals_v1::__7::any::_Manager_internal<std::__7::basic_string<char, std::__7::char_traits<char>, std::__7::allocator<char> > >::_S_manage(std::experimental::fundamentals_v1::__7::any::_Op, >std::experimental::fundamentals_v1::__7::any const*, >std::experimental::fundamentals_v1::__7::any::_Arg*)

Since this has no std::__7::string it doesn't need to be substituted.

Do any tests fail without the change to StdExpAnyPrinter? Which ones?

[François Dumont]

Post by François Dumont
We needed the StdExpVerAnyPrinter just because of the loopkup for
'std::string' which has to be 'std::__7::string'. But I used similar
technique exposed previously to get rid of it.

But I don't see any std::__7::string in the relevant symbols. Here's
the manager function for an std:experimental::__7::any storing a
std::experimental::fundamentals_v1::__7::any::_Manager_internal<std::__7::basic_string<char,
std::__7::char_traits<char>, std::__7::allocator<char> >

Post by François Dumont
::_S_manage(std::experimental::fundamentals_v1::__7::any::_Op,
std::experimental::fundamentals_v1::__7::any const*,
std::experimental::fundamentals_v1::__7::any::_Arg*)

Since this has no std::__7::string it doesn't need to be substituted.
Do any tests fail without the change to StdExpAnyPrinter? Which ones?

Yes, tests involving std::any are failing because of the std::string lookup:

Python Exception <class 'gdb.error'> No type named std::string.:
skipping: Python Exception <class 'gdb.error'> No type named std::string.:
Python Exception <class 'gdb.error'> No type named std::string.:
$9 = {_M_manager = 0x4046f4
<std::__7::any::_Manager_internal<bool>::_S_manage(std::__7::any::_Op,
std::__7::any const*, std::__7::any::_Arg*)>, _M_storage = {_M_ptr =
0x0skipping:
Python Exception <class 'gdb.error'> No type named std::string.:
, _M_buffer = {__data = "\000\000\000\000\000\000\000", __align = {<No
data fields>}}}}
got: $9 = {_M_manager = 0x4046f4
<std::__7::any::_Manager_internal<bool>::_S_manage(std::__7::any::_Op,
std::__7::any const*, std::__7::any::_Arg*)>, _M_storage = {_M_ptr =
0x0, _M_
buffer = {__data = "\000\000\000\000\000\000\000", __align = {<No data
fields>}}}}
FAIL: libstdc++-prettyprinters/cxx17.cc print ab

This lookup is needed to correctly handle std::any<std::string>. As
stated in the comment:

# FIXME need to expand 'std::string' so that
gdb.lookup_type works

But I don't know how to fix this so for the moment I just adapt it to
correctly handle std::__7::string.

Here is my latest version tested with an without version namespace.

Ok to commit ?

François

[Jonathan Wakely]

Post by François Dumont

Post by François Dumont
We needed the StdExpVerAnyPrinter just because of the loopkup for
'std::string' which has to be 'std::__7::string'. But I used
similar technique exposed previously to get rid of it.

But I don't see any std::__7::string in the relevant symbols. Here's
the manager function for an std:experimental::__7::any storing a
std::experimental::fundamentals_v1::__7::any::_Manager_internal<std::__7::basic_string<char,
std::__7::char_traits<char>, std::__7::allocator<char> >

Post by François Dumont
::_S_manage(std::experimental::fundamentals_v1::__7::any::_Op,
std::experimental::fundamentals_v1::__7::any const*,
std::experimental::fundamentals_v1::__7::any::_Arg*)

Since this has no std::__7::string it doesn't need to be substituted.
Do any tests fail without the change to StdExpAnyPrinter? Which ones?

$9 = {_M_manager = 0x4046f4
<std::__7::any::_Manager_internal<bool>::_S_manage(std::__7::any::_Op,
std::__7::any const*, std::__7::any::_Arg*)>, _M_storage = {_M_ptr =
, _M_buffer = {__data = "\000\000\000\000\000\000\000", __align = {<No
data fields>}}}}
got: $9 = {_M_manager = 0x4046f4
<std::__7::any::_Manager_internal<bool>::_S_manage(std::__7::any::_Op,
std::__7::any const*, std::__7::any::_Arg*)>, _M_storage = {_M_ptr =
0x0, _M_
buffer = {__data = "\000\000\000\000\000\000\000", __align = {<No data
fields>}}}}
FAIL: libstdc++-prettyprinters/cxx17.cc print ab
This lookup is needed to correctly handle std::any<std::string>. As
# FIXME need to expand 'std::string' so that
gdb.lookup_type works

Right, it's needed to handle std::any<std::string>.

If you're using the versioned namespace then you're not using that
type, you're using std::__7::any<std::__7::string> instead.

So you don't need to expand std::string, because it isn't there.

And you also don't need to expand std::__7::string, because that isn't
there either.

Post by François Dumont
But I don't know how to fix this so for the moment I just adapt it to
correctly handle std::__7::string.

But that's not correct. Please try to understand the point I'm making:
The name "std::__7::string" does not appear in a symbol name. So your
change to replace occurrences of that name is WRONG. We don't need to
replace something that isn't there!

The problem is that lookup for std::string fails in the versioned
namespace mode, so the solution is to not do the lookup.

Doing lookup for a different type and replacing a string that doesn't
need replacing is wrong.

This works for me:

@@ -946,9 +950,10 @@ class StdExpAnyPrinter(SingleObjContainerPrinter):
m = re.match(rx, func.function.name)
if not m:
raise ValueError("Unknown manager function in %s" % self.typename)
-
- # FIXME need to expand 'std::string' so that gdb.lookup_type works
- mgrname = re.sub("std::string(?!\w)", str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
+ mgrname = m.group(1)
+ if not typename.startswith('std::' + vers_nsp):
+ # FIXME need to expand 'std::string' so that gdb.lookup_type works
+ mgrname = re.sub("std::string(?!\w)", str(gdb.lookup_type('std::string').strip_typedefs()), mgrname)
mgrtype = gdb.lookup_type(mgrname)
self.contained_type = mgrtype.template_argument(0)
valptr = None

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
But I don't know how to fix this so for the moment I just adapt it to
correctly handle std::__7::string.

The name "std::__7::string" does not appear in a symbol name.

Ok, the only point I don't get yet is why std::string is a symbol but
std::__7::string is not. It seems inconsistent.

Post by Jonathan Wakely
m = re.match(rx, func.function.name)
raise ValueError("Unknown manager function in %s" % self.typename)
-
- # FIXME need to expand 'std::string' so that
gdb.lookup_type works
- mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
+ mgrname = m.group(1)
+ # FIXME need to expand 'std::string' so that
gdb.lookup_type works
+ mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), mgrname)

I think it doesn't work in all situations as this code is also used for
std::experimental::any so typename doesn't start with std::__7:: but
there is still no std::string symbol.

So I propose:

mgrname = m.group(1)
if 'std::string' in mgrname:
# FIXME need to expand 'std::string' so that
gdb.lookup_type works
mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))

as you will see in attach patch.

I eventually use '__7' explicitely in some pretty printers tests because
'__\d+' was not working, don't know.

Ok to commit once tests have completed ?

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely

Post by François Dumont
But I don't know how to fix this so for the moment I just adapt it
to correctly handle std::__7::string.

The name "std::__7::string" does not appear in a symbol name.

Ok, the only point I don't get yet is why std::string is a symbol but
std::__7::string is not. It seems inconsistent.

The demangler has special handling for std::basic_string<char,
std::char_traits<char>, std::allocator<char>> to treat it as
std::string, because that's much more user-friendly. But when it sees
a different symbol, like foo::basic_string<bar>, or std::__7::... it
doesn't match and doesn't get special handling.

Being consistent doesn't matter, the point is to be user-friendly.

Users want to see std::string, not std::basic_string<char, ..., ...>.

Looking at all the ugly changes needed to the tests, I'm wondering if
we actually want to strip the __7:: namespace out of the typenames
displayed by printers. That would mean we don't need to change the
tests, but more importantly, it would mean the users see "std::vector"
not "std::__7::vector". The versioned namespace is an implementation
detail, not something they write explicitly in their code.

Your change makes the printer tests pass for the versioned namespace,
but does it make the printers useful for users? We want the printers
to be useful, not just pass our testsuite.

Post by François Dumont

Post by Jonathan Wakely
m = re.match(rx, func.function.name)
raise ValueError("Unknown manager function in %s" % self.typename)
-
- # FIXME need to expand 'std::string' so that
gdb.lookup_type works
- mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
+ mgrname = m.group(1)
+ # FIXME need to expand 'std::string' so that
gdb.lookup_type works
+ mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), mgrname)

I think it doesn't work in all situations as this code is also used
but there is still no std::string symbol.

Ah yes, I forgot about the experimental one.

Post by François Dumont
mgrname = m.group(1)
# FIXME need to expand 'std::string' so that
gdb.lookup_type works
mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
as you will see in attach patch.

Looks good.

Post by François Dumont
I eventually use '__7' explicitely in some pretty printers tests
because '__\d+' was not working, don't know.

Yes, I also found that wasn't working. Presumably Tcl doesn't support
that syntax.

Post by François Dumont
Ok to commit once tests have completed ?

[Jonathan Wakely]

return
add_one_type_printer(obj, 'basic_string', pfx + 'string')
add_one_type_printer(obj, 'basic_string_view', pfx + 'string_view')
add_one_type_printer(obj, 'basic_ios', pfx + 'ios')

Looking at this part again, can't we handle the std::__7:: cases
inside add_one_type_printer instead of here?

The "pfx" prefixes here are intended for names that are imilar, like
std::string and std::wstring. If we want to handle both with an
alternative namespace then the place to do that is where we prepend
the namespace, surely?

def add_one_type_printer(obj, match, name):
printer = FilteringTypePrinter(match, 'std::' + name)
gdb.types.register_type_printer(obj, printer)
+ printer = FilteringTypePrinter(match, 'std::__7::' + name)
+ gdb.types.register_type_printer(obj, printer)

That will make the patch *much* smaller, and the logic is easier to
follow.

For the template type printers I think we just want to add (__7::)? to
the regular expressions. If we get a type like

std::__7::vector<T, std::__7::allocator<T> >

Then I think we want to print that as std::vector<T>, without __7::.

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely

Post by François Dumont
But I don't know how to fix this so for the moment I just adapt it
to correctly handle std::__7::string.

The name "std::__7::string" does not appear in a symbol name.

Ok, the only point I don't get yet is why std::string is a symbol but
std::__7::string is not. It seems inconsistent.

Post by Jonathan Wakely
m = re.match(rx, func.function.name)
raise ValueError("Unknown manager function in %s" % self.typename)
-
- # FIXME need to expand 'std::string' so that
gdb.lookup_type works
- mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
+ mgrname = m.group(1)
+ # FIXME need to expand 'std::string' so that
gdb.lookup_type works
+ mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), mgrname)

I think it doesn't work in all situations as this code is also used
but there is still no std::string symbol.
mgrname = m.group(1)
# FIXME need to expand 'std::string' so that
gdb.lookup_type works
mgrname = re.sub("std::string(?!\w)",
str(gdb.lookup_type('std::string').strip_typedefs()), m.group(1))
as you will see in attach patch.
I eventually use '__7' explicitely in some pretty printers tests
because '__\d+' was not working, don't know.
Ok to commit once tests have completed ?
François
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 3a111d7..9aba69a 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -36,6 +36,8 @@ import sys
# <https://sourceware.org/bugzilla/show_bug.cgi?id=17138>
+vers_nsp = '__7::'
+
### Python 3 stuff
Iterator = object
impl_type = self.val.type.fields()[0].type.tag
- if impl_type.startswith('std::__uniq_ptr_impl<'): # New implementation
+ if re.match('^std::(' + vers_nsp + ')?__uniq_ptr_impl<.*>$', impl_type): # New implementation
v = self.val['_M_t']['_M_t']['_M_head_impl']
v = self.val['_M_t']['_M_head_impl']
raise ValueError("Unsupported implementation for unique_ptr: %s" % self.val.type.fields()[0].type.tag)

And we could avoid three re.match expressions with complicated regular
expressions by creating a helper function to do the "startswith"
checks:

def is_specialization_of(type, template_name):
return re.match('^std::(%s)?%s<.*>$' % (vers_nsp, template_name), type) is not None

Then replace impl_type.startswith('std::__uniq_ptr_impl<') with
is_specialization_of(impl_type, '__uniq_ptr_impl')

And replace impl_type.startswith('std::tuple<') with
is_specialization_of(impl_type, 'tuple')

And replace nodetype.name.startswith('std::_Rb_tree_node') with
is_specialization_of(nodetype.name, '_Rb_tree_node')

That makes the code much easier to read.

[François Dumont]

Post by Jonathan Wakely
And we could avoid three re.match expressions with complicated regular
expressions by creating a helper function to do the "startswith"
return re.match('^std::(%s)?%s<.*>$' % (vers_nsp, template_name), type) is not None
Then replace impl_type.startswith('std::__uniq_ptr_impl<') with
is_specialization_of(impl_type, '__uniq_ptr_impl')
And replace impl_type.startswith('std::tuple<') with
is_specialization_of(impl_type, 'tuple')
And replace nodetype.name.startswith('std::_Rb_tree_node') with
is_specialization_of(nodetype.name, '_Rb_tree_node')
That makes the code much easier to read.

I agree that hiding the version namespace will be nicer. I just hope it
is possible. I don't think we can really dictate gdb to hide this
namespace during the rendering. In the attached path you will see what I
tried to do. Problem is that I am not sure that printers.py is
intercepting rendering of all types so version namespace will stay
visible in those cases.

I think the 2 failures that I have with this patch are reflecting this
problem:

type = holder<std::__7::basic_string<unsigned char,
std::__7::char_traits<unsigned char>, std::__7::allocator<unsigned char> > >
got: type = holder<std::__7::basic_string<unsigned char,
std::__7::char_traits<unsigned char>, std::__7::allocator<unsigned char> > >
FAIL: libstdc++-prettyprinters/whatis.cc whatis ustring_holder
type = holder<std::__7::basic_string<signed char,
std::__7::char_traits<signed char>, std::__7::allocator<signed char> > >
got: type = holder<std::__7::basic_string<signed char,
std::__7::char_traits<signed char>, std::__7::allocator<signed char> > >
FAIL: libstdc++-prettyprinters/whatis.cc whatis sstring_holder

Shall I simply use regex in those cases and adopt this approach ?

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely
And we could avoid three re.match expressions with complicated regular
expressions by creating a helper function to do the "startswith"
return re.match('^std::(%s)?%s<.*>$' % (vers_nsp, template_name), type) is not None
Then replace impl_type.startswith('std::__uniq_ptr_impl<') with
is_specialization_of(impl_type, '__uniq_ptr_impl')
And replace impl_type.startswith('std::tuple<') with
is_specialization_of(impl_type, 'tuple')
And replace nodetype.name.startswith('std::_Rb_tree_node') with
is_specialization_of(nodetype.name, '_Rb_tree_node')
That makes the code much easier to read.

I agree that hiding the version namespace will be nicer. I just hope
it is possible. I don't think we can really dictate gdb to hide this
namespace during the rendering. In the attached path you will see what
I tried to do. Problem is that I am not sure that printers.py is
intercepting rendering of all types so version namespace will stay
visible in those cases.
I think the 2 failures that I have with this patch are reflecting this
type = holder<std::__7::basic_string<unsigned char,
std::__7::char_traits<unsigned char>, std::__7::allocator<unsigned char> > >
got: type = holder<std::__7::basic_string<unsigned char,
std::__7::char_traits<unsigned char>, std::__7::allocator<unsigned char> > >
FAIL: libstdc++-prettyprinters/whatis.cc whatis ustring_holder
type = holder<std::__7::basic_string<signed char,
std::__7::char_traits<signed char>, std::__7::allocator<signed char> >
got: type = holder<std::__7::basic_string<signed char,
std::__7::char_traits<signed char>, std::__7::allocator<signed char> >
FAIL: libstdc++-prettyprinters/whatis.cc whatis sstring_holder
Shall I simply use regex in those cases and adopt this approach ?

I'd prefer not to have to use the regex matches in libstdc++.exp as
they complicate things.

For the two examples above, the whatis results are bad even for the
non-versioned namespace. For specializations of basic_string we only
have type printers that recognize the standard typedefs like
std::u16string, but not other specializations. We really want it to
show std::basic_string<unsigned char> not the full name. That would
require a TemplateTypePrinter for basic_string. The attached patch
works, and should be easy to incorporate into your changes for the
versioned namespace.

[François Dumont]

Post by Jonathan Wakely
I'd prefer not to have to use the regex matches in libstdc++.exp as
they complicate things.
For the two examples above, the whatis results are bad even for the
non-versioned namespace. For specializations of basic_string we only
have type printers that recognize the standard typedefs like
std::u16string, but not other specializations. We really want it to
show std::basic_string<unsigned char> not the full name. That would
require a TemplateTypePrinter for basic_string. The attached patch
works, and should be easy to incorporate into your changes for the
versioned namespace.

+ add_one_template_type_printer(obj, 'basic_string<T>',
+ 'basic_string<((un)?signed char), std::char_traits<\\1 ?>, std::allocator<\\1 ?> >',
+ 'basic_string<{1}>')
+

I had consider a similar approach but more generic like:

+ add_one_template_type_printer(obj, 'basic_string<T>',
+ 'basic_string<(.*)?, std::char_traits<\\1 ?>, std::allocator<\\1 ?> >',
+ 'basic_string<{1}>')
+


but it had bad effect on rendering of std::string type so I give up on this approach. Your version is indeed enough to cover not too exotic instantiations of std::basic_string.

I also updated 48362.cc test case as this test was already adapted for versioned namespace. But I had to keep one occurence of '__7' when displaying types inside a tuple. I think it is ok.

Tested with versioned namespace. Is it ok to commit after I completed tests without versioned namespace ?

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely
I'd prefer not to have to use the regex matches in libstdc++.exp as
they complicate things.
For the two examples above, the whatis results are bad even for the
non-versioned namespace. For specializations of basic_string we only
have type printers that recognize the standard typedefs like
std::u16string, but not other specializations. We really want it to
show std::basic_string<unsigned char> not the full name. That would
require a TemplateTypePrinter for basic_string. The attached patch
works, and should be easy to incorporate into your changes for the
versioned namespace.

+ add_one_template_type_printer(obj, 'basic_string<T>',
+ 'basic_string<((un)?signed char), std::char_traits<\\1 ?>, std::allocator<\\1 ?> >',
+ 'basic_string<{1}>')
+
+ add_one_template_type_printer(obj, 'basic_string<T>',
+ 'basic_string<(.*)?, std::char_traits<\\1 ?>, std::allocator<\\1 ?> >',
+ 'basic_string<{1}>')
+
but it had bad effect on rendering of std::string type so I give up on this approach. Your version is indeed enough to cover not too exotic instantiations of std::basic_string.

Yes, I tried that first as well.

Post by François Dumont
I also updated 48362.cc test case as this test was already adapted for versioned namespace. But I had to keep one occurence of '__7' when displaying types inside a tuple. I think it is ok.

Yes, I think so too.

Post by François Dumont
Tested with versioned namespace. Is it ok to commit after I completed tests without versioned namespace ?

I've committed the attached patch, which passes the tests for the
default configuration and the versioned namespace configuration.

I added another helper function, strip_versioned_namespace, which is
more expressive than doing typename.replace(vers_nsp, '') everywhere.
I've also renamed vers_nsp to _versioned_namespace (using the naming
convention for global variables private to the module). I've added
checks so that if that variable is None then the extra printers and
special cases for the versioned namespace are skipped. That's not
currently used, but it would allow us to optimise things later if
needed.

I also needed to update the new SharedPtrMethodsMatcher to add
"(__\d+)?" to the regular expression.

Post by François Dumont
add_one_type_printer(obj, 'discard_block_engine', 'ranlux48')
add_one_type_printer(obj, 'shuffle_order_engine', 'knuth_b')
+ # Consider optional versioned namespace
+ opt_nsp = '(' + vers_nsp + ')?'
+
# Do not show defaulted template arguments in class templates
add_one_template_type_printer(obj, 'unique_ptr<T>',
- 'unique_ptr<(.*), std::default_delete<\\1 ?> >',
- 'unique_ptr<{1}>')
+ '{0}unique_ptr<(.*), std::{0}default_delete<\\2 ?> >'.format(opt_nsp),
+ 'unique_ptr<{2}>')

This is ugly. Mixing python string formatting with regular expressions
makes it harder to read, and is inconsistent with how the versioned
namespace is handled elsewhere. In Printer.add_version and
add_one_type_printer we just register two names, one using std:: and
one using std::__7::. We can do the same for the template type
printers.

Post by François Dumont
libstdcxx_printer = Printer("libstdc++-v6")
# For _GLIBCXX_BEGIN_NAMESPACE_VERSION.
- vers = '(__7::)?'
+ vers = '(' + vers_nsp + ')?'
# For _GLIBCXX_BEGIN_NAMESPACE_CONTAINER.
container = '(__cxx1998::' + vers + ')?'

These variables don't seem to be used, so can just be removed.

[François Dumont]

Post by Jonathan Wakely
I've committed the attached patch, which passes the tests for the
default configuration and the versioned namespace configuration.
I added another helper function, strip_versioned_namespace, which is
more expressive than doing typename.replace(vers_nsp, '') everywhere.
I've also renamed vers_nsp to _versioned_namespace (using the naming
convention for global variables private to the module). I've added
checks so that if that variable is None then the extra printers and
special cases for the versioned namespace are skipped. That's not
currently used, but it would allow us to optimise things later if
needed.

Very nice feature indeed, see below.

Post by Jonathan Wakely
I also needed to update the new SharedPtrMethodsMatcher to add
"(__\d+)?" to the regular expression.

Post by François Dumont
add_one_type_printer(obj, 'discard_block_engine', 'ranlux48')
add_one_type_printer(obj, 'shuffle_order_engine', 'knuth_b')
+ # Consider optional versioned namespace
+ opt_nsp = '(' + vers_nsp + ')?'
+
# Do not show defaulted template arguments in class templates
add_one_template_type_printer(obj, 'unique_ptr<T>',
- 'unique_ptr<(.*), std::default_delete<\\1 ?> >',
- 'unique_ptr<{1}>')
+ '{0}unique_ptr<(.*), std::{0}default_delete<\\2 ?>

Post by François Dumont
'.format(opt_nsp),

+ 'unique_ptr<{2}>')

This is ugly. Mixing python string formatting with regular expressions
makes it harder to read, and is inconsistent with how the versioned
namespace is handled elsewhere. In Printer.add_version and
add_one_type_printer we just register two names, one using std:: and
one using std::__7::. We can do the same for the template type
printers.

Yes, your approach is much nicer even if it results in more type
printer registered.

My plan was to submit the attached patch but this doesn't work as
the python module seems to be loaded before libstdc++.so. If you know a
way to test for versioned namespace before starting registering printers
this patch might still be useful. Otherwise I will just forget it.

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely
I've committed the attached patch, which passes the tests for the
default configuration and the versioned namespace configuration.
I added another helper function, strip_versioned_namespace, which is
more expressive than doing typename.replace(vers_nsp, '') everywhere.
I've also renamed vers_nsp to _versioned_namespace (using the naming
convention for global variables private to the module). I've added
checks so that if that variable is None then the extra printers and
special cases for the versioned namespace are skipped. That's not
currently used, but it would allow us to optimise things later if
needed.

Very nice feature indeed, see below.

Post by Jonathan Wakely
I also needed to update the new SharedPtrMethodsMatcher to add
"(__\d+)?" to the regular expression.

Post by François Dumont
add_one_type_printer(obj, 'discard_block_engine', 'ranlux48')
add_one_type_printer(obj, 'shuffle_order_engine', 'knuth_b')
+ # Consider optional versioned namespace
+ opt_nsp = '(' + vers_nsp + ')?'
+
# Do not show defaulted template arguments in class templates
add_one_template_type_printer(obj, 'unique_ptr<T>',
- 'unique_ptr<(.*), std::default_delete<\\1 ?> >',
- 'unique_ptr<{1}>')
+ '{0}unique_ptr<(.*), std::{0}default_delete<\\2 ?>

Post by François Dumont
'.format(opt_nsp),

+ 'unique_ptr<{2}>')

This is ugly. Mixing python string formatting with regular expressions
makes it harder to read, and is inconsistent with how the versioned
namespace is handled elsewhere. In Printer.add_version and
add_one_type_printer we just register two names, one using std:: and
one using std::__7::. We can do the same for the template type
printers.

Yes, your approach is much nicer even if it results in more type
printer registered.
My plan was to submit the attached patch but this doesn't work as
the python module seems to be loaded before libstdc++.so. If you know
a way to test for versioned namespace before starting registering
printers this patch might still be useful. Otherwise I will just
forget it.

See the attached patch, which decides at configure-time whether to
enable the versioned namespace printers or not. This is what I had in
mind.

[François Dumont]

Hi

I tried to use your patch but failed so far. Situation with
autoreconf is a little bit strange. On my system the default autoreconf
is version 2.69. When using it I have:

/home/fdt/dev/gcc/git/libstdc++-v3>/usr/bin/autoreconf
configure.ac:4: error: Please use exactly Autoconf 2.64 instead of 2.69.

So I install version 2.64 and when using it I now have:

/home/fdt/dev/gcc/git/libstdc++-v3>autoreconf
configure.ac:74: error: Autoconf version 2.65 or higher is required

Is there some doc about this ?

I don't know what is the impact of it but is there a plan to allow
version 2.69 to work out of the box ?

Thanks,
François


So after I install the version 2.64 as required

Post by Jonathan Wakely

Post by François Dumont

Post by Jonathan Wakely
I've committed the attached patch, which passes the tests for the
default configuration and the versioned namespace configuration.
I added another helper function, strip_versioned_namespace, which is
more expressive than doing typename.replace(vers_nsp, '') everywhere.
I've also renamed vers_nsp to _versioned_namespace (using the naming
convention for global variables private to the module). I've added
checks so that if that variable is None then the extra printers and
special cases for the versioned namespace are skipped. That's not
currently used, but it would allow us to optimise things later if
needed.

Very nice feature indeed, see below.

Post by Jonathan Wakely
I also needed to update the new SharedPtrMethodsMatcher to add
"(__\d+)?" to the regular expression.

Post by François Dumont
add_one_type_printer(obj, 'discard_block_engine', 'ranlux48')
add_one_type_printer(obj, 'shuffle_order_engine', 'knuth_b')
+ # Consider optional versioned namespace
+ opt_nsp = '(' + vers_nsp + ')?'
+
# Do not show defaulted template arguments in class templates
add_one_template_type_printer(obj, 'unique_ptr<T>',
- 'unique_ptr<(.*), std::default_delete<\\1 ?> >',
- 'unique_ptr<{1}>')
+ '{0}unique_ptr<(.*), std::{0}default_delete<\\2 ?>

Post by François Dumont
'.format(opt_nsp),

+ 'unique_ptr<{2}>')

This is ugly. Mixing python string formatting with regular expressions
makes it harder to read, and is inconsistent with how the versioned
namespace is handled elsewhere. In Printer.add_version and
add_one_type_printer we just register two names, one using std:: and
one using std::__7::. We can do the same for the template type
printers.

Yes, your approach is much nicer even if it results in more type
printer registered.
My plan was to submit the attached patch but this doesn't work as
the python module seems to be loaded before libstdc++.so. If you know
a way to test for versioned namespace before starting registering
printers this patch might still be useful. Otherwise I will just
forget it.

See the attached patch, which decides at configure-time whether to
enable the versioned namespace printers or not. This is what I had in
mind.

[Jonathan Wakely]

Post by François Dumont
Hi
I tried to use your patch but failed so far. Situation with
autoreconf is a little bit strange. On my system the default
/home/fdt/dev/gcc/git/libstdc++-v3>/usr/bin/autoreconf
configure.ac:4: error: Please use exactly Autoconf 2.64 instead of 2.69.
/home/fdt/dev/gcc/git/libstdc++-v3>autoreconf
configure.ac:74: error: Autoconf version 2.65 or higher is required

I think this means you've already modified the file to have
AC_PREREQ(2.65), make sure you revert any changes and start again
using 2.64 (and automake 1.11.6).

Post by François Dumont
Is there some doc about this ?

https://gcc.gnu.org/install/prerequisites.html documents the
requirements for autoconf 2.64 and automake 1.11.6

Post by François Dumont
I don't know what is the impact of it but is there a plan to allow
version 2.69 to work out of the box ?

No. It's fairly easy to use the right versions.

One option is to install autoconf 2.64 and automake 1.11.6 to some
other path (e.g. $HOME/autotools-gcc) and remember to add that to
youur PATH when working on GCC e.g.

PATH=$HOME/autotools-gcc:$PATH autoreconf

Personally I configure them with:

./configure --prefix=/opt/autotools-gcc --program-suffix=_

and then have launcher scripts in /opt/autotools-gcc/bin which set
environment variables to find the right versions:

$ cat /opt/autotools-gcc/bin/autoreconf
#!/usr/bin/bash
export AUTOCONF=/opt/autotools-gcc/bin/autoconf_
export AUTOHEADER=/opt/autotools-gcc/bin/autoheader_
if [ -f /opt/autotools-gcc/bin/automake_ ]; then
export AUTOMAKE=/opt/autotools-gcc/bin/automake_
export ACLOCAL=/opt/autotools-gcc/bin/aclocal_
export AUTOM4TE=/opt/autotools-gcc/bin/autom4te_
fi
exec /opt/autotools-gcc/bin/${0##*/}_


If you use Fedora or RHEL7 there are RPM files available from:
https://copr.fedorainfracloud.org/coprs/jwakely/autotools-gcc/

[François Dumont]

Post by Jonathan Wakely

Post by François Dumont
I don't know what is the impact of it but is there a plan to allow
version 2.69 to work out of the box ?

No. It's fairly easy to use the right versions.

Just for my info, is there some blog/doc explaining why not
upgrading to latest version ? Aren't those tools better in latest versions ?

Otherwise thanks for your tip on how to manage this, it worked fine.

Attached is the latest patch. It is working fine with and without
versioned namespace.

However to validate it I had to manually change default
use_versioned_namespace value in register_libstdcxx_printers function.
This is because when running pretty printers tests, in the generated
.gdb file we have:

python register_libstdcxx_printers(None)

So always using default value (False) even if versioned namespace
have been activated. Can you advise on how to consider this when running
testsuite pretty printers ? Using again configuration ? Looking for some
definition in __7:: namespace in gdb-test.exp gdb-test proc and adapt
register_libstdcxx_printers call depending on the compilation result ?

François

[Jonathan Wakely]

Post by François Dumont

Post by Jonathan Wakely

Post by François Dumont
I don't know what is the impact of it but is there a plan to allow
version 2.69 to work out of the box ?

No. It's fairly easy to use the right versions.

Just for my info, is there some blog/doc explaining why not
upgrading to latest version ? Aren't those tools better in latest versions ?

There's nothing wrong with the versions we use, and upgrading can
cause problems. I don't know the details. But it's not difficult to
use the right versions.

Post by François Dumont
Otherwise thanks for your tip on how to manage this, it worked fine.
Attached is the latest patch. It is working fine with and without
versioned namespace.
However to validate it I had to manually change default
use_versioned_namespace value in register_libstdcxx_printers function.
This is because when running pretty printers tests, in the generated
python register_libstdcxx_printers(None)
So always using default value (False) even if versioned namespace
have been activated. Can you advise on how to consider this when
running testsuite pretty printers ? Using again configuration ?
Looking for some definition in __7:: namespace in gdb-test.exp
gdb-test proc and adapt register_libstdcxx_printers call depending on
the compilation result ?

Ah, I forgot about that generated file. Something like that should
work.

[Jonathan Wakely]

Hi
Here is the end result. I eventually chose to detect usage of
versioned namespace while generating the .gdb file. I haven't use any
caching mecanism considering the limited number of test cases.
Tested under Linux x86_64 with and without versioned namespace.
* python/Makefile.am (use_versioned_namespace): New.
(gdb.py): Subst use_versioned_namespace.
* python/Makefile.in: Regenerate.
* python/hook.in: Adapt.
* python/libstdcxx/v6/printers.py (Printer.add_version): Add name
versioned namespace if _versioned_namespace is defined.
(Printer.add_one_template_type_printer): Likewise
(add_one_type_printer): Likewise.
(register_libstdcxx_printers): Add parameter to indicate if versioned
namespace is active.
* testsuite/lib/gdb-test.exp (get_use_versioned_namespace): New.
(gdb-test): Use latter.
* testsuite/libstdc++-prettyprinters/48362.cc: Prefer note-test to
regexp-test.
Ok to commit to trunk ?

No, I thought the point of the use_versioned_namespace parameter was
to optimise thigns later, if needed. Do we need to do it?

We certainly don't need to do it during stage 4, as it's not actually
fixing any bug.

Let's reconsider during stage 1.

[François Dumont]

Post by Jonathan Wakely

Hi
Here is the end result. I eventually chose to detect usage of
versioned namespace while generating the .gdb file. I haven't use any
caching mecanism considering the limited number of test cases.
Tested under Linux x86_64 with and without versioned namespace.
* python/Makefile.am (use_versioned_namespace): New.
(gdb.py): Subst use_versioned_namespace.
* python/Makefile.in: Regenerate.
* python/hook.in: Adapt.
* python/libstdcxx/v6/printers.py (Printer.add_version): Add name
versioned namespace if _versioned_namespace is defined.
(Printer.add_one_template_type_printer): Likewise
(add_one_type_printer): Likewise.
(register_libstdcxx_printers): Add parameter to indicate if versioned
namespace is active.
* testsuite/lib/gdb-test.exp (get_use_versioned_namespace): New.
(gdb-test): Use latter.
* testsuite/libstdc++-prettyprinters/48362.cc: Prefer note-test to
regexp-test.
Ok to commit to trunk ?

No, I thought the point of the use_versioned_namespace parameter was
to optimise thigns later, if needed. Do we need to do it?

No we don't, just an enhancement indeed. Just wanted to know if the
patch was ok. I'll wait for stage 1.

François