[RFC] 12266 Fallout

[RFC] 12266 Fallout

[Keith Seitz]

Hi,

I am writing for some maintainer guidance on an issue that has arisen.

The original reporter for c++/12266 reports that the bug is not fixed. 
And indeed, it isn't!

The reporter's test case (C++):

typedef std::string foo;
void calltest (foo) {}

Setting a break at "calltest(foo)" does not work because inspect_type 
(part of 12266 patchset) uses check_typedef, which resolves foo -> 
std::string -> std::basic_string<char, std::char_traits<char>, 
std::alloactor<char> >. But because we are no longer using DMGL_VERBOSE 
in dwarf2_physname (and other places) to do demangling, the symbol table 
actually stores "calltest(std::string)".

Setting a break at "calltest(std::string)" works.

Setting a break at "calltest(std::basic_string<char, ...>)" also does 
not work for the same reason "foo" didn't work.

There are two ways I can see to fix this.
1) Add DMGL_VERBOSE where it is needed so that NO typedefs ever appear 
in a (non-TYPE_CODE_TYPEDEF) symbol's name, i.e, store 
"calltest(std::basic_string<...>)" in the symbol table instead of 
"calltest(std::string)".

2) Add a new version of check_typedef which "stops" at std::string (and 
std::ostream, std::istream, std::iostream), and add logic (somewhere) to 
deal with going from "std::basic_string<...>" back to "std::string" (and 
likewise for the others).

For whatever it may be worth, my preference is for #1. I've always felt 
storing the most fundamental representation of a symbol was the best 
option: it treats all symbols the same.

Along these lines, I have done a quick audit of the code (look for 
DMGL_PARAMS), and the only places where DMGL_VERBOSE needs to be added 
is dwarf2_physname, lookup_symbol_in_language, and 
symbol_find_demangled_name. All other users of DMGL_PARAMS are using it 
for output to the display, and leaving out DMGL_VERBOSE would be preferred.

This will have the immediate side effect of printing symbol names with 
"std::basic_string<...>" instead of the more preferred "std::string".
Actually in this case, the "right" output would be "calltest(foo)", but 
that is a different, but related, problem, for which I developed a patch 
a long time ago, part of the first 12266 patch submissions 
(dwarf2_print_name).

So, I humbly ask maintainers:

- Shall I continue #1 and start submitting patches?
- Shall I start teaching gdb about std::string et al and how to deal 
with them in the environment we have today?
- Have I missed something that would be preferable to anything I've 
mentioned?

Keith

Re: [RFC] 12266 Fallout

[Tom Tromey]

Keith> I am writing for some maintainer guidance on an issue that has arisen.

Keith> 1) Add DMGL_VERBOSE where it is needed so that NO typedefs ever appear
Keith> in a (non-TYPE_CODE_TYPEDEF) symbol's name

Keith> 2) Add a new version of check_typedef which "stops" at std::string
Keith> (and std::ostream, std::istream, std::iostream), and add logic
Keith> (somewhere) to deal with going from "std::basic_string<...>" back to
Keith> "std::string" (and likewise for the others).

Keith> Actually in this case, the "right" output would be "calltest(foo)",
Keith> but that is a different, but related, problem, for which I developed a
Keith> patch a long time ago, part of the first 12266 patch submissions
Keith> (dwarf2_print_name).

Let's call this option #3.

Keith> - Shall I continue #1 and start submitting patches?
Keith> - Shall I start teaching gdb about std::string et al and how to deal
Keith> with them in the environment we have today?
Keith> - Have I missed something that would be preferable to anything I've
Keith> mentioned?

I don't understand why check_typedef would necessarily be involved.
If we changed the canonicalizer to respect this ABI rule about std::
names, then wouldn't std::string be the type's actual name?
And so we wouldn't have to change check_typedef?

In any case, isn't #3 the best approach?  That is, separating the search
key from the print name, and making the print name closer to what users
expect.  I think we should always be considering what end state we want
to be in, and this seems to be it.

If you are looking for a quick fix, but plan to do #3, then #1 is fine
by me.

Tom

Re: [RFC] 12266 Fallout

[Keith Seitz]

On 08/26/2011 11:05 AM, Tom Tromey wrote:

> I don't understand why check_typedef would necessarily be involved.
> If we changed the canonicalizer to respect this ABI rule about std::
> names, then wouldn't std::string be the type's actual name?
> And so we wouldn't have to change check_typedef?

Yes, we could make that modification to cp-name-parser.y (and also add 
something to turn std::basic_{string, ostream, istream, iostream} back 
into std::{string, ostream, istream, iostream}.

> In any case, isn't #3 the best approach?  That is, separating the search
> key from the print name, and making the print name closer to what users
> expect.  I think we should always be considering what end state we want
> to be in, and this seems to be it.

I'm all about the "end result" (which is why I mentioned that gdb should
be printing "calltest(foo)"). But the crux of the matter is still 
whether we should store "calltest(std::string)" or 
"calltest(std::basic_string<...>)" in the symbol table. That's all. The 
later is a "quick fix," but I am attempting (poorly) to also argue that 
it is the correct way to do it.

> If you are looking for a quick fix, but plan to do #3, then #1 is fine
> by me.

The two go hand-in-hand IMO. Let me work up a patch and publish it on an 
archer branch. This will contain both the print name and DMGL_VERBOSE 
patches in one. Alas, the print name stuff is rather in limbo right now, 
so it may take me much of the day to fix it up and get this all 
published. But I will try to get something pushed before you leave EOD.

Keith

Re: [RFC] 12266 Fallout

[Jan Kratochvil]

On Fri, 26 Aug 2011 01:02:59 +0200, Keith Seitz wrote:
> The reporter's test case (C++):
> 
> typedef std::string foo;
> void calltest (foo) {}
> 
> Setting a break at "calltest(foo)" does not work because
> inspect_type (part of 12266 patchset) uses check_typedef, which
> resolves foo -> std::string -> std::basic_string<char,
> std::char_traits<char>, std::alloactor<char> >. But because we are
> no longer using DMGL_VERBOSE in dwarf2_physname (and other places)
> to do demangling, the symbol table actually stores
> "calltest(std::string)".

check_typedef always unwinds only the toplevel typedefs, not any inner ones.

I think you should move the comparison block

  /* Ignore any typedefs that should not be substituted.  */
  for (i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
    {
      if (strcmp (name, ignore_typedefs[i]) == 0)
	return 0;
    }

into the later comparisons:

              /* Find the last typedef for the type.  */
              while (TYPE_TARGET_TYPE (last) != NULL
                     && (TYPE_CODE (TYPE_TARGET_TYPE (last))
                         == TYPE_CODE_TYPEDEF))
                last = TYPE_TARGET_TYPE (last);

and it would get properly caught, wouldn't be?


> There are two ways I can see to fix this.
> 1) Add DMGL_VERBOSE where it is needed so that NO typedefs ever
> appear in a (non-TYPE_CODE_TYPEDEF) symbol's name, i.e, store
> "calltest(std::basic_string<...>)" in the symbol table instead of
> "calltest(std::string)".
> Actually in this case, the "right" output would be "calltest(foo)",
> but that is a different, but related, problem, for which I developed
> a patch a long time ago, part of the first 12266 patch submissions
> (dwarf2_print_name).

As I stated off-list before I agree the primary goal is to make working
`break func(any_form_of_types)'.

And as also Tom said when there is the possibility to print the DWARF type
- incl. typedefs - it should be also printed as was implemented in:

But if you just lookup the minimal (ELF) symbol - where the dwarf2_print_name
form cannot apply - by:
(gdb) info sym 0x0000003f1b25ed70

I guess it should say:
std::hash<std::string>::operator()(std::string) const in section .text of /usr/lib64/libstdc++.so.6

and not:
std::hash<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >::operator()(std::basic_string<char, std::char_traits<char>, std::allocator<char> >) const
in section .text of /usr/lib64/libstdc++.so.6

while nm -C says:
0000003f1b25ed70 T std::hash<std::string>::operator()(std::string) const


IMO if GDB starts to use DMGL_VERBOSE we should ask binutils to use
DMGL_VERBOSE for `nm -C' and other tools there.


> So, I humbly ask maintainers:
> 
> - Shall I continue #1 and start submitting patches?
> - Shall I start teaching gdb about std::string et al and how to deal
> with them in the environment we have today?
> - Have I missed something that would be preferable to anything I've
> mentioned?

I would prefer to keep `std::string' to be `std::string' as possible, which
I naively believe could be done without modifying/duplicating check_typedef by
the change suggested above.


Thanks,
Jan