RE: [PATCH v3 5/8] python: Create Python bindings for record history.

Metzger, Markus T
2016-11-29T15:48:26Z

> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Monday, November 21, 2016 4:49 PM
> To: gdb-patches@sourceware.org
> Cc: palves@redhat.com; Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH v3 5/8] python: Create Python bindings for record history.
>
> This patch adds three new functions to the gdb module in Python:
> - start_recording
> - stop_recording
> - current_recording
> start_recording and current_recording return an object of the new type
> gdb.Record, which can be used to access the recorded data.
>
> 2016-11-21 Tim Wiederhake <tim.wiederhake@intel.com>
>
> gdb/ChangeLog
>
> * Makefile.in (SUBDIR_PYTHON_OBS): Add python/py-record.o.
> (SUBDIR_PYTHON_SRCS): Add python/py-record.c.
> * python/py-record.c: New file.
> * python/py-record.h: New file.
> * python/python-internal.h (gdbpy_start_recording,
> gdbpy_current_recording, gdpy_stop_recording,
> gdbpy_initialize_record): New export.
> * python/python.c (_initialize_python): Add gdbpy_initialize_record.
> (python_GdbMethods): Add gdbpy_start_recording,
> gdbpy_current_recording and gdbpy_stop_recording.
> * target-debug.h (target_debug_print_struct_record_python_interface):
> New define.
> * target-delegates.c: Regenerate.
> * target.c (target_record_python_interface): New function.
> * target.h: Added struct record_python_interface forward declaration.
> Export target_record_python_interface.
> (struct target_ops): Add to_record_python_interface function.

> +/* Python Record object. */
> +
> +typedef struct
> +{
> + PyObject_HEAD
> +
> + struct record_python_interface interface;

You may want to document every field.

> +static PyObject *
> +recpy_method (PyObject *self, void* closure)
> +{
> + recpy_record_object *obj = (recpy_record_object *) self;
> +
> + if (obj->interface.method == NULL)
> + return PyString_FromString (_("unknown"));

Should this return None, instead?

> +
> + return PyString_FromString (obj->interface.method);
> +}
> +
> +/* Implementation of record.format. */
> +
> +static PyObject *
> +recpy_format (PyObject *self, void* closure)
> +{
> + recpy_record_object *obj = (recpy_record_object *) self;
> +
> + if (obj->interface.format == NULL)
> + return PyString_FromString (_("unknown"));

Same here.

> +/* Implementation of gdb.current_recording (self) -> gdb.Record. */
> +
> +PyObject *
> +gdbpy_current_recording (PyObject *self, PyObject *args)
> +{
> + recpy_record_object* obj;
> +
> + obj = PyObject_New (recpy_record_object, &recpy_record_type);
> + if (obj == NULL)
> + return NULL;
> +
> + memset (&obj->interface, 0, sizeof (struct record_python_interface));

How about "sizeof (obj->interface)" instead?

> +/* Implementation of gdb.stop_recording (self) -> None. */
> +
> +PyObject *
> +gdbpy_stop_recording (PyObject *self, PyObject *args)
> +{
> + TRY
> + {
> + execute_command_to_string ("record stop", 0);
> + Py_RETURN_NONE;
> + }
> + CATCH (except, RETURN_MASK_ALL)
> + {
> + gdbpy_convert_exception (except);
> + }
> + END_CATCH
> +
> + return NULL;

We shouldn't really get here, should we?

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928