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

Metzger, Markus T
2016-11-29T15:48:38Z

> -----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