From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6055 invoked by alias); 7 Nov 2008 03:39:26 -0000 Received: (qmail 5957 invoked by uid 22791); 7 Nov 2008 03:39:24 -0000 X-Spam-Check-By: sourceware.org Received: from igw3.br.ibm.com (HELO igw3.br.ibm.com) (32.104.18.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 07 Nov 2008 03:38:38 +0000 Received: from d24relay01.br.ibm.com (unknown [9.8.31.16]) by igw3.br.ibm.com (Postfix) with ESMTP id D6E8139017F for ; Fri, 7 Nov 2008 01:36:57 -0200 (BRDT) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.18.232.47]) by d24relay01.br.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id mA73cDHJ4263948 for ; Fri, 7 Nov 2008 00:38:13 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id mA73cZvW022341 for ; Fri, 7 Nov 2008 01:38:35 -0200 Received: from [9.8.9.146] ([9.8.9.146]) by d24av02.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id mA73cYQE022334; Fri, 7 Nov 2008 01:38:35 -0200 Subject: Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part From: =?ISO-8859-1?Q?S=E9rgio?= Durigan =?ISO-8859-1?Q?J=FAnior?= To: tromey@redhat.com Cc: gdb-patches@sourceware.org In-Reply-To: References: <1225773079.24532.52.camel@miki> Content-Type: text/plain; charset=iso-8859-1 Date: Fri, 07 Nov 2008 03:39:00 -0000 Message-Id: <1226029103.32321.96.camel@miki> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 8bit 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: 2008-11/txt/msg00115.txt.bz2 Hey Tom! On Tue, 2008-11-04 at 10:56 -0700, Tom Tromey wrote: > >>>>> "Sérgio" == Sérgio Durigan Júnior writes: > > Sérgio> +/* The maximum number of arguments the user can provide to > Sérgio> + the 'catch syscall' command. */ > Sérgio> +#define MAX_CATCH_SYSCALL_ARGS 10 > > Do we need a maximum? GNU style is not to have them. I don't know if we need :-). I just decided to put it there :-P. But of course I can take it off. > > Sérgio> + printf_filtered (_("'%s ()'"), syscall_name); > > I don't think the '()' is needed here. OK. > Sérgio> +static void > Sérgio> +print_mention_catch_syscall (struct breakpoint *b) > Sérgio> +{ > Sérgio> + if (b->syscall_to_be_caught != CATCHING_ANY_SYSCALL) > Sérgio> + printf_filtered (_("Catchpoint %d (syscall '%s ()')"), > > Same here. Roger that. > Sérgio> +/* Splits the argument using space as delimiter. > Sérgio> + Returns the number of args. */ > Sérgio> +static int > Sérgio> +catch_syscall_split_args (char *arg, int *syscalls_numbers) > > Sérgio> + memset ((void *) cur_name, '\0', 128 * sizeof (char)); > > I don't think this is needed. > Also, sizeof(char)==1 by definition. Yeah, I know hehe. It just happens that I like to be "explicit", so you'll likely find "sizeof (char)" in every code I make :-)... But there's no problem for me to take it off. > Sérgio> + for (i = 0; out == 0; i++) > Sérgio> + { > Sérgio> + c = *arg_p; > Sérgio> + cur_name[i] = c; > Sérgio> + if (isspace (c) || c == '\0') > Sérgio> + { > Sérgio> + out = 1; > Sérgio> + cur_name[i] = '\0'; > > I'd say, remove "out", make it an infinite loop, and use a 'break' in > the exit condition. Right. > Sérgio> +static void > Sérgio> +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. Forgot about that. Sorry. > Sérgio> + for (i = 0; i < nargs; i++) > Sérgio> + create_syscall_event_catchpoint (tempflag, syscalls_numbers[i], > Sérgio> + &catch_syscall_breakpoint_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. I have a question right here, then. Is the "breakpoint with multiple locations" implemented this way? I'm sorry for my ignorance on this :-( > > 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. I was talking to Thiago about this, and I don't know if we need this *right now*, for this patch. I think it's good the way it is, and as you said, it won't take much to modify things in order to make it work the way you want :-). Also, currently the "catch syscall" command doesn't allow the user to set commands, conditions, etc. So for now it's only "catch syscall [name|number]". What do you think? > Sérgio> +/* Complete syscalls names. Used by "catch syscall". */ > Sérgio> +char ** > Sérgio> +catch_syscall_completer (char *text, char *word) > Sérgio> +{ > Sérgio> + const char **list = > Sérgio> + gdbarch_get_syscalls_names (current_gdbarch); > Sérgio> + return (list == NULL) ? NULL : complete_on_enum (list, text, word); > Sérgio> +} > > 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. Right, I agree. Consider it done. Thanks for your comments :-). Regards, -- Sérgio Durigan Júnior Linux on Power Toolchain - Software Engineer Linux Technology Center - LTC IBM Brazil