Re: [RFC] Thread Name Printers

[Aaron Gamble <agamble at google dot com>]

Once again, thanks for the feedback.

On Wed, Aug 22, 2012 at 12:38 PM, Tom Tromey <tromey@redhat.com> wrote:
>
>>>>>> "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?
How about gdb.thread_printing.add_printer? It also seems fine to me to
have a method in the base class.
e.g.
myclass().add_printer("my printer")
# instantiates class and adds it to list of printers with alias "my printer"

>
> 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.
Ah, this was missed out by accident.
>
> 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.
Not entirely sure what you mean here, but I will look into it. My
unfamiliarity with the codebase is to blame.
>
> 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?

This is for the printers to do any one time setup before printing a
list of threads. A common case I can see is if the printer needs to
examine memory and traverse something like a linked list. Without a
call like this, or an indicator in print_thread, there is no way for a
printer to know the different between multiple calls to info threads.


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

Ah, sorry, I'm unfamiliar with the Python C api. I will add a call to
gdbpy_print_stack when printers == NULL. Are you also worried about
the potential exceptions raised in PyList_Check and PyList_Size? I
suppose in that case just having a call to gdbpy_print_stack at the
end of this function or in each case of printers == NULL would be
sufficient.

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

The thing is that we do not create a new list. The same list is
returned instead. I suppose if we add the check to
prepare_thread_name_printers and just return a new list, that would be
fine as well.

>
> 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.
The problem with only setting names on thread-creation events is that
a library managing threads and assigning internal thread names we may
wish to print will potentially not have set the name at the time the
thread is created.
e.g.
1. thread created.
2. thread-created event in gdb. (no name available yet)
3. library sets internal name for thread.

The thread name is assigned here because I think it is safe to assume
that once a thread has a name, that name will not change. Also, if a
user assigns a name via 'thread name foo', they would want that name
to override any thread name printer.

>
> 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
Ok. I'll use a global static variable for the list of printers in
python/py-infthread.c and use dummy functions for the #ifdef stuff.