Re: [patch] info threads sort by name and name regex matching

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Aaron" == Aaron Gamble <agamble@google.com> writes:

Aaron> Here is a patch for adding sorting by name in 'info threads' and regex
Aaron> pattern matching of thread names for filtering threads.

Thanks.  I think this is a good idea.

Aaron> With this patch sorting by name will happen always. (Perhaps a switch
Aaron> to enable sorting would be better?)

Yes, I think so.  I think sorting by name makes sense but it isn't
perhaps always what you want.  Sorting by number has the nice feature
that it is stable.

Aaron> Regex matching is specified by doing 'info threads r<regex>'. This is
Aaron> not ambiguous with previous behavior where parameters to info threads
Aaron> were only numbers. (spaces between 'r' and <regex> are ignored)

I'm curious why you chose this particular spelling.
Other possible approaches would be a subcommand, or a flag like "-r".
Either of these is perhaps more in keeping with gdb tradition.

I'm interested in other opinions here too.

Expect some bikeshedding on this point.

Aaron> +@item info threads @r{[}@var{id}@dots{}@b{|}r<@var{regex}>@r{]}
Aaron> +Display a summary of all threads currently in your program.  Optional
Aaron> +argument for specifying threads is either @code{r} followed by a regular
Aaron> +expression or @var{id}@dots{} one or more numeric thread ids separated by
Aaron> +spaces. The list of threads is sorted alphabetically by thread name.

I think the documentation should say what the regular expression matches
against.

Aaron> +# Start with a fresh gdb.
Aaron> +gdb_exit
Aaron> +gdb_start

What Hafiz said :)

Aaron> +return 0

There's no need for this in the .exp file.

Aaron> diff --git a/gdb/thread.c b/gdb/thread.c
[...]
Aaron> +#include <stdlib.h>

I think defs.h will already include this if it is available.

Aaron> +static int print_thread_info_regex_cflags = 0;
Aaron> +static int print_thread_info_regex_eflags = 0;

I don't think you need these.  Just put the constants directly in the
re* calls.  Also, I'm guessing you want REG_NOSUB at least.

Aaron> +  if (preg)
Aaron> +    {
Aaron> +      int err = regexec (preg, tp->cached_name, 0, NULL,
Aaron> +                         print_thread_info_regex_eflags);

I think cached_name can be NULL, because target_thread_name can return
NULL.

Aaron> +static int
Aaron> +print_thread_sort_cmp (const void *p1, const void *p2)
Aaron> +{
Aaron> +  const char *name1 = (*(struct thread_info **)p1)->cached_name;
Aaron> +  const char *name2 = (*(struct thread_info **)p2)->cached_name;

GNU style requires some spaces in here, before "p1" and "p2".

Aaron> +  if (name1 && name2)
Aaron> +    return strcoll (name1, name2);

strcoll isn't used in gdb yet.  So, you have to look to see whether
configury is required.  I usually check gnulib.

Using strcmp seems just as good though.

Aaron> +  /* Incase we receive null pointers instead of strings.

s/Incase/In case/ and s/null/NULL/

Aaron> +/* Caches the name that will be shown to the user for a thread.
Aaron> +   We keep track of this for sorting purposes.  */
Aaron> +
Aaron> +static void
Aaron> +thread_cache_name (struct thread_info *tp)
Aaron> +{
Aaron> +    tp->cached_name = tp->name ? tp->name : target_thread_name (tp);

Too much indentation.

I think the cached_name field should have a comment explaining that it
doesn't need to be freed, and that it is just transient.

Aaron>  void
Aaron>  print_thread_info (struct ui_out *uiout, char *requested_threads, int pid)
Aaron>  {
Aaron>    struct thread_info *tp;
Aaron>    ptid_t current_ptid;
Aaron>    struct cleanup *old_chain;
Aaron> -  char *extra_info, *name, *target_id;
Aaron>    int current_thread = -1;
Aaron> +  struct thread_info **threads = NULL;
Aaron> +  int n_threads, i, ret;
Aaron> +  regex_t preg_buffer;
Aaron> +  regex_t *preg = NULL;
Aaron> +
Aaron> +  if (requested_threads && requested_threads[0] == 'r')
Aaron> +    {
Aaron> +      /* User has supplied a regular expression.  */
Aaron> +      requested_threads = skip_spaces (&requested_threads[1]);
Aaron> +      ret = regcomp (&preg_buffer, requested_threads,
Aaron> +                     print_thread_info_regex_cflags);

Nothing ever calls regfree on preg_buffer.
You probably want to use make_regfree_cleanup.
You may want to rearrange the code so that either the assignment to
old_chain happens before this, or to make a null cleanup first.

Aaron> +      threads = xmalloc (sizeof (*threads) * thread_list_size);
Aaron> +      make_cleanup (free, threads);

I think making a VEC here would be better.
Then you wouldn't need thread_list_size at all.

Tom