Re: [RFC] Debug Methods in GDB Python

[Tom Tromey <tromey@redhat.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