From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16314 invoked by alias); 24 Aug 2012 22:00:40 -0000 Received: (qmail 15959 invoked by uid 22791); 24 Aug 2012 22:00:31 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-iy0-f169.google.com (HELO mail-iy0-f169.google.com) (209.85.210.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 24 Aug 2012 22:00:13 +0000 Received: by iahk25 with SMTP id k25so4912843iah.0 for ; Fri, 24 Aug 2012 15:00:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=k9p8jehIJe5M4JERF8BSmel3+fO+Z44fC3Z1OxfjXtg=; b=JgMy/S+zHLPh+W+yB7jMZSnS5ONBT4Q0JF/zBPXFTrUHx+unBm4h6ZsvP8eOvD8HuF g1cPzuSMG+bjk2obRwcGkpHfSAVzFHzLfhEreusffQIzgOzANKJFYV747NXetivsKYh2 zwzc7zSdAMIQdcgtfw2GN16CNDsGEdolhIWxeqzxYvJguTxR1nqYUiIxVNkASweB2DjX l6eDzhID820jS9HJBCIlsQZiNnNKwwaLHs5Hgvb4L46CBL15x6Ajde0Jxkl7aQCveaFE SQrsfcrI9jGw0R83Hsys6MI7zG+GJPPC20RCUNKnQ6osDSBwGjEyEj+mz8t32AHcS4lQ cyXw== Received: by 10.43.43.194 with SMTP id ud2mr5669623icb.13.1345845613096; Fri, 24 Aug 2012 15:00:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.43.43.194 with SMTP id ud2mr5669612icb.13.1345845612935; Fri, 24 Aug 2012 15:00:12 -0700 (PDT) Received: by 10.50.104.233 with HTTP; Fri, 24 Aug 2012 15:00:12 -0700 (PDT) In-Reply-To: <87lih5popk.fsf@fleche.redhat.com> References: <87ipcasomu.fsf@fleche.redhat.com> <87lih5popk.fsf@fleche.redhat.com> Date: Fri, 24 Aug 2012 22:00:00 -0000 Message-ID: Subject: Re: [RFC] Thread Name Printers From: Aaron Gamble To: Tom Tromey Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Gm-Message-State: ALoCoQlSLKeVXlbJ71OZZ0Ojyz9J589bIrzWYgtwaTU8KmNXTKqpgerQ3xnZl0hfe77xJB3GoqfW2EWd7a/4WMvHFqTfCDgmtayBR6rl1gVG+HaLLNg7oV4/uwaoJRfG5UdjxZKCgFxpYcWcvE11BRv1NvFq3KkGxfx5QRK+fXPP6bVxHdQAwCgwicgDCa8Jda+lAWzNX/+sACZRNAz3DlaKFNKxL0w8VA== X-IsSubscribed: yes 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/msg00772.txt.bz2 New patch. See inline comments. (FYI today is the last day of my internship, so this e-mail address will be dead very soon. I can be reached at invalidfunction or jaarongamble both at gmail.com) gdb/ChangeLog: * data-directory/Makefile.in: Add gdb/thread_printing.py. * doc/gdb.texinfo: Add section about thread name printers in Python section. Add reference to thread name printers in 'info threads' section. * python/lib/gdb/__init__.py: Import ThreadNamePrinter. * python/lib/gdb/thread_printing.py: new file. (class ThreadNamePrinter): base class, contains helper functions and list of registered printers. (class ExamplePrinter): example printer class for reference. (class EnableThreadNamePrinter, DisableThreadNamePrinter, InfoThreadNamePrinter): gdb.commands for managing and getting information about thread name printers. (register_thread_name_printer_commands): Registers thread name printer commands. * python/py-inferior.c (add_thread_object): Save pointer to thread_object in thread_info. * python/py-infthread.c (prepare_thread_name_printers): Prepares all the registered thread name printers and returns a list of the printers. Called once by thread.c:print_thread_info. (thread_printer_enabled): Returns TRUE if printer enabled. (apply_thread_name_pretty_printers): Iterates through the list of printers calling print_thread until one returns a string. Called for each thread by thread.c:print_thread_info * python/python-internal.h: Extern gdbpy_print_thread_cst PyObject string object. * pyhon/python.c: Declare and assign gdbpy_print_thread_cst. * testsuite/gdb.python/Makefile.in: add py-threadprettyprint. * testsuite/gdb.python/py-threadprettyprint.c: Basic threaded program for testing thread name printing. * testsuite/gdb.python/py-threadprettyprint.exp: Tests for thread name printers. * thread.c (print_thread_info): Calls prepare_thread_name_printers to initialize printers and get list of printers. Calls apply_thread_name_pretty_printers for each thread if no name for thread is already stored. (_initialize_thread): Add aliases for 'info threads' to disambiguate between 'info thread-name-printer' On Thu, Aug 23, 2012 at 9:17 AM, Tom Tromey wrote: > >>>>>> "Aaron" == Aaron Gamble writes: > > Tom> How about just 'gdb.threads.add_printer'? > Tom> Or just have the base class constructor do it, and make it private? > > Aaron> How about gdb.thread_printing.add_printer? It also seems fine to me to > Aaron> have a method in the base class. > Aaron> e.g. > Aaron> myclass().add_printer("my printer") > Aaron> # instantiates class and adds it to list of printers with alias "my printer" > > It's sort of a norm in gdb to have the instantiation of the class > install the CLI bits as a side effect. E.g., Command and Parameter do > this. I don't insist on it, though. I just stuck with myclass().add_printer("my alias") for now. > > I like the name gdb.threads over gdb.thread_printing, just because I > assume we'll want to have more thread utility code, and this would > provide a spot to put it. Done. changed to gdb.threads. > Tom> However, if you do this, then you probably ought to change how thread > Tom> object lifetimes are handled more generally, getting rid of > Tom> threadlist_entry, etc. > > Aaron> Not entirely sure what you mean here, but I will look into it. My > Aaron> unfamiliarity with the codebase is to blame. > > No worries. > > The current code is written in a way to let us associate a gdb.Thread > object with a thread_info in an indirect way. However, this probably > doesn't perform extremely well if there are many threads. > > So, your approach is superior. However, I think it is best to do the > conversion completely. This could be a separate refactoring patch -- > just rip out the old threadlist_entry stuff and replace it with your > approach. You can see how we handled this in breakpoint.h (search for > struct breakpoint_object) to make it work without including python.h > everywhere. I will leave this for dje@ to do. > Tom> What is the rationale for the 'prepare' step, anyway? > > Aaron> This is for the printers to do any one time setup before printing a > Aaron> list of threads. A common case I can see is if the printer needs to > Aaron> examine memory and traverse something like a linked list. Without a > Aaron> call like this, or an indicator in print_thread, there is no way for a > Aaron> printer to know the different between multiple calls to info threads. > > Ok, I see. > Should there also be an "unprepare" step so that these objects can > release resources after "info threads" exits? That's a good point. I've added a cleanup step. > Aaron> Ah, sorry, I'm unfamiliar with the Python C api. I will add a call to > Aaron> gdbpy_print_stack when printers == NULL. Are you also worried about > Aaron> the potential exceptions raised in PyList_Check and PyList_Size? I > Aaron> suppose in that case just having a call to gdbpy_print_stack at the > Aaron> end of this function or in each case of printers == NULL would be > Aaron> sufficient. > > Nearly all Python functions have to have their result checked for error. > This often leads to spaghetti code, unfortunately, but that's how it is. > > Exactly how to handle the error depends on the situation. > > In "Python-facing" code, the typical thing to do is propagate the error > -- release locally-acquired resources and return NULL (or -1 or whatever > it is for the current context). > > In "gdb-facing" code, usually we call gdbpy_print_stack. This isn't > ideal, since in many situations I think it would be preferable to > convert the Python exception to a gdb exception. > > In your particular case I think it is friendliest to the user to call > gdbpy_print_stack, even assuming we implement real exception handling, > just because this approach means that some bad Python code won't make > "info threads" fail. Added stack trace printing to "handle" exceptions. > Tom> If you're making a list of printers in the .py code, you might as well > Tom> filter there instead of having this. It's a lot easier to do it there. > > Aaron> The thing is that we do not create a new list. The same list is > Aaron> returned instead. I suppose if we add the check to > Aaron> prepare_thread_name_printers and just return a new list, that would be > Aaron> fine as well. > > I got a little lost here. > > If there is a single list, where disabled items are filtered when > iterating over it, then don't bother returning a list at all, just use > the global one from the .py code. > > Aaron> The problem with only setting names on thread-creation events is that > Aaron> a library managing threads and assigning internal thread names we may > Aaron> wish to print will potentially not have set the name at the time the > Aaron> thread is created. > > Ok, makes sense. > > Aaron> e.g. > Aaron> 1. thread created. > Aaron> 2. thread-created event in gdb. (no name available yet) > Aaron> 3. library sets internal name for thread. > > In this scenario I was picturing that the python code would set a > breakpoint to capture the interesting event. But I can accept that this > may not always be desirable. > > Aaron> The thread name is assigned here because I think it is safe to assume > Aaron> that once a thread has a name, that name will not change. Also, if a > Aaron> user assigns a name via 'thread name foo', they would want that name > Aaron> to override any thread name printer. > > Ok, thanks. > > Aaron> Ok. I'll use a global static variable for the list of printers in > Aaron> python/py-infthread.c and use dummy functions for the #ifdef stuff. > > IIUC it could all just be in the python code. You are entirely right. I changed this to just keep the printer in the Python world, along with some other changes that cleans things up.