This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Thread Name Printers


Oops. Forgot to actually attach patch.

On Fri, Aug 24, 2012 at 3:00 PM, Aaron Gamble <agamble@google.com> wrote:
> 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 <tromey@redhat.com> wrote:
>>
>>>>>>> "Aaron" == Aaron Gamble <agamble@google.com> 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.

Attachment: thread_name_printers_2.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]