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

[Eli Zaretskii <eliz@gnu.org>]

> From: =?ISO-8859-1?Q?S=E9rgio?= Durigan =?ISO-8859-1?Q?J=FAnior?= <sergiodj@linux.vnet.ibm.com>
> Date: Tue, 04 Nov 2008 02:31:19 -0200

Thanks, but I cannot say I like the non-portable assumptions of this
design.  For example:

> +void
> +clear_syscall_catchpoints_info (void)
> +{
> +  struct breakpoint *b;
> +
> +  ALL_BREAKPOINTS (b)
> +    if (syscall_catchpoint_p (b))
> +      b->syscall_number = UNKNOWN_SYSCALL;
> +}     ^^^^^^^^^^^^^^^^^

Who said that a syscall is necessarily defined by some number?

More generally, let's say I'd like to implement support for this on
Windows -- how would I need to go about it?

> +      /* We are in a entry breakpoint.  */
> +      entry_breakpoint,
> +

This comment is too obscure, IMO.  Please make it more explicit; I
think you want to say that it's an entry to a syscall, right?

> +  const char *syscall_name =
> +    gdbarch_syscall_name_from_number (current_gdbarch, b->syscall_number);

Does this mean the notion that a syscall is a number have crept into
gdbarch, which is supposed to be the most architecture-independent
part of GDB?

> +      /* Checking if the user provided a syscall name or a number.  */
> +      if (isdigit (cur_name[0]))

Is the assumption that no name will ever begin with a digit
universally valid?

> +            error (_("The number '%d' does not represent a valid syscall."),
                                                            ^^^^^^^^^^^^^^^
"a known syscall", I'd say.

> +          if (syscall_number == UNKNOWN_SYSCALL)
> +            error (_("Invalid syscall name '%s'."), cur_name);
                         ^^^^^^^
"Unknown" is more accurate.

>  static void
> @@ -7441,6 +7792,7 @@ delete_command (char *arg, int from_tty)
>  	    b->type != bp_shlib_event &&
>  	    b->type != bp_thread_event &&
>  	    b->type != bp_overlay_event &&
> +            b->type != bp_entry_breakpoint &&

The previous lines used TABs and spaces for indentation, while your
line uses only spaces.

> +  add_catch_command ("syscall", _("\
> +Catch calls to syscalls.\n\

"call to syscalls" is awkward.  I suggest to drop the "calls to" part.

> +With an argument, catch only calls of that syscall."),

Ditto, suggest to drop the "calls of" part.

> +  add_setshow_filename_cmd ("gdb_datadir", class_maintenance,
> +                           &gdb_datadir, _("Set GDB's datadir path."),
> +                           _("Show GDB's datadir path."),
> +                           _("\
> +When set, GDB uses the specified path to search for data files."),
> +                           NULL, NULL,
> +                           &maintenance_set_cmdlist,
> +                           &maintenance_show_cmdlist);

This should be described in the manual.