Is physname mangled or not? (PR c++/8216)

Is physname mangled or not? (PR c++/8216)

[Ulrich Weigand]

Hello,

PR c++/8216 says:

When doing ptype on a templated class, GDB will print
out constructors as having a return type of void
instead of as having no return type at all.

Code in c-typename.c:c_type_print_base does this in an attempt
to determine whether a method is a constructor:

              char *method_name = TYPE_FN_FIELDLIST_NAME (type, i);
              char *name = type_name_no_tag (type);
              int is_constructor = name && strcmp (method_name,
                                                   name) == 0;

              for (j = 0; j < len2; j++)
                {
                  char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
                  int is_full_physname_constructor =
                    is_constructor_name (physname)
                    || is_destructor_name (physname)
                    || method_name[0] == '~';

[...]
                  else if (!is_constructor      /* Constructors don't
                                                   have declared
                                                   types.  */
                           && !is_full_physname_constructor  /* " " */
                           && !is_type_conversion_operator (type, i, j))


So basically this says: a method is a constructor if either:
 - the method name equals the type name, or
 - the C++ ABI routine says the physname is a constructor

and, a method is a destructor if either:
 - the method name starts with a ~, or
 - the C++ ABI routine says the physname is a destructor

I'm not quite sure why we need those duplicate checks, but in
any case for the constructor they both fail:

 - For template classes, the method name of a constructor does
   *not* contain the template instance type list, while the type
   name does, and thus the strcmp fails

 - The is_constructor_name C++ ABI callback actually fails
   *always*.  This is because it works only if it gets a 
   *mangled* name as input, but TYPE_FN_FIELD_PHYSNAME returns
   a demangled name (at least with dwarf2read.c) ...

(For destructors, this problem doesn't apply since the ~ test
catches just about everything.  I'm just wondering why we still
try to do the is_destructor_name (physname) check as well ...)

This latter seems to have been due to a deliberate change of
physname: dwarf2read now always sets this to a demangled name,
while e.g. stabsread sets it to a mangled name.

So I guess my question is, how is this supposed to work?  Should
is_constructor_name accept demangled names?  Should there be some
generic routine that instead tests a demangled name for whether
it is a constructor (or destructor)?

Any suggestions to fix this would be appreciated ...

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Is physname mangled or not? (PR c++/8216)

[Jan Kratochvil]

On Tue, 03 May 2011 20:58:49 +0200, Ulrich Weigand wrote:
>  - For template classes, the method name of a constructor does
>    *not* contain the template instance type list, while the type
>    name does, and thus the strcmp fails

 <1><55>: Abbrev Number: 6 (DW_TAG_class_type)
    <56>   DW_AT_name        : (indirect string, offset: 0xb): C<int>
 <2><61>: Abbrev Number: 7 (DW_TAG_subprogram)
    <63>   DW_AT_name        : C
    <68>   DW_AT_declaration : 1
    <69>   DW_AT_object_pointer: <0x71>
 <3><71>: Abbrev Number: 4 (DW_TAG_formal_parameter)
    <72>   DW_AT_type        : <0x80>
    <76>   DW_AT_artificial  : 1
 <2><78>: Abbrev Number: 8 (DW_TAG_template_type_param)
    <79>   DW_AT_name        : T
    <7b>   DW_AT_type        : <0x119>
(less important attributes removed).

This is IMO a GCC debug/ bug, shouldn't <63> DW_AT_name be `C<int>'?


>  - The is_constructor_name C++ ABI callback actually fails
>    *always*.  This is because it works only if it gets a 
>    *mangled* name as input, but TYPE_FN_FIELD_PHYSNAME returns
>    a demangled name (at least with dwarf2read.c) ...

This physname change broke more issues, thanks for finding this one.
The change I described in the bottom patch of:
	Re: [RFA] Typedef'd method parameters [2/4]
	http://sourceware.org/ml/gdb-patches/2011-04/msg00524.html
that is the part:
	The mangled symbol name is not available for full symbols.  */
	 #define SYMBOL_LINKAGE_NAME(symbol)	(symbol)->ginfo.name
Although a similar comment should be now also at:
	TYPE_FN_FIELD_PHYSNAME
The other physname regressions of belonging to this no-longer-mangled category:
	New: regression by physname: PE32 prologue skip vs. static initializers
	http://sourceware.org/bugzilla/show_bug.cgi?id=12680
	New: physname regression: set print demangle off
	http://sourceware.org/bugzilla/show_bug.cgi?id=12707


> So I guess my question is, how is this supposed to work?  Should
> is_constructor_name accept demangled names?

It cannot - it does not know the type name, does it?


> Should there be some generic routine that instead tests a demangled name for
> whether it is a constructor (or destructor)?

With a GCC debug/ fix it should work.  Do you agree with its filing?


Thanks,
Jan

Re: Is physname mangled or not? (PR c++/8216)

[Daniel Jacobowitz]

On Wed, May 04, 2011 at 05:08:46PM +0200, Jan Kratochvil wrote:
> On Tue, 03 May 2011 20:58:49 +0200, Ulrich Weigand wrote:
> >  - For template classes, the method name of a constructor does
> >    *not* contain the template instance type list, while the type
> >    name does, and thus the strcmp fails
> 
>  <1><55>: Abbrev Number: 6 (DW_TAG_class_type)
>     <56>   DW_AT_name        : (indirect string, offset: 0xb): C<int>
>  <2><61>: Abbrev Number: 7 (DW_TAG_subprogram)
>     <63>   DW_AT_name        : C
>     <68>   DW_AT_declaration : 1
>     <69>   DW_AT_object_pointer: <0x71>
>  <3><71>: Abbrev Number: 4 (DW_TAG_formal_parameter)
>     <72>   DW_AT_type        : <0x80>
>     <76>   DW_AT_artificial  : 1
>  <2><78>: Abbrev Number: 8 (DW_TAG_template_type_param)
>     <79>   DW_AT_name        : T
>     <7b>   DW_AT_type        : <0x119>
> (less important attributes removed).
> 
> This is IMO a GCC debug/ bug, shouldn't <63> DW_AT_name be `C<int>'?

No.  The constructor is named C, not C<int>, I believe.

-- 
Daniel Jacobowitz
Mentor Graphics

Re: Is physname mangled or not? (PR c++/8216)

[Jan Kratochvil]

On Wed, 04 May 2011 18:59:08 +0200, Daniel Jacobowitz wrote:
> No.  The constructor is named C, not C<int>, I believe.

I agree now:

0000000000400506 W C<int>::C()
0000000000400506 W _ZN1CIiEC1Ev


Thanks,
Jan

Re: Is physname mangled or not? (PR c++/8216)

[Ulrich Weigand]

Jan Kratochvil wrote:

> This physname change broke more issues, thanks for finding this one.
> The change I described in the bottom patch of:
> 	Re: [RFA] Typedef'd method parameters [2/4]
> 	http://sourceware.org/ml/gdb-patches/2011-04/msg00524.html
> that is the part:
> 	The mangled symbol name is not available for full symbols.  */
> 	 #define SYMBOL_LINKAGE_NAME(symbol)	(symbol)->ginfo.name

Hmm, unless I'm missing something your comment

+   For full symbols return its demangled form of view of the linker, that is
+   with typedefs and toplevel const/volatile qualifiers of parameters removed,
+   for `f(int_typedef const)' it is `f(int)'.  If no typedefs/qualifiers are
+   in use it's the same as SYMBOL_NATURAL_NAME.  The mangled symbol name is not
+   available for full symbols.  */

is not fully accurate either.  As far as I can tell, that *is* true for
symbols read in by the DWARF reader.  However, for symbols read in by
the stabs reads (or any of the others), SYMBOL_LINKAGE_NAME still refers
to the mangled name ...   (Which strikes me as quite odd in the first
place; how is common code supposed to use this field?)

> > So I guess my question is, how is this supposed to work?  Should
> > is_constructor_name accept demangled names?
> 
> It cannot - it does not know the type name, does it?

Right, good point.

> > Should there be some generic routine that instead tests a demangled name for
> > whether it is a constructor (or destructor)?
> 
> With a GCC debug/ fix it should work.  Do you agree with its filing?

It seems that by now we have agreement that GCC is correct here.  So I guess
I see two options remaining:

- Code a test that compares class name and (demangled) function name, but
  explicitly removes template instance parameters first

or

- Have the symbol reader call is_constructor_name on the mangled name while
  it is still available, and store that information somewhere in the type
  information

Thoughts?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Is physname mangled or not? (PR c++/8216)

[Jan Kratochvil]

On Wed, 04 May 2011 19:32:43 +0200, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> +   For full symbols return its demangled form of view of the linker, that is
> +   with typedefs and toplevel const/volatile qualifiers of parameters removed,
> +   for `f(int_typedef const)' it is `f(int)'.  If no typedefs/qualifiers are
> +   in use it's the same as SYMBOL_NATURAL_NAME.  The mangled symbol name is not
> +   available for full symbols.  */
> 
> is not fully accurate either.  As far as I can tell, that *is* true for
> symbols read in by the DWARF reader.  However, for symbols read in by
> the stabs reads (or any of the others), SYMBOL_LINKAGE_NAME still refers
> to the mangled name ...   (Which strikes me as quite odd in the first
> place; how is common code supposed to use this field?)

Thanks for the update.  Yes, SYMBOL_LINKAGE_NAME is somehow broken now for
many use cases, I do not know myself what is the best way out now.


> > With a GCC debug/ fix it should work.  Do you agree with its filing?
> 
> It seems that by now we have agreement that GCC is correct here.

I haven't found in the C++ spec. why it isn't a different way but that is out
of topic here, the truth is the current linkage name is `C<int>::C()'.


> So I guess I see two options remaining:
> 
> - Code a test that compares class name and (demangled) function name, but
>   explicitly removes template instance parameters first

I find it as a good idea.


> - Have the symbol reader call is_constructor_name on the mangled name while
>   it is still available, and store that information somewhere in the type
>   information

The mangled name is never read (processed) from the DWARF file now.


Thanks,
Jan

Re: Is physname mangled or not? (PR c++/8216)

[Daniel Jacobowitz]

On Wed, May 04, 2011 at 07:32:43PM +0200, Ulrich Weigand wrote:
> It seems that by now we have agreement that GCC is correct here.  So I guess
> I see two options remaining:
> 
> - Code a test that compares class name and (demangled) function name, but
>   explicitly removes template instance parameters first
> 
> or
> 
> - Have the symbol reader call is_constructor_name on the mangled name while
>   it is still available, and store that information somewhere in the type
>   information
> 
> Thoughts?

I'd suggest the former.  Anything you do with mangled names will be
unexpectedly complex; sometimes you just can't count on having them.

-- 
Daniel Jacobowitz
Mentor Graphics

Re: Is physname mangled or not? (PR c++/8216)

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Wed, May 04, 2011 at 07:32:43PM +0200, Ulrich Weigand wrote:
> > It seems that by now we have agreement that GCC is correct here.  So I guess
> > I see two options remaining:
> > 
> > - Code a test that compares class name and (demangled) function name, but
> >   explicitly removes template instance parameters first
> > 
> > or
> > 
> > - Have the symbol reader call is_constructor_name on the mangled name while
> >   it is still available, and store that information somewhere in the type
> >   information
> > 
> > Thoughts?
> 
> I'd suggest the former.  Anything you do with mangled names will be
> unexpectedly complex; sometimes you just can't count on having them.

OK, I see.  I've done a little more digging through the history, and I guess
I understand a little better why things were done the way they were.

Mostly, it seems to have come down to the problem that some past compilers
used to fill in silly things for the debug data method names of special
methods like constructors (e.g. __comp_ctor).  See the long comment in
stabsread.c:read_member_functions.

To work around this, the code tries to "re-create" the proper method name
using a variety of approaches, amongst others by looking at the physname
(which at this point is a mangled name) if present (in stabs).

Mostly, this gets done when the method definition is read in.  However, there
are cases where this is apparently not possible, and the method is marked as
a "stub".  There is a routine check_stub_method_group that users of the
method list are apparently supposed to call before looking at method names,
which "unstubs" methods and corrects all names as appropriate.  This is done
in various places (search_struct_method etc.).

However, there are other places -in particular c_type_print_base, which is
what initially started this discussion- that do *not* call the helper
check_stub_method_group to fix things up.  Instead (apparently) those places
try to handle stubbed methods directly, duplicating various parts of what
check_stub_method_group would do (e.g. call gdb_mangle_name).

*This* in turn appears to be the reason why c_type_print_base does the
weird duplicate check for constructors/destructors looking both at the
method name and the physname -- the former is needed because physname
is not always available, the latter is needed because the method name
may be wrong for stubbed methods.


So, if the analysis so far is correct, it would appear to me that the
best way forward is actually to:

- change c_type_print_base (and other places) to actually just call
  check_stub_method_group, and then assume the method name is always
  correct -- removing all the extra code required for stubbed methods,
  including the physname-based constructor/destructor checks

- update the method name based checks to handle templates.

B.t.w. at yet another location in linespec.c:find_methods, an attempt
to handle templates seems to have been made when detecting constructors:

          else if (strncmp (class_name, name, name_len) == 0
                   && (class_name[name_len] == '\0'
                       || class_name[name_len] == '<'))


Does this look reasonable?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com