Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part

[Tom Tromey <tromey at redhat dot com>]

>>>>> "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.

SÃrgio> +    printf_filtered (_("'%s ()'"), syscall_name);

I don't think the '()' is needed here.

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.

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.

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.

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.

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.

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Ã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.

Tom