RFC: catch SIGSEGV in the demangler

RFC: catch SIGSEGV in the demangler

[Tom Tromey]

More than once I have seen gdb crashes caused by bugs in the demangler.
There is one open right now.  These crashes are particularly nasty
because they occur when reading debuginfo, or even when reading minsyms
-- and so prevent the user from debugging their program at all.

This patch arranges for gdb to catch SIGSEGV in the demangler and
recover from it.  It prints a warning and arranges for the demangling to
fail.  This lets debugging continue.

Example:

(gdb) maint demangle _ZSt7forwardIRN1x14refobjiteratorINS0_3refINS0_4mime30multipart_section_processorObjIZ15get_body_parserIZZN14mime_processor21make_section_iteratorERKNS2_INS3_10sectionObjENS0_10ptrrefBaseEEEbENKUlvE_clEvEUlSB_bE_ZZNS6_21make_section_iteratorESB_bENKSC_clEvEUlSB_E0_ENS1_INS2_INS0_20outputrefiteratorObjIiEES8_EEEERKSsSB_OT_OT0_EUlmE_NS3_32make_multipart_default_discarderISP_EEEES8_EEEEEOT_RNSt16remove_referenceISW_E4typeE
warning: Attempting to demangle the symbol '_ZSt7forwardIRN1x14refobjiteratorINS0_3refINS0_4mime30multipart_section_processorObjIZ15get_body_parserIZZN14mime_processor21make_section_iteratorERKNS2_INS3_10sectionObjENS0_10ptrrefBaseEEEbENKUlvE_clEvEUlSB_bE_ZZNS6_21make_section_iteratorESB_bENKSC_clEvEUlSB_E0_ENS1_INS2_INS0_20outputrefiteratorObjIiEES8_EEEERKSsSB_OT_OT0_EUlmE_NS3_32make_multipart_default_discarderISP_EEEES8_EEEEEOT_RNSt16remove_referenceISW_E4typeE' caused a crash in the demangler.  Please report this to the GDB maintainers.


Built and regtested on x86-64 Fedora 16.

I'm curious to know what you think.

Tom

    	* Makefile.in (SFILES): Add safe-demangle.c.
    	(COMMON_OBS): Add safe-demangle.o.
    	* safe-demangle.c: New file.
    	* exceptions.h (enum errors) <SEGV_ERROR>: New constant.
    	* gdb-demangle.h: Include demangle.h.
    	(gdb_cplus_demangle): Declare.
    	* c-lang.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* c-typeprint.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* cp-name-parser.y: Use gdb-demangle.h, gdb_cplus_demangle.
    	* cp-support.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* dwarf2read.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* gdbtypes.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* gnu-v2-abi.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* gnu-v3-abi.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* jv-lang.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* jv-typeprint.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* language.c: Use gdb-demangle.h, gdb_cplus_demangle.
    	* symtab.c: Use gdb-demangle.h, gdb_cplus_demangle.

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index b065d41..4397965 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -733,7 +733,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	proc-service.list progspace.c \
 	prologue-value.c psymtab.c \
 	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c reverse.c \
-	sentinel-frame.c \
+	safe-demangle.c sentinel-frame.c \
 	serial.c ser-base.c ser-unix.c skip.c \
 	solib.c solib-target.c source.c \
 	stabsread.c stack.c probe.c stap-probe.c std-regs.c \
@@ -899,7 +899,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	go-lang.o go-valprint.o go-typeprint.o \
 	jv-lang.o jv-valprint.o jv-typeprint.o \
 	m2-lang.o opencl-lang.o p-lang.o p-typeprint.o p-valprint.o \
-	sentinel-frame.o \
+	safe-demangle.o sentinel-frame.o \
 	complaints.o typeprint.o \
 	ada-typeprint.o c-typeprint.o f-typeprint.o m2-typeprint.o \
 	ada-valprint.o c-valprint.o cp-valprint.o d-valprint.o f-valprint.o \
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index c9b0f51..79768b1 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -29,7 +29,7 @@
 #include "gdb_assert.h"
 #include "charset.h"
 #include "gdb_string.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "cp-abi.h"
 #include "cp-support.h"
 #include "gdb_obstack.h"
@@ -972,7 +972,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 */
-  cplus_demangle,		/* Language specific symbol demangler */
+  gdb_cplus_demangle,		/* Language specific symbol demangler */
   cp_class_name_from_physname,  /* Language specific
 				   class_name_from_physname */
   c_op_print_tab,		/* expression operators for printing */
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index cb75a4e..ac36fa3 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -26,7 +26,7 @@
 #include "gdbcore.h"
 #include "target.h"
 #include "language.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "c-lang.h"
 #include "typeprint.h"
 #include "cp-abi.h"
@@ -1219,8 +1219,8 @@ c_type_print_base (struct type *type, struct ui_file *stream,
 		    mangled_name = TYPE_FN_FIELD_PHYSNAME (f, j);
 
 		  demangled_name =
-		    cplus_demangle (mangled_name,
-				    DMGL_ANSI | DMGL_PARAMS);
+		    gdb_cplus_demangle (mangled_name,
+					DMGL_ANSI | DMGL_PARAMS);
 		  if (demangled_name == NULL)
 		    {
 		      /* In some cases (for instance with the HP
diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y
index 01a1fa4..1305f3d 100644
--- a/gdb/cp-name-parser.y
+++ b/gdb/cp-name-parser.y
@@ -2201,7 +2201,7 @@ main (int argc, char **argv)
 	buf[strlen (buf) - 1] = 0;
 	/* Use DMGL_VERBOSE to get expanded standard substitutions.  */
 	c = trim_chars (buf, &extra_chars);
-	str2 = cplus_demangle (buf, DMGL_PARAMS | DMGL_ANSI | DMGL_VERBOSE);
+	str2 = gdb_cplus_demangle (buf, DMGL_PARAMS | DMGL_ANSI | DMGL_VERBOSE);
 	if (str2 == NULL)
 	  {
 	    printf ("Demangling error\n");
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 22e8fc4..5a47ddc 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -21,7 +21,7 @@
 #include "defs.h"
 #include "cp-support.h"
 #include "gdb_string.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "gdb_assert.h"
 #include "gdbcmd.h"
 #include "dictionary.h"
@@ -635,7 +635,7 @@ mangled_name_to_comp (const char *mangled_name, int options,
 
   /* If it doesn't, or if that failed, then try to demangle the
      name.  */
-  demangled_name = cplus_demangle (mangled_name, options);
+  demangled_name = gdb_cplus_demangle (mangled_name, options);
   if (demangled_name == NULL)
    return NULL;
   
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7af89c6..fe5d2a1 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7643,7 +7643,7 @@ dwarf2_physname (char *name, struct die_info *die, struct dwarf2_cu *cu)
 	}
       else
 	{
-	  demangled = cplus_demangle (mangled,
+	  demangled = gdb_cplus_demangle (mangled,
 				      (DMGL_PARAMS | DMGL_ANSI
 				       | (cu->language == language_java
 					  ? DMGL_JAVA | DMGL_RET_POSTFIX
@@ -13945,7 +13945,7 @@ fixup_partial_die (struct partial_die_info *part_die,
     {
       char *demangled;
 
-      demangled = cplus_demangle (part_die->linkage_name, DMGL_TYPES);
+      demangled = gdb_cplus_demangle (part_die->linkage_name, DMGL_TYPES);
       if (demangled)
 	{
 	  const char *base;
@@ -16995,7 +16995,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	  /* Avoid demangling DW_STRING (attr) the second time on a second
 	     call for the same DIE.  */
 	  if (!DW_STRING_IS_CANONICAL (attr))
-	    demangled = cplus_demangle (DW_STRING (attr), DMGL_TYPES);
+	    demangled = gdb_cplus_demangle (DW_STRING (attr), DMGL_TYPES);
 
 	  if (demangled)
 	    {
diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 0d67719..c4a424f 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -86,6 +86,10 @@ enum errors {
   /* DW_OP_GNU_entry_value resolving failed.  */
   NO_ENTRY_VALUE_ERROR,
 
+  /* SIGSEGV.  These are only generated under very special
+     circumstances.  */
+  SEGV_ERROR,
+
   /* Add more errors here.  */
   NR_ERRORS
 };
diff --git a/gdb/gdb-demangle.h b/gdb/gdb-demangle.h
index 2d62956..ec0677e 100644
--- a/gdb/gdb-demangle.h
+++ b/gdb/gdb-demangle.h
@@ -19,6 +19,8 @@
 #ifndef GDB_DEMANGLE_H
 #define GDB_DEMANGLE_H
 
+#include "demangle.h"
+
 /* Nonzero means that encoded C++/ObjC names should be printed out in their
    C++/ObjC form rather than raw.  */
 extern int demangle;
@@ -34,4 +36,9 @@ extern void set_demangling_style (char *);
 /* Check if a character is one of the commonly used C++ marker characters.  */
 extern int is_cplus_marker (int);
 
+/* gdb's demangler wrapper.  This is like gdb_cplus_demangle, but it has
+   special SEGV handling.  */
+
+extern char *gdb_cplus_demangle (const char *mangled, int options);
+
 #endif /* GDB_DEMANGLE_H */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 85ffbf1..3c5b4dd 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -30,7 +30,7 @@
 #include "language.h"
 #include "target.h"
 #include "value.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "complaints.h"
 #include "gdbcmd.h"
 #include "cp-abi.h"
@@ -1753,8 +1753,8 @@ check_stub_method (struct type *type, int method_id, int signature_id)
   struct gdbarch *gdbarch = get_type_arch (type);
   struct fn_field *f;
   char *mangled_name = gdb_mangle_name (type, method_id, signature_id);
-  char *demangled_name = cplus_demangle (mangled_name,
-					 DMGL_PARAMS | DMGL_ANSI);
+  char *demangled_name = gdb_cplus_demangle (mangled_name,
+					     DMGL_PARAMS | DMGL_ANSI);
   char *argtypetext, *p;
   int depth = 0, argcount = 1;
   struct field *argtypes;
diff --git a/gdb/gnu-v2-abi.c b/gdb/gnu-v2-abi.c
index 4a600d3..2b3f21c 100644
--- a/gdb/gnu-v2-abi.c
+++ b/gdb/gnu-v2-abi.c
@@ -251,7 +251,7 @@ gnuv2_value_rtti_type (struct value *v, int *full, int *top, int *using_enc)
     return NULL;
 
   /* If we just skip the prefix, we get screwed by namespaces.  */
-  demangled_name=cplus_demangle(linkage_name,DMGL_PARAMS|DMGL_ANSI);
+  demangled_name=gdb_cplus_demangle(linkage_name,DMGL_PARAMS|DMGL_ANSI);
   p = strchr (demangled_name, ' ');
   if (p)
     *p = '\0';
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index aef5ae0..948fd93 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -22,7 +22,7 @@
 #include "value.h"
 #include "cp-abi.h"
 #include "cp-support.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "objfiles.h"
 #include "valprint.h"
 #include "c-lang.h"
@@ -593,8 +593,8 @@ gnuv3_print_method_ptr (const gdb_byte *contents,
 	 possible paths to the method based on the adjustment.  */
       if (physname)
 	{
-	  char *demangled_name = cplus_demangle (physname,
-						 DMGL_ANSI | DMGL_PARAMS);
+	  char *demangled_name = gdb_cplus_demangle (physname,
+						     DMGL_ANSI | DMGL_PARAMS);
 
 	  fprintf_filtered (stream, "&virtual ");
 	  if (demangled_name == NULL)
diff --git a/gdb/jv-lang.c b/gdb/jv-lang.c
index f8e8718..f0064da 100644
--- a/gdb/jv-lang.c
+++ b/gdb/jv-lang.c
@@ -33,7 +33,7 @@
 #include "jv-lang.h"
 #include "gdbcore.h"
 #include "block.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "dictionary.h"
 #include <ctype.h>
 #include "gdb_assert.h"
@@ -1014,7 +1014,7 @@ nosideret:
 
 static char *java_demangle (const char *mangled, int options)
 {
-  return cplus_demangle (mangled, options | DMGL_JAVA);
+  return gdb_cplus_demangle (mangled, options | DMGL_JAVA);
 }
 
 /* Find the member function name of the demangled name NAME.  NAME
diff --git a/gdb/jv-typeprint.c b/gdb/jv-typeprint.c
index 69ebf5a..48da871 100644
--- a/gdb/jv-typeprint.c
+++ b/gdb/jv-typeprint.c
@@ -286,7 +286,7 @@ java_type_print_base (struct type *type, struct ui_file *stream, int show,
 		    mangled_name = physname;
 
 		  demangled_name =
-		    cplus_demangle (mangled_name,
+		    gdb_cplus_demangle (mangled_name,
 				    DMGL_ANSI | DMGL_PARAMS | DMGL_JAVA);
 
 		  if (demangled_name == NULL)
diff --git a/gdb/language.c b/gdb/language.c
index c873f2e..cf95d55 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -41,7 +41,7 @@
 #include "target.h"
 #include "parser-defs.h"
 #include "jv-lang.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "symfile.h"
 
 extern void _initialize_language (void);
@@ -764,7 +764,7 @@ static CORE_ADDR unk_lang_trampoline (struct frame_info *frame, CORE_ADDR pc)
 /* Unknown languages just use the cplus demangler.  */
 static char *unk_lang_demangle (const char *mangled, int options)
 {
-  return cplus_demangle (mangled, options);
+  return gdb_cplus_demangle (mangled, options);
 }
 
 static char *unk_lang_class_name (const char *mangled)
diff --git a/gdb/safe-demangle.c b/gdb/safe-demangle.c
new file mode 100644
index 0000000..6003298
--- /dev/null
+++ b/gdb/safe-demangle.c
@@ -0,0 +1,103 @@
+/* Safe interface to the demangler.
+   Copyright (c) 2013 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "gdb-demangle.h"
+#include "exceptions.h"
+#include <signal.h>
+
+/* The stack where SEGV should be delivered.  */
+
+static stack_t segv_stack;
+
+/* If non-zero, we are in the demangler, and SIGSEGV should be turned
+   into an exception.  */
+
+static int in_demangler;
+
+/* The SIGSEGV handler.  */
+
+static void
+handle_segv (int sig)
+{
+  struct gdb_exception except;
+
+  if (!in_demangler)
+    {
+      signal (sig, SIG_DFL);
+      raise (sig);
+    }
+
+  except.reason = RETURN_ERROR;
+  except.error = SEGV_ERROR;
+  except.message = NULL;
+  throw_exception (except);
+}
+
+\f
+
+/* A safe wrapper for cplus_demangle.  It catches SEGV and prints a
+   warning.  */
+
+char *
+gdb_cplus_demangle (const char *mangled, int options)
+{
+  volatile struct gdb_exception except;
+  char *result;
+
+  TRY_CATCH (except, RETURN_MASK_ERROR)
+    {
+      int save = in_demangler;
+
+      in_demangler = 1;
+      result = cplus_demangle (mangled, options);
+      in_demangler = save;
+    }
+
+  if (except.reason < 0 && except.error == SEGV_ERROR)
+    {
+      warning (_("Attempting to demangle the symbol '%s' caused a "
+		 "crash in the demangler.  Please report this to "
+		 "the GDB maintainers."),
+	       mangled);
+      result = NULL;
+    }
+
+  return result;
+}
+
+\f
+
+initialize_file_ftype _initialize_safe_demangle;
+
+void
+_initialize_safe_demangle (void)
+{
+  struct sigaction segv_action;
+
+  segv_stack.ss_size = SIGSTKSZ;
+  segv_stack.ss_sp = xmalloc (segv_stack.ss_size);
+  segv_stack.ss_flags = SS_ONSTACK;
+  sigaltstack (&segv_stack, NULL);
+
+  segv_action.sa_handler = handle_segv;
+  sigemptyset (&segv_action.sa_mask);
+  segv_action.sa_flags = SA_ONSTACK;
+
+  sigaction (SIGSEGV, &segv_action, NULL);
+}
diff --git a/gdb/symtab.c b/gdb/symtab.c
index f4ed8b9..490fb00 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -30,7 +30,7 @@
 #include "gdb_regex.h"
 #include "expression.h"
 #include "language.h"
-#include "demangle.h"
+#include "gdb-demangle.h"
 #include "inferior.h"
 #include "source.h"
 #include "filenames.h"		/* for FILENAME_CMP */
@@ -586,7 +586,7 @@ symbol_find_demangled_name (struct general_symbol_info *gsymbol,
       || gsymbol->language == language_auto)
     {
       demangled =
-        cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
+        gdb_cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
       if (demangled != NULL)
 	{
 	  gsymbol->language = language_cplus;
@@ -596,7 +596,7 @@ symbol_find_demangled_name (struct general_symbol_info *gsymbol,
   if (gsymbol->language == language_java)
     {
       demangled =
-        cplus_demangle (mangled,
+        gdb_cplus_demangle (mangled,
                         DMGL_PARAMS | DMGL_ANSI | DMGL_JAVA);
       if (demangled != NULL)
 	{
@@ -1144,7 +1144,7 @@ demangle_for_lookup (const char *name, enum language lang,
      lookup, so we can always binary search.  */
   if (lang == language_cplus)
     {
-      demangled_name = cplus_demangle (name, DMGL_ANSI | DMGL_PARAMS);
+      demangled_name = gdb_cplus_demangle (name, DMGL_ANSI | DMGL_PARAMS);
       if (demangled_name)
 	{
 	  modified_name = demangled_name;
@@ -1164,7 +1164,7 @@ demangle_for_lookup (const char *name, enum language lang,
     }
   else if (lang == language_java)
     {
-      demangled_name = cplus_demangle (name,
+      demangled_name = gdb_cplus_demangle (name,
 		      		       DMGL_ANSI | DMGL_PARAMS | DMGL_JAVA);
       if (demangled_name)
 	{

RE: catch SIGSEGV in the demangler

[Pierre Muller]

  Hi Tom,

before I forget to reply:

sigaction is not available everywhere and is tested by configure
see 	remote-sim.c:999

 I don't know the exact status of sigaltstack function, but
I would be surprised that it is supported on systems that
don't support sigaction...

Also, sigaction can return the previous signal handler,
so why not use this previous handle instead of SIG_DFL?

Would it make sense to still throw an exception even if not inside  the
demangler?
 
  I was also surprised by the fact that you called the new file
safe-demangle.c, why not simply name it gdb-demangle.c?

  But this are of course only minor details...
which don't mean that I don't like the overall idea of the patch.


Pierre Muller



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : lundi 14 janvier 2013 21:16
> À : gdb-patches@sourceware.org
> Objet : RFC: catch SIGSEGV in the demangler
> 
> More than once I have seen gdb crashes caused by bugs in the demangler.
> There is one open right now.  These crashes are particularly nasty
> because they occur when reading debuginfo, or even when reading minsyms
> -- and so prevent the user from debugging their program at all.
> 
> This patch arranges for gdb to catch SIGSEGV in the demangler and
> recover from it.  It prints a warning and arranges for the demangling to
> fail.  This lets debugging continue.
> 
> Example:
> 
> (gdb) maint demangle
>
_ZSt7forwardIRN1x14refobjiteratorINS0_3refINS0_4mime30multipart_section_proc
>
essorObjIZ15get_body_parserIZZN14mime_processor21make_section_iteratorERKNS2
>
_INS3_10sectionObjENS0_10ptrrefBaseEEEbENKUlvE_clEvEUlSB_bE_ZZNS6_21make_sec
>
tion_iteratorESB_bENKSC_clEvEUlSB_E0_ENS1_INS2_INS0_20outputrefiteratorObjIi
>
EES8_EEEERKSsSB_OT_OT0_EUlmE_NS3_32make_multipart_default_discarderISP_EEEES
> 8_EEEEEOT_RNSt16remove_referenceISW_E4typeE
> warning: Attempting to demangle the symbol
>
'_ZSt7forwardIRN1x14refobjiteratorINS0_3refINS0_4mime30multipart_section_pro
>
cessorObjIZ15get_body_parserIZZN14mime_processor21make_section_iteratorERKNS
>
2_INS3_10sectionObjENS0_10ptrrefBaseEEEbENKUlvE_clEvEUlSB_bE_ZZNS6_21make_se
>
ction_iteratorESB_bENKSC_clEvEUlSB_E0_ENS1_INS2_INS0_20outputrefiteratorObjI
>
iEES8_EEEERKSsSB_OT_OT0_EUlmE_NS3_32make_multipart_default_discarderISP_EEEE
> S8_EEEEEOT_RNSt16remove_referenceISW_E4typeE' caused a crash in the
> demangler.  Please report this to the GDB maintainers.
> 
> 
> Built and regtested on x86-64 Fedora 16.
> 
> I'm curious to know what you think.
> 
> Tom
> 
>     	* Makefile.in (SFILES): Add safe-demangle.c.
>     	(COMMON_OBS): Add safe-demangle.o.
>     	* safe-demangle.c: New file.
>     	* exceptions.h (enum errors) <SEGV_ERROR>: New constant.
>     	* gdb-demangle.h: Include demangle.h.
>     	(gdb_cplus_demangle): Declare.
>     	* c-lang.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* c-typeprint.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* cp-name-parser.y: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* cp-support.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* dwarf2read.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* gdbtypes.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* gnu-v2-abi.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* gnu-v3-abi.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* jv-lang.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* jv-typeprint.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* language.c: Use gdb-demangle.h, gdb_cplus_demangle.
>     	* symtab.c: Use gdb-demangle.h, gdb_cplus_demangle.
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index b065d41..4397965 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -733,7 +733,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-
> valprint.c ada-tasks.c \
>  	proc-service.list progspace.c \
>  	prologue-value.c psymtab.c \
>  	regcache.c reggroups.c remote.c remote-fileio.c remote-notif.c
> reverse.c \
> -	sentinel-frame.c \
> +	safe-demangle.c sentinel-frame.c \
>  	serial.c ser-base.c ser-unix.c skip.c \
>  	solib.c solib-target.c source.c \
>  	stabsread.c stack.c probe.c stap-probe.c std-regs.c \
> @@ -899,7 +899,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>  	go-lang.o go-valprint.o go-typeprint.o \
>  	jv-lang.o jv-valprint.o jv-typeprint.o \
>  	m2-lang.o opencl-lang.o p-lang.o p-typeprint.o p-valprint.o \
> -	sentinel-frame.o \
> +	safe-demangle.o sentinel-frame.o \
>  	complaints.o typeprint.o \
>  	ada-typeprint.o c-typeprint.o f-typeprint.o m2-typeprint.o \
>  	ada-valprint.o c-valprint.o cp-valprint.o d-valprint.o f-valprint.o
\
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index c9b0f51..79768b1 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -29,7 +29,7 @@
>  #include "gdb_assert.h"
>  #include "charset.h"
>  #include "gdb_string.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "cp-abi.h"
>  #include "cp-support.h"
>  #include "gdb_obstack.h"
> @@ -972,7 +972,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 */
> -  cplus_demangle,		/* Language specific symbol demangler */
> +  gdb_cplus_demangle,		/* Language specific symbol
demangler */
>    cp_class_name_from_physname,  /* Language specific
>  				   class_name_from_physname */
>    c_op_print_tab,		/* expression operators for printing */
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index cb75a4e..ac36fa3 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -26,7 +26,7 @@
>  #include "gdbcore.h"
>  #include "target.h"
>  #include "language.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "c-lang.h"
>  #include "typeprint.h"
>  #include "cp-abi.h"
> @@ -1219,8 +1219,8 @@ c_type_print_base (struct type *type, struct ui_file
> *stream,
>  		    mangled_name = TYPE_FN_FIELD_PHYSNAME (f, j);
> 
>  		  demangled_name =
> -		    cplus_demangle (mangled_name,
> -				    DMGL_ANSI | DMGL_PARAMS);
> +		    gdb_cplus_demangle (mangled_name,
> +					DMGL_ANSI | DMGL_PARAMS);
>  		  if (demangled_name == NULL)
>  		    {
>  		      /* In some cases (for instance with the HP
> diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y
> index 01a1fa4..1305f3d 100644
> --- a/gdb/cp-name-parser.y
> +++ b/gdb/cp-name-parser.y
> @@ -2201,7 +2201,7 @@ main (int argc, char **argv)
>  	buf[strlen (buf) - 1] = 0;
>  	/* Use DMGL_VERBOSE to get expanded standard substitutions.  */
>  	c = trim_chars (buf, &extra_chars);
> -	str2 = cplus_demangle (buf, DMGL_PARAMS | DMGL_ANSI | DMGL_VERBOSE);
> +	str2 = gdb_cplus_demangle (buf, DMGL_PARAMS | DMGL_ANSI |
> DMGL_VERBOSE);
>  	if (str2 == NULL)
>  	  {
>  	    printf ("Demangling error\n");
> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 22e8fc4..5a47ddc 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -21,7 +21,7 @@
>  #include "defs.h"
>  #include "cp-support.h"
>  #include "gdb_string.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "gdb_assert.h"
>  #include "gdbcmd.h"
>  #include "dictionary.h"
> @@ -635,7 +635,7 @@ mangled_name_to_comp (const char *mangled_name, int
> options,
> 
>    /* If it doesn't, or if that failed, then try to demangle the
>       name.  */
> -  demangled_name = cplus_demangle (mangled_name, options);
> +  demangled_name = gdb_cplus_demangle (mangled_name, options);
>    if (demangled_name == NULL)
>     return NULL;
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 7af89c6..fe5d2a1 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -7643,7 +7643,7 @@ dwarf2_physname (char *name, struct die_info *die,
> struct dwarf2_cu *cu)
>  	}
>        else
>  	{
> -	  demangled = cplus_demangle (mangled,
> +	  demangled = gdb_cplus_demangle (mangled,
>  				      (DMGL_PARAMS | DMGL_ANSI
>  				       | (cu->language == language_java
>  					  ? DMGL_JAVA | DMGL_RET_POSTFIX
> @@ -13945,7 +13945,7 @@ fixup_partial_die (struct partial_die_info
> *part_die,
>      {
>        char *demangled;
> 
> -      demangled = cplus_demangle (part_die->linkage_name, DMGL_TYPES);
> +      demangled = gdb_cplus_demangle (part_die->linkage_name,
DMGL_TYPES);
>        if (demangled)
>  	{
>  	  const char *base;
> @@ -16995,7 +16995,7 @@ dwarf2_name (struct die_info *die, struct
dwarf2_cu
> *cu)
>  	  /* Avoid demangling DW_STRING (attr) the second time on a second
>  	     call for the same DIE.  */
>  	  if (!DW_STRING_IS_CANONICAL (attr))
> -	    demangled = cplus_demangle (DW_STRING (attr), DMGL_TYPES);
> +	    demangled = gdb_cplus_demangle (DW_STRING (attr), DMGL_TYPES);
> 
>  	  if (demangled)
>  	    {
> diff --git a/gdb/exceptions.h b/gdb/exceptions.h
> index 0d67719..c4a424f 100644
> --- a/gdb/exceptions.h
> +++ b/gdb/exceptions.h
> @@ -86,6 +86,10 @@ enum errors {
>    /* DW_OP_GNU_entry_value resolving failed.  */
>    NO_ENTRY_VALUE_ERROR,
> 
> +  /* SIGSEGV.  These are only generated under very special
> +     circumstances.  */
> +  SEGV_ERROR,
> +
>    /* Add more errors here.  */
>    NR_ERRORS
>  };
> diff --git a/gdb/gdb-demangle.h b/gdb/gdb-demangle.h
> index 2d62956..ec0677e 100644
> --- a/gdb/gdb-demangle.h
> +++ b/gdb/gdb-demangle.h
> @@ -19,6 +19,8 @@
>  #ifndef GDB_DEMANGLE_H
>  #define GDB_DEMANGLE_H
> 
> +#include "demangle.h"
> +
>  /* Nonzero means that encoded C++/ObjC names should be printed out in
their
>     C++/ObjC form rather than raw.  */
>  extern int demangle;
> @@ -34,4 +36,9 @@ extern void set_demangling_style (char *);
>  /* Check if a character is one of the commonly used C++ marker
characters.
> */
>  extern int is_cplus_marker (int);
> 
> +/* gdb's demangler wrapper.  This is like gdb_cplus_demangle, but it has
> +   special SEGV handling.  */
> +
> +extern char *gdb_cplus_demangle (const char *mangled, int options);
> +
>  #endif /* GDB_DEMANGLE_H */
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 85ffbf1..3c5b4dd 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -30,7 +30,7 @@
>  #include "language.h"
>  #include "target.h"
>  #include "value.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "complaints.h"
>  #include "gdbcmd.h"
>  #include "cp-abi.h"
> @@ -1753,8 +1753,8 @@ check_stub_method (struct type *type, int method_id,
> int signature_id)
>    struct gdbarch *gdbarch = get_type_arch (type);
>    struct fn_field *f;
>    char *mangled_name = gdb_mangle_name (type, method_id, signature_id);
> -  char *demangled_name = cplus_demangle (mangled_name,
> -					 DMGL_PARAMS | DMGL_ANSI);
> +  char *demangled_name = gdb_cplus_demangle (mangled_name,
> +					     DMGL_PARAMS | DMGL_ANSI);
>    char *argtypetext, *p;
>    int depth = 0, argcount = 1;
>    struct field *argtypes;
> diff --git a/gdb/gnu-v2-abi.c b/gdb/gnu-v2-abi.c
> index 4a600d3..2b3f21c 100644
> --- a/gdb/gnu-v2-abi.c
> +++ b/gdb/gnu-v2-abi.c
> @@ -251,7 +251,7 @@ gnuv2_value_rtti_type (struct value *v, int *full, int
> *top, int *using_enc)
>      return NULL;
> 
>    /* If we just skip the prefix, we get screwed by namespaces.  */
> -  demangled_name=cplus_demangle(linkage_name,DMGL_PARAMS|DMGL_ANSI);
> +  demangled_name=gdb_cplus_demangle(linkage_name,DMGL_PARAMS|DMGL_ANSI);
>    p = strchr (demangled_name, ' ');
>    if (p)
>      *p = '\0';
> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
> index aef5ae0..948fd93 100644
> --- a/gdb/gnu-v3-abi.c
> +++ b/gdb/gnu-v3-abi.c
> @@ -22,7 +22,7 @@
>  #include "value.h"
>  #include "cp-abi.h"
>  #include "cp-support.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "objfiles.h"
>  #include "valprint.h"
>  #include "c-lang.h"
> @@ -593,8 +593,8 @@ gnuv3_print_method_ptr (const gdb_byte *contents,
>  	 possible paths to the method based on the adjustment.  */
>        if (physname)
>  	{
> -	  char *demangled_name = cplus_demangle (physname,
> -						 DMGL_ANSI | DMGL_PARAMS);
> +	  char *demangled_name = gdb_cplus_demangle (physname,
> +						     DMGL_ANSI |
DMGL_PARAMS);
> 
>  	  fprintf_filtered (stream, "&virtual ");
>  	  if (demangled_name == NULL)
> diff --git a/gdb/jv-lang.c b/gdb/jv-lang.c
> index f8e8718..f0064da 100644
> --- a/gdb/jv-lang.c
> +++ b/gdb/jv-lang.c
> @@ -33,7 +33,7 @@
>  #include "jv-lang.h"
>  #include "gdbcore.h"
>  #include "block.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "dictionary.h"
>  #include <ctype.h>
>  #include "gdb_assert.h"
> @@ -1014,7 +1014,7 @@ nosideret:
> 
>  static char *java_demangle (const char *mangled, int options)
>  {
> -  return cplus_demangle (mangled, options | DMGL_JAVA);
> +  return gdb_cplus_demangle (mangled, options | DMGL_JAVA);
>  }
> 
>  /* Find the member function name of the demangled name NAME.  NAME
> diff --git a/gdb/jv-typeprint.c b/gdb/jv-typeprint.c
> index 69ebf5a..48da871 100644
> --- a/gdb/jv-typeprint.c
> +++ b/gdb/jv-typeprint.c
> @@ -286,7 +286,7 @@ java_type_print_base (struct type *type, struct
ui_file
> *stream, int show,
>  		    mangled_name = physname;
> 
>  		  demangled_name =
> -		    cplus_demangle (mangled_name,
> +		    gdb_cplus_demangle (mangled_name,
>  				    DMGL_ANSI | DMGL_PARAMS | DMGL_JAVA);
> 
>  		  if (demangled_name == NULL)
> diff --git a/gdb/language.c b/gdb/language.c
> index c873f2e..cf95d55 100644
> --- a/gdb/language.c
> +++ b/gdb/language.c
> @@ -41,7 +41,7 @@
>  #include "target.h"
>  #include "parser-defs.h"
>  #include "jv-lang.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "symfile.h"
> 
>  extern void _initialize_language (void);
> @@ -764,7 +764,7 @@ static CORE_ADDR unk_lang_trampoline (struct
frame_info
> *frame, CORE_ADDR pc)
>  /* Unknown languages just use the cplus demangler.  */
>  static char *unk_lang_demangle (const char *mangled, int options)
>  {
> -  return cplus_demangle (mangled, options);
> +  return gdb_cplus_demangle (mangled, options);
>  }
> 
>  static char *unk_lang_class_name (const char *mangled)
> diff --git a/gdb/safe-demangle.c b/gdb/safe-demangle.c
> new file mode 100644
> index 0000000..6003298
> --- /dev/null
> +++ b/gdb/safe-demangle.c
> @@ -0,0 +1,103 @@
> +/* Safe interface to the demangler.
> +   Copyright (c) 2013 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> */
> +
> +#include "defs.h"
> +#include "gdb-demangle.h"
> +#include "exceptions.h"
> +#include <signal.h>
> +
> +/* The stack where SEGV should be delivered.  */
> +
> +static stack_t segv_stack;
> +
> +/* If non-zero, we are in the demangler, and SIGSEGV should be turned
> +   into an exception.  */
> +
> +static int in_demangler;
> +
> +/* The SIGSEGV handler.  */
> +
> +static void
> +handle_segv (int sig)
> +{
> +  struct gdb_exception except;
> +
> +  if (!in_demangler)
> +    {
> +      signal (sig, SIG_DFL);
> +      raise (sig);
> +    }
> +
> +  except.reason = RETURN_ERROR;
> +  except.error = SEGV_ERROR;
> +  except.message = NULL;
> +  throw_exception (except);
> +}
> +
> +

> +
> +/* A safe wrapper for cplus_demangle.  It catches SEGV and prints a
> +   warning.  */
> +
> +char *
> +gdb_cplus_demangle (const char *mangled, int options)
> +{
> +  volatile struct gdb_exception except;
> +  char *result;
> +
> +  TRY_CATCH (except, RETURN_MASK_ERROR)
> +    {
> +      int save = in_demangler;
> +
> +      in_demangler = 1;
> +      result = cplus_demangle (mangled, options);
> +      in_demangler = save;
> +    }
> +
> +  if (except.reason < 0 && except.error == SEGV_ERROR)
> +    {
> +      warning (_("Attempting to demangle the symbol '%s' caused a "
> +		 "crash in the demangler.  Please report this to "
> +		 "the GDB maintainers."),
> +	       mangled);
> +      result = NULL;
> +    }
> +
> +  return result;
> +}
> +
> +

> +
> +initialize_file_ftype _initialize_safe_demangle;
> +
> +void
> +_initialize_safe_demangle (void)
> +{
> +  struct sigaction segv_action;
> +
> +  segv_stack.ss_size = SIGSTKSZ;
> +  segv_stack.ss_sp = xmalloc (segv_stack.ss_size);
> +  segv_stack.ss_flags = SS_ONSTACK;
> +  sigaltstack (&segv_stack, NULL);
> +
> +  segv_action.sa_handler = handle_segv;
> +  sigemptyset (&segv_action.sa_mask);
> +  segv_action.sa_flags = SA_ONSTACK;
> +
> +  sigaction (SIGSEGV, &segv_action, NULL);
> +}
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index f4ed8b9..490fb00 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -30,7 +30,7 @@
>  #include "gdb_regex.h"
>  #include "expression.h"
>  #include "language.h"
> -#include "demangle.h"
> +#include "gdb-demangle.h"
>  #include "inferior.h"
>  #include "source.h"
>  #include "filenames.h"		/* for FILENAME_CMP */
> @@ -586,7 +586,7 @@ symbol_find_demangled_name (struct general_symbol_info
> *gsymbol,
>        || gsymbol->language == language_auto)
>      {
>        demangled =
> -        cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
> +        gdb_cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
>        if (demangled != NULL)
>  	{
>  	  gsymbol->language = language_cplus;
> @@ -596,7 +596,7 @@ symbol_find_demangled_name (struct general_symbol_info
> *gsymbol,
>    if (gsymbol->language == language_java)
>      {
>        demangled =
> -        cplus_demangle (mangled,
> +        gdb_cplus_demangle (mangled,
>                          DMGL_PARAMS | DMGL_ANSI | DMGL_JAVA);
>        if (demangled != NULL)
>  	{
> @@ -1144,7 +1144,7 @@ demangle_for_lookup (const char *name, enum language
> lang,
>       lookup, so we can always binary search.  */
>    if (lang == language_cplus)
>      {
> -      demangled_name = cplus_demangle (name, DMGL_ANSI | DMGL_PARAMS);
> +      demangled_name = gdb_cplus_demangle (name, DMGL_ANSI |
DMGL_PARAMS);
>        if (demangled_name)
>  	{
>  	  modified_name = demangled_name;
> @@ -1164,7 +1164,7 @@ demangle_for_lookup (const char *name, enum language
> lang,
>      }
>    else if (lang == language_java)
>      {
> -      demangled_name = cplus_demangle (name,
> +      demangled_name = gdb_cplus_demangle (name,
>  		      		       DMGL_ANSI | DMGL_PARAMS | DMGL_JAVA);
>        if (demangled_name)
>  	{

Re: catch SIGSEGV in the demangler

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre> sigaction is not available everywhere and is tested by configure
Pierre> see 	remote-sim.c:999

Pierre>  I don't know the exact status of sigaltstack function, but
Pierre> I would be surprised that it is supported on systems that
Pierre> don't support sigaction...

Yeah, thanks.

Pierre> Also, sigaction can return the previous signal handler,
Pierre> so why not use this previous handle instead of SIG_DFL?

I don't mind changing this, but it seems like if this gives a result
other than SIG_DFL, then we've done something weird.  There should be
only one handler installed.

Pierre> Would it make sense to still throw an exception even if not inside  the
Pierre> demangler?

I was thinking of perhaps expanding the scope somewhat.

One idea I had was to introduce a new RETURN_SEGV to return_reason and
*not* add this to RETURN_MASK_ALL.

Then, have a special throw_segv that first looks to see if anything
expects to catch it, and if not, reset the handler and re-raise the
signal.

This way we could let code handle SEGV when appropriate, without tying
it to the demangler -- but also without catching all SEGVs that occur in
gdb.

I think we probably want to let some SEGVs through for the benefit of
external crash-catchers like ABRT.  But, maybe not -- I've also been
wondering if a SEGV should be treated like internal_error instead.
Maybe sometimes gdb could limp on after a SEGV.

Pierre>   I was also surprised by the fact that you called the new file
Pierre> safe-demangle.c, why not simply name it gdb-demangle.c?

I didn't want to confuse it with the existing demangle.c.

Tom

Re: catch SIGSEGV in the demangler

[Pedro Alves]

On 01/17/2013 07:29 PM, Tom Tromey wrote:
> 
> Then, have a special throw_segv that first looks to see if anything
> expects to catch it, and if not, reset the handler and re-raise the
> signal.

+static void
+handle_segv (int sig)
+{
+  struct gdb_exception except;
+
+  if (!in_demangler)
+    {
+      signal (sig, SIG_DFL);
+      raise (sig);
+    }

The original idea was to do return instead of raise:

+static void
+handle_segv (int sig)
+{
+  struct gdb_exception except;
+
+  if (!in_demangler)
+    {
+      signal (sig, SIG_DFL);
+      return;
+    }

SIGSEGV being a synchronous signal, this makes it so that the original
instruction that triggered the segv is reexecuted, and the SIGSEGV is raised
again.  The difference is that this way our handler is transparent -- the
segv's siginfo will be more rich, including a si_addr that points at the
address that caused the fault, (si_code will still show it was a userspace
generated signal), and "handle_segv" will not appear in the backtrace.
Did you try that and decided against?

-- 
Pedro Alves

Re: catch SIGSEGV in the demangler

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> SIGSEGV being a synchronous signal, this makes it so that the
Pedro> original instruction that triggered the segv is reexecuted, and
Pedro> the SIGSEGV is raised again.  The difference is that this way our
Pedro> handler is transparent -- the segv's siginfo will be more rich,
Pedro> including a si_addr that points at the address that caused the
Pedro> fault, (si_code will still show it was a userspace generated
Pedro> signal), and "handle_segv" will not appear in the backtrace.  Did
Pedro> you try that and decided against?

I didn't try, because the C standard says this is undefined behavior:

   If and when the function returns, if the value of sig is SIGFPE,
   SIGILL, SIGSEGV, or any other implementation-defined value
   corresponding to a computational exception, the behavior is
   undefined; otherwise the program will resume execution at the point
   it was interrupted.

I couldn't find anything in POSIX suggesting otherwise.

It seems to me that the failing spot will still be in the backtrace.
So, the damage isn't so severe:

(gdb) bt
#0  0x0000003be3036540 in __sigprocmask (how=2, set=0x2f32850, oset=0x0)
    at ../sysdeps/unix/sysv/linux/ia64/sigprocmask.c:43
#1  0x0000003be303610b in __libc_siglongjmp (env=0x2f32808, val=-1)
    at longjmp.c:36
#2  0x0000003be3c0e179 in longjmp (env=<optimized out>, val=<optimized out>)
    at ../nptl/sysdeps/pthread/pt-longjmp.c:27
#3  0x0000000000722e48 in throw_exception (exception=...)
    at ../../archer/gdb/exceptions.c:234
#4  0x0000000000802a33 in handle_segv (sig=11)
    at ../../archer/gdb/safe-demangle.c:49
#5  <signal handler called>
#6  0x0000003be30e87c8 in __GI___poll (fds=0x2e2cff0, nfds=3, 
    timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:83
#7  0x000000000072d1be in gdb_wait_for_event (block=1)
    at ../../archer/gdb/event-loop.c:863


Well, ok, the stack trace is weird, since in this scenario we aren't
actually calling longjmp.  I'm not sure what is going on there.

If returning actually works everywhere, I am fine with doing that.

Tom

Re: catch SIGSEGV in the demangler

[Pedro Alves]

On 01/18/2013 03:01 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> SIGSEGV being a synchronous signal, this makes it so that the
> Pedro> original instruction that triggered the segv is reexecuted, and
> Pedro> the SIGSEGV is raised again.  The difference is that this way our
> Pedro> handler is transparent -- the segv's siginfo will be more rich,
> Pedro> including a si_addr that points at the address that caused the
> Pedro> fault, (si_code will still show it was a userspace generated
> Pedro> signal), and "handle_segv" will not appear in the backtrace.  Did
> Pedro> you try that and decided against?
> 
> I didn't try, because the C standard says this is undefined behavior:
> 
>    If and when the function returns, if the value of sig is SIGFPE,
>    SIGILL, SIGSEGV, or any other implementation-defined value
>    corresponding to a computational exception, the behavior is
>    undefined; otherwise the program will resume execution at the point
>    it was interrupted.
> 
> I couldn't find anything in POSIX suggesting otherwise.

Interesting, I'd expect that to be implementation defined.

Catching SIGILL, emulating an unsupported instruction, advancing
the program counter in the passed in context (3rd arg), and returning,
is a not unheard of technique (it's better when the kernel does it,
but still).

> 
> It seems to me that the failing spot will still be in the backtrace.
> So, the damage isn't so severe:

You have the failing spot, but lose si_addr, which you can use
to know which address wasn't accessed.  True, you can infer that
from the instruction that faulted, but it's not as nice.

> 
> (gdb) bt
> #0  0x0000003be3036540 in __sigprocmask (how=2, set=0x2f32850, oset=0x0)
>     at ../sysdeps/unix/sysv/linux/ia64/sigprocmask.c:43
> #1  0x0000003be303610b in __libc_siglongjmp (env=0x2f32808, val=-1)
>     at longjmp.c:36
> #2  0x0000003be3c0e179 in longjmp (env=<optimized out>, val=<optimized out>)
>     at ../nptl/sysdeps/pthread/pt-longjmp.c:27
> #3  0x0000000000722e48 in throw_exception (exception=...)
>     at ../../archer/gdb/exceptions.c:234
> #4  0x0000000000802a33 in handle_segv (sig=11)
>     at ../../archer/gdb/safe-demangle.c:49
> #5  <signal handler called>
> #6  0x0000003be30e87c8 in __GI___poll (fds=0x2e2cff0, nfds=3, 
>     timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:83
> #7  0x000000000072d1be in gdb_wait_for_event (block=1)
>     at ../../archer/gdb/event-loop.c:863
> 
> 
> Well, ok, the stack trace is weird, since in this scenario we aren't
> actually calling longjmp.  I'm not sure what is going on there.

Did you mean to catch the SIGSEGV caused by the raise?
That seems like what you'd get if !in_demangler (you got to
throw_exception)?  You got SIGSEGV set to nopass by mistake
perhaps, caught the raise(SIGSEGV), and continued, which
suppressed the signal, and moved along into the throw?

> 
> If returning actually works everywhere, I am fine with doing that.

I think it does, but we can always do a raise where it doesn't work.

-- 
Pedro Alves

Re: catch SIGSEGV in the demangler

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Tom> Well, ok, the stack trace is weird, since in this scenario we aren't
Tom> actually calling longjmp.  I'm not sure what is going on there.

Pedro> Did you mean to catch the SIGSEGV caused by the raise?
Pedro> That seems like what you'd get if !in_demangler (you got to
Pedro> throw_exception)?  You got SIGSEGV set to nopass by mistake
Pedro> perhaps, caught the raise(SIGSEGV), and continued, which
Pedro> suppressed the signal, and moved along into the throw?

I started gdb and kill -SEGV'd it from another terminal.
Then I made the stack trace from the core file.

Ah.  SEGV is blocked in the handler.  So the raise queues it and then it
falls through to throw_exception.  Oops.  The fix is to just return.

Tom> If returning actually works everywhere, I am fine with doing that.

Pedro> I think it does, but we can always do a raise where it doesn't work.

Ok.  I'm trying it out.

I'm primarily interested in opinions on whether this is even a good
idea.  I'm on the fence about it myself.

It's far from clear that it is safe to call throw_exception from a
signal handler.

Tom

Re: catch SIGSEGV in the demangler

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> The original idea was to do return instead of raise:
Pedro> +static void
Pedro> +handle_segv (int sig)
Pedro> +{
Pedro> +  struct gdb_exception except;
Pedro> +
Pedro> +  if (!in_demangler)
Pedro> +    {
Pedro> +      signal (sig, SIG_DFL);
Pedro> +      return;
Pedro> +    }

This works fine, but has the effect of causing gdb to ignore an explicit
'kill -SEGV'.  I had to hack "maint dump-me" to really see it work.

Tom

Re: catch SIGSEGV in the demangler

[Pedro Alves]

On 01/18/2013 04:31 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> The original idea was to do return instead of raise:
> Pedro> +static void
> Pedro> +handle_segv (int sig)
> Pedro> +{
> Pedro> +  struct gdb_exception except;
> Pedro> +
> Pedro> +  if (!in_demangler)
> Pedro> +    {
> Pedro> +      signal (sig, SIG_DFL);
> Pedro> +      return;
> Pedro> +    }
> 
> This works fine, but has the effect of causing gdb to ignore an explicit
> 'kill -SEGV'.  I had to hack "maint dump-me" to really see it work.

Yeah, you're not supposed to send synchronous signals
asynchronously. :-)

We can detect that using sigaction/SA_SIGINFO, and
then look at si_code, e.g.:

void
handler (int signo, siginfo_t *info)
{
  if (expecting_segv)
    {
      signal (SIGSEGV, SIG_DFL);

      if (info->si_code == SI_USER)
	raise (signo);
      else
	return;
    }
}

Doesn't cover all si_codes, but should work
fine in practice.

-- 
Pedro Alves

Re: catch SIGSEGV in the demangler

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I was thinking of perhaps expanding the scope somewhat.

Tom> One idea I had was to introduce a new RETURN_SEGV to return_reason and
Tom> *not* add this to RETURN_MASK_ALL.

Tom> Then, have a special throw_segv that first looks to see if anything
Tom> expects to catch it, and if not, reset the handler and re-raise the
Tom> signal.

Tom> This way we could let code handle SEGV when appropriate, without tying
Tom> it to the demangler -- but also without catching all SEGVs that occur in
Tom> gdb.

I implemented this and I think it reads a bit better.
Now any code that knows what to do with a SEGV can

TRY_CATCH (except, RETURN_MASK_SEGV) { ... }

... but if nothing on the call stack does this, SEGVs are just allowed
to cause gdb to crash.

Tom

Re: catch SIGSEGV in the demangler

[Pedro Alves]

On 01/18/2013 04:08 PM, Tom Tromey wrote:
> 
> I'm primarily interested in opinions on whether this is even a good
> idea.  I'm on the fence about it myself.

Me too.  I'd rather all bugs in the demangler were fixed.  :-)

> It's far from clear that it is safe to call throw_exception from a
> signal handler.

Yeah.  It's risky.  It uses the heap (malloc/free), so
if the wrapped code crashes within malloc or free (e.g.,
corrupted heap), then the handler may e.g., deadlock
(the handler crashing wouldn't be that bad, since we're
already crashing).  It also calls clear_sigint_flag,
which with python enabled is probably not very reentrant or
async signal safe either, as it calls into python, which I can
imagine to be a problem is you wrap all SEGVs throughtout
GDB's execution, instead of just over the demangler, which
won't normally call into python.

So dunno either.

-- 
Pedro Alves

Re: catch SIGSEGV in the demangler

[Tom Tromey]

Tom> I'm primarily interested in opinions on whether this is even a good
Tom> idea.  I'm on the fence about it myself.

Pedro> Me too.  I'd rather all bugs in the demangler were fixed.  :-)

Yeah.  But there will be future crashes too.

Tom> It's far from clear that it is safe to call throw_exception from a
Tom> signal handler.

Pedro> Yeah.  It's risky.  It uses the heap (malloc/free), so
Pedro> if the wrapped code crashes within malloc or free (e.g.,
Pedro> corrupted heap), then the handler may e.g., deadlock
Pedro> (the handler crashing wouldn't be that bad, since we're
Pedro> already crashing).  It also calls clear_sigint_flag,
Pedro> which with python enabled is probably not very reentrant or
Pedro> async signal safe either, as it calls into python, which I can
Pedro> imagine to be a problem is you wrap all SEGVs throughtout
Pedro> GDB's execution, instead of just over the demangler, which
Pedro> won't normally call into python.

I think Python isn't an issue.  PyOS_InterruptOccurred is reasonably
unobjectionable.  It is a low-level thing, part of Python's own
signal-handling code.

We probably shouldn't call clear_quit_flag in there at all.
It wouldn't be hard to avoid this in the SEGV case at least.
That way a SEGV wouldn't obscure a QUIT.

Tom