Re: [RFC] Thread Name Printers

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Aaron" == Aaron Gamble <agamble@google.com> writes:

Aaron> This is a patch that adds thread name printers to GDB. Like
Aaron> pretty-printers, they are Python classes that augment the output of
Aaron> thread names.

Thanks.

Could you say why you implemented thread name printers instead of just
having Python code just directly set thread names?  This is already
possible by just assigning to the thread's 'name' attribute.

Maybe it is so you can enable or disable them dynamically?

More on this below.

Aaron> +Once a printer has been defined, it may be added to the global list
Aaron> +of printers in gdb.thread_printing.ThreadNamePrinter by using
Aaron> +gdb.thread_printing.ThreadNamePrinter.add_printer(@var{alias}

I tend to think this is one too many names.
How about just 'gdb.threads.add_printer'?
Or just have the base class constructor do it, and make it private?

Aaron> +@defun thread_name_printer.prepare (@var{self})
Aaron> +@value{GDBN} will call this method for each printer at the beginning of the
Aaron> +'info threads' command.
Aaron> +
Aaron> +In this method printers may gather any information and store it internally
Aaron> +for use when printing thread names. 

I think it should read "In this method the printer may ...".

Aaron> Access to gdb internal data is provided
Aaron> +by the gdb module functions.

I think this last sentence isn't needed.

Aaron> +    gdb.prepare_thread_name_printers =  ThreadNamePrinter.prepare_thread_name_printers

Extra space after the '='.

Aaron> +class ExamplePrinter(ThreadNamePrinter):
Aaron> +    """Reference class for thread name printers.

I'd prefer to have this in the documentation.

Aaron> +def register_thread_name_printer_commands():
Aaron> +    EnableThreadNamePrinter()
Aaron> +    DisableThreadNamePrinter()
Aaron> +    InfoThreadNamePrinter()
Aaron> +
Aaron> +register_thread_name_printer_commands()

No need for a separate function, just invoke them directly.

The commands probably be in python/commands/ rather than in the base
directory.  The base directory is really for library-ish functionality,
not commands.

Aaron> +  tp->thread_object = thread_obj;

I didn't see a patch to gdbthread.h.

However, if you do this, then you probably ought to change how thread
object lifetimes are handled more generally, getting rid of
threadlist_entry, etc.

Aaron> +/* Initializes the list of thread name printers.
Aaron> +   Returns a list of printers or NULL.
Aaron> +   FIXME: Returns a PyObject to non-python code
Aaron> +   Perhaps do this another way.  */
Aaron> +
Aaron> +PyObject *
Aaron> +prepare_thread_name_printers (void)

Yeah, I'm not very fond of this approach.

What is the rationale for the 'prepare' step, anyway?

Aaron> +      printers = PyObject_CallMethod (gdb_module,
Aaron> +                                      "prepare_thread_name_printers", NULL);
Aaron> +      if (printers != NULL && (printers == Py_None
Aaron> +                               || (!PyList_Check (printers)
Aaron> +                               || PyList_Size (printers) == 0)))
Aaron> +        {

If you are explicitly ignoring a Python exception, you have to clear the
exception state.

In gdb, though, it is more normal to do this with gdbpy_print_stack.

Aaron> +/* Returns TRUE if the given printer PyObject is enabled.
Aaron> +   Returns FALSE otherwise.  */
Aaron> +
Aaron> +static int
Aaron> +thread_printer_enabled (PyObject *printer)
Aaron> +{

If you're making a list of printers in the .py code, you might as well
filter there instead of having this.  It's a lot easier to do it there.

Aaron> +  PyObject *enabled = PyObject_GetAttrString(printer, "enabled");
Aaron> +  int ret = FALSE;
Aaron> +  if (enabled)
Aaron> +    {

For example, this does the wrong thing with exceptions.

Aaron> +      if (PyObject_IsTrue(enabled))

Likewise.

Aaron> +/* Gets a new thread name by applying each printer in a list of printers
Aaron> +   to a thread. If a printer returns a new name, a pointer to this name
Aaron> +   is stored in the threads name member. The name exists in the heap and
Aaron> +   will be freed when the thread is destroyed.  */
Aaron> +
Aaron> +char *
Aaron> +apply_thread_name_pretty_printers (PyObject *printers,
Aaron> +                                   char *name, struct thread_info *info)

Given that contract, I think this should take and return 'const char *'.
But see below.

Aaron> +  TRY_CATCH (except, RETURN_MASK_ALL)
Aaron> +  {
Aaron> +    py_name = PyString_FromString (name);
Aaron> +    seq = PySequence_Fast (printers, "expected sequence");
Aaron> +    len = PySequence_Size (printers);

Exception handling is missing, here and elsewhere.

I think all the code in this TRY_CATCH is calling into Python, not into gdb.
So, you don't need TRY_CATCH at all.

Aaron> +            /* This newly allocated name will be freed in free_thread.  */
Aaron> +            xfree (info->name);
Aaron> +            info->name = name;

This seems weird to me.

If a printer sets the thread's name, then this seems like a lot of code
for what amounts to an assignment.  It seems like you could already
achieve all this with code to listen to thread-creation events and set
thread names appropriately.

Aaron> diff --git a/gdb/thread.c b/gdb/thread.c
Aaron> index 7e8eec5..3147384 100644
Aaron> --- a/gdb/thread.c
Aaron> +++ b/gdb/thread.c
Aaron> @@ -34,6 +34,8 @@
Aaron>  #include "regcache.h"
Aaron>  #include "gdb.h"
Aaron>  #include "gdb_string.h"
Aaron> +#include "python/python.h"
Aaron> +#include "python/python-internal.h"

python-internal.h shouldn't generally be included in gdb.
varobj does, but it is kind of an exception.
I'd entertain other exceptions, of course, but I think in this case one
isn't needed.

Aaron> +#ifdef HAVE_PYTHON
Aaron> +  /* Get list of printers, they will initialize themselves here.  */
Aaron> +  printers = prepare_thread_name_printers ();
Aaron> +  if (printers != NULL)
Aaron> +    make_cleanup_py_decref (printers);
Aaron> +#endif

The normal approach in cases like this is to avoid #ifdef, and instead
define a dummy function that does nothing in the no-Python case.
Then, declare these interfaces in python.h.

For example, you could have prepare_thread_name_printers which just
iterates and calls the 'prepare' methods.  (If that is really needed.)
This could just be:

    void prepare_thread_name_printers (void);

Then you could have apply_thread_name_pretty_printers, just dropping the
PyObject* argument:

    char *apply_thread_name_pretty_printers (char *, struct thread_info *);

Tom