Re: [RFC] Thread Name Printers

[Tom Tromey <tromey@redhat.com>]

>>>>> "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 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