diff mbox

[v2,3/3] mangler/demangler dogfooding

Message ID 538663B8.30205@redhat.com
State New
Headers show

Commit Message

Pedro Alves May 28, 2014, 10:31 p.m. UTC 
On 05/27/2014 12:57 PM, Pedro Alves wrote:

> +    dmgl_opts = (DMGL_VERBOSE
> +		 | DMGL_ANSI
> +		 | DMGL_GNU_V3
> +		 | DMGL_RET_POSTFIX
> +		 | DMGL_PARAMS);

Hmm, don't know why I had put DMGL_RET_POSTFIX there in the
first place.  That's unintended... GDB only uses that for java.
Without that, the set of symbols we can't demangle when running
the testsuite drops from 2600 to 300.  Clearly that shows 
DMGL_RET_POSTFIX is buggy, but that's not something we really
about...

And related, I've now added type demangling coverage too.

> +    mangled_str = (const char *) obstack_base (mangle_obstack);
> +    str = cplus_demangle_v3 (mangled_str, dmgl_opts);

Err, I forgot to free the demangled string.  Fixed now.

> +#if 0

I've now added a #define for this, instead of #if 0.

> +    /* XXX The above catches potential demangler crashes.  And,
> +       ideally, we'd also abort if demangling fails.  However, we
> +       can't do that because the demangler isn't able to demangle all
> +       symbols we generate by default.  Some failures might be
> +       demangler bugs, others unknown mangler bugs, and others known
> +       mangler bugs fixed with a higher -fabi-version, which the
> +       demangler doesn't have a workaround for.  */
> +    if ((str != NULL) != (mangled_str[0] == '_' && mangled_str[1] == 'Z'))
> +      internal_error ("demangling failed for: %s", mangled_str);

I've now made this check complete in accordance to cp-demangle.c.

Here's v2.  Okay to apply, once patch #2 is in?

8<--------
Subject: [PATCH] mangler/demangler dogfooding

This patch teaches g++ to try to demangle any symbol it
generates/mangles, when checking is enabled in the build, as soon as
the symbol is mangled.  The idea here is validate the demangling as
close as possible to the generator as possible.

This allows catching demangler bugs/crashes much earlier in the cycle,
when building libstdc++ and friends, when running g++'s testsuite, and
even potentially earlier than that, when developers work on new
C++11/14-and-beyond features, which influence mangling, validating
against some random test that's not in the testsuite yet (and
sometimes doesn't make it there either), rather than much later in
production when the user is trying to debug the code, or the program
tries to generate a backtrace.  Both libstdc++ and the testsuite
include a good set of tricky symbols to demangle, and the latter
naturally includes all sort of random, weird, code full of corner
cases.  And, I assume g++ maintainers once in a while run WIP g++
through some piles of very complicated C++ code.

It seems clear to me that ideally we should be able to do

 assert (demangle (mangle (symbol));

But, unfortunately, we can't yet.  I built g++ and ran the testsuite
with a variant of this patch that would print the mangled symbol if
the demangling fails, but would otherwise continue without aborting,
and I found out that:

- we can't demangle ~40 symbols generated while building libstdc++
  and friends.  E.g.:

  _ZN9__gnu_cxx13new_allocatorINSt13__future_base13_State_baseV2EE9constructIS2_IEEEvPT_DpOT0_
  _ZNSt15_Sp_counted_ptrIDnLN9__gnu_cxx12_Lock_policyE1EEC1ERKS3_

- we can't demangle around ~300 symbols generated while running the
  g++ testsuite.  E.g.,

  _Z1f1SIXadL3FooEEE
  _ZZ4mainENKUlRT_E_clI1SEEDaS0_

Of course, these all may well be mangler bugs rather than demangler
bugs.  It's possible that these are already known mangler bugs even,
that have been fixed, but require a higher -fabi-version=X.  I didn't
investigate that.

Bootstrapped and regtested on x86_64 Fedora 20.

gcc/
2014-05-28  Pedro Alves  <palves@redhat.com>

	* cp/mangle.c: Include demangle.h.
	(EXTRA_DEMANGLE_CHECKING): Define if not defined.
	(globals) <type>: New field.
	(start_mangling): Clear it.
	(finish_mangling_internal)
	[ENABLE_CHECKING || EXTRA_DEMANGLE_CHECKING]: Demangle the just
	mangled symbol.
	(mangle_decl_string): Set G.type if mangling a type.
	(mangle_type_string): Set G.type.
---
 gcc/cp/mangle.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

Comments

Jason Merrill June 2, 2014, 1:38 p.m. UTC | #1 
On 05/28/2014 06:31 PM, Pedro Alves wrote:
> Some failures might be
> +       demangler bugs, others unknown mangler bugs, and others known
> +       mangler bugs fixed with a higher -fabi-version that the
> +       demangler doesn't have a workaround for.

You can avoid the latter by skipping demangling if G.need_abi_warning is 
set.  If you do that, are there still any failures?

Jason
diff mbox

Patch

diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index 4205fec..b323c9c 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -58,6 +58,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "target.h"
 #include "cgraph.h"
 #include "wide-int.h"
+#include "demangle.h"
 
 /* Debugging support.  */
 
@@ -66,6 +67,14 @@  along with GCC; see the file COPYING3.  If not see
 #define DEBUG_MANGLE 0
 #endif
 
+/* If ENABLE_CHECKING, we try to demangle what we mangle in order to
+   catch nasty demangler crashes early.  But by default, we don't
+   actually make sure the result is sane.  Define this to enable such
+   extra validation.  */
+#ifndef EXTRA_DEMANGLE_CHECKING
+#define EXTRA_DEMANGLE_CHECKING 1
+#endif
+
 /* Macros for tracing the write_* functions.  */
 #if DEBUG_MANGLE
 # define MANGLE_TRACE(FN, INPUT) \
@@ -104,6 +113,9 @@  typedef struct GTY(()) globals {
   /* True if the mangling will be different in a future version of the
      ABI.  */
   bool need_abi_warning;
+
+  /* True if we mangled a type.  */
+  bool type;
 } globals;
 
 static GTY (()) globals G;
@@ -3345,6 +3357,7 @@  start_mangling (const tree entity)
 {
   G.entity = entity;
   G.need_abi_warning = false;
+  G.type = false;
   obstack_free (&name_obstack, name_base);
   mangle_obstack = &name_obstack;
   name_base = obstack_alloc (&name_obstack, 0);
@@ -3367,6 +3380,48 @@  finish_mangling_internal (const bool warn)
 
   /* Null-terminate the string.  */
   write_char ('\0');
+
+#if ENABLE_CHECKING || EXTRA_DEMANGLE_CHECKING
+  /* Make sure we can demangle what we just generated.  */
+  {
+    const char *mangled_str;
+    char *str;
+    int dmgl_opts;
+    int mangled_p ATTRIBUTE_UNUSED;
+
+    dmgl_opts = (G.type
+		 ? DMGL_TYPES
+		 : DMGL_VERBOSE | DMGL_ANSI | DMGL_PARAMS);
+
+    mangled_str = (const char *) obstack_base (mangle_obstack);
+    str = cplus_demangle_v3 (mangled_str, dmgl_opts);
+
+#if EXTRA_DEMANGLE_CHECKING
+    /* XXX The above catches potential demangler crashes.  And,
+       ideally, we'd also abort if demangling fails.  However, we
+       can't do that by default because the demangler isn't able to
+       demangle all symbols we generate.  Some failures might be
+       demangler bugs, others unknown mangler bugs, and others known
+       mangler bugs fixed with a higher -fabi-version that the
+       demangler doesn't have a workaround for.  */
+    mangled_p = (G.type
+		 || (mangled_str[0] == '_' && mangled_str[1] == 'Z')
+		 || (strncmp (mangled_str, "_GLOBAL_", 8) == 0
+		     && (mangled_str[8] == '.'
+			 || mangled_str[8] == '_'
+			 || mangled_str[8] == '$')
+		     && (mangled_str[9] == 'D' || mangled_str[9] == 'I')
+		     && mangled_str[10] == '_'));
+    if ((str != NULL) != mangled_p)
+      internal_error ("mangler/demangler failure for %s: %s",
+		      G.type ? "type" :"fn",
+		      mangled_str);
+
+#endif
+
+    free (str);
+  }
+#endif
 }
 
 
@@ -3438,7 +3493,10 @@  mangle_decl_string (const tree decl)
   start_mangling (decl);
 
   if (TREE_CODE (decl) == TYPE_DECL)
-    write_type (TREE_TYPE (decl));
+    {
+      G.type = true;
+      write_type (TREE_TYPE (decl));
+    }
   else
     write_mangled_name (decl, true);
 
@@ -3535,6 +3593,7 @@  mangle_type_string (const tree type)
   const char *result;
 
   start_mangling (type);
+  G.type = true;
   write_type (type);
   result = finish_mangling (/*warn=*/false);
   if (DEBUG_MANGLE)