This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH 4/7] python: Create Python bindings for record history.
Hi Simon,
Thanks for the comments!
> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@polymtl.ca]
> Sent: Thursday, October 27, 2016 5:53 PM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: gdb-patches@sourceware.org; palves@redhat.com; Metzger, Markus T
> <markus.t.metzger@intel.com>
> Subject: Re: [PATCH 4/7] python: Create Python bindings for record history.
>
> On 2016-10-27 02:28, Tim Wiederhake wrote:
> > 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.
>
> Hi Tim,
>
> Thanks for these patches, I think that offering a Python API for controlling
> and querying process record is very interesting.
>
> > 2016-10-26 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.
> > (py-record.o): New rule.
> > * 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: Regenerated.
>
> Little detail: consistently use the present tense in the ChangeLog entries (e.g.
> Regenerate instead of Regenerated).
>
> > * 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): Added to_record_python_interface function.
>
> Same.
>
> > +/* Executes a command silently. Returns non-zero on success; returns
> > zero and
> > + sets Python exception on failure. */
> > +
> > +static int
> > +gdbpy_execute_silenty (char *command)
>
> silenty -> silently?
Oops.
> Ideally we should really be using internal APIs instead of using this function...
> unfortunately they don't seem to exist yet for these tasks.
> Having GDB build command strings, only to parse them itself right away feels
> like a waste of resources, but it also makes the code harder to follow.
>
> > +/* Implementation of gdb.start_recording (method) -> gdb.Record. */
> > +
> > +PyObject *
> > +gdbpy_start_recording (PyObject *self, PyObject *args) {
> > + char *method = "full";
>
> Should probably be const.
>
> > +
> > + if (!PyArg_ParseTuple (args, "|s", &method))
> > + return NULL;
> > +
> > + if (strncmp (method, "full", sizeof ("full")) == 0)
> > + {
> > + if (!gdbpy_execute_silenty ("record full"))
> > + return NULL;
> > + }
> > + else if (strncmp (method, "btrace", sizeof ("btrace")) == 0)
> > + {
> > + if (!gdbpy_execute_silenty ("record btrace pt"))
> > + {
> > + PyErr_Clear ();
> > + if (!gdbpy_execute_silenty ("record btrace bts"))
> > + return NULL;
> > + }
> > + }
> > + else if ((strncmp (method, "pt", sizeof ("pt")) == 0)
> > + || (strncmp (method, "btrace pt", sizeof ("btrace pt")) == 0))
> > + {
> > + if (!gdbpy_execute_silenty ("record btrace pt"))
> > + return NULL;
> > + }
> > + else if ((strncmp (method, "bts", sizeof ("bts")) == 0)
> > + || (strncmp (method, "btrace bts", sizeof ("btrace bts")) == 0))
> > + {
> > + if (!gdbpy_execute_silenty ("record btrace bts"))
> > + return NULL;
> > + }
> > + else
> > + return PyErr_Format (PyExc_TypeError, _("Unknown recording
> > method."));
> > +
> > + return gdbpy_current_recording (self, args); }
>
> This function seems to have too much knowledge of the record formats, and
> will need to be updated for every new record format. And if we want to
> offer a similar API in the Guile bindings, the knowledge will need to be
> duplicated. This is where an internal API would be interesting.
> Assuming there would be an internal function
>
> record_start (const char *method, const char *format);
>
> Then gdbpy_start_recording could be a simple wrapper around that. If the
> method/format doesn't exist, it will throw an exception, which you can catch
> and format correctly.
>
> > diff --git a/gdb/target.h b/gdb/target.h index 176f332..faaed0e 100644
> > --- a/gdb/target.h
> > +++ b/gdb/target.h
> > @@ -39,6 +39,7 @@ struct traceframe_info; struct expression; struct
> > dcache_struct; struct inferior;
> > +struct record_python_interface;
> >
> > #include "infrun.h" /* For enum exec_direction_kind. */
> > #include "breakpoint.h" /* For enum bptype. */
> > @@ -1230,6 +1231,12 @@ struct target_ops
> > ULONGEST begin, ULONGEST end, int flags)
> > TARGET_DEFAULT_NORETURN (tcomplain ());
> >
> > + /* Fill in the record python interface object and return non-zero.
> > + Return zero on failure or if no recording is active. */
> > + int (*to_record_python_interface) (struct target_ops *,
> > + struct record_python_interface *)
> > + TARGET_DEFAULT_RETURN (0);
> > +
>
> I am going to nag you again with "internal API" stuff :). Introducing
> some Python-specific stuff here doesn't feel right. It should probably
> use a structure (or class) similar to record_python_interface, but which
> doesn't know anything about Python. Then it's up to the Python layer to
> query whatever it needs and convert that into whatever structure it
> wants. But at least, other parts of GDB could use it too.
The gdb.Record Python object is recording method agnostic. It calls to the current record target to create instruction or function call segment objects (or lists of those).
Btrace recording will generally create different objects than full recording (they might have the same interface to the Python world, but they might have to store different data to identify an instruction or function call segment), and the record target is the only instance to know what type of object to create. Even if we stuff all information gdb.Record would need in a non-Python-y struct, gdb.Record still lacks the information of the instruction / call segment object type.
Making gdb.Record aware of the current recording method does not solve this problem, rather it moves the problem from instruction / call segment object creation time to gdb.Record creation time. If you (or others) have any idea about this, please let me know.
>
> Thanks,
>
> Simon
Tim
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