Message ID | 1414956944-8856-3-git-send-email-gabriel@krisman.be |
---|---|
State | New |
Headers | show |
On Sunday, November 02 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. Nice, looks good :-). A few comments. > 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 | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 95 insertions(+), 9 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index cab6c56..8520361 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -12111,10 +12111,40 @@ 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 (syscall_number, &s); > + { > + get_syscall_by_number (syscall_number, &s); > + VEC_safe_push (int, result, s.number); > + } > + else if ((cur_name[0] == 'g' && cur_name[1] == ':') > + || strstr (cur_name, "group:") == cur_name) Here you compare chars and use strstr, and then below you use two calls to strncmp. I prefer if you use strncmp here as well, because it would be simpler. > + { > + /* 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 (group_name); > + > + if (syscall_list == NULL) > + error (_("Unknown syscall group '%s'."), group_name); > + > + for (i = 0; syscall_list[i].name; i++) Explicit comparison: syscall_list[i].name != NULL > + { > + /* 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); I prefer without the extra newline separating the comment and the code. Likewise for everywhere else you did this ;-). > + } > + > + xfree (syscall_list); > + } > else > { > /* We have a name. Let's check if it's valid and convert it > @@ -12126,10 +12156,11 @@ 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); > @@ -15417,11 +15448,66 @@ 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); > + 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--) You can use prefix[-1] instead of *(prefix-1) here. But if you decide to the latter, then you should write *(prefix - 1). > + ; > + > + if (strncmp (prefix, "g:", strlen ("g:")) == 0 > + || strncmp (prefix, "group:", strlen ("group:")) == 0) > + { > + /* Perform completion inside 'group:' namespace only. */ > + > + group_list = get_syscall_group_names (); > + 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 (); > + group_list = get_syscall_group_names (); > + cleanups = make_cleanup (xfree, group_list); > + > + /* Append "group:" prefix to syscall groups. */ > + for (i = 0; group_list[i]; i++) Explicit comparison. > + { > + const char *group = group_list[i]; > + size_t str_length = (sizeof (char) > + *(strlen (group) + strlen ("group:") + 1)); ^^^ Missing space between operator and parenthesis. > + char *prefixed_group = xmalloc (str_length); > + > + xsnprintf (prefixed_group, str_length, "group:%s", group); You can use xstrprintf here, and drop the xmalloc dance. > + 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 Otherwise, the patch looks good to me (I am not the maintainer of this part of the code).
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index cab6c56..8520361 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -12111,10 +12111,40 @@ 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 (syscall_number, &s); + { + get_syscall_by_number (syscall_number, &s); + VEC_safe_push (int, result, s.number); + } + else if ((cur_name[0] == 'g' && cur_name[1] == ':') + || strstr (cur_name, "group:") == cur_name) + { + /* 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 (group_name); + + if (syscall_list == NULL) + error (_("Unknown syscall group '%s'."), group_name); + + for (i = 0; syscall_list[i].name; 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 @@ -12126,10 +12156,11 @@ 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); @@ -15417,11 +15448,66 @@ 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); + 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:", strlen ("g:")) == 0 + || strncmp (prefix, "group:", strlen ("group:")) == 0) + { + /* Perform completion inside 'group:' namespace only. */ + + group_list = get_syscall_group_names (); + 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 (); + group_list = get_syscall_group_names (); + cleanups = make_cleanup (xfree, group_list); + + /* Append "group:" prefix to syscall groups. */ + for (i = 0; group_list[i]; i++) + { + const char *group = group_list[i]; + size_t str_length = (sizeof (char) + *(strlen (group) + strlen ("group:") + 1)); + char *prefixed_group = xmalloc (str_length); + + xsnprintf (prefixed_group, str_length, "group:%s", group); + 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; }