[v2,2/4] Add support to catch groups of syscalls.
Commit Message
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): Add word completion for system call
groups.
---
gdb/breakpoint.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 86 insertions(+), 9 deletions(-)
Comments
On Friday, November 21 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. Just a few nits.
> 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): Add word completion for system call
> groups.
> ---
> gdb/breakpoint.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 7b56260..098e28d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12058,10 +12058,39 @@ catch_syscall_split_args (char *arg)
> cur_name[i] = '\0';
> arg += i;
>
> - /* Check if the user provided a syscall name or a number. */
> + /* Check if the user provided a syscall name, group, or a
> + number. */
> syscall_number = (int) strtol (cur_name, &endptr, 0);
> if (*endptr == '\0')
> - get_syscall_by_number (gdbarch, syscall_number, &s);
> + {
> + get_syscall_by_number (gdbarch, syscall_number, &s);
> + VEC_safe_push (int, result, s.number);
> + }
> + else if (strncmp (cur_name, "g:", 2) == 0
> + || strncmp (cur_name, "group:", 6) == 0)
I'm not a fan of using numbers here. I prefer something like:
if (strncmp (cur_name, "g:", sizeof ("g:") -1) == 0)
which is a dialect used everywhere in GDB.
> + {
> + /* We have a syscall group. Let's expand it into a syscall
> + list before inserting. */
> + struct syscall *syscall_list;
> + const char *group_name;
> +
> + /* Skip over "g:" and "group:" prefix strings. */
> + group_name = strchr (cur_name, ':') + 1;
> +
> + syscall_list = get_syscalls_by_group (gdbarch, group_name);
> +
> + if (syscall_list == NULL)
> + error (_("Unknown syscall group '%s'."), group_name);
> +
> + for (i = 0; syscall_list[i].name != NULL; i++)
> + {
> + /* Insert each syscall that are part of the group. No
> + need to check if it is valid. */
> + VEC_safe_push (int, result, syscall_list[i].number);
> + }
> +
> + xfree (syscall_list);
> + }
> else
> {
> /* We have a name. Let's check if it's valid and convert it
> @@ -12073,10 +12102,10 @@ catch_syscall_split_args (char *arg)
> because GDB cannot do anything useful if there's no
> syscall number to be caught. */
> error (_("Unknown syscall name '%s'."), cur_name);
> - }
>
> - /* Ok, it's valid. */
> - VEC_safe_push (int, result, s.number);
> + /* Ok, it's valid. */
> + VEC_safe_push (int, result, s.number);
> + }
> }
>
> discard_cleanups (cleanup);
> @@ -15350,11 +15379,59 @@ static VEC (char_ptr) *
> catch_syscall_completer (struct cmd_list_element *cmd,
> const char *text, const char *word)
> {
> - const char **list = get_syscall_names (get_current_arch ());
> - VEC (char_ptr) *retlist
> - = (list == NULL) ? NULL : complete_on_enum (list, word, word);
> + struct gdbarch *gdbarch = get_current_arch ();
> + struct cleanup *cleanups;
I know you're initializing the "cleanups" both on "if" and on "else",
but I'd prefer if you did:
struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
This way, if someone touches the code and removes the initialization
from some the if/else parts, the code would still DTRT.
> + VEC (char_ptr) *group_retlist = NULL;
> + VEC (char_ptr) *syscall_retlist = NULL;
> + VEC (char_ptr) *retlist = NULL;
> + const char **group_list = NULL;
> + const char **syscall_list = NULL;
> + const char *prefix;
> + int i;
> +
> + /* Completion considers ':' to be a word separator, so we use this to
> + verify whether the previous word was a group prefix. If so, we
> + build the completion list using group names only. */
> + for (prefix = word; prefix != text && prefix[-1] != ' '; prefix--)
> + ;
> +
> + if (strncmp (prefix, "g:", 2) == 0 || strncmp (prefix, "group:", 6) == 0)
Same comment about using 'sizeof ("str") - 1' applies here.
> + {
> + /* Perform completion inside 'group:' namespace only. */
> + group_list = get_syscall_group_names (gdbarch);
> + cleanups = make_cleanup (xfree, group_list);
If you follow my advice above, there's no need to do this assignment;
just call make_cleanup.
> + retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
> + }
> + else
> + {
> + /* Complete with both, syscall names and groups. */
> + syscall_list = get_syscall_names (gdbarch);
> + group_list = get_syscall_group_names (gdbarch);
> + cleanups = make_cleanup (xfree, group_list);
> +
> + /* Append "group:" prefix to syscall groups. */
> + for (i = 0; group_list[i] != NULL; i++)
> + {
> + char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
> +
> + group_list[i] = prefixed_group;
> + make_cleanup (xfree, prefixed_group);
> + }
> +
> + syscall_retlist = (syscall_list == NULL) ?
> + NULL : complete_on_enum (syscall_list, word, word);
> + group_retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
> +
> + retlist = VEC_merge (char_ptr, syscall_retlist, group_retlist);
> + }
> +
> + VEC_free (char_ptr, syscall_retlist);
> + VEC_free (char_ptr, group_retlist);
> + xfree (syscall_list);
> + do_cleanups (cleanups);
>
> - xfree (list);
> return retlist;
> }
>
> --
> 1.9.3
Other than that, the patch looks good to me now. I'd say there's no
need to send a v3 just because of those changes; just wait for some
global maintainer to approve it for you now :-).
Thanks for doing this.
Gabriel Krisman Bertazi <gabriel@krisman.be> writes:
> 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): Add word completion for system call
> groups.
Hi.
A few nits inline.
Ok with those changes, and things mentioned in other reviews.
> ---
> gdb/breakpoint.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 7b56260..098e28d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12058,10 +12058,39 @@ catch_syscall_split_args (char *arg)
> cur_name[i] = '\0';
> arg += i;
>
> - /* Check if the user provided a syscall name or a number. */
> + /* Check if the user provided a syscall name, group, or a
> + number. */
Put number on previous line.
> syscall_number = (int) strtol (cur_name, &endptr, 0);
> if (*endptr == '\0')
> - get_syscall_by_number (gdbarch, syscall_number, &s);
> + {
> + get_syscall_by_number (gdbarch, syscall_number, &s);
> + VEC_safe_push (int, result, s.number);
> + }
> + else if (strncmp (cur_name, "g:", 2) == 0
> + || strncmp (cur_name, "group:", 6) == 0)
> + {
> + /* We have a syscall group. Let's expand it into a syscall
> + list before inserting. */
> + struct syscall *syscall_list;
> + const char *group_name;
> +
> + /* Skip over "g:" and "group:" prefix strings. */
> + group_name = strchr (cur_name, ':') + 1;
> +
> + syscall_list = get_syscalls_by_group (gdbarch, group_name);
> +
> + if (syscall_list == NULL)
> + error (_("Unknown syscall group '%s'."), group_name);
> +
> + for (i = 0; syscall_list[i].name != NULL; i++)
> + {
> + /* Insert each syscall that are part of the group. No
> + need to check if it is valid. */
> + VEC_safe_push (int, result, syscall_list[i].number);
> + }
> +
> + xfree (syscall_list);
> + }
> else
> {
> /* We have a name. Let's check if it's valid and convert it
> @@ -12073,10 +12102,10 @@ catch_syscall_split_args (char *arg)
> because GDB cannot do anything useful if there's no
> syscall number to be caught. */
> error (_("Unknown syscall name '%s'."), cur_name);
> - }
>
> - /* Ok, it's valid. */
> - VEC_safe_push (int, result, s.number);
> + /* Ok, it's valid. */
> + VEC_safe_push (int, result, s.number);
> + }
> }
>
> discard_cleanups (cleanup);
> @@ -15350,11 +15379,59 @@ static VEC (char_ptr) *
> catch_syscall_completer (struct cmd_list_element *cmd,
> const char *text, const char *word)
> {
> - const char **list = get_syscall_names (get_current_arch ());
> - VEC (char_ptr) *retlist
> - = (list == NULL) ? NULL : complete_on_enum (list, word, word);
> + struct gdbarch *gdbarch = get_current_arch ();
> + struct cleanup *cleanups;
> + VEC (char_ptr) *group_retlist = NULL;
> + VEC (char_ptr) *syscall_retlist = NULL;
> + VEC (char_ptr) *retlist = NULL;
> + const char **group_list = NULL;
> + const char **syscall_list = NULL;
> + const char *prefix;
> + int i;
> +
> + /* Completion considers ':' to be a word separator, so we use this to
> + verify whether the previous word was a group prefix. If so, we
> + build the completion list using group names only. */
> + for (prefix = word; prefix != text && prefix[-1] != ' '; prefix--)
> + ;
> +
> + if (strncmp (prefix, "g:", 2) == 0 || strncmp (prefix, "group:", 6) == 0)
> + {
> + /* Perform completion inside 'group:' namespace only. */
> + group_list = get_syscall_group_names (gdbarch);
> + cleanups = make_cleanup (xfree, group_list);
> + retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
The indentation is wrong here. Here's one correct way.
retlist = (group_list == NULL
? NULL : complete_on_enum (group_list, word, word));
or
retlist = (group_list == NULL
? NULL
: complete_on_enum (group_list, word, word));
> + }
> + else
> + {
> + /* Complete with both, syscall names and groups. */
> + syscall_list = get_syscall_names (gdbarch);
> + group_list = get_syscall_group_names (gdbarch);
> + cleanups = make_cleanup (xfree, group_list);
> +
> + /* Append "group:" prefix to syscall groups. */
> + for (i = 0; group_list[i] != NULL; i++)
> + {
> + char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
> +
> + group_list[i] = prefixed_group;
> + make_cleanup (xfree, prefixed_group);
> + }
> +
> + syscall_retlist = (syscall_list == NULL) ?
> + NULL : complete_on_enum (syscall_list, word, word);
> + group_retlist = (group_list == NULL) ?
> + NULL : complete_on_enum (group_list, word, word);
Similarly, indentation is wrong. See above.
> +
> + retlist = VEC_merge (char_ptr, syscall_retlist, group_retlist);
> + }
> +
> + VEC_free (char_ptr, syscall_retlist);
> + VEC_free (char_ptr, group_retlist);
> + xfree (syscall_list);
> + do_cleanups (cleanups);
>
> - xfree (list);
> return retlist;
> }
@@ -12058,10 +12058,39 @@ catch_syscall_split_args (char *arg)
cur_name[i] = '\0';
arg += i;
- /* Check if the user provided a syscall name or a number. */
+ /* Check if the user provided a syscall name, group, or a
+ number. */
syscall_number = (int) strtol (cur_name, &endptr, 0);
if (*endptr == '\0')
- get_syscall_by_number (gdbarch, syscall_number, &s);
+ {
+ get_syscall_by_number (gdbarch, syscall_number, &s);
+ VEC_safe_push (int, result, s.number);
+ }
+ else if (strncmp (cur_name, "g:", 2) == 0
+ || strncmp (cur_name, "group:", 6) == 0)
+ {
+ /* We have a syscall group. Let's expand it into a syscall
+ list before inserting. */
+ struct syscall *syscall_list;
+ const char *group_name;
+
+ /* Skip over "g:" and "group:" prefix strings. */
+ group_name = strchr (cur_name, ':') + 1;
+
+ syscall_list = get_syscalls_by_group (gdbarch, group_name);
+
+ if (syscall_list == NULL)
+ error (_("Unknown syscall group '%s'."), group_name);
+
+ for (i = 0; syscall_list[i].name != NULL; i++)
+ {
+ /* Insert each syscall that are part of the group. No
+ need to check if it is valid. */
+ VEC_safe_push (int, result, syscall_list[i].number);
+ }
+
+ xfree (syscall_list);
+ }
else
{
/* We have a name. Let's check if it's valid and convert it
@@ -12073,10 +12102,10 @@ catch_syscall_split_args (char *arg)
because GDB cannot do anything useful if there's no
syscall number to be caught. */
error (_("Unknown syscall name '%s'."), cur_name);
- }
- /* Ok, it's valid. */
- VEC_safe_push (int, result, s.number);
+ /* Ok, it's valid. */
+ VEC_safe_push (int, result, s.number);
+ }
}
discard_cleanups (cleanup);
@@ -15350,11 +15379,59 @@ static VEC (char_ptr) *
catch_syscall_completer (struct cmd_list_element *cmd,
const char *text, const char *word)
{
- const char **list = get_syscall_names (get_current_arch ());
- VEC (char_ptr) *retlist
- = (list == NULL) ? NULL : complete_on_enum (list, word, word);
+ struct gdbarch *gdbarch = get_current_arch ();
+ struct cleanup *cleanups;
+ VEC (char_ptr) *group_retlist = NULL;
+ VEC (char_ptr) *syscall_retlist = NULL;
+ VEC (char_ptr) *retlist = NULL;
+ const char **group_list = NULL;
+ const char **syscall_list = NULL;
+ const char *prefix;
+ int i;
+
+ /* Completion considers ':' to be a word separator, so we use this to
+ verify whether the previous word was a group prefix. If so, we
+ build the completion list using group names only. */
+ for (prefix = word; prefix != text && prefix[-1] != ' '; prefix--)
+ ;
+
+ if (strncmp (prefix, "g:", 2) == 0 || strncmp (prefix, "group:", 6) == 0)
+ {
+ /* Perform completion inside 'group:' namespace only. */
+ group_list = get_syscall_group_names (gdbarch);
+ cleanups = make_cleanup (xfree, group_list);
+ retlist = (group_list == NULL) ?
+ NULL : complete_on_enum (group_list, word, word);
+ }
+ else
+ {
+ /* Complete with both, syscall names and groups. */
+ syscall_list = get_syscall_names (gdbarch);
+ group_list = get_syscall_group_names (gdbarch);
+ cleanups = make_cleanup (xfree, group_list);
+
+ /* Append "group:" prefix to syscall groups. */
+ for (i = 0; group_list[i] != NULL; i++)
+ {
+ char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
+
+ group_list[i] = prefixed_group;
+ make_cleanup (xfree, prefixed_group);
+ }
+
+ syscall_retlist = (syscall_list == NULL) ?
+ NULL : complete_on_enum (syscall_list, word, word);
+ group_retlist = (group_list == NULL) ?
+ NULL : complete_on_enum (group_list, word, word);
+
+ retlist = VEC_merge (char_ptr, syscall_retlist, group_retlist);
+ }
+
+ VEC_free (char_ptr, syscall_retlist);
+ VEC_free (char_ptr, group_retlist);
+ xfree (syscall_list);
+ do_cleanups (cleanups);
- xfree (list);
return retlist;
}