From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49433 invoked by alias); 29 Nov 2016 15:48:38 -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 49359 invoked by uid 89); 29 Nov 2016 15:48:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=approve 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; Tue, 29 Nov 2016 15:48:27 +0000 Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP; 29 Nov 2016 07:48:25 -0800 X-ExtLoop1: 1 Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga006.fm.intel.com with ESMTP; 29 Nov 2016 07:48:24 -0800 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.52]) by irsmsx105.ger.corp.intel.com ([169.254.7.43]) with mapi id 14.03.0248.002; Tue, 29 Nov 2016 15:48:19 +0000 From: "Metzger, Markus T" To: "Wiederhake, Tim" , "gdb-patches@sourceware.org" CC: "palves@redhat.com" Subject: RE: [PATCH v3 6/8] python: Implement btrace Python bindings for record history. Date: Tue, 29 Nov 2016 15:48:00 -0000 Message-ID: References: <1479743318-24523-1-git-send-email-tim.wiederhake@intel.com> <1479743318-24523-7-git-send-email-tim.wiederhake@intel.com> In-Reply-To: <1479743318-24523-7-git-send-email-tim.wiederhake@intel.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00956.txt.bz2 > -----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 > Subject: [PATCH v3 6/8] python: Implement btrace Python bindings for reco= rd > history. >=20 > This patch implements the gdb.Record Python object methods and fields for > record target btrace. Additionally, add a stub for record target full. >=20 > 2016-11-21 Tim Wiederhake >=20 > gdb/ChangeLog: >=20 > * Makefile.in (SUBDIR_PYTHON_OBS): Add py-btrace.o. > (SUBDIR_PYTHON_SRCS): Add py-btrace.c. > * python/py-btrace.c: New file. > * python/py-btrace.h: New file. > * python/python-internal.h (gdbpy_initialize_btrace): New export. > * python/python.c (_initialize_python): Add gdbpy_initialize_btrace. > * record-btrace.c: Add conditional includes for python/py-record.h > and python/py-btrace.h > (record_btrace_python_interface): New function. > (init_record_btrace_ops): Add record_btrace_python_interface. > * record-full.c: Add conditional include for python/py-record.h. > (record_full_python_interface): New function. > (init_record_full_ops): Add record_full_python_interface. > +/* Python object for btrace records. This can either be an instruction = or a > + function call segment, depending on the chosen type. */ That's not the object for a btrace recording, right? Maybe just omit the f= irst sentence and say right away that this can either be a btrace instruction or function= segment. > + if (!PySlice_Check (value)) > + return PyErr_Format (PyExc_TypeError, _("Index must be int or slice.= ")); > + > + if (0 !=3D PySlice_GetIndicesEx (BTPY_PYSLICE (value), length, &start,= &stop, > + &step, &slicelength)) > + return NULL; > + > + return btpy_list_new (obj->ptid, obj->first + obj->step * start, > + obj->first + obj->step * stop, obj->step * step, > + obj->element_type); Is PySlice_GetIndicesEx checking that the above operations can't overflow? > +/* Implementation of BtraceList.count (self, value) -> int. */ > + > +static PyObject * > +btpy_list_count (PyObject *self, PyObject *value) > +{ > + const LONGEST index =3D btpy_list_position (self, value); > + > + if (index < 0) > + return PyInt_FromLong (0); > + else > + return PyInt_FromLong (1); > +} Hmmm, we need instruction instances to be unique for "record goto". OTOH it might be nice to count the number of occurrences of a given instruction at = a given PC. Would it make sense to define equality as "same PC" for the purpose of= count ()? > +/* Implementation of > + BtraceRecord.instruction_history [list]. */ > + > +static PyObject * > +recpy_bt_instruction_history (PyObject* self, void* closure) > +{ > + struct thread_info * const tinfo =3D find_thread_ptid (inferior_ptid); > + struct btrace_insn_iterator iterator; > + unsigned long first =3D 0; > + unsigned long last =3D 0; > + > + if (tinfo =3D=3D NULL) > + Py_RETURN_NONE; > + > + btrace_fetch (tinfo); > + > + if (btrace_is_empty (tinfo)) > + Py_RETURN_NONE; > + > + btrace_insn_begin (&iterator, &tinfo->btrace); > + first =3D btrace_insn_number (&iterator); > + > + btrace_insn_end (&iterator, &tinfo->btrace); > + last =3D btrace_insn_number (&iterator); > + > + return btpy_list_new (inferior_ptid, first, last, 1, &btpy_insn_type); IIRC last was included in the list. The btrace end iterator points one pas= t the end of the list. Is this intentional to include the current instruction? > +/* Implementation of > + BtraceRecord.function_call_history [list]. */ > + > +static PyObject * > +recpy_bt_function_call_history (PyObject* self, void* closure) > +{ > + struct thread_info * const tinfo =3D find_thread_ptid (inferior_ptid= ); > + struct btrace_call_iterator iterator; > + unsigned long first =3D 0; > + unsigned long last =3D 0; > + > + if (tinfo =3D=3D NULL) > + Py_RETURN_NONE; > + > + btrace_fetch (tinfo); > + > + if (btrace_is_empty (tinfo)) > + Py_RETURN_NONE; > + > + btrace_call_begin (&iterator, &tinfo->btrace); > + first =3D btrace_call_number (&iterator); > + > + btrace_call_end (&iterator, &tinfo->btrace); > + last =3D btrace_call_number (&iterator); > + > + return btpy_list_new (inferior_ptid, first, last, 1, &btpy_call_type= ); Same here. > +struct PyGetSetDef btpy_insn_getset[] =3D { > + { "number", btpy_number, NULL, "instruction number", NULL}, > + { "error", btpy_insn_error, NULL, "error number for gaps", NULL}, > + { "symbol", btpy_insn_symbol, NULL, "associated symbol", NULL}, > + { "pc", btpy_insn_pc, NULL, "instruction address", NULL}, > + { "data", btpy_insn_data, NULL, "raw instruction data", NULL}, > + { "decoded", btpy_insn_decode, NULL, "decoded instruction", NULL}, Doesn't this also give the error string for gaps? > + { "size", btpy_insn_size, NULL, "instruction size in byte", NULL}, > + { "is_speculative", btpy_insn_is_speculative, NULL, "if the instructio= n was \ > +executed speculatively", NULL}, > + {NULL} > +}; > + > +/* BtraceFunctionCall members. */ > + > +static PyGetSetDef btpy_call_getset[] =3D { > + { "number", btpy_number, NULL, "function call number", NULL}, > + { "level", btpy_call_level, NULL, "call stack level", NULL}, > + { "symbol", btpy_call_symbol, NULL, "associated line and symbol", NULL= }, > + { "instructions", btpy_call_instructions, NULL, "list of instructions = in \ > +this function call", NULL}, Shouldn't this be "function segment" instead of "function call"? The patch looks otherwise good to me but we need to find a maintainer to approve the patch. 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