[PATCH 0/3 v4] Demangler crash handler

[PATCH 0/3 v4] Demangler crash handler

[Gary Benson]

Hi all,

This patch is an updated version of the demangler crash handler I
posted yesterday.  The main changes from the previous version are:

 1) All #ifdef SIGSEGV conditionals have been removed.

 2) If the user set "ulimit -c 0" then no core file will be
    created and a warning will be printed.  This mirrors what
    internal_error and internal_warning currently do.
    
 3) A separate signal stack is created for the SIGSEGV handler
    to allow it to function correctly if the normal process
    stack overflows.  This signal stack is currently only used
    by the SIGSEGV handler--all other signal handlers use the
    normal process stack as before.

Doug requested that I change the patch to emit warnings for every
demangler crash, not just the first.  I've not done this, my reason
being that subsequent failures could have been caused by the first,
by memory corruption or some leftover state: they could be bogus,
and could lead to us chasing bugs that don't exist.  I prefer this
way, but I'm not hung up on it and if it's a blocker for Doug or
anyone else I will add the extra warnings.

I've split the patches as follows:

 1/3 - adds a new category of internal problem for demangler
       warnings.  This patch is unchanged from the previous
       version (PATCH 1/2 v3):
       https://sourceware.org/ml/gdb-patches/2014-06/msg00142.html

 2/3 - refactors and exposes the core-dumping functions in utils.c.
       This is a completely new patch.

 3/3 - the crash catcher itself.  This patch differs from the
       previous version by the removal of the #ifdef SIGSEGV
       conditionals, the addition of a check to see if a core
       dump should be performed, and the creation of a separate
       stack to allow the signal handler to function when the
       normal stack is exhausted.

I would push all three 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.

  maint set demangler-warning quit (yes|no|ask)
  maint show demangler-warning quit
    Control whether GDB should exit if it catches a crash in the
    symbol name demangler.
  
  * 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/3 v4] 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-05  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-05  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

[PATCH 2/3 v4] Refactor and expose core-dumping functionality

[Gary Benson]

This patch exposes the functions that dump core outside utils.c.
The function can_dump_core has been split into two new functions,
check_can_dump_core and warn_cant_dump_core so that the check and
the printed warning can be separated.  A new function
check_can_dump_core_warn replaces the original can_dump_core.


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

	* utils.h (check_can_dump_core): New declaration.
	(warn_cant_dump_core): Likewise.
	(dump_core): Likewise.
	* utils.c (dump_core): Made nonstatic.
	(can_dump_core): Removed function.
	(check_can_dump_core): New function.
	(warn_cant_dump_core): Likewise.
	(check_can_dump_core_warn): Likewise.
	(internal_vproblem): Replace calls to can_dump_core with
	calls to check_can_dump_core_warn.

diff --git a/gdb/utils.h b/gdb/utils.h
index 0ba7879..a6115c1 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -374,4 +374,18 @@ extern ULONGEST align_down (ULONGEST v, int n);
 
 extern LONGEST gdb_sign_extend (LONGEST value, int bit);
 
+/* Check whether GDB will be able to dump core using the dump_core
+   function.  */
+
+extern int check_can_dump_core (void);
+
+/* Print a warning that we cannot dump core.  */
+
+extern void warn_cant_dump_core (const char *reason);
+
+/* 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 a72f3bd..0f25436 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
@@ -615,8 +615,8 @@ dump_core (void)
 /* Check whether GDB will be able to dump core using the dump_core
    function.  */
 
-static int
-can_dump_core (const char *reason)
+int
+check_can_dump_core (void)
 {
 #ifdef HAVE_GETRLIMIT
   struct rlimit rlim;
@@ -626,18 +626,37 @@ can_dump_core (const char *reason)
     return 1;
 
   if (rlim.rlim_max == 0)
-    {
-      fprintf_unfiltered (gdb_stderr,
-			  _("%s\nUnable to dump core, use `ulimit -c"
-			    " unlimited' before executing GDB next time.\n"),
-			  reason);
-      return 0;
-    }
+    return 0;
 #endif /* HAVE_GETRLIMIT */
 
   return 1;
 }
 
+/* Print a warning that we cannot dump core.  */
+
+void
+warn_cant_dump_core (const char *reason)
+{
+  fprintf_unfiltered (gdb_stderr,
+		      _("%s\nUnable to dump core, use `ulimit -c"
+			" unlimited' before executing GDB next time.\n"),
+		      reason);
+}
+
+/* Check whether GDB will be able to dump core using the dump_core
+   function, and print a warning if we cannot.  */
+
+static int
+check_can_dump_core_warn (const char *reason)
+{
+  int can_dump_core = check_can_dump_core ();
+
+  if (!can_dump_core)
+    warn_cant_dump_core (reason);
+
+  return can_dump_core;
+}
+
 /* Allow the user to configure the debugger behavior with respect to
    what to do when an internal problem is detected.  */
 
@@ -756,7 +775,7 @@ internal_vproblem (struct internal_problem *problem,
 
   if (problem->should_dump_core == internal_problem_ask)
     {
-      if (!can_dump_core (reason))
+      if (!check_can_dump_core_warn (reason))
 	dump_core_p = 0;
       else
 	{
@@ -767,7 +786,7 @@ internal_vproblem (struct internal_problem *problem,
 	}
     }
   else if (problem->should_dump_core == internal_problem_yes)
-    dump_core_p = can_dump_core (reason);
+    dump_core_p = check_can_dump_core_warn (reason);
   else if (problem->should_dump_core == internal_problem_no)
     dump_core_p = 0;
   else

Re: [PATCH 2/3 v4] Refactor and expose core-dumping functionality

[Doug Evans]

On Thu, Jun 5, 2014 at 6:03 AM, Gary Benson <gbenson@redhat.com> wrote:
> This patch exposes the functions that dump core outside utils.c.
> The function can_dump_core has been split into two new functions,
> check_can_dump_core and warn_cant_dump_core so that the check and
> the printed warning can be separated.  A new function
> check_can_dump_core_warn replaces the original can_dump_core.
>
>
> gdb/
> 2014-06-05  Gary Benson  <gbenson@redhat.com>
>
>         * utils.h (check_can_dump_core): New declaration.
>         (warn_cant_dump_core): Likewise.
>         (dump_core): Likewise.
>         * utils.c (dump_core): Made nonstatic.
>         (can_dump_core): Removed function.
>         (check_can_dump_core): New function.
>         (warn_cant_dump_core): Likewise.
>         (check_can_dump_core_warn): Likewise.
>         (internal_vproblem): Replace calls to can_dump_core with
>         calls to check_can_dump_core_warn.

check_can_dump_core feels a bit clumsy over the original can_dump_core.
can_dump_core (or can_dump_core_p) reads better to me.
[And now it does what it says it does, without the side-effect of the printf.]

Not sure if I'd rename check_can_dump_core_warn (or delete it).

Re: [PATCH 2/3 v4] Refactor and expose core-dumping functionality

[Gary Benson]

Doug Evans wrote:
> On Thu, Jun 5, 2014 at 6:03 AM, Gary Benson <gbenson@redhat.com> wrote:
> > This patch exposes the functions that dump core outside utils.c.
> > The function can_dump_core has been split into two new functions,
> > check_can_dump_core and warn_cant_dump_core so that the check and
> > the printed warning can be separated.  A new function
> > check_can_dump_core_warn replaces the original can_dump_core.
> 
> check_can_dump_core feels a bit clumsy over the original
> can_dump_core.  can_dump_core (or can_dump_core_p) reads better to
> me.  [And now it does what it says it does, without the side-effect
> of the printf.]
> 
> Not sure if I'd rename check_can_dump_core_warn (or delete it).

Ok, I renamed check_can_dump_core back to the original can_dump_core,
and I renamed check_can_dump_core_warn as can_dump_core_warn (it's
called twice, so there would be duplication if I removed it).

I won't post another patch series for such a small change but the
updated patches are in my github (http://tinyurl.com/dmcc-v4-2 and
http://tinyurl.com/dmcc-v4-3) if you would like to take a look.
Patch 2 is the important one, patch 3 only has a couple of lines
changed.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 3/3 v4] 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.


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

	* configure.ac [AC_CHECK_FUNCS] <sigaltstack>: New check.
	* configure: Regenerate.
	* config.in: Likewise.
	* main.c (signal.h): New include.
	(setup_alternate_signal_stack): New function.
	(captured_main): Call the above.
	* 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_attempt_core_dump): Likewise.
	(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-05  Gary Benson  <gbenson@redhat.com>

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

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 903f378..f41ba2f 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1328,7 +1328,7 @@ AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid getgid \
 		sigaction sigprocmask sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid lstat \
-		ptrace64])
+		ptrace64 sigaltstack])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/configure b/gdb/configure
index 56c92d3..e23a58f 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -10507,7 +10507,7 @@ for ac_func in canonicalize_file_name realpath getrusage getuid getgid \
 		sigaction sigprocmask sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid lstat \
-		ptrace64
+		ptrace64 sigaltstack
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/config.in b/gdb/config.in
index cd4ce92..8585b49 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -366,6 +366,9 @@
 /* Define to 1 if you have the `sigaction' function. */
 #undef HAVE_SIGACTION
 
+/* Define to 1 if you have the `sigaltstack' function. */
+#undef HAVE_SIGALTSTACK
+
 /* Define to 1 if you have the <signal.h> header file. */
 #undef HAVE_SIGNAL_H
 
diff --git a/gdb/main.c b/gdb/main.c
index a9fc378..7031cc3 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -45,6 +45,7 @@
 
 #include "filenames.h"
 #include "filestuff.h"
+#include <signal.h>
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
@@ -288,6 +289,27 @@ get_init_files (const char **system_gdbinit,
   *local_gdbinit = localinit;
 }
 
+/* Try to set up an alternate signal stack for SIGSEGV handlers.
+   This allows us to handle SIGSEGV signals generated when the
+   normal process stack is exhausted.  If this stack is not set
+   up (sigaltstack is unavailable or fails) and a SIGSEGV is
+   generated when the normal stack is exhausted then the program
+   will behave as though no SIGSEGV handler was installed.  */
+
+static void
+setup_alternate_signal_stack (void)
+{
+#ifdef HAVE_SIGALTSTACK
+  stack_t ss;
+
+  ss.ss_sp = xmalloc (SIGSTKSZ);
+  ss.ss_size = SIGSTKSZ;
+  ss.ss_flags = 0;
+
+  sigaltstack(&ss, NULL);
+#endif
+}
+
 /* Call command_loop.  If it happens to return, pass that through as a
    non-zero return status.  */
 
@@ -785,6 +807,9 @@ captured_main (void *data)
       quiet = 1;
   }
 
+  /* Try to set up an alternate signal stack for SIGSEGV handlers.  */
+  setup_alternate_signal_stack ();
+
   /* Initialize all files.  Give the interpreter a chance to take
      control of the console via the deprecated_init_ui_hook ().  */
   gdb_init (gdb_program_name);
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 91533e8..f4dde70 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,140 @@ cp_lookup_rtti_type (const char *name, struct block *block)
   return rtti_type;
 }
 
+#ifdef 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;
+
+/* If nonzero, attempt to dump core from the signal handler.  */
+
+static int gdb_demangle_attempt_core_dump = 1;
+
+/* Signal handler for gdb_demangle.  */
+
+static void
+gdb_demangle_signal_handler (int signo)
+{
+  if (gdb_demangle_attempt_core_dump)
+    {
+      if (fork () == 0)
+	dump_core ();
+
+      gdb_demangle_attempt_core_dump = 0;
+    }
+
+  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;
+
+#ifdef HAVE_WORKING_FORK
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  struct sigaction sa, old_sa;
+#else
+  void (*ofunc) ();
+#endif
+  static int can_dump_core = -1;
+
+  if (can_dump_core == -1)
+    {
+      can_dump_core = check_can_dump_core ();
+
+      if (!can_dump_core)
+	gdb_demangle_attempt_core_dump = 0;
+    }
+
+  if (catch_demangler_crashes)
+    {
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+      sa.sa_handler = gdb_demangle_signal_handler;
+      sigemptyset (&sa.sa_mask);
+      sa.sa_flags = SA_ONSTACK;
+      sigaction (SIGSEGV, &sa, &old_sa);
+#else
+      ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
+#endif
+
+      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+    }
+#endif
+
+  if (crash_signal == 0)
+    result = bfd_demangle (NULL, name, options);
+
+#ifdef 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)
+	    {
+	      char *dmw_msg;
+	      struct cleanup *back_to;
+
+	      dmw_msg = xstrprintf (_("unable to demangle '%s' "
+				      "(demangler failed with signal %d)"),
+				    name, crash_signal);
+	      back_to = make_cleanup (xfree, dmw_msg);
+
+	      if (!can_dump_core)
+		{
+		  char *cdc_msg = xstrprintf ("%s:%d: %s: %s",
+					      __FILE__, __LINE__,
+					      "demangler-warning",
+					      dmw_msg);
+
+		  make_cleanup (xfree, cdc_msg);
+		  warn_cant_dump_core (cdc_msg);
+		}
+
+	      demangler_warning (__FILE__, __LINE__, "%s", dmw_msg);
+
+	      do_cleanups (back_to);
+
+	      error_reported = 1;
+	    }
+
+	  result = NULL;
+	}
+    }
+#endif
+
+  return result;
 }
 
 /* Don't allow just "maintenance cplus".  */
@@ -1585,4 +1714,17 @@ _initialize_cp_support (void)
 Usage: info vtbl EXPRESSION\n\
 Evaluate EXPRESSION and display the virtual function table for the\n\
 resulting object."));
+
+#ifdef 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 9f7fa18..242117b 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 3/3 v4] Demangler crash handler

[Florian Weimer]

* Gary Benson:

> +      sigaction (SIGSEGV, &sa, &old_sa);

> +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);

> +      sigaction (SIGSEGV, &old_sa, NULL);

That's quite a bit of additional work for each demangler invocation.
Is this visible with things like tab completion?

Re: [PATCH 3/3 v4] Demangler crash handler

[Gary Benson]

Florian Weimer wrote:
> * Gary Benson:
> > +      sigaction (SIGSEGV, &sa, &old_sa);
> > +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
> > +      sigaction (SIGSEGV, &old_sa, NULL);
> 
> That's quite a bit of additional work for each demangler invocation.
> Is this visible with things like tab completion?

I tested this a while back: it's not noticable.

FWIW I did time gdb -nx -batch \
                /usr/lib64/libreoffice/program/soffice.bin \
                -ex "start" -ex "complete b" > /dev/null

That invokes the demangler some 3,100,000 times.  I don't remember
the exact times but they were of the order of 50s in both cases.

Thanks,
Gary		    

-- 
http://gbenson.net/

Re: [PATCH 3/3 v4] Demangler crash handler

[Florian Weimer]

* Gary Benson:

> Florian Weimer wrote:
>> * Gary Benson:
>> > +      sigaction (SIGSEGV, &sa, &old_sa);
>> > +      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
>> > +      sigaction (SIGSEGV, &old_sa, NULL);
>> 
>> That's quite a bit of additional work for each demangler invocation.
>> Is this visible with things like tab completion?
>
> I tested this a while back: it's not noticable.
>
> FWIW I did time gdb -nx -batch \
>                 /usr/lib64/libreoffice/program/soffice.bin \
>                 -ex "start" -ex "complete b" > /dev/null
>
> That invokes the demangler some 3,100,000 times.  I don't remember
> the exact times but they were of the order of 50s in both cases.

Good to know, thanks.

Re: [PATCH 3/3 v4] Demangler crash handler

[Andrew Burgess]

On 05/06/2014 2:03 PM, Gary Benson wrote:

> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 91533e8..f4dde70 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c

> +
> +/* Signal handler for gdb_demangle.  */
> +
> +static void
> +gdb_demangle_signal_handler (int signo)
> +{
> +  if (gdb_demangle_attempt_core_dump)
> +    {
> +      if (fork () == 0)
> +	dump_core ();

This worries me a little, when a problem case occurs gdb will dump 
core regardless of the users ulimit setting, without first asking
the user, and doesn't tell the user that a core file was created.

This feels quite unexpected behaviour to me, especially the bit about
disregarding the ulimit setting without first asking for permission.

Catching the crash feels like a good idea, but I'd prefer that gdb ask
before circumventing the ulimit and dumping core.  Alternatively we
could just not dump core from gdb, report the bad symbol and let the
user file a bug.  With the demangler being so deterministic it should
be possible to reproduce, if not, then we just ask the user to turn
off the crash catch, adjust their ulimit (like we would with any other
gdb SEGV crash), and rerun the test.

If we really want to create the core file by default, but aren't going
to ask, then I'd propose we honour the ulimit setting, and make sure
that the user is told that a core file was just written.

Thanks,
Andrew

Re: [PATCH 3/3 v4] Demangler crash handler

[Gary Benson]

Andrew Burgess wrote:
> On 05/06/2014 2:03 PM, Gary Benson wrote:
> > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> > index 91533e8..f4dde70 100644
> > --- a/gdb/cp-support.c
> > +++ b/gdb/cp-support.c
> 
> > +
> > +/* Signal handler for gdb_demangle.  */
> > +
> > +static void
> > +gdb_demangle_signal_handler (int signo)
> > +{
> > +  if (gdb_demangle_attempt_core_dump)
> > +    {
> > +      if (fork () == 0)
> > +	dump_core ();
> 
> This worries me a little, when a problem case occurs gdb will dump
> core regardless of the users ulimit setting, without first asking
> the user, and doesn't tell the user that a core file was created.
> 
> This feels quite unexpected behaviour to me, especially the bit
> about disregarding the ulimit setting without first asking for
> permission.
> 
> Catching the crash feels like a good idea, but I'd prefer that gdb
> ask before circumventing the ulimit and dumping core.

This part of the same patch:

+  if (core_dump_allowed == -1)
+    {
+      core_dump_allowed = can_dump_core ();
+
+      if (!core_dump_allowed)
+        gdb_demangle_attempt_core_dump = 0;
+    }

calls this:

  int
  can_dump_core (void)
  {
  #ifdef HAVE_GETRLIMIT
    struct rlimit rlim;
  
    /* Be quiet and assume we can dump if an error is returned.  */
    if (getrlimit (RLIMIT_CORE, &rlim) != 0)
      return 1;
  
    if (rlim.rlim_max == 0)
      return 0;
  #endif /* HAVE_GETRLIMIT */
  
    return 1;
  }
		  
which inhibits the core dump if the user's ulimit is 0.

> Alternatively we could just not dump core from gdb, report the bad
> symbol and let the user file a bug.  With the demangler being so
> deterministic it should be possible to reproduce, if not, then we
> just ask the user to turn off the crash catch, adjust their ulimit
> (like we would with any other gdb SEGV crash), and rerun the test.

That was and is my preferred solution, but Mark Kettenis indicated
that he would not accept the patch unless a meaningful core file was
created.

> If we really want to create the core file by default, but aren't
> going to ask, then I'd propose we honour the ulimit setting, and
> make sure that the user is told that a core file was just written.

The problem with asking is that you'd have to ask within the signal
handler, and no code that prints to the screen is safe to call from
within a signal handler.

Even indicating that a core file was written is probably impossible:
you just have to abort and hope for the best.  The nearest I could
do is set a flag in the signal handler and have the code it returns
to print "Attempting to dump core" or some such thing.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 3/3 v4] Demangler crash handler

[Andrew Burgess]

On 09/06/2014 10:01 AM, Gary Benson wrote:
> Andrew Burgess wrote:
>> On 05/06/2014 2:03 PM, Gary Benson wrote:
>>> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
>>> index 91533e8..f4dde70 100644
>>> --- a/gdb/cp-support.c
>>> +++ b/gdb/cp-support.c
>>
>>> +
>>> +/* Signal handler for gdb_demangle.  */
>>> +
>>> +static void
>>> +gdb_demangle_signal_handler (int signo)
>>> +{
>>> +  if (gdb_demangle_attempt_core_dump)
>>> +    {
>>> +      if (fork () == 0)
>>> +	dump_core ();
>>
>> This worries me a little, when a problem case occurs gdb will dump
>> core regardless of the users ulimit setting, without first asking
>> the user, and doesn't tell the user that a core file was created.
>>
>> This feels quite unexpected behaviour to me, especially the bit
>> about disregarding the ulimit setting without first asking for
>> permission.
>>
>> Catching the crash feels like a good idea, but I'd prefer that gdb
>> ask before circumventing the ulimit and dumping core.
> 
> This part of the same patch:
> 
> +  if (core_dump_allowed == -1)
> +    {
> +      core_dump_allowed = can_dump_core ();
> +
> +      if (!core_dump_allowed)
> +        gdb_demangle_attempt_core_dump = 0;
> +    }
> 
> calls this:
> 
>   int
>   can_dump_core (void)
>   {
>   #ifdef HAVE_GETRLIMIT
>     struct rlimit rlim;
>   
>     /* Be quiet and assume we can dump if an error is returned.  */
>     if (getrlimit (RLIMIT_CORE, &rlim) != 0)
>       return 1;
>   
>     if (rlim.rlim_max == 0)
>       return 0;
>   #endif /* HAVE_GETRLIMIT */
>   
>     return 1;
>   }
> 		  
> which inhibits the core dump if the user's ulimit is 0.

Ahh, yes I see.

So the problem here is this function is geared towards the /old/ use of the function
where we are about to ask the user if we should dump core.  For that, this function 
was correct, we check the hard limit of the resource.  If the hard limit is high then
we ask the user, and dump core.

However, in doing so we circumvent the soft limit rlim.rlim_cur.  So I think my point
still stands.  The user has said "no core files please", and we create one without 
asking.  If we must go down this road then I think we need two functions to check
the two different limits.

>> Alternatively we could just not dump core from gdb, report the bad
>> symbol and let the user file a bug.  With the demangler being so
>> deterministic it should be possible to reproduce, if not, then we
>> just ask the user to turn off the crash catch, adjust their ulimit
>> (like we would with any other gdb SEGV crash), and rerun the test.
> 
> That was and is my preferred solution, but Mark Kettenis indicated
> that he would not accept the patch unless a meaningful core file was
> created.

I don't understand that position, but I'd hope he'd agree that we
should respect the user ulimit over creating a core file...

> 
>> If we really want to create the core file by default, but aren't
>> going to ask, then I'd propose we honour the ulimit setting, and
>> make sure that the user is told that a core file was just written.
> 
> The problem with asking is that you'd have to ask within the signal
> handler, and no code that prints to the screen is safe to call from
> within a signal handler.

Indeed.  I did wonder about some horrible synchronisation scheme where
the "master" gdb process queries the user then signals the fork()ed 
child to indicate if it should dump core or not .... but it felt like
huge overkill.

> Even indicating that a core file was written is probably impossible:
> you just have to abort and hope for the best.  The nearest I could
> do is set a flag in the signal handler and have the code it returns
> to print "Attempting to dump core" or some such thing.

I think an "attempting ..." style message would be enough, the 
gdb_demangle_attempt_core_dump flag could be used to indicate
if we've tried to dump core or not.

Thanks,
Andrew

Re: [PATCH 3/3 v4] Demangler crash handler

[Gary Benson]

Andrew Burgess wrote:
> On 09/06/2014 10:01 AM, Gary Benson wrote:
> > Andrew Burgess wrote:
> > > On 05/06/2014 2:03 PM, Gary Benson wrote:
> > > > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> > > > index 91533e8..f4dde70 100644
> > > > --- a/gdb/cp-support.c
> > > > +++ b/gdb/cp-support.c
> > >
> > > > +
> > > > +/* Signal handler for gdb_demangle.  */
> > > > +
> > > > +static void
> > > > +gdb_demangle_signal_handler (int signo)
> > > > +{
> > > > +  if (gdb_demangle_attempt_core_dump)
> > > > +    {
> > > > +      if (fork () == 0)
> > > > +	dump_core ();
> > >
> > > This worries me a little, when a problem case occurs gdb will
> > > dump core regardless of the users ulimit setting, without first
> > > asking the user, and doesn't tell the user that a core file was
> > > created.
> > >
> > > This feels quite unexpected behaviour to me, especially the bit
> > > about disregarding the ulimit setting without first asking for
> > > permission.
> > >
> > > Catching the crash feels like a good idea, but I'd prefer that
> > > gdb ask before circumventing the ulimit and dumping core.
> > 
> > This part of the same patch:
> > 
> > +  if (core_dump_allowed == -1)
> > +    {
> > +      core_dump_allowed = can_dump_core ();
> > +
> > +      if (!core_dump_allowed)
> > +        gdb_demangle_attempt_core_dump = 0;
> > +    }
> > 
> > calls this:
> > 
> >   int
> >   can_dump_core (void)
> >   {
> >   #ifdef HAVE_GETRLIMIT
> >     struct rlimit rlim;
> >   
> >     /* Be quiet and assume we can dump if an error is returned.  */
> >     if (getrlimit (RLIMIT_CORE, &rlim) != 0)
> >       return 1;
> >   
> >     if (rlim.rlim_max == 0)
> >       return 0;
> >   #endif /* HAVE_GETRLIMIT */
> >   
> >     return 1;
> >   }
> > 		  
> > which inhibits the core dump if the user's ulimit is 0.
> 
> Ahh, yes I see.
> 
> So the problem here is this function is geared towards the /old/ use
> of the function where we are about to ask the user if we should dump
> core.  For that, this function was correct, we check the hard limit
> of the resource.  If the hard limit is high then we ask the user,
> and dump core.
> 
> However, in doing so we circumvent the soft limit rlim.rlim_cur.  So
> I think my point still stands.  The user has said "no core files
> please", and we create one without asking.  If we must go down this
> road then I think we need two functions to check the two different
> limits.

Ah, I didn't realize the code in dump_core was to override the user's
soft limit.  I will update the patch.

> > > Alternatively we could just not dump core from gdb, report the
> > > bad symbol and let the user file a bug.  With the demangler
> > > being so deterministic it should be possible to reproduce, if
> > > not, then we just ask the user to turn off the crash catch,
> > > adjust their ulimit (like we would with any other gdb SEGV
> > > crash), and rerun the test.
> > 
> > That was and is my preferred solution, but Mark Kettenis indicated
> > that he would not accept the patch unless a meaningful core file
> > was created.
> 
> I don't understand that position, but I'd hope he'd agree that we
> should respect the user ulimit over creating a core file...

Yes, this seems reasonable.

> > > If we really want to create the core file by default, but aren't
> > > going to ask, then I'd propose we honour the ulimit setting, and
> > > make sure that the user is told that a core file was just written.
> > 
> > The problem with asking is that you'd have to ask within the signal
> > handler, and no code that prints to the screen is safe to call from
> > within a signal handler.
> 
> Indeed.  I did wonder about some horrible synchronisation scheme
> where the "master" gdb process queries the user then signals the
> fork()ed child to indicate if it should dump core or not .... but
> it felt like huge overkill.

Yeah, I thought down this road too :)

> > Even indicating that a core file was written is probably
> > impossible: you just have to abort and hope for the best.
> > The nearest I could do is set a flag in the signal handler
> > and have the code it returns to print "Attempting to dump
> > core" or some such thing.
> 
> I think an "attempting ..." style message would be enough, the 
> gdb_demangle_attempt_core_dump flag could be used to indicate
> if we've tried to dump core or not.

I will add this to the updated patch.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/3 v4] Demangler crash handler

[Doug Evans]

On Thu, Jun 5, 2014 at 6:01 AM, Gary Benson <gbenson@redhat.com> wrote:
> Hi all,
>
> This patch is an updated version of the demangler crash handler I
> posted yesterday.  The main changes from the previous version are:
>
>  1) All #ifdef SIGSEGV conditionals have been removed.
>
>  2) If the user set "ulimit -c 0" then no core file will be
>     created and a warning will be printed.  This mirrors what
>     internal_error and internal_warning currently do.
>
>  3) A separate signal stack is created for the SIGSEGV handler
>     to allow it to function correctly if the normal process
>     stack overflows.  This signal stack is currently only used
>     by the SIGSEGV handler--all other signal handlers use the
>     normal process stack as before.
>
> Doug requested that I change the patch to emit warnings for every
> demangler crash, not just the first.

Umm, that's not what I said, to be precise, and it was more of a
suggestion for discussion than a request.

> I've not done this, my reason
> being that subsequent failures could have been caused by the first,
> by memory corruption or some leftover state: they could be bogus,
> and could lead to us chasing bugs that don't exist.  I prefer this
> way, but I'm not hung up on it and if it's a blocker for Doug or
> anyone else I will add the extra warnings.

It's not a blocker to me.

Re: [PATCH 0/3 v4] Demangler crash handler

[Gary Benson]

Doug Evans wrote:
> On Thu, Jun 5, 2014 at 6:01 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Doug requested that I change the patch to emit warnings for every
> > demangler crash, not just the first.
> 
> Umm, that's not what I said, to be precise, and it was more of a
> suggestion for discussion than a request.

Ah, I misunderstood.

> 
> > I've not done this, my reason
> > being that subsequent failures could have been caused by the first,
> > by memory corruption or some leftover state: they could be bogus,
> > and could lead to us chasing bugs that don't exist.  I prefer this
> > way, but I'm not hung up on it and if it's a blocker for Doug or
> > anyone else I will add the extra warnings.
> 
> It's not a blocker to me.

Great :)

Thanks,
Gary

-- 
http://gbenson.net/