Re: [RFA] Use rust_demangle

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-04-18 08:31, Tom Tromey wrote:
> PR rust/20268 is about using the extra Rust-specific demangling that
> was added to libiberty.

It looks like you got the wrong PR number, I think it should be 20367.

> Initially this looked easy: just change rust_language_defn to
> reference rust_demangle.  However, it's not quite so simple:

There are many things packed in a single patch, I think they would each 
deserve more explanation so that non-rusters can understand.

> * Writing a "real" test was tricky because some bugs in the Rust
>   compiler cause gdb to pick funny names for the affected functions,
>   and when debuginfo is available it's hard to access the minsyms.
>   This I fixed by adding a comment and compiling without debuginfo.
> 
> * This revealed a couple of spots in symtab.c where gdb wasn't
>   checking for language_rust.
> 
> * The C++ demangler runs before the Rust demangler.  And, because the
>   Rust mangling scheme is C++-like, Rust symbols will never be seen by
>   the Rust demangler.  The fix is to have the C++ demangler return
>   NULL for Rust symbols.

Here for example, could you provide an example of when things go wrong 
(a Rust symbol wrongfully demangled as a C++ symbol)?

> * ... but that affects some tests cases, like demangle.exp, because
>   gdb then need special treatment for DMGL_AUTO.
> 
> * ... and, in light of this change, "set demangle-style" seems a bit
>   weird because it allows the setting of styles to things obviously
>   not related to C++ at all, hence a change in demangle.c.

If other languages have mangling too, why shouldn't we be able to set 
the mangling to that style?  For example, why shouldn't we be able to do 
"set demangle-style rust"?

> Two future ideas came to mind.  One is to remove obsolete things like
> the Lucide demangling style.  The other is to change the demangler's
> "auto" mode to also return the language of the symbol, if it could be
> discovered.  This would avoid double demangling.
> 
> Built and regtested on the buildbot.
> 
> ChangeLog
> 2017-04-16  Tom Tromey  <tom@tromey.com>
> 
> 	PR rust/20268:
> 	* demangle.c (_initialize_demangler): Filter out non-C++ styles.
> 	* symtab.c (symbol_natural_name, symbol_demangled_name): Demangle
> 	for language_rust.
> 	* cp-support.h (gdb_demangle_cxx): Declare.
> 	* cp-support.c (gdb_demangle_cxx): New function.
> 	(gdb_sniff_from_mangled_name): Use it.
> 	* c-lang.c (cplus_language_defn): Reference gdb_demangle_cxx.
> 	* rust-lang.c (rust_sniff_from_mangled_name): Call rust_demangle.
> 	(rust_language_defn): Reference rust_demangle.
> 
> testsuite/ChangeLog
> 2017-04-14  Tom Tromey  <tom@tromey.com>
> 
> 	PR rust/20268:
> 	* gdb.rust/rdemangle.exp: New file.
> 	* gdb.rust/rdemangle.rs: New file.
> ---
>  gdb/ChangeLog                        | 13 +++++++++++
>  gdb/c-lang.c                         |  2 +-
>  gdb/cp-support.c                     | 23 ++++++++++++++++++-
>  gdb/cp-support.h                     |  8 ++++++-
>  gdb/demangle.c                       | 36 
> ++++++++++++++++++++----------
>  gdb/rust-lang.c                      |  4 ++--
>  gdb/symtab.c                         |  2 ++
>  gdb/testsuite/ChangeLog              |  6 +++++
>  gdb/testsuite/gdb.rust/rdemangle.exp | 39 
> ++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.rust/rdemangle.rs  | 43 
> ++++++++++++++++++++++++++++++++++++
>  10 files changed, 159 insertions(+), 17 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.rust/rdemangle.exp
>  create mode 100644 gdb/testsuite/gdb.rust/rdemangle.rs
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index f033ced..a96507a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,16 @@
> +2017-04-16  Tom Tromey  <tom@tromey.com>
> +
> +	PR rust/20268:
> +	* demangle.c (_initialize_demangler): Filter out non-C++ styles.
> +	* symtab.c (symbol_natural_name, symbol_demangled_name): Demangle
> +	for language_rust.
> +	* cp-support.h (gdb_demangle_cxx): Declare.
> +	* cp-support.c (gdb_demangle_cxx): New function.
> +	(gdb_sniff_from_mangled_name): Use it.
> +	* c-lang.c (cplus_language_defn): Reference gdb_demangle_cxx.
> +	* rust-lang.c (rust_sniff_from_mangled_name): Call rust_demangle.
> +	(rust_language_defn): Reference rust_demangle.
> +
>  2017-04-17  Joel Brobecker  <brobecker@adacore.com>
> 
>  	* NEWS: Create a new section for the next release branch.
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index 19a8608..874916e 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -995,7 +995,7 @@ const struct language_defn cplus_language_defn =
>    "this",                       /* name_of_this */
>    cp_lookup_symbol_nonlocal,	/* lookup_symbol_nonlocal */
>    cp_lookup_transparent_type,   /* lookup_transparent_type */
> -  gdb_demangle,			/* Language specific symbol demangler */
> +  gdb_demangle_cxx,	        /* Language specific symbol demangler */

Should it be called cp_demangle for consistency?

>    gdb_sniff_from_mangled_name,
>    cp_class_name_from_physname,  /* Language specific
>  				   class_name_from_physname */
> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 5704466..1a86532 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -1588,10 +1588,31 @@ gdb_demangle (const char *name, int options)
> 
>  /* See cp-support.h.  */
> 
> +char *
> +gdb_demangle_cxx (const char *mangled, int options)
> +{
> +  int style = (DMGL_GNU | DMGL_LUCID | DMGL_ARM
> +	       | DMGL_HP | DMGL_EDG | DMGL_GNU_V3);
> +
> +  if ((current_demangling_style & DMGL_AUTO) == 0)
> +    style &= current_demangling_style;

This deserves a comment, what does it accomplish?  Restrict the style to 
a restricted set of styles?

> +
> +  char *ret = gdb_demangle (mangled, (options | DMGL_PARAMS | 
> DMGL_ANSI
> +				      | style));
> +  if (rust_is_mangled (ret))
> +    {
> +      xfree (ret);
> +      ret = NULL;
> +    }

The reference to rust here looks strange enough that it deserves a 
comment.

> +  return ret;
> +}
> +
> +/* See cp-support.h.  */
> +
>  int
>  gdb_sniff_from_mangled_name (const char *mangled, char **demangled)
>  {
> -  *demangled = gdb_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
> +  *demangled = gdb_demangle_cxx (mangled, 0);
>    return *demangled != NULL;
>  }
> 
> diff --git a/gdb/cp-support.h b/gdb/cp-support.h
> index 9054bf6..aaca67e 100644
> --- a/gdb/cp-support.h
> +++ b/gdb/cp-support.h
> @@ -157,7 +157,13 @@ extern struct cmd_list_element 
> *maint_cplus_cmd_list;
> 
>  char *gdb_demangle (const char *name, int options);
> 
> -/* Like gdb_demangle, but suitable for use as 
> la_sniff_from_mangled_name.  */
> +/* A wrapper for gdb_demangle that will only successfully demangle C++
> +   names.  */
> +
> +char *gdb_demangle_cxx (const char *mangled, int options);
> +
> +/* Like gdb_demangle_cxx, but suitable for use as
> +   la_sniff_from_mangled_name.  */
> 
>  int gdb_sniff_from_mangled_name (const char *mangled, char 
> **demangled);
> 
> diff --git a/gdb/demangle.c b/gdb/demangle.c
> index 2d79c1d..d9ed896 100644
> --- a/gdb/demangle.c
> +++ b/gdb/demangle.c
> @@ -232,25 +232,37 @@ extern initialize_file_ftype
> _initialize_demangler; /* -Wmissing-prototypes */
>  void
>  _initialize_demangler (void)
>  {
> -  int i, ndems;
> +  int i, out, ndems;
> 
>    /* Fill the demangling_style_names[] array, and set the default
>       demangling style chosen at compilation time.  */
> -  for (ndems = 0;
> -       libiberty_demanglers[ndems].demangling_style != 
> unknown_demangling;
> -       ndems++)
> -    ;
> +  for (i = ndems = 0;
> +       libiberty_demanglers[i].demangling_style != unknown_demangling;
> +       i++)
> +    {
> +      if (libiberty_demanglers[i].demangling_style != gnat_demangling
> +	  && libiberty_demanglers[i].demangling_style != dlang_demangling
> +	  && libiberty_demanglers[i].demangling_style != rust_demangling)
> +	++ndems;
> +    }
> 
>    demangling_style_names = XCNEWVEC (const char *, ndems + 1);
> -  for (i = 0;
> +  for (i = out = 0;
>         libiberty_demanglers[i].demangling_style != unknown_demangling;
>         i++)
>      {
> -      demangling_style_names[i]
> -	= xstrdup (libiberty_demanglers[i].demangling_style_name);
> -
> -      if (current_demangling_style_string == NULL
> -	  && strcmp (DEFAULT_DEMANGLING_STYLE, demangling_style_names[i]) == 
> 0)
> -	current_demangling_style_string = demangling_style_names[i];
> +      if (libiberty_demanglers[i].demangling_style != gnat_demangling
> +	  && libiberty_demanglers[i].demangling_style != dlang_demangling
> +	  && libiberty_demanglers[i].demangling_style != rust_demangling)
> +	{

If this condition is replicated in the two loops, it should be factored 
out in a function (although check comment below).

> +	  demangling_style_names[out]
> +	    = xstrdup (libiberty_demanglers[out].demangling_style_name);

Shouldn't this be libiberty_demangers[i].demangling_style_name?

For looping, I'd prefer if you iterated with a pointer:

   for (const demangler_engine *dem_iter = libiberty_demanglers;
        dem_iter->demangling_style != unknown_demangling;
        dem_iter++)

> +
> +	  if (current_demangling_style_string == NULL
> +	      && strcmp (DEFAULT_DEMANGLING_STYLE,
> +			 demangling_style_names[out]) == 0)
> +	    current_demangling_style_string = demangling_style_names[out];
> +	  ++out;
> +	}

This seems fragile to me.  set_demangling_command relies on the fact 
that the names are at the same indices in demangling_style_names as the 
corresponding demangler in libiberty_demanglers.  With your patch it 
only works because dlang, rust and gnat are the last three items of the 
libiberty_demanglers array, so all the indices happen to match.  If you 
were to omit lucid, for example, the indices would be off.

I think that could be fixed by making set_demangling_command not use 
demangling_style_names.  Btw, do you have any idea why it does

   current_demangling_style_string = demangling_style_names[i];

?  At that point we have just verified that the demangling style name 
chosen by the user matches the name of a demangler (the strcmp), why do 
we need to set it to a string from the demangling_style_names array?  
Can't we just leave it as is?  I think we could just delete that line.  
Then, demangling_style_names would only be used for passing to 
add_setshow_enum_cmd, so it could be a static local variable in 
_initialize_demangler.  And making it an std::vector would probably 
allow you to get rid of the first loop and simplify the second one.  
Something like that:

   static std::vector<const char *> demangling_style_names;

   for (const demangler_engine *dem_iter = libiberty_demanglers;
        dem_iter->demangling_style != unknown_demangling;
        dem_iter++)
     {
       if (want_demangler (dem_iter->demangling_style))
	{
	  const char *dem_name = xstrdup (dem_iter->demangling_style_name);

	  demangling_style_names.push_back (dem_name);

	  if (current_demangling_style_string == NULL
	      && strcmp (DEFAULT_DEMANGLING_STYLE, dem_name))
	    current_demangling_style_string = dem_name;
	}
     }



Thanks,

Simon