From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29570 invoked by alias); 23 Aug 2012 16:18:04 -0000 Received: (qmail 29503 invoked by uid 22791); 23 Aug 2012 16:18:01 -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; Thu, 23 Aug 2012 16:17:45 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7NGHiL3021226 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 23 Aug 2012 12:17:44 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q7NGHhrp005527 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 23 Aug 2012 12:17:43 -0400 From: Tom Tromey To: Aaron Gamble Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Thread Name Printers References: <87ipcasomu.fsf@fleche.redhat.com> Date: Thu, 23 Aug 2012 16:18:00 -0000 In-Reply-To: (Aaron Gamble's message of "Wed, 22 Aug 2012 17:29:11 -0700") Message-ID: <87lih5popk.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/msg00667.txt.bz2 >>>>> "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 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. 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. 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? 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. 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. Tom