From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17579 invoked by alias); 2 Dec 2012 09:38:30 -0000 Received: (qmail 17564 invoked by uid 22791); 2 Dec 2012 09:38:29 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CP X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 02 Dec 2012 09:38:15 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qB29cFER004159 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 2 Dec 2012 04:38:15 -0500 Received: from host2.jankratochvil.net (ovpn-116-104.ams2.redhat.com [10.36.116.104]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qB29c8A8001460 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sun, 2 Dec 2012 04:38:10 -0500 Date: Sun, 02 Dec 2012 09:38:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: RFC: implement "catch signal" Message-ID: <20121202093807.GA21883@host2.jankratochvil.net> References: <874nkpv03j.fsf@fleche.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <874nkpv03j.fsf@fleche.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00009.txt.bz2 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 . */ > + > +#include "defs.h" > +#include "arch-utils.h" > +#include > +#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 ? "" : ""); > + 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