From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75267 invoked by alias); 28 Oct 2016 14:12:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 75240 invoked by uid 89); 28 Oct 2016 14:12:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Pythonspecific, querying, silenty, tense X-HELO: mga03.intel.com Received: from mga03.intel.com (HELO mga03.intel.com) (134.134.136.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Oct 2016 14:12:22 +0000 Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP; 28 Oct 2016 07:11:47 -0700 X-ExtLoop1: 1 Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75]) by orsmga004.jf.intel.com with ESMTP; 28 Oct 2016 07:11:46 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX153.ger.corp.intel.com (163.33.192.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 28 Oct 2016 15:11:45 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.177]) by IRSMSX156.ger.corp.intel.com ([169.254.3.66]) with mapi id 14.03.0248.002; Fri, 28 Oct 2016 15:11:45 +0100 From: "Wiederhake, Tim" To: Simon Marchi CC: "gdb-patches@sourceware.org" , "palves@redhat.com" , "Metzger, Markus T" Subject: RE: [PATCH 4/7] python: Create Python bindings for record history. Date: Fri, 28 Oct 2016 14:12:00 -0000 Message-ID: <9676A094AF46E14E8265E7A3F4CCE9AF90C9AC@irsmsx105.ger.corp.intel.com> References: <1477549711-2603-1-git-send-email-tim.wiederhake@intel.com> <1477549711-2603-5-git-send-email-tim.wiederhake@intel.com> In-Reply-To: Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00803.txt.bz2 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 > Cc: gdb-patches@sourceware.org; palves@redhat.com; Metzger, Markus T > > Subject: Re: [PATCH 4/7] python: Create Python bindings for record histor= y. >=20 > 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. >=20 > Hi Tim, >=20 > Thanks for these patches, I think that offering a Python API for controll= ing > and querying process record is very interesting. >=20 > > 2016-10-26 Tim Wiederhake > > > > 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. >=20 > Little detail: consistently use the present tense in the ChangeLog entrie= s (e.g. > Regenerate instead of Regenerated). >=20 > > * 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. >=20 > Same. >=20 > > +/* Executes a command silently. Returns non-zero on success; returns > > zero and > > + sets Python exception on failure. */ > > + > > +static int > > +gdbpy_execute_silenty (char *command) >=20 > silenty -> silently? Oops. > Ideally we should really be using internal APIs instead of using this fun= ction... > unfortunately they don't seem to exist yet for these tasks. > Having GDB build command strings, only to parse them itself right away fe= els > like a waste of resources, but it also makes the code harder to follow. >=20 > > +/* Implementation of gdb.start_recording (method) -> gdb.Record. */ > > + > > +PyObject * > > +gdbpy_start_recording (PyObject *self, PyObject *args) { > > + char *method =3D "full"; >=20 > Should probably be const. >=20 > > + > > + if (!PyArg_ParseTuple (args, "|s", &method)) > > + return NULL; > > + > > + if (strncmp (method, "full", sizeof ("full")) =3D=3D 0) > > + { > > + if (!gdbpy_execute_silenty ("record full")) > > + return NULL; > > + } > > + else if (strncmp (method, "btrace", sizeof ("btrace")) =3D=3D 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")) =3D=3D 0) > > + || (strncmp (method, "btrace pt", sizeof ("btrace pt")) =3D=3D 0)) > > + { > > + if (!gdbpy_execute_silenty ("record btrace pt")) > > + return NULL; > > + } > > + else if ((strncmp (method, "bts", sizeof ("bts")) =3D=3D 0) > > + || (strncmp (method, "btrace bts", sizeof ("btrace bts")) =3D=3D 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); } >=20 > 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 >=20 > record_start (const char *method, const char *format); >=20 > 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 ca= tch > 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); > > + >=20 > 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 object= s (or lists of those). Btrace recording will generally create different objects than full recordin= g (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 se= gment), and the record target is the only instance to know what type of obj= ect 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 instruct= ion / 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 objec= t creation time to gdb.Record creation time. If you (or others) have any id= ea about this, please let me know. >=20 > Thanks, >=20 > 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