Re: [PATCH 4/7] python: Create Python bindings for record history.

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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?

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.

Thanks,

Simon