Re: [RFA] Typedef'd method parameters [2/4]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 21 Apr 2011 23:15:33 +0200, Keith Seitz wrote:
> Comments/questions/concerns?

This patchset has a regression for:
FAIL: gdb.fortran/library-module.exp: print var_i in lib
FAIL: gdb.fortran/library-module.exp: print var_i in main
FAIL: gdb.fortran/library-module.exp: print var_j
FAIL: gdb.fortran/module.exp: stopped language detection
FAIL: gdb.fortran/module.exp: fully qualified name of DW_TAG_constant
FAIL: gdb.fortran/module.exp: print var_i value 1
FAIL: gdb.fortran/module.exp: print var_i value 2
FAIL: gdb.fortran/module.exp: print var_b
FAIL: gdb.fortran/module.exp: print var_d
FAIL: gdb.fortran/module.exp: print var_i value 14

As this Fortran modules code is by me going to check more if I haven't misused
some C++ structures inappropriately.



> --- c-typeprint.c	22 Mar 2011 17:35:22 -0000	1.70
> +++ c-typeprint.c	20 Apr 2011 23:38:49 -0000
> @@ -44,14 +44,22 @@ static void c_type_print_varspec_prefix 
>  static void c_type_print_modifier (struct type *,
>  				   struct ui_file *,
>  				   int, int);
> +
> +static void c_type_print_base_internal (struct type *, struct ui_file *,
> +					int, int, int);
> +
>  
> -/* LEVEL is the depth to indent lines by.  */
> +/* The real c_print_type.  See c_print_type below for a description
> +   of parameters and usage.
>  
> -void
> -c_print_type (struct type *type,
> -	      const char *varstring,
> -	      struct ui_file *stream,
> -	      int show, int level)
> +   LINKAGE_NAME should be non-zero if we are printing a linkage name
> +   on the stream.  */
> +
> +static void
> +c_print_type_internal (struct type *type,

Such kind of functions is called c_print_type_1 in GDB AFAIK, not requesting
a change.


[...]
> @@ -386,17 +406,25 @@ c_type_print_modifier (struct type *type
>  
>  
>  /* Print out the arguments of TYPE, which should have TYPE_CODE_METHOD
> -   or TYPE_CODE_FUNC, to STREAM.  Artificial arguments, such as "this"
> -   in non-static methods, are displayed if LINKAGE_NAME is zero.  If
> -   LINKAGE_NAME is non-zero and LANGUAGE is language_cplus the topmost
> -   parameter types get removed their possible const and volatile qualifiers to
> +   or TYPE_CODE_FUNC, to STREAM.
> +
> +   LINKAGE_NAME is non-zero when the SYMBOL_LINKAGE_NAME of TYPE
> +   (a function or method) is printed.  Artifical parameters such as "this"
> +   in non-static methods are skipped and all typedefs will be removed/expanded.
> +   Additionally if LANGUAGE is language_cplus, topmost parameter types
> +   may have any possible const and volatile qualifiers removed to
>     match demangled linkage name parameters part of such function type.
> +
> +   PRINT_NAME is non-zero when the SYMBOL_PRINT_NAME of TYPE is
> +   printed.  In this case, artificial parameters are skipped, but
> +   any typedef'd parameter types are left intact.
> +

What is both LINKAGE_NAME and PRINT_NAME are set?  There is already introduced
`enum name_kind' in this patchset for that, it would make the caller
statements more clear.


> +/* Print the name of the type (or the ultimate pointer target,
> +   function value or array element), or the description of a structure
> +   or union.
> +
> +   SHOW positive means print details about the type (e.g. enum
> +   values), and print structure elements passing SHOW - 1 for show.
> +
> +   SHOW negative means just print the type name or struct tag if there
> +   is one.  If there is no name, print something sensible but concise
> +   like "struct {...}".
> +
> +   SHOW zero means just print the type name or struct tag if there is
> +   one.  If there is no name, print something sensible but not as
> +   concise like "struct {int x; int y;}".
> +
> +   LEVEL is the number of spaces to indent by.
> +   We increase it for some recursive calls.  */
> +
> +void
> +c_type_print_base (struct type *type, struct ui_file *stream,
> +		   int show, int level)
> +{
> +  c_type_print_base_internal (type, stream, show, level, 0);
> +}

It has 4 callers in total, it may not be worth creating a wrapper.


[...]
> --- dwarf2read.c	15 Apr 2011 15:05:04 -0000	1.524
> +++ dwarf2read.c	20 Apr 2011 23:38:52 -0000
[...]
> @@ -11002,6 +11023,20 @@ new_symbol_full (struct die_info *die, s
>        linkagename = dwarf2_physname (name, die, cu);
>        SYMBOL_SET_NAMES (sym, linkagename, strlen (linkagename), 0, objfile);
>  
> +      /* For C++ set the symbol's demangled name if it is different than
> +	 the computed physname.  This can happen when the source defines
> +	 a method with typedef'd parameters.  This is ultimately used by
> +	 the type printer.  */
> +      if (cu->language == language_cplus && die->tag == DW_TAG_subprogram)
> +	{
> +	  const char *print_name = dwarf2_print_name (name, die, cu);

Missing empty line.

> +	  if (strcmp (print_name, linkagename))
> +	    {
> +	      symbol_set_demangled_name (&(sym->ginfo),
> +					 (char *) print_name, NULL);
> +	    }
> +	}
> +
>        /* Fortran does not have mangling standard and the mangling does differ
>  	 between gfortran, iFort etc.  */
>        if (cu->language == language_fortran
[...]
> --- symtab.c	19 Apr 2011 18:04:07 -0000	1.266
> +++ symtab.c	20 Apr 2011 23:38:55 -0000
> @@ -740,17 +740,21 @@ symbol_demangled_name (const struct gene
>    return NULL;
>  }
>  
> -/* Return the search name of a symbol---generally the demangled or
> -   linkage name of the symbol, depending on how it will be searched for.
> -   If there is no distinct demangled name, then returns the same value
> -   (same pointer) as SYMBOL_LINKAGE_NAME.  */
> +/* Return the search name of a symbol.  This is typically the smybol's 
                                                                 ^^^^^^=symbol
> +   physname (SYMBOL_LINKAGE_NAME). Symbols with mangled names
> +   are special: return their SYMBOL_NATURAL_NAME instead.  */

I do not find this comment sufficient.  With the extended macros comments
I would now prefer just:

/* Return the SYMBOL_SEARCH_NAME kind of name for GSYMBOL.  */


>  char *
>  symbol_search_name (const struct general_symbol_info *gsymbol)
>  {
>    if (gsymbol->language == language_ada)
>      return gsymbol->name;
>    else
> -    return symbol_natural_name (gsymbol);
> +    {
> +      if (gsymbol->name && gsymbol->name[0] == '_' && gsymbol->name[1] == 'Z')
> +	return symbol_natural_name (gsymbol);
> +      else
> +	return gsymbol->name;
> +    }
>  }
>  
>  /* Initialize the structure fields to zero values.  */
> @@ -2900,8 +2904,8 @@ compare_search_syms (const void *sa, con
>    struct symbol_search **sym_a = (struct symbol_search **) sa;
>    struct symbol_search **sym_b = (struct symbol_search **) sb;
>  
> -  return strcmp (SYMBOL_PRINT_NAME ((*sym_a)->symbol),
> -		 SYMBOL_PRINT_NAME ((*sym_b)->symbol));
> +  return strcmp (SYMBOL_SEARCH_NAME ((*sym_a)->symbol),
> +		 SYMBOL_SEARCH_NAME ((*sym_b)->symbol));

This change is difficult to evaluate as SYMBOL_PRINT_NAME
#define SYMBOL_PRINT_NAME(symbol)                                       \
  (demangle ? SYMBOL_NATURAL_NAME (symbol) : SYMBOL_LINKAGE_NAME (symbol))
is question about whether to DEMANGLE or NOT to DEMANGLE, that is about
`set print demangle', which is already broken by physname:
	[Bug symtab/12707] New: physname regression: set print demangle off

As this comparison is used only for symbols sorting (and not even any symbols
duplication removal) and print_symbol_info still uses SYMBOL_PRINT_NAME
I would prefer to keep it as is.  Or do you see any problem with it?
This function should affect only:
	rbreak
	info variables
	info functions
	info types



I would check in / include with this patchset a fixup of the sometimes already
incorrect before this patchset and definitely incorrect after this patchset
comments.


Thanks,
Jan


gdb/
2011-04-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symtab.h (SYMBOL_NATURAL_NAME, SYMBOL_LINKAGE_NAME)
	(SYMBOL_DEMANGLED_NAME, SYMBOL_SEARCH_NAME): Extend the comments.

--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -219,21 +219,33 @@ extern void symbol_set_names (struct general_symbol_info *symbol,
 /* Return SYMBOL's "natural" name, i.e. the name that it was called in
    the original source code.  In languages like C++ where symbols may
    be mangled for ease of manipulation by the linker, this is the
-   demangled name.  */
+   demangled name.  This function always returns original typedefs and
+   const/volatile qualifiers present in parameter types such as
+   `f(int_typedef const)' */
 
 #define SYMBOL_NATURAL_NAME(symbol) \
   (symbol_natural_name (&(symbol)->ginfo))
 extern char *symbol_natural_name (const struct general_symbol_info *symbol);
 
-/* Return SYMBOL's name from the point of view of the linker.  In
-   languages like C++ where symbols may be mangled for ease of
-   manipulation by the linker, this is the mangled name; otherwise,
-   it's the same as SYMBOL_NATURAL_NAME.  */
+/* For minimal symbols return SYMBOL's name from the point of view of the
+   linker.  In languages like C++ where symbols may be mangled for ease of
+   manipulation by the linker, this is the mangled name,
+   for `f(int_typedef const)' it is `_Z1fi'; otherwise, it's the same as
+   SYMBOL_NATURAL_NAME.
+
+   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.  */
 
 #define SYMBOL_LINKAGE_NAME(symbol)	(symbol)->ginfo.name
 
 /* Return the demangled name for a symbol based on the language for
-   that symbol.  If no demangled name exists, return NULL.  */
+   that symbol.  This function always returns typedefs and toplevel
+   const/volatile qualifiers of parameters preserved.  If no demangled name
+   exists, return NULL.  */
+
 #define SYMBOL_DEMANGLED_NAME(symbol) \
   (symbol_demangled_name (&(symbol)->ginfo))
 extern char *symbol_demangled_name (const struct general_symbol_info *symbol);
@@ -265,10 +277,22 @@ extern char *symbol_demangled_name (const struct general_symbol_info *symbol);
   (strcmp_iw (SYMBOL_NATURAL_NAME (symbol), (name)) == 0)
 
 /* Macro that returns the name to be used when sorting and searching symbols.
-   In  C++, Chill, and Java, we search for the demangled form of a name,
-   and so sort symbols accordingly.  In Ada, however, we search by mangled
-   name.  If there is no distinct demangled name, then SYMBOL_SEARCH_NAME
-   returns the same value (same pointer) as SYMBOL_LINKAGE_NAME.  */
+   In C++, Chill, and Java, we search for the demangled form of a linkage name,
+   and so sort symbols accordingly:
+   
+   For minimal symbols the demangled SYMBOL_NATURAL_NAME form is returned, no
+   typedefs nor toplevel const/volatile qualifiers are relevant for minimal
+   symbols, for `f(int_typedef const)' it returns `f(int)'.
+   
+   For full symbols SYMBOL_LINKAGE_NAME is already the demangled form with
+   typedefs and toplevel const/volatile qualifiers removed.  It is preferred
+   over SYMBOL_NATURAL_NAME which may contain the typedefs.
+   For `f(int_typedef const)' it also returns `f(int)'.
+
+   In Ada, however, we search by mangled name.  If there is no distinct
+   demangled name, then SYMBOL_SEARCH_NAME returns the same value (same
+   pointer) as SYMBOL_LINKAGE_NAME.  */
+
 #define SYMBOL_SEARCH_NAME(symbol)					 \
    (symbol_search_name (&(symbol)->ginfo))
 extern char *symbol_search_name (const struct general_symbol_info *);