Re: [RFC] Debug Methods in GDB Python

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Siva> Took me longer than I had expected I but could spend some time last
Siva> couple of weeks and address all of Tom's comments from last time. Like
Siva> before, I do not have docs or tests as the Python side API is largely
Siva> un-reviewed I guess. However, I have put in code comments in the
Siva> latest version. The patch is attached and the ChangeLog is as below:

Thanks.

I appreciate that you don't want to write all the docs and tests for a
moving target.  On the other hand, it's harder to review a patch that
doesn't have these.

A compromise would be if you wrote just the docs describing how it
works, say the Python API.  Then we could discuss this; and once ok, the
implementation would follow.

But, that is for future changes.  For this one, I think we're in general
agreement about the exposed API, and so polishing is a vital component.

Siva> +/* Registers an extension language with GDB.  */
Siva> +
Siva> +void
Siva> +register_ext_lang (struct ext_lang *lang)
Siva> +{
Siva> +  if (ext_lang_vec == NULL)
Siva> +    ext_lang_vec = VEC_alloc (ext_lang_p, 1);
Siva> +      
Siva> +  VEC_safe_push (ext_lang_p, ext_lang_vec, lang);

The "extension language" code here seems like a lot of work for no
real benefit.

It's fine for the functions themselves, since we'd like to keep the API
to the Python layer reasonably thin.  But for get_matching_ext_methods,
e.g., it is simple to follow the existing pattern: declare a function in
python.h and provide both with- and without-Python implementations.

Siva> +struct ext_lang
Siva> +  {
Siva> +    clone_ext_obj_ftype *clone_ext_object;
Siva> +    free_ext_obj_ftype *free_ext_obj;
Siva> +    get_matching_ext_methods_ftype *get_matching_ext_methods;
Siva> +    get_ext_fn_argtypes_ftype *get_ext_fn_argtypes;
Siva> +    invoke_method_ftype *invoke_method;
Siva> +  };

I think this struct should be given a different name.
Typical in gdb is "ops".

Siva> +extern struct ext_fn_descriptor *new_ext_function (struct ext_lang *lang,
Siva> +                                                   int is_method,
Siva> +                                                   void *ext_obj);

It seems like 'lang' should be const here.

Siva> +    and the method name.  It next get the argument types of these methods via

s/get/gets/

Siva> +    for old_method in existing_method_list:
Siva> +        objfile.debug_methods.remove(old_method)
Siva> +    objfile.debug_methods.extend(debug_methods)

I notice this patch adds "debug_methods" to the objfile but not the
program space.  Updating the latter, and also providing a global
debug_methods, is more in line with the other hooks we provide.

Siva> +py_free_ext_object (void *ext_object)
Siva> +{
Siva> +  struct py_ext_object *o = (struct py_ext_object *) ext_object;

You don't need casts to or from "void *".

Siva> +  enabled = PyObject_IsTrue (enabled_field);
Siva> +  if (enabled == -1)
Siva> +    {
Siva> +      PyErr_Clear ();
Siva> +      do_cleanups (cleanups);

Silently ignoring errors doesn't seem right.
There are a few instances of this.

I'm not really sure about the error-handling strategy in this function.

Siva> +static VEC (ext_fn_descriptor_p) *
Siva> +py_debugmethod_name_match (struct type *obj_type, const char *method_name)
[...]

Siva> +              if (method_vec == NULL)
Siva> +                method_vec = VEC_alloc (ext_fn_descriptor_p, 1);

You don't need to explicitly allocate a vec like this.
VEC_safe_push handles it.

Siva> +      self->debug_methods = PyList_New (0);
Siva> +      if (!self->debug_methods)

Now we write "== NULL".

Siva> +	  object->debug_methods = PyList_New (0);
Siva> +	  if (!object->debug_methods)

Ditto.

Siva> +int gdbpy_initialize_debugmethods (void)
Siva> +  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;

I'm curious if you ran this through the checker.

It isn't a requirement; the checker gives enough false reports that it
is a bit of a pain.

Tom