[Patch v18 4/4] Add xmethod support to the Python API
Doug Evans
xdje42@gmail.com
Sun May 25 23:31:00 GMT 2014
Siva Chandra <sivachandra@google.com> writes:
> The attached patch addresses all of Doug's comments for this part from
> last time. The previous version for this part was not numbered 17. I
> am syncing all parts to v18 now to avoid confusion.
>
> ChangeLog:
> 2014-05-23 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.
Still a few things I think we need to address.
Sorry for not catching them sooner.
I think they're simple though.
> diff --git a/gdb/python/lib/gdb/command/xmethods.py b/gdb/python/lib/gdb/command/xmethods.py
> new file mode 100644
> index 0000000..f61e7fb
> --- /dev/null
> +++ b/gdb/python/lib/gdb/command/xmethods.py
> @@ -0,0 +1,272 @@
> + [...]
> +
> +class DisableXMethod(gdb.Command):
> + """GDB command to disable a specified (group of) xmethod(s).
> +
> + Usage: disable xmethod [locus-regexp [name-regexp]]
> +
> + LOCUS-REGEXP is a regular expression matching the location of the
> + xmethod matcherss. If it is omitted, all registered xmethod matchers
matchers.
> diff --git a/gdb/python/lib/gdb/xmethod.py b/gdb/python/lib/gdb/xmethod.py
> new file mode 100644
> index 0000000..9d0deff
> --- /dev/null
> +++ b/gdb/python/lib/gdb/xmethod.py
> @@ -0,0 +1,254 @@
> + [...]
> +
> + def get_arg_types(self):
> + """Return arguments types of an xmethod.
> +
> + A sequence of gdb.Type objects corresponding to the arguments of the
> + xmethod are returned. If the xmethod takes no arguments, then 'None'
> + or an empty sequence is returned. If the xmethod takes only a single
> + argument, then a gdb.Type object or a sequence with a single gdb.Type
> + element is returned.
> + """
> + raise NotImplementedError("XMethod get_arg_types")
raise NotImplementedError("XMethodWorker get_arg_types")
> +
> + def invoke(self, obj, args):
> + """Invoke the xmethod.
> +
> + Args:
> + obj: The gdb.Value of the object on which the method is to be
> + invoked.
> + args: The tuple of arguments to the method. Each element of the
> + tuple is a gdb.Value object.
> +
> + Returns:
> + A gdb.Value corresponding to the value returned by the xmethod.
> + Returns 'None' if the method does not return anything.
> + """
> + raise NotImplementedError("XMethod invoke")
raise NotImplementedError("XMethodWorker invoke")
> +# A helper function for register_xmethod_matcher which returns an error
> +# object if MATCHER is not having the requisite attributes in the proper
> +# format.
> +
> +def validate_xmethod_matcher(matcher):
I think all the internal routines in this file should have an "_" prefix.
My patch in the 1/4 reply lists them all.
I don't feel as strongly about gdb/command/xmethods.py since it isn't
likely to be imported for random usage.
> diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
> new file mode 100644
> index 0000000..0868c9f
> --- /dev/null
> +++ b/gdb/python/py-xmethods.c
> @@ -0,0 +1,648 @@
> +/* Support for debug methods in Python.
> +
> + Copyright (C) 2013-2014 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include "extension-priv.h"
> +#include "objfiles.h"
> +#include "value.h"
> +#include "language.h"
> +
> +#include "python.h"
> +#include "python-internal.h"
> +
> +static const char enabled_field_name[] = "enabled";
> +static const char match_method_name[] = "match";
> +static const char get_arg_types_method_name[] = "get_arg_types";
> +static const char invoke_method_name[] = "invoke";
> +static const char matchers_attr_str[] = "xmethods";
> +
> +static PyObject *py_match_method_name = NULL;
> +static PyObject *py_get_arg_types_method_name = NULL;
> +static PyObject *py_invoke_method_name = NULL;
> +
> +struct gdbpy_worker_data
> +{
> + PyObject *worker;
> + PyObject *this_type;
> +};
> +
> +static struct xmethod_worker *new_python_xmethod_worker
> + (PyObject *item, PyObject *py_obj_type);
> +
> +/* Implementation of free_xmethod_worker_data for Python. */
> +
> +void
> +gdbpy_free_xmethod_worker_data
> + (const struct extension_language_defn *extlang, void *data)
The line length here can be handled using gdb's existing formatting
rules, so let's do that.
gdbpy_free_xmethod_worker_data (const struct extension_language_defn *extlang,
void *data)
Plus this function needs to acquire the GIL. gdb can crash otherwise.
I went with the canonical ensure_python_env in the patch in the 1/4 reply,
though I see py-breakpoint.c has some direct calls to
PyGILState_Ensure, PyGILState_Release.
> +{
> + 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)
Similarly,
gdbpy_clone_xmethod_worker_data (const struct extension_language_defn *extlang,
void *data)
And similarly this function should, I think, acquire the GIL.
> +{
> + 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;
> +}
> +
> + [...]
> +
> +/* Implementation of get_matching_xmethod_workers for Python. */
> +
> +enum ext_lang_rc
> +gdbpy_get_matching_xmethod_workers
> + (const struct extension_language_defn *extlang,
> + struct type *obj_type, const char *method_name,
> + xmethod_worker_vec **dm_vec)
> +{
> + struct cleanup *cleanups;
> + struct objfile *objfile;
> + VEC (xmethod_worker_ptr) *worker_vec = NULL;
> + PyObject *py_type, *py_progspace;
> + PyObject *py_xmethod_matcher_list = NULL, *list_iter, *matcher;
> +
> + gdb_assert (obj_type != NULL && method_name != NULL);
> +
> + cleanups = ensure_python_env (get_current_arch (), current_language);
> +
> + py_type = type_to_type_object (obj_type);
> + if (py_type == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (py_type);
> +
> + /* Create an empty list of debug methods. */
> + py_xmethod_matcher_list = PyList_New (0);
> + if (py_xmethod_matcher_list == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + /* Gather debug method matchers registered with the object files. */
I'd extend this comment thusly:
/* Gather debug method matchers registered with the object files.
This could be done differently by iterating over each objfile's matcher
list individually, but there's no data yet to show it's needed. */
> + ALL_OBJFILES (objfile)
> + {
> + PyObject *py_objfile = objfile_to_objfile_object (objfile);
> + PyObject *objfile_matchers, *temp = py_xmethod_matcher_list;
> +
> + if (py_objfile == NULL)
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (py_xmethod_matcher_list);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + objfile_matchers = objfpy_get_xmethods (py_objfile, NULL);
> + py_xmethod_matcher_list = PySequence_Concat (temp,
> + objfile_matchers);
Left over TODO from xmethod rename I think:
py_xmethod_matcher_list = PySequence_Concat (temp, objfile_matchers);
> + Py_DECREF (temp);
> + Py_DECREF (objfile_matchers);
> + if (py_xmethod_matcher_list == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> +
> + /* Gather debug methods matchers registered with the current program
> + space. */
> + py_progspace = pspace_to_pspace_object (current_program_space);
> + if (py_progspace != NULL)
> + {
> + PyObject *temp = py_xmethod_matcher_list;
> + PyObject *pspace_matchers = pspy_get_xmethods (py_progspace, NULL);
> +
> + py_xmethod_matcher_list = PySequence_Concat (temp, pspace_matchers);
> + Py_DECREF (temp);
> + Py_DECREF (pspace_matchers);
> + if (py_xmethod_matcher_list == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> + else
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (py_xmethod_matcher_list);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + /* Gather debug method matchers registered globally. */
> + if (gdb_python_module != NULL
> + && PyObject_HasAttrString (gdb_python_module, matchers_attr_str))
> + {
> + PyObject *gdb_matchers;
> + PyObject *temp = py_xmethod_matcher_list;
> +
> + gdb_matchers = PyObject_GetAttrString (gdb_python_module,
> + matchers_attr_str);
> + if (gdb_matchers != NULL)
> + {
> + py_xmethod_matcher_list = PySequence_Concat (temp,
> + gdb_matchers);
Left over TODO from xmethod rename I think:
py_xmethod_matcher_list = PySequence_Concat (temp, gdb_matchers);
> + Py_DECREF (temp);
> + Py_DECREF (gdb_matchers);
> + if (py_xmethod_matcher_list == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> + else
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (py_xmethod_matcher_list);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> +
> + /* Safe to make a cleanup for py_xmethod_matcher_list now as it
> + will not change any more. */
> + make_cleanup_py_decref (py_xmethod_matcher_list);
> +
> + list_iter = PyObject_GetIter (py_xmethod_matcher_list);
> + if (list_iter == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + while ((matcher = PyIter_Next (list_iter)) != NULL)
> + {
> + PyObject *match_result = invoke_match_method (matcher, py_type,
> + method_name);
> +
> + if (match_result == NULL)
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (matcher);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + if (match_result == Py_None)
> + ; /* This means there was no match. */
> + else if (PySequence_Check (match_result))
> + {
> + PyObject *iter = PyObject_GetIter (match_result);
> + PyObject *py_worker;
> +
> + if (iter == NULL)
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (matcher);
> + Py_DECREF (match_result);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + while ((py_worker = PyIter_Next (iter)) != NULL)
> + {
> + struct xmethod_worker *worker;
> +
Regarding not requiring user code to "enabled" handling:
Can we check the enabled flag here before pushing?
> + worker = new_python_xmethod_worker (py_worker, py_type);
> + VEC_safe_push (xmethod_worker_ptr, worker_vec, worker);
> + Py_DECREF (py_worker);
> + }
> + Py_DECREF (iter);
> + /* Report any error that could have occurred while iterating. */
> + if (PyErr_Occurred ())
> + {
> + gdbpy_print_stack ();
> + Py_DECREF (matcher);
> + Py_DECREF (match_result);
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> + else
> + {
> + struct xmethod_worker *worker;
> +
Also here? [check the "enabled" flag before pushing]
> + worker = new_python_xmethod_worker (match_result, py_type);
> + VEC_safe_push (xmethod_worker_ptr, worker_vec, worker);
> + }
> +
> + Py_DECREF (match_result);
> + Py_DECREF (matcher);
> + }
> + Py_DECREF (list_iter);
> + /* Report any error that could have occurred while iterating. */
> + if (PyErr_Occurred ())
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + do_cleanups (cleanups);
> + *dm_vec = worker_vec;
> +
> + return EXT_LANG_RC_OK;
> +}
> +
> +/* Implementation of get_xmethod_arg_types for Python. */
> +
> +enum ext_lang_rc
> +gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
> + struct xmethod_worker *worker,
> + int *nargs, struct type ***arg_types)
My notes say this arg list can be re-indented.
> +{
> + struct gdbpy_worker_data *worker_data = worker->data;
> + PyObject *py_worker = worker_data->worker;
> + PyObject *get_arg_types_method;
> + PyObject *py_argtype_list, *list_iter = NULL, *item;
> + struct cleanup *cleanups;
> + struct type **type_array, *obj_type;
> + int i = 1, arg_count;
> +
> + /* Set nargs to -1 so that any premature return from this function returns
> + an invalid/unusable number of arg types. */
> + *nargs = -1;
> +
> + cleanups = ensure_python_env (get_current_arch (), current_language);
> +
> + get_arg_types_method = PyObject_GetAttrString (py_worker,
> + get_arg_types_method_name);
> + if (get_arg_types_method == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (get_arg_types_method);
> +
> + py_argtype_list = PyObject_CallMethodObjArgs (py_worker,
> + py_get_arg_types_method_name,
> + NULL);
> + if (py_argtype_list == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (py_argtype_list);
> + if (py_argtype_list == Py_None)
> + arg_count = 0;
> + else if (PySequence_Check (py_argtype_list))
> + {
> + arg_count = PySequence_Size (py_argtype_list);
> + if (arg_count == -1)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + list_iter = PyObject_GetIter (py_argtype_list);
> + if (list_iter == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (list_iter);
> + }
> + else
> + arg_count = 1;
> +
> + /* Include the 'this' argument in the size. */
> + type_array = XCNEWVEC (struct type *, arg_count + 1);
> + i = 1;
> + if (list_iter != NULL)
> + {
> + while ((item = PyIter_Next (list_iter)) != NULL)
> + {
> + struct type *arg_type = type_object_to_type (item);
> +
> + Py_DECREF (item);
> + if (arg_type == NULL)
> + {
> + PyErr_SetString (PyExc_TypeError,
> + _("Arg type returned by the get_arg_types "
> + "method of a debug method worker object is "
> + "not a gdb.Type object."));
> + break;
> + }
> +
> + type_array[i] = arg_type;
> + i++;
> + }
> + }
> + else if (arg_count == 1)
> + {
> + /* py_argtype_list is not actually a list but a single gdb.Type
> + object. */
> + struct type *arg_type = type_object_to_type (py_argtype_list);
> +
> + if (arg_type == NULL)
> + PyErr_SetString (PyExc_TypeError,
> + _("Arg type returned by the get_arg_types method "
> + "of a debug method worker object is not a gdb.Type "
> + "object."));
Wrap the above PyErr_SetString call in {} since it's more than one line.
> + else
> + {
> + type_array[1] = arg_type;
I'd use "i" here instead of "1".
> + i++;
> + }
> + }
> + if (PyErr_Occurred ())
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> + xfree (type_array);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + /* Add the type of 'this' as the first argument. */
> + obj_type = type_object_to_type (worker_data->this_type);
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;
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_OK;
> +}
> +
> +/* Implementation of invoke_xmethod for Python. */
> +
> +enum ext_lang_rc
> +gdbpy_invoke_xmethod (const struct extension_language_defn *extlang,
> + struct xmethod_worker *worker,
> + struct value *obj, struct value **args, int nargs,
> + struct value **result)
My notes say this arg list can be reindented.
> +{
> + int i;
> + struct cleanup *cleanups;
> + PyObject *py_value_obj, *py_arg_tuple, *py_result;
> + PyObject *invoke_method;
> + struct type *obj_type, *this_type;
> + struct value *res = NULL;
> + struct gdbpy_worker_data *worker_data = worker->data;
> + PyObject *xmethod_worker = worker_data->worker;
> +
> + cleanups = ensure_python_env (get_current_arch (), current_language);
> +
> + invoke_method = PyObject_GetAttrString (xmethod_worker,
> + invoke_method_name);
> + if (invoke_method == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (invoke_method);
> +
This whole block from here ...
> + obj_type = check_typedef (value_type (obj));
> + this_type = check_typedef (type_object_to_type (worker_data->this_type));
> + if (TYPE_CODE (obj_type) == TYPE_CODE_PTR)
> + {
> + struct type *this_ptr = lookup_pointer_type (this_type);
> +
> + if (!types_equal (obj_type, this_ptr))
> + obj = value_cast (this_ptr, obj);
> + }
> + else if (TYPE_CODE (obj_type) == TYPE_CODE_REF)
> + {
> + struct type *this_ref = lookup_reference_type (this_type);
> +
> + if (!types_equal (obj_type, this_ref))
> + obj = value_cast (this_ref, obj);
> + }
> + else
> + {
> + if (!types_equal (obj_type, this_type))
> + obj = value_cast (this_type, obj);
> + }
> + py_value_obj = value_to_value_object (obj);
> + if (py_value_obj == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (py_value_obj);
... to here needs to be wrapped in a TRY_CATCH.
There's several examples in python/*.c
The problem here is that check_typedef and value_cast can throw gdb
exceptions. We need to catch them, flag them if necessary, and
return EXT_LANG_RC_ERROR (I think).
> +
> + py_arg_tuple = PyTuple_New (nargs);
> + if (py_arg_tuple == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (py_arg_tuple);
> +
> + for (i = 0; i < nargs; i++)
> + {
> + PyObject *py_value_arg = value_to_value_object (args[i]);
> +
> + if (py_value_arg == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> +
> + PyTuple_SET_ITEM (py_arg_tuple, i, py_value_arg);
> + }
> +
> + py_result = PyObject_CallMethodObjArgs (xmethod_worker,
> + py_invoke_method_name,
> + py_value_obj,
> + py_arg_tuple, NULL);
> + if (py_result == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + make_cleanup_py_decref (py_result);
> +
> + if (py_result != Py_None)
> + {
> + res = convert_value_from_python (py_result);
> + if (res == NULL)
> + {
> + gdbpy_print_stack ();
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_ERROR;
> + }
> + }
> + else
> + {
> + res = allocate_value (lookup_typename (python_language, python_gdbarch,
> + "void", NULL, 0));
> + }
> +
> + *result = res;
> + do_cleanups (cleanups);
> +
> + return EXT_LANG_RC_OK;
> +}
More information about the Gdb-patches
mailing list