This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Use rust_demangle
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 21 Apr 2017 23:52:06 -0400
- Subject: Re: [RFA] Use rust_demangle
- Authentication-results: sourceware.org; auth=none
- References: <20170418123111.11785-1-tom@tromey.com>
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