From: "François Dumont" <frs.dumont@gmail.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Fix gdb FilteringTypePrinter (again)
Date: Thu, 17 Nov 2022 06:28:41 +0100 [thread overview]
Message-ID: <854a1216-130c-a091-65c2-fb041f21629e@gmail.com> (raw)
In-Reply-To: <CACb0b4k-bb7=CDcFWYKDYcasu=HQ1JggKFjw6EkKYWnE9Smv=A@mail.gmail.com>
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.
prev parent reply other threads:[~2022-11-17 5:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 17:38 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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=854a1216-130c-a091-65c2-fb041f21629e@gmail.com \
--to=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).