Re: RFC: implement "catch signal"

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 16 Nov 2012 20:06:40 +0100, Tom Tromey wrote:
[...]
> --- /dev/null
> +++ b/gdb/break-catch-sig.c
> @@ -0,0 +1,496 @@
> +/* Everything about signal catchpoints, for GDB.
> +
> +   Copyright (C) 2011, 2012 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include <ctype.h>
> +#include "breakpoint.h"
> +#include "gdbcmd.h"
> +#include "inferior.h"
> +#include "annotate.h"
> +#include "valprint.h"
> +#include "cli/cli-utils.h"
> +#include "completer.h"
> +
> +#define INTERNAL_SIGNAL(x) ((x) == GDB_SIGNAL_TRAP || (x) == GDB_SIGNAL_INT)
> +
> +typedef enum gdb_signal gdb_signal_type;
> +
> +DEF_VEC_I (gdb_signal_type);
> +
> +/* An instance of this type is used to represent a signal catchpoint.
> +   It includes a "struct breakpoint" as a kind of base class; users
> +   downcast to "struct breakpoint *" when needed.  A breakpoint is
> +   really of this type iff its ops pointer points to
> +   SIGNAL_CATCHPOINT_OPS.  */
> +
> +struct signal_catchpoint
> +{
> +  /* The base class.  */
> +  struct breakpoint base;
> +
> +  /* Signal numbers used for the 'catch signal' feature.  If no signal
> +     has been specified for filtering, its value is NULL.  Otherwise,
> +     it holds a list of all signals to be caught.  The list elements
> +     are allocated with xmalloc.  */
> +  VEC (gdb_signal_type) *signals_to_be_caught;
> +

Missing comment
/* If SIGNALS_TO_BE_CAUGHT is NULL then all the signals are caught,
   INTERNAL_SIGNAL are additionally caught only iff CATCH_ALL.  If
   SIGNALS_TO_BE_CAUGHT is not NULL then CATCH_ALL is undefined.  */

> +  int catch_all;
> +};
> +
> +/* The breakpoint_ops structure to be used in signal catchpoints.  */
> +
> +static struct breakpoint_ops signal_catchpoint_ops;
> +


> +/* An object of this type holds global information about all signal
> +   catchpoints.  */
> +
> +struct signal_catch_info
> +{
> +  /* Count of each signal.  */
> +
> +  unsigned int *counts;
> +};
> +
> +/* Global information about all signal catchpoints.  */
> +
> +static struct signal_catch_info signal_catch_info;

It is a single global variable, I find it overengineered.


[...]
> +/* Implement the "remove_location" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static int
> +signal_catchpoint_remove_location (struct bp_location *bl)
> +{
> +  struct signal_catchpoint *c = (void *) bl->owner;
> +  struct signal_catch_info *info = &signal_catch_info;
> +  int i;
> +  gdb_signal_type iter;
> +
> +  if (c->signals_to_be_caught != NULL)
> +    {
> +      for (i = 0;
> +	   VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> +	   i++)

Here could be > 0 assert.

> +	--info->counts[iter];
> +    }
> +  else
> +    {
> +      for (i = 0; i < GDB_SIGNAL_LAST; ++i)
> +	{
> +	  if (c->catch_all || !INTERNAL_SIGNAL (i))
> +	    ++info->counts[i];

Here should be --.  And possibly an assert.

> +	}
> +    }
> +
> +  signal_catch_update (info->counts);
> +
> +  return 0;
> +}
> +
> +/* Implement the "breakpoint_hit" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static int
> +signal_catchpoint_breakpoint_hit (const struct bp_location *bl,
> +				  struct address_space *aspace,
> +				  CORE_ADDR bp_addr,
> +				  const struct target_waitstatus *ws)
> +{
> +  const struct signal_catchpoint *c = (void *) bl->owner;
> +  gdb_signal_type signal_number;
> +
> +  if (ws->kind != TARGET_WAITKIND_STOPPED)
> +    return 0;
> +
> +  signal_number = ws->value.sig;
> +
> +  /* If we are catching specific signals in this breakpoint, then we
> +     must guarantee that the called signal is the same signal we are
> +     catching.  */
> +  if (c->signals_to_be_caught)
> +    {
> +      int i;
> +      gdb_signal_type iter;
> +
> +      for (i = 0;
> +           VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> +           i++)
> +	if (signal_number == iter)
> +	  break;
> +      /* Not the same.  */
> +      if (!iter)
> +	return 0;
> +    }
> +
> +  return c->catch_all || !INTERNAL_SIGNAL (signal_number);
> +}
> +
> +/* Implement the "print_it" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static enum print_stop_action
> +signal_catchpoint_print_it (bpstat bs)
> +{
> +  struct breakpoint *b = bs->breakpoint_at;
> +  ptid_t ptid;
> +  struct target_waitstatus last;
> +  const char *signal_name;
> +
> +  get_last_target_status (&ptid, &last);
> +
> +  signal_name = gdb_signal_to_name (last.value.sig);

Here could be signal_to_name_or_int as signal number is not separately printed
here.

> +
> +  annotate_catchpoint (b->number);
> +
> +  printf_filtered (_("\nCatchpoint %d (signal %s), "), b->number, signal_name);
> +
> +  return PRINT_SRC_AND_LOC;
> +}
> +
> +/* Implement the "print_one" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static void
> +signal_catchpoint_print_one (struct breakpoint *b,
> +			     struct bp_location **last_loc)
> +{
> +  struct signal_catchpoint *c = (void *) b;
> +  struct value_print_options opts;
> +  struct ui_out *uiout = current_uiout;
> +
> +  get_user_print_options (&opts);

Empty line before comment.

> +  /* Field 4, the address, is omitted (which makes the columns
> +     not line up too nicely with the headers, but the effect
> +     is relatively readable).  */
> +  if (opts.addressprint)
> +    ui_out_field_skip (uiout, "addr");
> +  annotate_field (5);
> +
> +  if (c->signals_to_be_caught
> +      && VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1)
> +    ui_out_text (uiout, "signals \"");
> +  else
> +    ui_out_text (uiout, "signal \"");
> +
> +  if (c->signals_to_be_caught)
> +    {
> +      int i;
> +      gdb_signal_type iter;
> +      char *text = xstrdup ("");
> +
> +      for (i = 0;
> +           VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> +           i++)
> +        {
> +          char *x = text;
> +	  const char *name = signal_to_name_or_int (iter);
> +
> +	  text = xstrprintf (i > 0 ? "%s, %s" : "%s%s", text, name);
> +	  xfree (x);
> +        }
> +      ui_out_field_string (uiout, "what", text);

From the MI point of view ", " delimited signal names in a single field are
ugly.

> +      xfree (text);
> +    }
> +  else
> +    ui_out_field_string (uiout, "what",
> +			 c->catch_all ? "<any signal>" : "<standard signals>");
> +  ui_out_text (uiout, "\" ");
> +}
> +
> +/* Implement the "print_mention" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static void
> +signal_catchpoint_print_mention (struct breakpoint *b)
> +{
> +  struct signal_catchpoint *c = (void *) b;
> +
> +  if (c->signals_to_be_caught)
> +    {
> +      int i;
> +      gdb_signal_type iter;
> +
> +      if (VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1)
> +        printf_filtered (_("Catchpoint %d (signals"), b->number);
> +      else
> +        printf_filtered (_("Catchpoint %d (signal"), b->number);
> +
> +      for (i = 0;
> +           VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> +           i++)
> +        {
> +	  const char *name = signal_to_name_or_int (iter);
> +
> +	  printf_filtered (" %s", name);

Sometimes the delimited is " " and sometimes ", ", it may be for example less
convenient for copy-pastes.

(gdb) catch signal SIGQUIT SIGCHLD SIGINT
Catchpoint 1 (signals SIGQUIT SIGCHLD SIGINT)
(gdb) info breakpoints 
Num     Type           Disp Enb Address            What
1       catchpoint     keep y                      signals "SIGQUIT, SIGCHLD, SIGINT" 


> +        }
> +      printf_filtered (")");
> +    }
> +  else if (c->catch_all)
> +    printf_filtered (_("Catchpoint %d (any signal)"), b->number);
> +  else
> +    printf_filtered (_("Catchpoint %d (standard signals)"), b->number);
> +}
> +
> +/* Implement the "print_recreate" breakpoint_ops method for signal
> +   catchpoints.  */
> +
> +static void
> +signal_catchpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
> +{
> +  struct signal_catchpoint *c = (void *) b;
> +
> +  fprintf_unfiltered (fp, "catch signal");
> +
> +  if (c->signals_to_be_caught)
> +    {
> +      int i;
> +      gdb_signal_type iter;
> +
> +      for (i = 0;
> +           VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter);
> +           i++)
> +	fprintf_unfiltered (fp, " %s", signal_to_name_or_int (iter));
> +    }
> +  else if (c->catch_all)
> +    fprintf_unfiltered (fp, " all");
> +}
[...]
> +void
> +_initialize_break_catch_sig (void)
> +{
> +  initialize_signal_catchpoint_ops ();
> +
> +  signal_catch_info.counts = XCNEWVEC (unsigned int, GDB_SIGNAL_LAST);
> +
> +  add_catch_command ("signal", _("\
> +Catch signals by their names and/or numbers.\n\
> +Usage: catch signal [[NAME|NUMBER]...|all]\n\

For example:
	Usage: catch signal [[NAME|NUMBER] [NAME|NUMBER]...|all]\n\

to see there the delimiter is " " and not ", ".  Maybe not needed.


> +Arguments say which signals to catch.  If no arguments\n\
> +are given, every \"normal\" signal will be caught.\n\
> +The argument \"all\" means to also catch signals used by GDB.\n\
> +Arguments, if given, should be one or more signal names\n\
> +(if your system supports that), or signal numbers."),
> +		     catch_signal_command,
> +		     signal_completer,
> +		     CATCH_PERMANENT,
> +		     CATCH_TEMPORARY);
> +}
[...]
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -318,6 +318,9 @@ static unsigned char *signal_stop;
>  static unsigned char *signal_print;
>  static unsigned char *signal_program;
>  
> +/* Table of signals that are registered with "catch signal".  */

Not descriptive enough:
/* Array of GDB_SIGNAL_LAST size with the count how many each signal is
   registered by "catch signal".  */

Although then also for signal_stop+signal_print+signal_program above:

/* Tables of how to react to signals; the user sets them.  */
->
/* Array of GDB_SIGNAL_LAST size with boolean value whether to "handle stop",
   "handle print" or "handle pass" respectively.  */


> +static unsigned int *signal_catch;
> +
>  /* Table of signals that the target may silently handle.
>     This is automatically determined from the flags above,
>     and simply cached here.  */
[...]
> @@ -6251,6 +6255,18 @@ signal_pass_update (int signo, int state)
>    return ret;
>  }
>  
> +/* Update the global 'signal_catch' from INFO and notify the
> +   target.  */
> +
> +void
> +signal_catch_update (unsigned int *info)
> +{
> +  memcpy (signal_catch, info, GDB_SIGNAL_LAST * sizeof (unsigned int));
> +  signal_cache_update (-1);
> +  target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);

I do not see why to call target_program_signals here.

> +  target_program_signals ((int) GDB_SIGNAL_LAST, signal_program);
> +}
> +
>  static void
>  sig_print_header (void)
>  {
[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/catch-signal.c
[...]
> +int
> +main ()
> +{
> +  signal (SIGHUP, handle);
> +  signal (SIGUSR1, SIG_IGN);
> +
> +  kill (getpid (), SIGHUP);	/* first HUP */

Why not raise ().

> +
> +  kill (getpid (), SIGHUP);	/* second HUP */
> +
> +  kill (getpid (), SIGHUP);	/* third HUP */
> +
> +  kill (getpid (), SIGHUP);	/* fourth HUP */
> +}
> +
[...]


Thanks,
Jan