RE: [PATCH v3 6/8] python: Implement btrace Python bindings for record history.

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----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 6/8] python: Implement btrace Python bindings for record
> history.
> 
> This patch implements the gdb.Record Python object methods and fields for
> record target btrace.  Additionally, add a stub for record target full.
> 
> 2016-11-21  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* 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 first 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 != 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 = 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 = find_thread_ptid (inferior_ptid);
> +  struct btrace_insn_iterator iterator;
> +  unsigned long first = 0;
> +  unsigned long last = 0;
> +
> +   if (tinfo == NULL)
> +     Py_RETURN_NONE;
> +
> +   btrace_fetch (tinfo);
> +
> +   if (btrace_is_empty (tinfo))
> +     Py_RETURN_NONE;
> +
> +   btrace_insn_begin (&iterator, &tinfo->btrace);
> +   first = btrace_insn_number (&iterator);
> +
> +   btrace_insn_end (&iterator, &tinfo->btrace);
> +   last = 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 past 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 = find_thread_ptid (inferior_ptid);
> +    struct btrace_call_iterator iterator;
> +    unsigned long first = 0;
> +    unsigned long last = 0;
> +
> +    if (tinfo == NULL)
> +      Py_RETURN_NONE;
> +
> +    btrace_fetch (tinfo);
> +
> +    if (btrace_is_empty (tinfo))
> +      Py_RETURN_NONE;
> +
> +    btrace_call_begin (&iterator, &tinfo->btrace);
> +    first = btrace_call_number (&iterator);
> +
> +    btrace_call_end (&iterator, &tinfo->btrace);
> +    last = btrace_call_number (&iterator);
> +
> +    return btpy_list_new (inferior_ptid, first, last, 1, &btpy_call_type);

Same here.


> +struct PyGetSetDef btpy_insn_getset[] = {
> +  { "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 instruction was \
> +executed speculatively", NULL},
> +  {NULL}
> +};
> +
> +/* BtraceFunctionCall members.  */
> +
> +static PyGetSetDef btpy_call_getset[] = {
> +  { "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