From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 413 invoked by alias); 22 Aug 2012 18:52:07 -0000 Received: (qmail 401 invoked by uid 22791); 22 Aug 2012 18:52:04 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 22 Aug 2012 18:51:49 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7MIplPR024695 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 22 Aug 2012 14:51:48 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q7MIpkvI026710 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 22 Aug 2012 14:51:47 -0400 From: Tom Tromey To: Aaron Gamble Cc: gdb-patches@sourceware.org Subject: Re: [patch] info threads sort by name and name regex matching References: Date: Wed, 22 Aug 2012 18:52:00 -0000 In-Reply-To: (Aaron Gamble's message of "Tue, 21 Aug 2012 14:18:42 -0700") Message-ID: <87pq6isqt9.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00630.txt.bz2 >>>>> "Aaron" == Aaron Gamble 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'. This is Aaron> not ambiguous with previous behavior where parameters to info threads Aaron> were only numbers. (spaces between 'r' and 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 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