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

["Sérgio Durigan Júnior" <sergiodj@linux.vnet.ibm.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