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

[Doug Evans <xdje42@gmail.com>]

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;
>  }