This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part
- From: Sérgio Durigan Júnior <sergiodj at linux dot vnet dot ibm dot com>
- To: tromey at redhat dot com
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 07 Nov 2008 01:38:22 -0200
- Subject: Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part
- References: <1225773079.24532.52.camel@miki> <m3d4hbpe7s.fsf@fleche.redhat.com>
Hey Tom!
On Tue, 2008-11-04 at 10:56 -0700, Tom Tromey wrote:
> >>>>> "Sérgio" == Sérgio Durigan Júnior <sergiodj@linux.vnet.ibm.com> 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