Re: [RFC PATCH 0/3] Catch syscall group

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

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

> Hello,
>
> This multi-part patch implements on top of the 'catch syscall' command
> the ability to catch a group of related syscalls using a single
> command.

Thanks for the patch :-).

> The idea is to be able to catch a group of syscalls with a single
> command, just like we can do with the "-e trace" option in strace.  We
> separate the syscalls in groups according to their functionality and
> allow users to say:
>
> (gdb) catch syscall network
>
> This creates a catchpoint for every network related syscall, such as
> bind, accept, connect...

As we have talked before, I think it would be nice if "catch syscall"
took an argument in this case, specifying that you are going to pass
groups to it.

  # catch syscall group network
  (gdb) catch syscall -g network

This syntax could also be expanded for when the user wants to provide a
group *and* a syscall to be caught:

  # catch syscall write and group network
  (gdb) catch syscall write -g network

And also for when the user wants to specify multiple syscalls and/or
groups:

  # 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

> (Please note that adding this feature to GDB was first suggested by Tom
> Tromey on PR 15879. :-)
>  <https://sourceware.org/bugzilla/show_bug.cgi?id=15879>).

It is good to mention the bug in the ChangeLog, even if you are not
directly fixing it.

> Right now, I've created syscall groups only for x86_64 and sorted the
> syscalls among them by using the same groups that strace has.

I will make comments in the patch itself, but it would be nice if you
could do that for the other existing XML files as well.  You didn't
mention if you intend to do so or not.

I know it will be kind of boring, but unfortunately those XML files are
not being really maintained nowadays (a problem I intend to address soon
with some patches and proposals), so if you don't touch those files now
they will probably be forgotten for quite some time...

However, this is just a request, so if you don't have the time to do
that now I'd still vote for including this feature on GDB.

> I am sending this as an RFC because I still have a few questions before
> proposing this as a patch:
>
> 1) Whether a feature like this would be acceptable on GDB?

Certainly.

> 2) About the command to catch syscall groups: I think it is more
> practical to just say "catch syscall network", instead of "catch syscall
> group network" or something like that.  What would be the GDB way to do
> that?  (I implemented the former, but I'd be happy to rewrite
> this part of the patch.)

I think I covered this question above, but the "de facto" way is to
implement parameters to be passed to commands.  In this case, "-g GROUP"
would be the best approach, IMO.

I think providing the group name without having some specifier like "-g"
can be very confusing to the users.

> 3) I think it is nice to have a command to list the syscall groups
> available.  Maybe by saying "catch syscall group" with no arguments.
> What do you think?

I agree it is nice to have such a command.  And since I proposed using
"-g", I guess just "catch syscall -g" should list the available groups.

> 4) What kind of documentation should be updated when proposing the
> patch?  Do I have to update only the GDB manual or should I provide
> patches for other documentation as well?

You need to update:

- doc/gdb.texinfo (the GDB official manual), specifically in the part of
  the "catch syscall" command (search for "catch syscall", it will be
  the first match).  As you will see, this part contains several
  examples of how to use the command, so you will want to add one or
  more examples of the group feature there as well.

- gdb/NEWS, mentioning the new feature for "catch syscall"

- gdb/breakpoint.c:_initialize_breakpoint, specifically the part that
  adds the "catch syscall" command (search for add_catch_command
  ("syscall").  There, you will see the help string that gets printed
  when the user issues a "help catch syscall".  You will need to mention
  the new arguments there.  Look for BREAK_ARGS_HELP if you want to see
  some examples.

> 5) Regarding the code design I propose here, do you have any concerns
> that I should fix right away before submiting this as a patch?  Please
> share your thoughts :)

I will comment on each patch separately :-).

> This patch series is divided as follows: Part 1 updates the xml-syscall
> interface to support the syscall group feature; Part 2 has the actual
> catchpoint code; Finally, Part 3 has the updated x86_64 xml, which
> defines the syscall groups for this architecture, and includes tests for
> this feature on x86_64.

Thanks for splitting the patch!

Cheers,

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