From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30636 invoked by alias); 22 Aug 2012 19:39:20 -0000 Received: (qmail 30628 invoked by uid 22791); 22 Aug 2012 19:39:19 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS 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 19:38:52 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7MJcopt008749 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 22 Aug 2012 15:38:50 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q7MJcnrO027294 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 22 Aug 2012 15:38:49 -0400 From: Tom Tromey To: Aaron Gamble Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Thread Name Printers References: Date: Wed, 22 Aug 2012 19:39:00 -0000 In-Reply-To: (Aaron Gamble's message of "Tue, 21 Aug 2012 13:58:51 -0700") Message-ID: <87ipcasomu.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/msg00632.txt.bz2 >>>>> "Aaron" == Aaron Gamble writes: Aaron> This is a patch that adds thread name printers to GDB. Like Aaron> pretty-printers, they are Python classes that augment the output of Aaron> thread names. Thanks. Could you say why you implemented thread name printers instead of just having Python code just directly set thread names? This is already possible by just assigning to the thread's 'name' attribute. Maybe it is so you can enable or disable them dynamically? More on this below. Aaron> +Once a printer has been defined, it may be added to the global list Aaron> +of printers in gdb.thread_printing.ThreadNamePrinter by using Aaron> +gdb.thread_printing.ThreadNamePrinter.add_printer(@var{alias} I tend to think this is one too many names. How about just 'gdb.threads.add_printer'? Or just have the base class constructor do it, and make it private? Aaron> +@defun thread_name_printer.prepare (@var{self}) Aaron> +@value{GDBN} will call this method for each printer at the beginning of the Aaron> +'info threads' command. Aaron> + Aaron> +In this method printers may gather any information and store it internally Aaron> +for use when printing thread names. I think it should read "In this method the printer may ...". Aaron> Access to gdb internal data is provided Aaron> +by the gdb module functions. I think this last sentence isn't needed. Aaron> + gdb.prepare_thread_name_printers = ThreadNamePrinter.prepare_thread_name_printers Extra space after the '='. Aaron> +class ExamplePrinter(ThreadNamePrinter): Aaron> + """Reference class for thread name printers. I'd prefer to have this in the documentation. Aaron> +def register_thread_name_printer_commands(): Aaron> + EnableThreadNamePrinter() Aaron> + DisableThreadNamePrinter() Aaron> + InfoThreadNamePrinter() Aaron> + Aaron> +register_thread_name_printer_commands() No need for a separate function, just invoke them directly. The commands probably be in python/commands/ rather than in the base directory. The base directory is really for library-ish functionality, not commands. Aaron> + tp->thread_object = thread_obj; I didn't see a patch to gdbthread.h. However, if you do this, then you probably ought to change how thread object lifetimes are handled more generally, getting rid of threadlist_entry, etc. Aaron> +/* Initializes the list of thread name printers. Aaron> + Returns a list of printers or NULL. Aaron> + FIXME: Returns a PyObject to non-python code Aaron> + Perhaps do this another way. */ Aaron> + Aaron> +PyObject * Aaron> +prepare_thread_name_printers (void) Yeah, I'm not very fond of this approach. What is the rationale for the 'prepare' step, anyway? Aaron> + printers = PyObject_CallMethod (gdb_module, Aaron> + "prepare_thread_name_printers", NULL); Aaron> + if (printers != NULL && (printers == Py_None Aaron> + || (!PyList_Check (printers) Aaron> + || PyList_Size (printers) == 0))) Aaron> + { If you are explicitly ignoring a Python exception, you have to clear the exception state. In gdb, though, it is more normal to do this with gdbpy_print_stack. Aaron> +/* Returns TRUE if the given printer PyObject is enabled. Aaron> + Returns FALSE otherwise. */ Aaron> + Aaron> +static int Aaron> +thread_printer_enabled (PyObject *printer) Aaron> +{ If you're making a list of printers in the .py code, you might as well filter there instead of having this. It's a lot easier to do it there. Aaron> + PyObject *enabled = PyObject_GetAttrString(printer, "enabled"); Aaron> + int ret = FALSE; Aaron> + if (enabled) Aaron> + { For example, this does the wrong thing with exceptions. Aaron> + if (PyObject_IsTrue(enabled)) Likewise. Aaron> +/* Gets a new thread name by applying each printer in a list of printers Aaron> + to a thread. If a printer returns a new name, a pointer to this name Aaron> + is stored in the threads name member. The name exists in the heap and Aaron> + will be freed when the thread is destroyed. */ Aaron> + Aaron> +char * Aaron> +apply_thread_name_pretty_printers (PyObject *printers, Aaron> + char *name, struct thread_info *info) Given that contract, I think this should take and return 'const char *'. But see below. Aaron> + TRY_CATCH (except, RETURN_MASK_ALL) Aaron> + { Aaron> + py_name = PyString_FromString (name); Aaron> + seq = PySequence_Fast (printers, "expected sequence"); Aaron> + len = PySequence_Size (printers); Exception handling is missing, here and elsewhere. I think all the code in this TRY_CATCH is calling into Python, not into gdb. So, you don't need TRY_CATCH at all. Aaron> + /* This newly allocated name will be freed in free_thread. */ Aaron> + xfree (info->name); Aaron> + info->name = name; This seems weird to me. If a printer sets the thread's name, then this seems like a lot of code for what amounts to an assignment. It seems like you could already achieve all this with code to listen to thread-creation events and set thread names appropriately. Aaron> diff --git a/gdb/thread.c b/gdb/thread.c Aaron> index 7e8eec5..3147384 100644 Aaron> --- a/gdb/thread.c Aaron> +++ b/gdb/thread.c Aaron> @@ -34,6 +34,8 @@ Aaron> #include "regcache.h" Aaron> #include "gdb.h" Aaron> #include "gdb_string.h" Aaron> +#include "python/python.h" Aaron> +#include "python/python-internal.h" python-internal.h shouldn't generally be included in gdb. varobj does, but it is kind of an exception. I'd entertain other exceptions, of course, but I think in this case one isn't needed. Aaron> +#ifdef HAVE_PYTHON Aaron> + /* Get list of printers, they will initialize themselves here. */ Aaron> + printers = prepare_thread_name_printers (); Aaron> + if (printers != NULL) Aaron> + make_cleanup_py_decref (printers); Aaron> +#endif The normal approach in cases like this is to avoid #ifdef, and instead define a dummy function that does nothing in the no-Python case. Then, declare these interfaces in python.h. For example, you could have prepare_thread_name_printers which just iterates and calls the 'prepare' methods. (If that is really needed.) This could just be: void prepare_thread_name_printers (void); Then you could have apply_thread_name_pretty_printers, just dropping the PyObject* argument: char *apply_thread_name_pretty_printers (char *, struct thread_info *); Tom