From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2516 invoked by alias); 4 Nov 2008 17:57:13 -0000 Received: (qmail 2403 invoked by uid 22791); 4 Nov 2008 17:57:12 -0000 X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 04 Nov 2008 17:56:30 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id mA4HuQCV025616; Tue, 4 Nov 2008 12:56:26 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id mA4HuPPw016361; Tue, 4 Nov 2008 12:56:25 -0500 Received: from opsy.redhat.com (vpn-14-82.rdu.redhat.com [10.11.14.82]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id mA4HuOoW014695; Tue, 4 Nov 2008 12:56:24 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 67127508089; Tue, 4 Nov 2008 10:56:23 -0700 (MST) To: =?utf-8?Q?S=C3=A9rgio?= Durigan =?utf-8?Q?J=C3=BAnior?= Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part References: <1225773079.24532.52.camel@miki> From: Tom Tromey Reply-To: tromey@redhat.com X-Attribution: Tom Date: Tue, 04 Nov 2008 17:57:00 -0000 In-Reply-To: <1225773079.24532.52.camel@miki> (=?utf-8?Q?=22S=C3=A9rgio?= Durigan =?utf-8?Q?J=C3=BAnior=22's?= message of "Tue\, 04 Nov 2008 02\:31\:19 -0200") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: 2008-11/txt/msg00040.txt.bz2 >>>>> "S=C3=A9rgio" =3D=3D S=C3=A9rgio Durigan J=C3=BAnior writes: S=C3=A9rgio> +/* The maximum number of arguments the user can provide to S=C3=A9rgio> + the 'catch syscall' command. */ S=C3=A9rgio> +#define MAX_CATCH_SYSCALL_ARGS 10 Do we need a maximum? GNU style is not to have them. S=C3=A9rgio> + printf_filtered (_("'%s ()'"), syscall_name); I don't think the '()' is needed here. S=C3=A9rgio> +static void S=C3=A9rgio> +print_mention_catch_syscall (struct breakpoint *b) S=C3=A9rgio> +{ S=C3=A9rgio> + if (b->syscall_to_be_caught !=3D CATCHING_ANY_SYSCALL) S=C3=A9rgio> + printf_filtered (_("Catchpoint %d (syscall '%s ()')"), Same here. S=C3=A9rgio> +/* Splits the argument using space as delimiter. S=C3=A9rgio> + Returns the number of args. */ S=C3=A9rgio> +static int S=C3=A9rgio> +catch_syscall_split_args (char *arg, int *syscalls_numbers) S=C3=A9rgio> + memset ((void *) cur_name, '\0', 128 * sizeof (char)); I don't think this is needed. Also, sizeof(char)=3D=3D1 by definition. S=C3=A9rgio> + for (i =3D 0; out =3D=3D 0; i++) S=C3=A9rgio> + { S=C3=A9rgio> + c =3D *arg_p; S=C3=A9rgio> + cur_name[i] =3D c; S=C3=A9rgio> + if (isspace (c) || c =3D=3D '\0') S=C3=A9rgio> + { S=C3=A9rgio> + out =3D 1; S=C3=A9rgio> + cur_name[i] =3D '\0'; I'd say, remove "out", make it an infinite loop, and use a 'break' in the exit condition. S=C3=A9rgio> +static void S=C3=A9rgio> +catch_syscall_command_1 (char *arg, int from_tty, struct cmd_= list_element *command) I think you need a line break before the 'struct' here. S=C3=A9rgio> + for (i =3D 0; i < nargs; i++) S=C3=A9rgio> + create_syscall_event_catchpoint (tempflag, syscalls_n= umbers[i], S=C3=A9rgio> + &catch_syscall_break= point_ops); This makes a separate catchpoint for each argument to "catch syscall". I think it would be more useful to make a single catchpoint. A single catchpoint gives the user a way to set commands, conditions, etc, for a whole range of syscalls at once. It is analogous, I think, to having a breakpoint with multiple locations. What do you think of that? It would mean some changes in the logic and some changes in the data structure -- but nothing too major. Usually a catchpoint would have a small number of syscalls, so I'd say that just using a linked list would be fine. S=C3=A9rgio> +/* Complete syscalls names. Used by "catch syscall". */ S=C3=A9rgio> +char ** S=C3=A9rgio> +catch_syscall_completer (char *text, char *word) S=C3=A9rgio> +{ S=C3=A9rgio> + const char **list =3D S=C3=A9rgio> + gdbarch_get_syscalls_names (current_gdbarch); S=C3=A9rgio> + return (list =3D=3D NULL) ? NULL : complete_on_enum (list, = text, word); S=C3=A9rgio> +} I think you should just put this in breakpoint.c and make it static. My reasoning is that it is likely that only this one particular command will need this completion function. Tom