Re: [RFC] Thread Name Printers

[Aaron Gamble <agamble at google dot com>]

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.