Re: [RFC] Debug Methods in GDB Python

Tom Tromey
2013-07-22T20:47:44Z

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