This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Debug Methods in GDB Python
- From: Tom Tromey <tromey at redhat dot com>
- To: Siva Chandra <sivachandra at google dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 10 May 2013 13:33:06 -0600
- Subject: Re: [RFC] Debug Methods in GDB Python
- References: <CAGyQ6gyKTCdcjMcbfnc4zV3+yEt9tCTJzS8eW92dQrQzikRaTQ at mail dot gmail dot com> <CAGyQ6gxzG8vuPyFKHpacHS7W7jMEReidWDBkNJjywOXADXgVnw at mail dot gmail dot com>
>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:
Siva> Attached is a new version of the patch which I have now regression
Siva> tested. Except for tests and docs, I think it is complete wrt the
Siva> feature set I have in mind. Hence, the caveats from the previous mail
Siva> still apply.
I'm sorry about the delay here.
I think the approach is generally a good one.
I didn't read the overloading bits in detail. I'm not too concerned
about that though.
Siva> +static ULONGEST enabled_ext_languages = EXT_LANG_PYTHON;
I don't think we need this.
I'd rather just have this follow the "virtual methods" approach where a
function is an object that has whatever methods it needs, plus a
destructor.
Siva> + struct ext_fn_descriptor *ext_fn =
Siva> + (struct ext_fn_descriptor *) xzalloc (sizeof (struct ext_fn_descriptor));
FWIW, XCNEW encapsulates all this.
Siva> + if (item->is_method)
Siva> + {
Siva> + if (item->lang == EXT_LANG_PYTHON)
Siva> + py_free_ext_object (item->ext_object);
Siva> + }
... then here this could just call the destructor method, and not need
to know anything about Python specifically.
Siva> +struct ext_fn_descriptor *
Siva> +ext_fn_list_extend (struct ext_fn_descriptor *l1, struct ext_fn_descriptor *l2)
Siva> +{
Siva> + struct ext_fn_descriptor *item = l1;
Siva> +
Siva> + if (!l1)
Siva> + return l2;
Siva> +
Siva> + if (!l2)
Siva> + return l1;
Siva> +
Siva> + while (item->next)
Siva> + item = item->next;
Siva> +
Siva> + item->next = l2;
Why a linked list instead of a VEC?
We already have all this support code written for VECs.
Siva> +#ifdef HAVE_PYTHON
Siva> + if ((enabled_ext_languages && EXT_LANG_PYTHON)
Siva> + && (ext_fn->lang == EXT_LANG_PYTHON))
Siva> + new_ext_fn->ext_object = py_clone_ext_object (ext_fn->ext_object);
Siva> +#endif
This is the kind of thing that can be avoided with the virtual method
approach. Basically any #ifdef HAVE_PYTHON is suspect.
Lots of littler nits in the patch -- function comments, more things
should be 'static'.
Siva> +PyObject *
Siva> +gdbpy_enable_debug_methods (PyObject *self, PyObject *args)
Why is a special enabling method needed?
Siva> + if (PyCallable_Check (match_methods_temp)
Siva> + && PyCallable_Check (get_argtypes_temp)
Siva> + && PyCallable_Check (invoke_method_temp))
Siva> + {
...
Siva> + }
Siva> +
Siva> + return result;
This can return NULL without setting the exception.
Also, it does decrefs before increfs, but I think it is better to do
this the other way around.
Siva> + struct ext_fn_descriptor *ext_fn = new_ext_function ();
Siva> + struct py_ext_object *ext_object;
Siva> +
Siva> + ext_object = (struct py_ext_object *) xzalloc (sizeof (struct py_ext_object));
Siva> + ext_object->object = item;
Siva> + ext_object->match_py_obj_type = py_obj_type;
Siva> + ext_object->match_method = method;
Siva> +
Siva> + ext_fn->lang = EXT_LANG_PYTHON;
Siva> + ext_fn->is_method = 1;
Siva> + ext_fn->ext_object = (void *) ext_object;
Siva> +
Siva> + return ext_fn;
It seems like there should be a way to hang the needed data directly on
the ext_fn, say via subclassing.
Siva> +struct ext_fn_descriptor *
Siva> +py_debugmethod_name_match (struct type *obj_type, const char *method_name)
Siva> +{
Siva> + PyObject *py_type, *py_debugmethod_list = NULL, *list_iter, *item;
Siva> + struct cleanup *cleanups;
Siva> + struct ext_fn_descriptor *method_list = NULL;
Siva> +
Siva> + if (!obj_type || !match_methods_callable)
Siva> + return NULL;
Siva> +
Siva> + cleanups = ensure_python_env (get_current_arch (), current_language);
I don't think this is thread-safe.
ensure_python_env acquires the GIL, but this checks
match_methods_callable outside the lock.
Siva> + list_iter = PyObject_GetIter (py_debugmethod_list);
Siva> + while ((item = PyIter_Next (list_iter)))
Error check for list_iter.
Siva> + {
Siva> + struct ext_fn_descriptor *ext_fn;
Siva> +
Siva> + ext_fn = new_python_ext_method (item, py_type, method_name);
Siva> + method_list = ext_fn_list_prepend (method_list, ext_fn);
Siva> + }
Siva> +
Siva> + Py_DECREF (list_iter);
Need an error check here too -- the loop may exit with an error.
Siva> + py_argtype_list = PyObject_CallFunction (get_argtypes_callable,
Siva> + "OOs",
Siva> + ext_object->object,
Siva> + ext_object->match_py_obj_type,
Siva> + ext_object->match_method);
Siva> +
Siva> + list_iter = PyObject_GetIter (py_argtype_list);
Error checking here and elsewhere.
Siva> + { "enable_debug_methods", gdbpy_enable_debug_methods, METH_VARARGS,
Siva> + "inferiors (meth_match_function, args_match_function) -> None.\n\
Siva> +Enables Python debug methods feature." },
Wrong doc string.
Tom