Re: [RFC PATCH 2/3] Add support to catch groups of syscalls.

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, October 07 2014, Gabriel Krisman Bertazi wrote:

> This implements the catchpoint side.  While parsing 'catch syscall'
> arguments, we verify if the argument is a syscall group and expand it to
> a list of syscalls that are part of that group.

Thanks for the patch.  Comments below.

> gdb/
>
> 	* breakpoint.c (catch_syscall_split_args): Verify if argument
> 	is a syscall group and expand it to a list of syscalls when
> 	creating catchpoints.
> 	(catch_syscall_completer): Include syscall groups to the list of
> 	word completion.
> ---
>  gdb/breakpoint.c | 73 +++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 57 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e2170b4..5243916 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12117,22 +12117,50 @@ catch_syscall_split_args (char *arg)
>        /* Check if the user provided a syscall name or a number.  */
>        syscall_number = (int) strtol (cur_name, &endptr, 0);
>        if (*endptr == '\0')
> -	get_syscall_by_number (syscall_number, &s);
> +	{
> +	  get_syscall_by_number (syscall_number, &s);
> +
> +	  /* Ok, it's valid.  */
> +	  VEC_safe_push (int, result, s.number);
> +	}
>        else
>  	{
> -	  /* We have a name.  Let's check if it's valid and convert it
> -	     to a number.  */
> -	  get_syscall_by_name (cur_name, &s);
> -
> -	  if (s.number == UNKNOWN_SYSCALL)
> -	    /* Here we have to issue an error instead of a warning,
> -	       because GDB cannot do anything useful if there's no
> -	       syscall number to be caught.  */
> +	  const char **syscall_list;
> +
> +	  /*  If we have a syscall group, expand it to a list of
> +	      syscalls.  */
> +	  syscall_list = get_syscall_names_by_group (cur_name);
> +
> +	  if (syscall_list == NULL)
>  	    error (_("Unknown syscall name '%s'."), cur_name);

This part...

> -	}
>  
> -      /* Ok, it's valid.  */
> -      VEC_safe_push (int, result, s.number);
> +	  if (syscall_list[0] == NULL)
> +	    {
> +	      /* If syscall_list is empty, cur_name is a syscall name
> +		 instead of a group.  We push it into the group
> +		 list.  */
> +	      syscall_list[0] = cur_name;
> +	      syscall_list[1] = NULL;
> +	    }

... and this part are the reasons I made the comment on the first patch
about leaving to much to the caller.  For example, the caller has to
know that syscall_list == NULL and *syscall_list == NULL are different
things which mean entirely different things.  However, *syscall_list ==
NULL should not happen in this interface, as I see it.

As I mentioned, you're doing this here because you have no way to know
whether the user is asking to catch a syscall name or a group of
syscalls.  This would be solved with my early proposal, of implementing
the "-g" modifier on the catch syscall command.  It would also make the
code simpler to follow here.

> +
> +	  for (i = 0; syscall_list[i]; i++)
> +	    {
> +	      get_syscall_by_name (syscall_list[i], &s);
> +
> +	      if (s.number == UNKNOWN_SYSCALL)
> +		{
> +		  /* Here we have to issue an error instead of a warning,
> +		     because GDB cannot do anything useful if there's no
> +		     syscall number to be caught.  */
> +		  error (_("Unknown syscall name '%s'."), syscall_list[i]);
> +		}
> +
> +	      /* Ok, it's valid.  */
> +	      VEC_safe_push (int, result, s.number);
> +	    }
> +
> +	  xfree (syscall_list);
> +	}
>      }
>  
>    discard_cleanups (cleanup);
> @@ -15615,11 +15643,24 @@ static VEC (char_ptr) *
>  catch_syscall_completer (struct cmd_list_element *cmd,
>                           const char *text, const char *word)
>  {
> -  const char **list = get_syscall_names ();
> -  VEC (char_ptr) *retlist
> -    = (list == NULL) ? NULL : complete_on_enum (list, word, word);
> +  VEC (char_ptr) *retlist;
> +  const char **syscall_list = get_syscall_names ();
> +  const char **group_list = get_syscall_group_names ();
> +
> +  VEC (char_ptr) *sys_retlist
> +    = (syscall_list == NULL) ? NULL : complete_on_enum (syscall_list,
> +							word, word);
> +
> +  VEC (char_ptr) *group_retlist
> +    = (group_list == NULL) ? NULL : complete_on_enum (group_list, word, word);

No newlines between variables being declared.

> +
> +  retlist = VEC_merge (char_ptr, sys_retlist, group_retlist);
> +
> +  xfree (syscall_list);
> +  xfree (group_list);
> +  VEC_free (char_ptr, sys_retlist);
> +  VEC_free (char_ptr, group_retlist);
>  
> -  xfree (list);
>    return retlist;
>  }
>  
> -- 
> 1.9.3

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/