Re: [PATCH v2 2/7] Add `thread_from_thread_handle' function to (Python) gdb module

[Phil Muldoon <pmuldoon at redhat dot com>]

On 09/04/17 07:06, Kevin Buettner wrote:
> gdb/ChangeLog:
>     	* python/py-infthread.c (gdbpy_thread_from_thread_handle): New
>     	function.
>     	* python/python-internal.h (thread_object_type): Declare.
>     	(gdbpy_thread_from_thread_handle): Declare.
>     	* python/python.c (thread_from_thread_handle): Register.
> ---
>  gdb/python/py-infthread.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |  3 +++
>  gdb/python/python.c          |  4 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 5482bf9..5739984 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -294,6 +294,47 @@ gdbpy_selected_thread (PyObject *self, PyObject *args)
>    Py_RETURN_NONE;
>  }
>  
> +/* Implementation of gdb.thread_from_thread_handle (handle) 
> +                        ->  gdb.InferiorThread.  */
> +
> +PyObject *
> +gdbpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  PyObject *handle_obj, *result;
> +  static char *keywords[] = { "thread_handle", NULL };
> +
> +  if (! PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj))
> +    return NULL;
> +
> +  result = Py_None;
> +
> +  if (gdbpy_is_value_object (handle_obj))
> +    {
> +      TRY
> +	{
> +	  struct thread_info *thread_info;
> +	  struct value *val = value_object_to_value (handle_obj);
> +
> +	  thread_info = find_thread_by_handle (val);
> +	  if (thread_info != NULL)
> +	    {
> +	      result = (PyObject *) find_thread_object (thread_info->ptid);
> +	      if (result)
> +		Py_INCREF (result);

Initially this caught me off guard and wondered why find_thread_object required a reference count. But it does return a borrowed reference, though it does not seem to be documented as such.  I think the code surrounding this should be documented, but not in this patch. I'll add it to one of my TODOs.
> +	    }
> +	}
> +      CATCH (except, RETURN_MASK_ALL)
> +	{
> +	  if (except.reason < 0)
> +	    gdbpy_convert_exception (except);
> +	  return NULL;
> +	}
> +      END_CATCH
> +    }
> +
> +  return result;
> +}

The Python bits look good to me, as do the tests and documentation.  Please wait on a maintainer to give the overall patch a yes/no before proceeding.

Cheers

Phil