[RFC,2/3] Add support to catch groups of syscalls.

  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.

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(-)
  

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
  

On Wed, Oct 8, 2014 at 12:07 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> 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.

Yeah, let's agree on a syntax before we get through too many patch iterations.

Regarding:
> # catch syscalls write, read, chdir, and groups network and signal
> (gdb) catch syscall write read chdir -g network,signal
> # or maybe without comma-separated values for groups, to keep consistency
> (gdb) catch syscall write read chdir -g network signal

I dislike "network,signal" if we don't also accept "read,write".  I
gather the comma is there to remove ambiguity as to what "-g network
signal" means.
I also kinda dislike interpreting "-g" to mean all remaining arguments
(for a few reasons).
"catch syscall write -g network" feels clumsy if I can't also do
something like "catch syscall -g network -s write" or some such).

One could just say that syscall names and syscall group names share
the same namespace, but
I can imagine a system that happens to have a syscall that is the name
of a group on another system.
E.g., maybe there's a system where "signal" is a syscall.  It's a
logical name for the group.
Then if one happened to be unfortunate enough to work with two systems
where "signal" is a syscall name on one system and a group name on
another system, I can imagine tripping over the use of the same name
to mean different things and getting frustrated.

How about appending "-group" or some such to group names?

[I don't want to have too long a discussion or be too picky.
OTOH I also don't want to just pick something and then regret it.]
  

On Wednesday, October 08 2014, Doug Evans wrote:

> Regarding:
>> # catch syscalls write, read, chdir, and groups network and signal
>> (gdb) catch syscall write read chdir -g network,signal
>> # or maybe without comma-separated values for groups, to keep consistency
>> (gdb) catch syscall write read chdir -g network signal
>
> I dislike "network,signal" if we don't also accept "read,write".  I
> gather the comma is there to remove ambiguity as to what "-g network
> signal" means.

Yeah.

> I also kinda dislike interpreting "-g" to mean all remaining arguments
> (for a few reasons).

Since there are very few groups (compared to syscalls names), I also
thought that "-g" could be used multiple times, like:

  (gdb) catch syscall -g network -g signal

But...

> "catch syscall write -g network" feels clumsy if I can't also do
> something like "catch syscall -g network -s write" or some such).

... this comment also applies even if we consider "-g" to refer to the
next argument.

However, while I understand your feeling that having "-g" without having
"-s" seems odd, I don't think I completely agree.  I think that syscall
groups are meta-information, and deserved to be treated differently.
The command name "catch syscall" makes the user understand immediately
what kind of argument the command expects (i.e., a syscall).  It would
be weird to make the user need to issue a "-s" to specify a syscall
name.

I am trying to think what would happen if we were talking about
breakpoints and breakpoint groups.  I think it would be fairly
reasonable to have a syntax like:

  (gdb) break -g mygroup

And not expect something like:

  (gdb) break -b function

Do you see what I mean?

> One could just say that syscall names and syscall group names share
> the same namespace, but
> I can imagine a system that happens to have a syscall that is the name
> of a group on another system.

Yes, that is the rationale behind my proposal.  And I don't think
syscall group names and syscall names share the same namespace, as
explained above.

> E.g., maybe there's a system where "signal" is a syscall.  It's a
> logical name for the group.
> Then if one happened to be unfortunate enough to work with two systems
> where "signal" is a syscall name on one system and a group name on
> another system, I can imagine tripping over the use of the same name
> to mean different things and getting frustrated.
>
> How about appending "-group" or some such to group names?

Hm, it seems OK, but I am thinking about one specific syscall that might
make things confusing here: exit_group(2).  Consider:

  (gdb) catch syscall signal-group exit_group

This can be very confusing for the user.

> [I don't want to have too long a discussion or be too picky.
> OTOH I also don't want to just pick something and then regret it.]

Yeah, I understand your reasons.

Along the lines of your proposal above, I guess we can add a "g:" prefix
to group names:

  (gdb) catch syscall read chdir g:network g:signal signal

WDYT?
  

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> On Wednesday, October 08 2014, Doug Evans wrote:
>
>> Regarding:
>>> # catch syscalls write, read, chdir, and groups network and signal
>>> (gdb) catch syscall write read chdir -g network,signal
>>> # or maybe without comma-separated values for groups, to keep consistency
>>> (gdb) catch syscall write read chdir -g network signal
>>
>> I dislike "network,signal" if we don't also accept "read,write".  I
>> gather the comma is there to remove ambiguity as to what "-g network
>> signal" means.
>
> Yeah.
>
>> I also kinda dislike interpreting "-g" to mean all remaining arguments
>> (for a few reasons).
>
> Since there are very few groups (compared to syscalls names), I also
> thought that "-g" could be used multiple times, like:
>
>   (gdb) catch syscall -g network -g signal
>
> But...

Doug and Sergio,

Thank you for your review and valuable suggestions.

The suggestion I feel more comfortable with is using -g for each syscall
group.  I agree that syscall groups should be treated on a different
namespace than syscall names, which are the expected argument for a
command called  "catch syscall".

>> How about appending "-group" or some such to group names?
>
> Hm, it seems OK, but I am thinking about one specific syscall that might
> make things confusing here: exit_group(2).  Consider:
>
>   (gdb) catch syscall signal-group exit_group
>
> This can be very confusing for the user.
>
>> [I don't want to have too long a discussion or be too picky.
>> OTOH I also don't want to just pick something and then regret it.]
>
> Yeah, I understand your reasons.
>
> Along the lines of your proposal above, I guess we can add a "g:" prefix
> to group names:
>
>   (gdb) catch syscall read chdir g:network g:signal signal
>
> WDYT?

I dislike the proposal of adding prefixes/suffixes to the group names
because I feel it might be harder to type, and also because it feels a
little unusual, if we consider the common way of providing arguments to
commands.

Using -g to specify syscall groups, as Sergio said, has also the
advantage of providing us with an intuitive command to list available
syscall groups, by saying "catch syscall -g" with no arguments.

So, my vote goes to using '-g' for each syscall group we want to
catch. Is that ok for you, guys?

I will still wait a few days to see if anyone has more suggestions
before sending the updated patch that fixes the syntax and the other
things you guys pointed out.

Thanks!
  

On Sunday, October 12 2014, Gabriel Krisman Bertazi wrote:

> I dislike the proposal of adding prefixes/suffixes to the group names
> because I feel it might be harder to type, and also because it feels a
> little unusual, if we consider the common way of providing arguments to
> commands.
>
> Using -g to specify syscall groups, as Sergio said, has also the
> advantage of providing us with an intuitive command to list available
> syscall groups, by saying "catch syscall -g" with no arguments.
>
> So, my vote goes to using '-g' for each syscall group we want to
> catch. Is that ok for you, guys?

Yeah, this is still OK to me :-).

> I will still wait a few days to see if anyone has more suggestions
> before sending the updated patch that fixes the syntax and the other
> things you guys pointed out.

Good idea.

Thanks,
  

On Sun, Oct 12, 2014 at 2:36 PM, Gabriel Krisman Bertazi
<gabriel@krisman.be> wrote:
> Using -g to specify syscall groups, as Sergio said, has also the
> advantage of providing us with an intuitive command to list available
> syscall groups, by saying "catch syscall -g" with no arguments.

That can't work as "catch syscall" catches all syscalls, and it would
be confusing to have "catch syscall" catch all syscalls whereas "catch
syscall -g" just lists syscall groups.

> So, my vote goes to using '-g' for each syscall group we want to
> catch. Is that ok for you, guys?

Would you still allow "catch syscall open -g network"?
I'm not really comfortable with that (far more so than "catch syscall
open network-group").
If you want to require -g at the front, and thus disallow catching
both syscalls and syscall groups in the same command then that would
be fine with me.
Still need a solution for listing them.  Arguably since we don't
provide a way to list syscalls (sigh, modulo the hack I showed, which
should be fixed so that it no longer works anyways :-)), providing a
way to list syscall groups is for a separate patch.  Kudos if you
still want to provide a way to list syscalls and groups though.
  

Doug Evans <dje@google.com> writes:

Sorry I took so long to answer your message. I got a lot of college
stuff to deal with right now... :(

> Would you still allow "catch syscall open -g network"?

Yes.  As a matter of fact, I still don't see the problem of allowing
"catch syscall open -g network exit" to catch open and exit syscalls and
the network group on the same command, provided that -g refers only to
the next token.  By the way, for the "-group" suffix you suggested,
would we also allow "catch syscall open network-group exit", right?

> I'm not really comfortable with that (far more so than "catch syscall
> open network-group").
> If you want to require -g at the front, and thus disallow catching
> both syscalls and syscall groups in the same command then that would
> be fine with me.

I really think we shouldn't disallow catching syscalls and syscalls
group on the same command, no matter which syntax we pick.  GDB wiki
says that GDB should be more permissive about command's syntax, in a
sense that user shouldn't spend more time than needed to find out how a
command works.  I think disallowing catching syscalls and groups on the
same command would reduce expressiveness in this case.

> Still need a solution for listing them.  Arguably since we don't
> provide a way to list syscalls (sigh, modulo the hack I showed, which
> should be fixed so that it no longer works anyways :-)), providing a
> way to list syscall groups is for a separate patch.  Kudos if you
> still want to provide a way to list syscalls and groups though.

So, definitively allowing "catch syscall -g" to list syscalls is not a
good idea.  Sergio suggested off-list to use another option, maybe -lg
to list syscall groups.  Then, a future patch could also extend catch
syscall to list all syscalls using a -l option or something like that.
Sergio, sorry if I got your suggestion wrong.

OTOH, I might be over-thinking this simple stuff :).  I'm ok with the
namespace (suffix) syntax, but I think we should go with "g:" (or even
"group:network", if it's not too verbose) instead of "-group", to avoid
the issue pointed out by Sergio with the exit_group syscall.

Cheers,
  

On Monday, October 20 2014, Gabriel Krisman Bertazi wrote:

>> I'm not really comfortable with that (far more so than "catch syscall
>> open network-group").
>> If you want to require -g at the front, and thus disallow catching
>> both syscalls and syscall groups in the same command then that would
>> be fine with me.
>
> I really think we shouldn't disallow catching syscalls and syscalls
> group on the same command, no matter which syntax we pick.  GDB wiki
> says that GDB should be more permissive about command's syntax, in a
> sense that user shouldn't spend more time than needed to find out how a
> command works.  I think disallowing catching syscalls and groups on the
> same command would reduce expressiveness in this case.

I agree.

>> Still need a solution for listing them.  Arguably since we don't
>> provide a way to list syscalls (sigh, modulo the hack I showed, which
>> should be fixed so that it no longer works anyways :-)), providing a
>> way to list syscall groups is for a separate patch.  Kudos if you
>> still want to provide a way to list syscalls and groups though.
>
> So, definitively allowing "catch syscall -g" to list syscalls is not a
> good idea.  Sergio suggested off-list to use another option, maybe -lg
> to list syscall groups.  Then, a future patch could also extend catch
> syscall to list all syscalls using a -l option or something like that.
> Sergio, sorry if I got your suggestion wrong.

It is alright, I completely forgot I made that suggestion!  Thanks for
bringing it to the table.

Anyway, yeah, I guess '-lg' (or -list-groups) should be OK.

> OTOH, I might be over-thinking this simple stuff :).  I'm ok with the
> namespace (suffix) syntax, but I think we should go with "g:" (or even
> "group:network", if it's not too verbose) instead of "-group", to avoid
> the issue pointed out by Sergio with the exit_group syscall.

Yeah, maybe this is a bit over-thinking, but OTOH we are talking about
user interface, which cannot be changed easily after we make a release.

BTW, I like the idea of using the "g:" prefix, so I say "go for it" if
you think it is OK.

Sorry for not being able to comment more on the thread now, I am busy
with other things.  However, I think you covered all the issues with
your message, so you should be good to go as long as Doug has no other
comments.

Cheers,
  

On Mon, Oct 20, 2014 at 12:39 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Monday, October 20 2014, Gabriel Krisman Bertazi wrote:
>
>>> I'm not really comfortable with that (far more so than "catch syscall
>>> open network-group").
>>> If you want to require -g at the front, and thus disallow catching
>>> both syscalls and syscall groups in the same command then that would
>>> be fine with me.
>>
>> I really think we shouldn't disallow catching syscalls and syscalls
>> group on the same command, no matter which syntax we pick.  GDB wiki
>> says that GDB should be more permissive about command's syntax, in a
>> sense that user shouldn't spend more time than needed to find out how a
>> command works.  I think disallowing catching syscalls and groups on the
>> same command would reduce expressiveness in this case.
>
> I agree.
>
>>> Still need a solution for listing them.  Arguably since we don't
>>> provide a way to list syscalls (sigh, modulo the hack I showed, which
>>> should be fixed so that it no longer works anyways :-)), providing a
>>> way to list syscall groups is for a separate patch.  Kudos if you
>>> still want to provide a way to list syscalls and groups though.
>>
>> So, definitively allowing "catch syscall -g" to list syscalls is not a
>> good idea.  Sergio suggested off-list to use another option, maybe -lg
>> to list syscall groups.  Then, a future patch could also extend catch
>> syscall to list all syscalls using a -l option or something like that.
>> Sergio, sorry if I got your suggestion wrong.
>
> It is alright, I completely forgot I made that suggestion!  Thanks for
> bringing it to the table.
>
> Anyway, yeah, I guess '-lg' (or -list-groups) should be OK.
>
>> OTOH, I might be over-thinking this simple stuff :).  I'm ok with the
>> namespace (suffix) syntax, but I think we should go with "g:" (or even
>> "group:network", if it's not too verbose) instead of "-group", to avoid
>> the issue pointed out by Sergio with the exit_group syscall.
>
> Yeah, maybe this is a bit over-thinking, but OTOH we are talking about
> user interface, which cannot be changed easily after we make a release.
>
> BTW, I like the idea of using the "g:" prefix, so I say "go for it" if
> you think it is OK.
>
> Sorry for not being able to comment more on the thread now, I am busy
> with other things.  However, I think you covered all the issues with
> your message, so you should be good to go as long as Doug has no other
> comments.

I can live with "g:foo g:bar" more than "-g foo [-g?] bar", though I'm
willing to defer to a majority if it arises (depending on what the
majority decides on :-)).

I can also live with "catch syscall -l/-lg" though there are other
things where we want to provide the ability to list things (e.g.,
catch signal) and I would want consistency throughout.  Another
thought is "catch list foo".

"g:" is pretty non-descript. "group:" is clearer.
OTOH, we do try to minimize typing where we can.
I'm hesitant to get too elaborate here and suggest supporting both.
OTOH, we can start with "g:" and add a "group:" alias later.

Anyone else have a preference?