Re: [Patch v19 4/4] Add xmethod support to the Python API

[Doug Evans <xdje42 at gmail dot com>]

Siva Chandra <sivachandra@google.com> writes:

> The attached patch addresses Doug's comments on v18.

Well, almost all of them ...

> ChangeLog
>
> 2014-05-30  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * python/py-xmethods.c: New file.
>         * python/py-objfile.c (objfile_object): New field 'xmethods'.
>         (objfpy_dealloc): XDECREF on the new xmethods field.
>         (objfpy_new, objfile_to_objfile_object): Initialize xmethods
>         field.
>         (objfpy_get_xmethods): New function.
>         (objfile_getset): New entry 'xmethods'.
>         * python/py-progspace.c (pspace_object): New field 'xmethods'.
>         (pspy_dealloc): XDECREF on the new xmethods field.
>         (pspy_new, pspace_to_pspace_object): Initialize xmethods
>         field.
>         (pspy_get_xmethods): New function.
>         (pspace_getset): New entry 'xmethods'.
>         * python/python-internal.h: Add declarations for new functions.
>         * python/python.c (_initialize_python): Invoke
>         gdbpy_initialize_xmethods.
>         * python/lib/gdb/__init__.py (xmethods): New
>         attribute.
>         * python/lib/gdb/xmethod.py: New file.
>         * python/lib/gdb/command/xmethods.py: New file.
>
>         testuite/
>         * gdb.python/py-xmethods.cc: New testcase to test xmethods.
>         * gdb.python/py-xmethods.exp: New tests to test xmethods.
>         * gdb.python/py-xmethods.py: Python script supporting the
>         new testcase and tests.
>
> diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
> new file mode 100644
> index 0000000..a4d6d91
> --- /dev/null
> +++ b/gdb/python/py-xmethods.c
> +/* Implementation of free_xmethod_worker_data for Python.  */
> +
> +void
> +gdbpy_free_xmethod_worker_data (const struct extension_language_defn *extlang,
> +				void *data)
> +{
> +  struct gdbpy_worker_data *worker_data = data;
> +
> +  gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
> +
> +  Py_DECREF (worker_data->worker);
> +  Py_DECREF (worker_data->this_type);
> +  xfree (worker_data);
> +}
> +
> +/* Implementation of clone_xmethod_worker_data for Python.  */
> +
> +void *
> +gdbpy_clone_xmethod_worker_data (const struct extension_language_defn *extlang,
> +				 void *data)
> +{
> +  struct gdbpy_worker_data *worker_data = data, *new_data;
> +
> +  gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
> +
> +  new_data = XCNEW (struct gdbpy_worker_data);
> +  new_data->worker = worker_data->worker;
> +  new_data->this_type = worker_data->this_type;
> +  Py_INCREF (new_data->worker);
> +  Py_INCREF (new_data->this_type);
> +
> +  return new_data;
> +}

I ran the testsuite and gdb was crashing.
These two requested changes were missed.

diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index a4d6d91..44bb460 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -53,12 +53,18 @@ gdbpy_free_xmethod_worker_data (const struct extension_language_defn *extlang,
 				void *data)
 {
   struct gdbpy_worker_data *worker_data = data;
+  struct cleanup *cleanups;
 
   gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
 
+  /* We don't do much here, but we still need the GIL.  */
+  cleanups = ensure_python_env (get_current_arch (), current_language);
+
   Py_DECREF (worker_data->worker);
   Py_DECREF (worker_data->this_type);
   xfree (worker_data);
+
+  do_cleanups (cleanups);
 }
 
 /* Implementation of clone_xmethod_worker_data for Python.  */
@@ -68,15 +74,21 @@ gdbpy_clone_xmethod_worker_data (const struct extension_language_defn *extlang,
 				 void *data)
 {
   struct gdbpy_worker_data *worker_data = data, *new_data;
+  struct cleanup *cleanups;
 
   gdb_assert (worker_data->worker != NULL && worker_data->this_type != NULL);
 
+  /* We don't do much here, but we still need the GIL.  */
+  cleanups = ensure_python_env (get_current_arch (), current_language);
+
   new_data = XCNEW (struct gdbpy_worker_data);
   new_data->worker = worker_data->worker;
   new_data->this_type = worker_data->this_type;
   Py_INCREF (new_data->worker);
   Py_INCREF (new_data->this_type);
 
+  do_cleanups (cleanups);
+
   return new_data;
 }
 


Also, this request was missed too.

@@ -480,6 +492,7 @@ gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
 
   /* Add the type of 'this' as the first argument.  */
   obj_type = type_object_to_type (worker_data->this_type);
+  /* FIXME: Add comment explaining why passing 1 for "is const" is ok here.  */
   type_array[0] = make_cv_type (1, 0, lookup_pointer_type (obj_type), NULL);
   *nargs = i;
   *arg_types = type_array;

As a reader I look at the call to make_cv_type and ask myself "why is
that always necessarily ok?"  A comment explaining why would prevent me,
the reader, from having to put in time digging deeper.


Ok with those changes.