[PATCH 0/2 v3] Demangler crash handler

[PATCH 0/2 v3] Demangler crash handler

[Gary Benson]

Hi all,

This patch is an updated version of the demangler crash handler I
posted two weeks ago.  There are two main changes from the previous
version:

 1) The signal handler creates a core file before returning.
    This ensures that a core file is created at the correct
    location.

 2) I have switched the crash handler to be enabled by default.
    I think this is appropriate now that a core file is always
    created.

To make this work correctly I have had to add a new class of
internal problem, which I've called demangler_warning.  This
has the same behaviour as internal_warning except that it does
not prompt the user to create a core file because that's already
been done.  I know Doug didn't want this but it's necessary to
avoid either overwriting the useful core file with a non-useful
one or confusing the user with a second, non-useful core file.

I've split this patch into two parts for ease of discussion.  The
first patch adds the new internal problem functionality, and the
second patch is the crash handler itself.  This differs from the
previous version by the addition of core file generation and in
that it calls demangler_warning instead of internal_warning.

I would push both patches as one commit.  The news file entries
for the commit would be:

  * New options
  
  maint set catch-demangler-crashes (on|off)
  maint show catch-demangler-crashes
    Control whether GDB should attempt to catch crashes in the symbol
    name demangler.  The default is to attempt to catch crashes.  If
    enabled, the first time a crash is caught, a core file is created,
    the offending symbol is displayed and the user is presented with
    the option to terminate the current session.
  
  maint set demangler-warning quit (yes|no|ask)
    When GDB catches a crash in the symbol name demangler it can offer
    the user the opportunity to both quit GDB and create a core file of
    the current GDB session.  These options control whether or not to
    do either of these.  The default is to create a core file and to ask
    the user whether to quit.
  
  * New commands
  
  maint demangler-warning
    Cause GDB to call the internal function demangler_warning and
    hence behave as though an internal error in the demangler has
    been detected.

Is this ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 1/2 v3] Add new internal problem for demangler warnings

[Gary Benson]

This patch adds a new category of internal problem for demangler
warnings.  Demangler warnings behave in much the same way as internal
warnings except that they do not create core files and no option to
change this is presented to the user.


gdb/
2014-06-04  Gary Benson  <gbenson@redhat.com>

	* utils.h (demangler_vwarning): New declaration.
	(demangler_warning): Likewise.
	* utils.c (struct internal_problem)
	<user_settable_should_quit>: New field.
	<user_settable_should_dump_core>: Likewise
	(internal_error_problem): Add values for above new fields.
	(internal_warning_problem): Likewise.
	(demangler_warning_problem): New static global.
	(demangler_vwarning): New function.
	(demangler_warning): Likewise.
	(add_internal_problem_command): Selectively add commands.
	(_initialize_utils): New internal problem command.
	* maint.c (maintenance_demangler_warning): New function.
	(_initialize_maint_cmds): New command.

gdb/doc/
2014-06-04  Gary Benson  <gbenson@redhat.com>

	* gdb.texinfo (Maintenance Commands): Document new
	"maint demangler-warning" command and new
	"maint set/show demangler-warning" option.

diff --git a/gdb/utils.h b/gdb/utils.h
index 33371ac..31f9c19 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -313,6 +313,14 @@ extern void internal_warning (const char *file, int line,
 extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
 
 extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
+
+extern void demangler_vwarning (const char *file, int line,
+			       const char *, va_list ap)
+     ATTRIBUTE_PRINTF (3, 0);
+
+extern void demangler_warning (const char *file, int line,
+			      const char *, ...) ATTRIBUTE_PRINTF (3, 4);
+
 \f
 /* Misc. utilities.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 86df1c7..686b153 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -659,7 +659,9 @@ static const char *const internal_problem_modes[] =
 struct internal_problem
 {
   const char *name;
+  int user_settable_should_quit;
   const char *should_quit;
+  int user_settable_should_dump_core;
   const char *should_dump_core;
 };
 
@@ -794,7 +796,7 @@ internal_vproblem (struct internal_problem *problem,
 }
 
 static struct internal_problem internal_error_problem = {
-  "internal-error", internal_problem_ask, internal_problem_ask
+  "internal-error", 1, internal_problem_ask, 1, internal_problem_ask
 };
 
 void
@@ -815,7 +817,7 @@ internal_error (const char *file, int line, const char *string, ...)
 }
 
 static struct internal_problem internal_warning_problem = {
-  "internal-warning", internal_problem_ask, internal_problem_ask
+  "internal-warning", 1, internal_problem_ask, 1, internal_problem_ask
 };
 
 void
@@ -834,6 +836,26 @@ internal_warning (const char *file, int line, const char *string, ...)
   va_end (ap);
 }
 
+static struct internal_problem demangler_warning_problem = {
+  "demangler-warning", 1, internal_problem_ask, 0, internal_problem_no
+};
+
+void
+demangler_vwarning (const char *file, int line, const char *fmt, va_list ap)
+{
+  internal_vproblem (&demangler_warning_problem, file, line, fmt, ap);
+}
+
+void
+demangler_warning (const char *file, int line, const char *string, ...)
+{
+  va_list ap;
+
+  va_start (ap, string);
+  demangler_vwarning (file, line, string, ap);
+  va_end (ap);
+}
+
 /* Dummy functions to keep add_prefix_cmd happy.  */
 
 static void
@@ -894,45 +916,51 @@ add_internal_problem_command (struct internal_problem *problem)
 			  (char *) NULL),
 		  0/*allow-unknown*/, &maintenance_show_cmdlist);
 
-  set_doc = xstrprintf (_("Set whether GDB should quit "
-			  "when an %s is detected"),
-			problem->name);
-  show_doc = xstrprintf (_("Show whether GDB will quit "
-			   "when an %s is detected"),
-			 problem->name);
-  add_setshow_enum_cmd ("quit", class_maintenance,
-			internal_problem_modes,
-			&problem->should_quit,
-			set_doc,
-			show_doc,
-			NULL, /* help_doc */
-			NULL, /* setfunc */
-			NULL, /* showfunc */
-			set_cmd_list,
-			show_cmd_list);
-
-  xfree (set_doc);
-  xfree (show_doc);
-
-  set_doc = xstrprintf (_("Set whether GDB should create a core "
-			  "file of GDB when %s is detected"),
-			problem->name);
-  show_doc = xstrprintf (_("Show whether GDB will create a core "
-			   "file of GDB when %s is detected"),
-			 problem->name);
-  add_setshow_enum_cmd ("corefile", class_maintenance,
-			internal_problem_modes,
-			&problem->should_dump_core,
-			set_doc,
-			show_doc,
-			NULL, /* help_doc */
-			NULL, /* setfunc */
-			NULL, /* showfunc */
-			set_cmd_list,
-			show_cmd_list);
+  if (problem->user_settable_should_quit)
+    {
+      set_doc = xstrprintf (_("Set whether GDB should quit "
+			      "when an %s is detected"),
+			    problem->name);
+      show_doc = xstrprintf (_("Show whether GDB will quit "
+			       "when an %s is detected"),
+			     problem->name);
+      add_setshow_enum_cmd ("quit", class_maintenance,
+			    internal_problem_modes,
+			    &problem->should_quit,
+			    set_doc,
+			    show_doc,
+			    NULL, /* help_doc */
+			    NULL, /* setfunc */
+			    NULL, /* showfunc */
+			    set_cmd_list,
+			    show_cmd_list);
+
+      xfree (set_doc);
+      xfree (show_doc);
+    }
 
-  xfree (set_doc);
-  xfree (show_doc);
+  if (problem->user_settable_should_dump_core)
+    {
+      set_doc = xstrprintf (_("Set whether GDB should create a core "
+			      "file of GDB when %s is detected"),
+			    problem->name);
+      show_doc = xstrprintf (_("Show whether GDB will create a core "
+			       "file of GDB when %s is detected"),
+			     problem->name);
+      add_setshow_enum_cmd ("corefile", class_maintenance,
+			    internal_problem_modes,
+			    &problem->should_dump_core,
+			    set_doc,
+			    show_doc,
+			    NULL, /* help_doc */
+			    NULL, /* setfunc */
+			    NULL, /* showfunc */
+			    set_cmd_list,
+			    show_cmd_list);
+
+      xfree (set_doc);
+      xfree (show_doc);
+    }
 }
 
 /* Return a newly allocated string, containing the PREFIX followed
@@ -3523,4 +3551,5 @@ _initialize_utils (void)
 {
   add_internal_problem_command (&internal_error_problem);
   add_internal_problem_command (&internal_warning_problem);
+  add_internal_problem_command (&demangler_warning_problem);
 }
diff --git a/gdb/maint.c b/gdb/maint.c
index 873c33c..3f83891 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -131,6 +131,15 @@ maintenance_internal_warning (char *args, int from_tty)
   internal_warning (__FILE__, __LINE__, "%s", (args == NULL ? "" : args));
 }
 
+/* Stimulate the internal error mechanism that GDB uses when an
+   demangler problem is detected.  Allows testing of the mechanism.  */
+
+static void
+maintenance_demangler_warning (char *args, int from_tty)
+{
+  demangler_warning (__FILE__, __LINE__, "%s", (args == NULL ? "" : args));
+}
+
 /* Someday we should allow demangling for things other than just
    explicit strings.  For example, we might want to be able to specify
    the address of a string in either GDB's process space or the
@@ -1052,6 +1061,12 @@ Give GDB an internal warning.\n\
 Cause GDB to behave as if an internal warning was reported."),
 	   &maintenancelist);
 
+  add_cmd ("demangler-warning", class_maintenance,
+	   maintenance_demangler_warning, _("\
+Give GDB a demangler warning.\n\
+Cause GDB to behave as if a demangler warning was reported."),
+	   &maintenancelist);
+
   add_cmd ("demangle", class_maintenance, maintenance_demangle, _("\
 Demangle a C++/ObjC mangled name.\n\
 Call internal GDB demangler routine to demangle a C++ link name\n\
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9f7fa18..6104f55 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33267,13 +33267,18 @@ with the @code{SIGQUIT} signal.
 
 @kindex maint internal-error
 @kindex maint internal-warning
+@kindex maint demangler-warning
+@cindex demangler crashes
 @item maint internal-error @r{[}@var{message-text}@r{]}
 @itemx maint internal-warning @r{[}@var{message-text}@r{]}
-Cause @value{GDBN} to call the internal function @code{internal_error}
-or @code{internal_warning} and hence behave as though an internal error
-or internal warning has been detected.  In addition to reporting the
-internal problem, these functions give the user the opportunity to
-either quit @value{GDBN} or create a core file of the current
+@itemx maint demangler-warning @r{[}@var{message-text}@r{]}
+
+Cause @value{GDBN} to call the internal function @code{internal_error},
+@code{internal_warning} or @code{demangler_warning} and hence behave
+as though an internal problam has been detected.  In addition to
+reporting the internal problem, these functions give the user the
+opportunity to either quit @value{GDBN} or (for @code{internal_error}
+and @code{internal_warning}) create a core file of the current
 @value{GDBN} session.
 
 These commands take an optional parameter @var{message-text} that is
@@ -33293,15 +33298,20 @@ Create a core file? (y or n) @kbd{n}
 
 @cindex @value{GDBN} internal error
 @cindex internal errors, control of @value{GDBN} behavior
+@cindex demangler crashes
 
 @kindex maint set internal-error
 @kindex maint show internal-error
 @kindex maint set internal-warning
 @kindex maint show internal-warning
+@kindex maint set demangler-warning
+@kindex maint show demangler-warning
 @item maint set internal-error @var{action} [ask|yes|no]
 @itemx maint show internal-error @var{action}
 @itemx maint set internal-warning @var{action} [ask|yes|no]
 @itemx maint show internal-warning @var{action}
+@itemx maint set demangler-warning @var{action} [ask|yes|no]
+@itemx maint show demangler-warning @var{action}
 When @value{GDBN} reports an internal problem (error or warning) it
 gives the user the opportunity to both quit @value{GDBN} and create a
 core file of the current @value{GDBN} session.  These commands let you
@@ -33315,7 +33325,10 @@ quit.  The default is to ask the user what to do.
 
 @item corefile
 You can specify that @value{GDBN} should always (yes) or never (no)
-create a core file.  The default is to ask the user what to do.
+create a core file.  The default is to ask the user what to do.  Note
+that there is no @code{corefile} option for @code{demangler-warning}:
+demangler warnings always create a core file and this cannot be
+disabled.
 @end table
 
 @kindex maint packet

Re: [PATCH 1/2 v3] Add new internal problem for demangler warnings

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 11:08:53 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,
>         Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,
>         Mark Kettenis <mark.kettenis@xs4all.nl>,
>         Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> 
> This patch adds a new category of internal problem for demangler
> warnings.  Demangler warnings behave in much the same way as internal
> warnings except that they do not create core files and no option to
> change this is presented to the user.

OK for the documentation part.

Thanks.

Re: [PATCH 1/2 v3] Add new internal problem for demangler warnings

[Gary Benson]

Eli Zaretskii wrote:
> > Date: Wed, 4 Jun 2014 11:08:53 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,
> >         Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,
> >         Mark Kettenis <mark.kettenis@xs4all.nl>,
> >         Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> > 
> > This patch adds a new category of internal problem for demangler
> > warnings.  Demangler warnings behave in much the same way as internal
> > warnings except that they do not create core files and no option to
> > change this is presented to the user.
> 
> OK for the documentation part.

Thanks Eli.

Cheers,
Gary

--
http://gbenson.net/

[PATCH 2/2 v3] Demangler crash handler

[Gary Benson]

This patch wraps calls to the demangler with a segmentation fault
handler.  The first time a segmentation fault is caught a core file
is generated and the user is prompted to file a bug and offered the
choice to exit or to continue their GDB session.  A maintainence
option is provided to allow the user to disable the crash handler
if required.

Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
various other SIGSEGV checks in GDB so I have left the preprocessor
conditionals intact for consistency.  I hope this is ok.


gdb/
2014-06-04  Gary Benson  <gbenson@redhat.com>

	* utils.h (dump_core): New declaration.
	* utils.c (dump_core): Make non-static.
	* cp-support.c (signal.h): New include.
	(catch_demangler_crashes): New flag.
	(SIGJMP_BUF): New define.
	(SIGSETJMP): Likewise.
	(SIGLONGJMP): Likewise.
	(gdb_demangle_jmp_buf): New static global.
	(gdb_demangle_signal_handler): New function.
	(gdb_demangle): If catch_demangler_crashes is set, install the
	above signal handler before calling bfd_demangle, and restore
	the original signal handler afterwards.  Display the offending
	symbol and call demangler_warning the first time a segmentation
	fault is caught.
	(_initialize_cp_support): New maint set/show command.

gdb/doc/
2014-06-04  Gary Benson  <gbenson@redhat.com>

	* gdb.texinfo (Maintenance Commands): Document new
	"maint set/show catch-demangler-crashes" option.

diff --git a/gdb/utils.h b/gdb/utils.h
index 31f9c19..8ada98e 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -374,4 +374,8 @@ extern ULONGEST align_down (ULONGEST v, int n);
 
 extern LONGEST gdb_sign_extend (LONGEST value, int bit);
 
+/* Dump core trying to increase the core soft limit to hard limit first.  */
+
+extern void dump_core (void);
+
 #endif /* UTILS_H */
diff --git a/gdb/utils.c b/gdb/utils.c
index 686b153..733c697 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -600,7 +600,7 @@ error_stream (struct ui_file *stream)
 
 /* Dump core trying to increase the core soft limit to hard limit first.  */
 
-static void
+void
 dump_core (void)
 {
 #ifdef HAVE_SETRLIMIT
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 91533e8..96f7d89 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -36,6 +36,7 @@
 #include "value.h"
 #include "cp-abi.h"
 #include "language.h"
+#include <signal.h>
 
 #include "safe-ctype.h"
 
@@ -1505,12 +1506,111 @@ cp_lookup_rtti_type (const char *name, struct block *block)
   return rtti_type;
 }
 
+#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
+
+/* If nonzero, attempt to catch crashes in the demangler and print
+   useful debugging information.  */
+
+static int catch_demangler_crashes = 1;
+
+/* Wrap set/long jmp so that it's more portable.  */
+
+#if defined(HAVE_SIGSETJMP)
+#define SIGJMP_BUF		sigjmp_buf
+#define SIGSETJMP(buf)		sigsetjmp((buf), 1)
+#define SIGLONGJMP(buf,val)	siglongjmp((buf), (val))
+#else
+#define SIGJMP_BUF		jmp_buf
+#define SIGSETJMP(buf)		setjmp(buf)
+#define SIGLONGJMP(buf,val)	longjmp((buf), (val))
+#endif
+
+/* Stack context and environment for demangler crash recovery.  */
+
+static SIGJMP_BUF gdb_demangle_jmp_buf;
+
+/* Signal handler for gdb_demangle.  */
+
+static void
+gdb_demangle_signal_handler (int signo)
+{
+  static int core_dumped = 0;
+
+  if (!core_dumped)
+    {
+      if (fork () == 0)
+	dump_core ();
+
+      core_dumped = 1;
+    }
+
+  SIGLONGJMP (gdb_demangle_jmp_buf, signo);
+}
+
+#endif
+
 /* A wrapper for bfd_demangle.  */
 
 char *
 gdb_demangle (const char *name, int options)
 {
-  return bfd_demangle (NULL, name, options);
+  char *result = NULL;
+  int crash_signal = 0;
+
+#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  struct sigaction sa, old_sa;
+
+  if (catch_demangler_crashes)
+    {
+      sa.sa_handler = gdb_demangle_signal_handler;
+      sigemptyset (&sa.sa_mask);
+      sa.sa_flags = 0;
+      sigaction (SIGSEGV, &sa, &old_sa);
+    }
+#else
+  void (*ofunc) ();
+
+  if (catch_demangler_crashes)
+    ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
+#endif
+
+  if (catch_demangler_crashes)
+    crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+#endif
+
+  if (crash_signal == 0)
+    result = bfd_demangle (NULL, name, options);
+
+#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
+  if (catch_demangler_crashes)
+    {
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      sigaction (SIGSEGV, &old_sa, NULL);
+#else
+      signal (SIGSEGV, ofunc);
+#endif
+
+      if (crash_signal != 0)
+	{
+	  static int error_reported = 0;
+
+	  if (!error_reported)
+	    {
+	      demangler_warning (__FILE__, __LINE__,
+				 _("unable to demangle '%s' "
+				   "(demangler failed with signal %d)"),
+				 name, crash_signal);
+
+	      error_reported = 1;
+	    }
+
+	  result = NULL;
+	}
+    }
+#endif
+
+  return result;
 }
 
 /* Don't allow just "maintenance cplus".  */
@@ -1585,4 +1685,17 @@ _initialize_cp_support (void)
 Usage: info vtbl EXPRESSION\n\
 Evaluate EXPRESSION and display the virtual function table for the\n\
 resulting object."));
+
+#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
+  add_setshow_boolean_cmd ("catch-demangler-crashes", class_maintenance,
+			   &catch_demangler_crashes, _("\
+Set whether to attempt to catch demangler crashes."), _("\
+Show whether to attempt to catch demangler crashes."), _("\
+If enabled GDB will attempt to catch demangler crashes and\n\
+display the offending symbol."),
+			   NULL,
+			   NULL,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
+#endif
 }
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6104f55..6b39452 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33235,6 +33235,17 @@ Expand symbol tables.
 If @var{regexp} is specified, only expand symbol tables for file
 names matching @var{regexp}.
 
+@kindex maint set catch-demangler-crashes
+@kindex maint show catch-demangler-crashes
+@cindex demangler crashes
+@item maint set catch-demangler-crashes [on|off]
+@itemx maint show catch-demangler-crashes
+Control whether @value{GDBN} should attempt to catch crashes in the
+symbol name demangler.  The default is to attempt to catch crashes.
+If enabled, the first time a crash is caught, a core file is created,
+the offending symbol is displayed and the user is presented with the
+option to terminate the current session.
+
 @kindex maint cplus first_component
 @item maint cplus first_component @var{name}
 Print the first C@t{++} class/namespace component of @var{name}.

Re: [PATCH 2/2 v3] Demangler crash handler

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 11:09:57 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,
>         Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,
>         Mark Kettenis <mark.kettenis@xs4all.nl>,
>         Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> 
> This patch wraps calls to the demangler with a segmentation fault
> handler.  The first time a segmentation fault is caught a core file
> is generated and the user is prompted to file a bug and offered the
> choice to exit or to continue their GDB session.  A maintainence
> option is provided to allow the user to disable the crash handler
> if required.

The documentation part is OK.

> Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
> various other SIGSEGV checks in GDB

They should all be removed.

Re: [PATCH 2/2 v3] Demangler crash handler

[Gary Benson]

Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > 
> > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > found various other SIGSEGV checks in GDB
> 
> They should all be removed.

Ok, I'll do this.  Should I commit the change as obvious?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 2/2 v3] Demangler crash handler

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 14:36:03 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
>         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
>         tromey@redhat.com
> 
> Eli Zaretskii wrote:
> > > From: Gary Benson <gbenson@redhat.com>
> > > 
> > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > > found various other SIGSEGV checks in GDB
> > 
> > They should all be removed.
> 
> Ok, I'll do this.  Should I commit the change as obvious?

I think so, yes.

Re: [PATCH 2/2 v3] Demangler crash handler

[Gary Benson]

Eli Zaretskii wrote:
> > Date: Wed, 4 Jun 2014 14:36:03 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
> >         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
> >         tromey@redhat.com
> > 
> > Eli Zaretskii wrote:
> > > > From: Gary Benson <gbenson@redhat.com>
> > > > 
> > > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
> > > > found various other SIGSEGV checks in GDB
> > > 
> > > They should all be removed.
> > 
> > Ok, I'll do this.  Should I commit the change as obvious?
> 
> I think so, yes.

Ok, I'll do that.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 2/2 v3] Demangler crash handler

[Doug Evans]

On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
> Eli Zaretskii wrote:
>> > Date: Wed, 4 Jun 2014 14:36:03 +0100
>> > From: Gary Benson <gbenson@redhat.com>
>> > Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
>> >         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
>> >         tromey@redhat.com
>> >
>> > Eli Zaretskii wrote:
>> > > > From: Gary Benson <gbenson@redhat.com>
>> > > >
>> > > > Eli pointed out that SIGSEGV is an ANSI-standard signal but I
>> > > > found various other SIGSEGV checks in GDB
>> > >
>> > > They should all be removed.
>> >
>> > Ok, I'll do this.  Should I commit the change as obvious?
>>
>> I think so, yes.
>
> Ok, I'll do that.

Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?

[assuming that's correct ...]
If one goes down this path, I think the patch while perhaps "obvious"
would become a bit involved (why just SEGV?) and thus the obviousness
diminishes.
I think it diminishes to a point where the obviousness is gone.
Please submit any such patch for review.
Thanks!

Re: [PATCH 2/2 v3] Demangler crash handler

[Gary Benson]

Doug Evans wrote:
> On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Eli Zaretskii wrote:
> > > > From: Gary Benson <gbenson@redhat.com>
> > > >
> > > > Eli Zaretskii wrote:
> > > > > > From: Gary Benson <gbenson@redhat.com>
> > > > > >
> > > > > > Eli pointed out that SIGSEGV is an ANSI-standard signal
> > > > > > but I found various other SIGSEGV checks in GDB
> > > > >
> > > > > They should all be removed.
> > > >
> > > > Ok, I'll do this.  Should I commit the change as obvious?
> > >
> > > I think so, yes.
> >
> > Ok, I'll do that.
> 
> Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?

Yes.

> If one goes down this path, I think the patch while perhaps
> "obvious" would become a bit involved (why just SEGV?) and
> thus the obviousness diminishes.
> I think it diminishes to a point where the obviousness is gone.
> Please submit any such patch for review.

Having started looking into this I am inclined to agree.

I couldn't find an authoritative list, but the Linux kernel sources
indicate that SIGINT, SIGILL, SIGABRT, SIGFPE, SIGSEGV and SIGTERM
are ANSI.  Eli said that list agreed with his references, so I'll
work on unwrapping those.

I will post the patch for review.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 2/2 v3] Demangler crash handler

[Doug Evans]

On Wed, Jun 4, 2014 at 11:25 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Wed, Jun 4, 2014 at 7:28 AM, Gary Benson <gbenson@redhat.com> wrote:
>> > Eli Zaretskii wrote:
>> > > > From: Gary Benson <gbenson@redhat.com>
>> > > >
>> > > > Eli Zaretskii wrote:
>> > > > > > From: Gary Benson <gbenson@redhat.com>
>> > > > > >
>> > > > > > Eli pointed out that SIGSEGV is an ANSI-standard signal
>> > > > > > but I found various other SIGSEGV checks in GDB
>> > > > >
>> > > > > They should all be removed.
>> > > >
>> > > > Ok, I'll do this.  Should I commit the change as obvious?
>> > >
>> > > I think so, yes.
>> >
>> > Ok, I'll do that.
>>
>> Are we talking about #ifdef SIGSEGV in, e.g., common/signals.c?
>
> Yes.
>
>> If one goes down this path, I think the patch while perhaps
>> "obvious" would become a bit involved (why just SEGV?) and
>> thus the obviousness diminishes.
>> I think it diminishes to a point where the obviousness is gone.
>> Please submit any such patch for review.
>
> Having started looking into this I am inclined to agree.
>
> I couldn't find an authoritative list, but the Linux kernel sources
> indicate that SIGINT, SIGILL, SIGABRT, SIGFPE, SIGSEGV and SIGTERM
> are ANSI.  Eli said that list agreed with his references, so I'll
> work on unwrapping those.
>
> I will post the patch for review.

One thing that I think should be considered is that we'll go from the
simple state of "just ifdef every signal" in places like
common/signals.c to having some signals you are required to not ifdef
and some you do, and needing to know which category every signal fits
in.  I don't have a strong opinion, but I'm ok with the status quo.

Re: [PATCH 2/2 v3] Demangler crash handler

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 18:11:16 -0700
> From: Doug Evans <xdje42@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
> 	Andrew Burgess <aburgess@broadcom.com>, Florian Weimer <fw@deneb.enyo.de>, 
> 	Mark Kettenis <mark.kettenis@xs4all.nl>, Pedro Alves <palves@redhat.com>, 
> 	Tom Tromey <tromey@redhat.com>
> 
> One thing that I think should be considered is that we'll go from the
> simple state of "just ifdef every signal" in places like
> common/signals.c to having some signals you are required to not ifdef
> and some you do, and needing to know which category every signal fits
> in.  I don't have a strong opinion, but I'm ok with the status quo.

The list of ANSI-standard signals is short, and it's easy to check
which signal is or isn't.

Ifdef's are ugly.  Minimizing their use is a Good Thing.

Re: [PATCH 2/2 v3] Demangler crash handler

[Doug Evans]

Hi.  A few comments inline.

Gary Benson <gbenson@redhat.com> writes:
> This patch wraps calls to the demangler with a segmentation fault
> handler.  The first time a segmentation fault is caught a core file
> is generated and the user is prompted to file a bug and offered the
> choice to exit or to continue their GDB session.  A maintainence
> option is provided to allow the user to disable the crash handler
> if required.
>
> Eli pointed out that SIGSEGV is an ANSI-standard signal but I found
> various other SIGSEGV checks in GDB so I have left the preprocessor
> conditionals intact for consistency.  I hope this is ok.
>
>
> gdb/
> 2014-06-04  Gary Benson  <gbenson@redhat.com>
>
> 	* utils.h (dump_core): New declaration.
> 	* utils.c (dump_core): Make non-static.
> 	* cp-support.c (signal.h): New include.
> 	(catch_demangler_crashes): New flag.
> 	(SIGJMP_BUF): New define.
> 	(SIGSETJMP): Likewise.
> 	(SIGLONGJMP): Likewise.
> 	(gdb_demangle_jmp_buf): New static global.
> 	(gdb_demangle_signal_handler): New function.
> 	(gdb_demangle): If catch_demangler_crashes is set, install the
> 	above signal handler before calling bfd_demangle, and restore
> 	the original signal handler afterwards.  Display the offending
> 	symbol and call demangler_warning the first time a segmentation
> 	fault is caught.
> 	(_initialize_cp_support): New maint set/show command.
>
> [...]
>
> +/* Stack context and environment for demangler crash recovery.  */
> +
> +static SIGJMP_BUF gdb_demangle_jmp_buf;
> +
> +/* Signal handler for gdb_demangle.  */
> +
> +static void
> +gdb_demangle_signal_handler (int signo)
> +{
> +  static int core_dumped = 0;
> +
> +  if (!core_dumped)
> +    {
> +      if (fork () == 0)
> +	dump_core ();

IIUC you're skipping the can_dump_core() check.
If the user has set ulimit -c 0, I think that needs to be obeyed.
I realize can_dump_core may call fprintf which we can't do here,
but you could still IMO call getrlimit.
IWBN to still call can_dump_core (or whatever) so that the
implementation of the check is still tucked away in a function.

> +
> +      core_dumped = 1;
> +    }
> +
> +  SIGLONGJMP (gdb_demangle_jmp_buf, signo);
> +}
> +
> +#endif
> +
>  /* A wrapper for bfd_demangle.  */
>  
>  char *
>  gdb_demangle (const char *name, int options)
>  {
> -  return bfd_demangle (NULL, name, options);
> +  char *result = NULL;
> +  int crash_signal = 0;
> +
> +#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> +  struct sigaction sa, old_sa;
> +
> +  if (catch_demangler_crashes)
> +    {
> +      sa.sa_handler = gdb_demangle_signal_handler;
> +      sigemptyset (&sa.sa_mask);
> +      sa.sa_flags = 0;
> +      sigaction (SIGSEGV, &sa, &old_sa);
> +    }
> +#else
> +  void (*ofunc) ();
> +
> +  if (catch_demangler_crashes)
> +    ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
> +#endif
> +
> +  if (catch_demangler_crashes)
> +    crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
> +#endif
> +
> +  if (crash_signal == 0)
> +    result = bfd_demangle (NULL, name, options);
> +
> +#if defined (SIGSEGV) && defined (HAVE_WORKING_FORK)
> +  if (catch_demangler_crashes)
> +    {
> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> +      sigaction (SIGSEGV, &old_sa, NULL);
> +#else
> +      signal (SIGSEGV, ofunc);
> +#endif
> +
> +      if (crash_signal != 0)
> +	{
> +	  static int error_reported = 0;
> +
> +	  if (!error_reported)

For myself as a user I'd like the warning for every demangle failure.
[I'd throttle it by unique symbols though.]

> +	    {
> +	      demangler_warning (__FILE__, __LINE__,
> +				 _("unable to demangle '%s' "
> +				   "(demangler failed with signal %d)"),
> +				 name, crash_signal);
> +
> +	      error_reported = 1;
> +	    }
> +
> +	  result = NULL;
> +	}
> +    }
> +#endif
> +
> +  return result;
>  }

Re: [PATCH 2/2 v3] Demangler crash handler

[Gary Benson]

Doug Evans wrote:
> Gary Benson <gbenson@redhat.com> writes:
> > +static void
> > +gdb_demangle_signal_handler (int signo)
> > +{
> > +  static int core_dumped = 0;
> > +
> > +  if (!core_dumped)
> > +    {
> > +      if (fork () == 0)
> > +	dump_core ();
> 
> IIUC you're skipping the can_dump_core() check.
> If the user has set ulimit -c 0, I think that needs to be obeyed.
> I realize can_dump_core may call fprintf which we can't do here,
> but you could still IMO call getrlimit.
> IWBN to still call can_dump_core (or whatever) so that the
> implementation of the check is still tucked away in a function.

Ah, I saw can_dump_core but didn't get what it was doing.
I'll refactor it so that the check is performed once, before the
signal handler is installed, and the message is printed outside
the signal handler if a crash is caught.

> > +      if (crash_signal != 0)
> > +	{
> > +	  static int error_reported = 0;
> > +
> > +	  if (!error_reported)
> 
> For myself as a user I'd like the warning for every demangle failure.
> [I'd throttle it by unique symbols though.]

My reasoning here was that any subsequent failures could be caused by
the first, by memory corruption or some leftover state, so they could
be bogus and lead us into wild goose chases.

I can certainly put together a patch with throttled warnings as you
describe if you prefer.  Let me know :)

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/2 v3] Demangler crash handler

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 11:07:55 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: Andrew Burgess <aburgess@broadcom.com>, Doug Evans <xdje42@gmail.com>,        Eli Zaretskii <eliz@gnu.org>, Florian Weimer <fw@deneb.enyo.de>,        Mark Kettenis <mark.kettenis@xs4all.nl>,        Pedro Alves <palves@redhat.com>, Tom Tromey <tromey@redhat.com>
> 
> I would push both patches as one commit.  The news file entries
> for the commit would be:
> 
>   * New options
>   
>   maint set catch-demangler-crashes (on|off)
>   maint show catch-demangler-crashes
>     Control whether GDB should attempt to catch crashes in the symbol
>     name demangler.  The default is to attempt to catch crashes.  If
>     enabled, the first time a crash is caught, a core file is created,
>     the offending symbol is displayed and the user is presented with
>     the option to terminate the current session.
>   
>   maint set demangler-warning quit (yes|no|ask)
>     When GDB catches a crash in the symbol name demangler it can offer
>     the user the opportunity to both quit GDB and create a core file of
>     the current GDB session.  These options control whether or not to
>     do either of these.  The default is to create a core file and to ask
>     the user whether to quit.
>   
>   * New commands
>   
>   maint demangler-warning
>     Cause GDB to call the internal function demangler_warning and
>     hence behave as though an internal error in the demangler has
>     been detected.
> 
> Is this ok to commit?

The above is too detailed for NEWS.  Do not just copy the text from
the manual, but instead provide a very short (preferably a
single-sentence) summary of the new option.

Thanks.

Re: [PATCH 0/2 v3] Demangler crash handler

[Gary Benson]

Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > 
> > The news file entries for the commit would be:
> > 
> >   * New options
> >   
> >   maint set catch-demangler-crashes (on|off)
> >   maint show catch-demangler-crashes
> >     Control whether GDB should attempt to catch crashes in the symbol
> >     name demangler.  The default is to attempt to catch crashes.  If
> >     enabled, the first time a crash is caught, a core file is created,
> >     the offending symbol is displayed and the user is presented with
> >     the option to terminate the current session.
> >   
> >   maint set demangler-warning quit (yes|no|ask)
> >     When GDB catches a crash in the symbol name demangler it can offer
> >     the user the opportunity to both quit GDB and create a core file of
> >     the current GDB session.  These options control whether or not to
> >     do either of these.  The default is to create a core file and to ask
> >     the user whether to quit.
> >   
> >   * New commands
> >   
> >   maint demangler-warning
> >     Cause GDB to call the internal function demangler_warning and
> >     hence behave as though an internal error in the demangler has
> >     been detected.
> > 
> > Is this ok to commit?
> 
> The above is too detailed for NEWS.  Do not just copy the text
> from the manual, but instead provide a very short (preferably a
> single-sentence) summary of the new option.

How about these?

  maint set catch-demangler-crashes (on|off)
  maint show catch-demangler-crashes
    Control whether GDB should attempt to catch crashes in the symbol
    name demangler.
  
  maint set demangler-warning quit (yes|no|ask)
    Control whether GDB should exit if it catches a crash in the
    symbol name demangler.

I don't know I could shorten "maint demangler-warning"'s entry any
more without it becoming meaningless.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/2 v3] Demangler crash handler

[Eli Zaretskii]

> Date: Wed, 4 Jun 2014 14:40:55 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
>         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
>         tromey@redhat.com
> 
> > The above is too detailed for NEWS.  Do not just copy the text
> > from the manual, but instead provide a very short (preferably a
> > single-sentence) summary of the new option.
> 
> How about these?
> 
>   maint set catch-demangler-crashes (on|off)
>   maint show catch-demangler-crashes
>     Control whether GDB should attempt to catch crashes in the symbol
>     name demangler.
>   
>   maint set demangler-warning quit (yes|no|ask)
>     Control whether GDB should exit if it catches a crash in the
>     symbol name demangler.

Perfect, thanks.

> I don't know I could shorten "maint demangler-warning"'s entry any
> more without it becoming meaningless.

It is already short enough.

Re: [PATCH 0/2 v3] Demangler crash handler

[Gary Benson]

Eli Zaretskii wrote:
> > Date: Wed, 4 Jun 2014 14:40:55 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb-patches@sourceware.org, aburgess@broadcom.com, xdje42@gmail.com,
> >         fw@deneb.enyo.de, mark.kettenis@xs4all.nl, palves@redhat.com,
> >         tromey@redhat.com
> > 
> > > The above is too detailed for NEWS.  Do not just copy the text
> > > from the manual, but instead provide a very short (preferably a
> > > single-sentence) summary of the new option.
> > 
> > How about these?
> > 
> >   maint set catch-demangler-crashes (on|off)
> >   maint show catch-demangler-crashes
> >     Control whether GDB should attempt to catch crashes in the
> >     symbol name demangler.
> >   
> >   maint set demangler-warning quit (yes|no|ask)
> >     Control whether GDB should exit if it catches a crash in the
> >     symbol name demangler.
> 
> Perfect, thanks.
> 
> > I don't know I could shorten "maint demangler-warning"'s entry any
> > more without it becoming meaningless.
> 
> It is already short enough.

Awesome.  Thanks for the speedy review!

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/2 v3] Demangler crash handler

[Mark Kettenis]

> Date: Wed, 4 Jun 2014 11:07:55 +0100
> From: Gary Benson <gbenson@redhat.com>
> 
> Hi all,
> 
> This patch is an updated version of the demangler crash handler I
> posted two weeks ago.  There are two main changes from the previous
> version:
> 
>  1) The signal handler creates a core file before returning.
>     This ensures that a core file is created at the correct
>     location.
> 
>  2) I have switched the crash handler to be enabled by default.
>     I think this is appropriate now that a core file is always
>     created.
> 
> To make this work correctly I have had to add a new class of
> internal problem, which I've called demangler_warning.  This
> has the same behaviour as internal_warning except that it does
> not prompt the user to create a core file because that's already
> been done.  I know Doug didn't want this but it's necessary to
> avoid either overwriting the useful core file with a non-useful
> one or confusing the user with a second, non-useful core file.
> 
> I've split this patch into two parts for ease of discussion.  The
> first patch adds the new internal problem functionality, and the
> second patch is the crash handler itself.  This differs from the
> previous version by the addition of core file generation and in
> that it calls demangler_warning instead of internal_warning.
> 
> I would push both patches as one commit.  The news file entries
> for the commit would be:
> 
>   * New options
>   
>   maint set catch-demangler-crashes (on|off)
>   maint show catch-demangler-crashes
>     Control whether GDB should attempt to catch crashes in the symbol
>     name demangler.  The default is to attempt to catch crashes.  If
>     enabled, the first time a crash is caught, a core file is created,
>     the offending symbol is displayed and the user is presented with
>     the option to terminate the current session.
>   
>   maint set demangler-warning quit (yes|no|ask)
>     When GDB catches a crash in the symbol name demangler it can offer
>     the user the opportunity to both quit GDB and create a core file of
>     the current GDB session.  These options control whether or not to
>     do either of these.  The default is to create a core file and to ask
>     the user whether to quit.
>   
>   * New commands
>   
>   maint demangler-warning
>     Cause GDB to call the internal function demangler_warning and
>     hence behave as though an internal error in the demangler has
>     been detected.
> 
> Is this ok to commit?

Not for me.  You're running too much code between entering the SIGSEGV
handler and dumping core for my taste.

I still very much agree with Pedro; this should not be necessary.
Drop this.  Spend your time on fixing the actual bugs.

Re: [PATCH 0/2 v3] Demangler crash handler

[Gary Benson]

Mark Kettenis wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > Is this ok to commit?
> 
> Not for me.  You're running too much code between entering the
> SIGSEGV handler and dumping core for my taste.

I don't understand.  This is the signal handler:

  static void
  gdb_demangle_signal_handler (int signo)
  {
    static int core_dumped = 0;
  
    if (!core_dumped)
      {
        if (fork () == 0)
          dump_core ();
  
        core_dumped = 1;
      }
  
    SIGLONGJMP (gdb_demangle_jmp_buf, signo);
  }

and this is dump_core:

  void
  dump_core (void)
  {
  #ifdef HAVE_SETRLIMIT
    struct rlimit rlim = { RLIM_INFINITY, RLIM_INFINITY };
  
    setrlimit (RLIMIT_CORE, &rlim);
  #endif /* HAVE_SETRLIMIT */
  
    abort ();	/* NOTE: GDB has only three calls to abort().  */
  }

This is what happens:

  1. the signal handler is entered
  2. fork
  3. setrlimit
  4. abort

I could remove the setrlimit but presumably somebody added that to
make a successful core dump more likely.

Would you accept this patch if I changed the dump_core to abort
in gdb_demangle_signal_handler?  (and updated all the comments
about having only three calls to abort)?

> I still very much agree with Pedro; this should not be necessary.

"Should" is the operative word here.  It *should* not be necessary
because the demangler *should* never crash.  But this isn't utopia.
The demangler is code, and code has bugs.  People make mistakes.
Things are valid now that may not be valid in the future.  And GDB
should not just crash if some symbol in the inferior isn't handled
or doesn't make sense or whatever.

> Drop this.  Spend your time on fixing the actual bugs.

Are you trying to tell me you will never approve a patch that installs
a SIGSEGV handler across demangler calls?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/2 v3] Demangler crash handler

[Andrew Burgess]

On 04/06/2014 2:34 PM, Gary Benson wrote:
> Mark Kettenis wrote:
>>> From: Gary Benson <gbenson@redhat.com>
>>> Is this ok to commit?
>>
>> Not for me.  You're running too much code between entering the
>> SIGSEGV handler and dumping core for my taste.
> 
> I don't understand.  This is the signal handler:
> 
>   static void
>   gdb_demangle_signal_handler (int signo)
>   {
>     static int core_dumped = 0;
>   
>     if (!core_dumped)
>       {
>         if (fork () == 0)
>           dump_core ();
>   
>         core_dumped = 1;
>       }
>   
>     SIGLONGJMP (gdb_demangle_jmp_buf, signo);
>   }
> 
> and this is dump_core:
> 
>   void
>   dump_core (void)
>   {
>   #ifdef HAVE_SETRLIMIT
>     struct rlimit rlim = { RLIM_INFINITY, RLIM_INFINITY };
>   
>     setrlimit (RLIMIT_CORE, &rlim);
>   #endif /* HAVE_SETRLIMIT */
>   
>     abort ();	/* NOTE: GDB has only three calls to abort().  */
>   }
> 
> This is what happens:
> 
>   1. the signal handler is entered
>   2. fork
>   3. setrlimit
>   4. abort
> 
> I could remove the setrlimit but presumably somebody added that to
> make a successful core dump more likely.
> 
> Would you accept this patch if I changed the dump_core to abort
> in gdb_demangle_signal_handler?  (and updated all the comments
> about having only three calls to abort)?
> 
>> I still very much agree with Pedro; this should not be necessary.
> 
> "Should" is the operative word here.  It *should* not be necessary
> because the demangler *should* never crash.  But this isn't utopia.
> The demangler is code, and code has bugs.  People make mistakes.
> Things are valid now that may not be valid in the future.  And GDB
> should not just crash if some symbol in the inferior isn't handled
> or doesn't make sense or whatever.

By this logic should / would we not extend the SIGSEGV handler to cover
all gdb code?  If the target is running in synchronous mode we'd
install our SEGV handler when the target stops and remove it when the
target restarts (asynchronous mode would need more thought), then any
bugs in gdb that cause a SEGV would result in a core dump ...

I'm just not sure why the demangler should get special treatment.

Thanks,
Andrew

Re: [PATCH 0/2 v3] Demangler crash handler

[Doug Evans]

On Wed, Jun 4, 2014 at 7:53 AM, Andrew Burgess <aburgess@broadcom.com> wrote:
>> "Should" is the operative word here.  It *should* not be necessary
>> because the demangler *should* never crash.  But this isn't utopia.
>> The demangler is code, and code has bugs.  People make mistakes.
>> Things are valid now that may not be valid in the future.  And GDB
>> should not just crash if some symbol in the inferior isn't handled
>> or doesn't make sense or whatever.
>
> By this logic should / would we not extend the SIGSEGV handler to cover
> all gdb code?  If the target is running in synchronous mode we'd
> install our SEGV handler when the target stops and remove it when the
> target restarts (asynchronous mode would need more thought), then any
> bugs in gdb that cause a SEGV would result in a core dump ...
>
> I'm just not sure why the demangler should get special treatment.

It has a very specific entry point, and thus adding one here is easy
(setting aside the technicalities of the implementation).

I'd say this is ok if only to provide a proving ground for whether
this is useful in practice.

Re: [PATCH 0/2 v3] Demangler crash handler

[Gary Benson]

Andrew Burgess wrote:
> On 04/06/2014 2:34 PM, Gary Benson wrote:
> > Mark Kettenis wrote:
> > > I still very much agree with Pedro; this should not be necessary.
> > 
> > "Should" is the operative word here.  It *should* not be necessary
> > because the demangler *should* never crash.  But this isn't utopia.
> > The demangler is code, and code has bugs.  People make mistakes.
> > Things are valid now that may not be valid in the future.  And GDB
> > should not just crash if some symbol in the inferior isn't handled
> > or doesn't make sense or whatever.
> 

> By this logic should / would we not extend the SIGSEGV handler to
> cover all gdb code?

I'm not suggesting this.

> I'm just not sure why the demangler should get special treatment.

It's a combination of the severity of the impact (bugs cause GDB to
dump core on startup, with no reasonable workaround) and the fact that
the demangler is unusually recoverable (it allocates no heap, uses no
resources, and has no global state: you can peel back the stack and
carry on*).  A lesser issue is that bugs are easily reproducible when
you have the offending symbol.  I see no reason why users should be
inconvenienced (by not being able to use GDB) and why developers
should do extra work (digging symbols out of core dumps) when a
small piece of code can mitigate the impact on the user and do the
legwork for the developer, all without losing performance or consuming
resources. 

Thanks,
Gary

--
* I'm aware that memory can be corrupted before the segmentation
fault is triggered, meaning GDB *might* subsequently crash.  My
position is that "might crash later" is better than "definitely
crashing now".