Re: [RFC] Debug Methods in GDB Python

Tom Tromey
2013-05-10T19:33:13Z

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