RE: [PATCH v10 05/10] python: Introduce gdb.RecordAuxiliary class.

Willgerodt, Felix via Gdb-patches
2023-08-30T08:40:20Z

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 9. August 2023 12:01
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; simark@simark.ca
> Subject: RE: [PATCH v10 05/10] python: Introduce gdb.RecordAuxiliary class.
>
> Hello Felix,
>
> >> >> >@@ -455,10 +511,10 @@ btpy_list_item (PyObject *self, Py_ssize_t
> index)
> >> >> >
> >> >> > number = obj->first + (obj->step * index);
> >> >> >
> >> >> >- if (obj->element_type == &recpy_insn_type)
> >> >> >- return recpy_insn_new (obj->thread, RECORD_METHOD_BTRACE,
> >> number);
> >> >> >- else
> >> >> >+ if (obj->element_type == &recpy_func_type)
> >> >> > return recpy_func_new (obj->thread, RECORD_METHOD_BTRACE,
> >> number);
> >> >> >+ else
> >> >> >+ return btpy_item_new (obj->thread, number);
> >> >> > }
> >> >>
> >> >> Why do we need to change the order?
> >> >
> >> >Because we no longer call recpy_insn_new only for recpy_insn_type but also
> for
> >> >recpy_aux_type.
> >>
> >> I think it would be clearer if we checked each type separately and errored
> >> out by default. Like a switch.
> >
> >I can put something like this:
> >
> > if (obj->element_type == &recpy_func_type)
> > return recpy_func_new (obj->thread, RECORD_METHOD_BTRACE, number);
> > else if (obj->element_type == &recpy_insn_type
> > || obj->element_type == &recpy_aux_type)
> > return btpy_item_new (obj->thread, number);
> > else
> > return PyErr_Format (gdbpy_gdb_error, _("Not a valid BtraceList object."));
> >
> >I don't think we can reach the error case right now though.
>
> We will reach the error case when we add a new element type and forget to
> update this function.
>
> LGTM.
>
> Maybe it makes sense to split btpy_item_new() again so we do not accidentally
> create an insn object with a recpy_aux_type or vice versa.
>
> How about aux objects in the function call history? This is supported on CLI if
> I read patch 3 correctly.

Yes, it is showing in CLI. In Python it is a bit more hidden, but also accessible.
The function call history is a list of RecoredFunctionSegment objects. No aux
objects there. But, the RecordFunctionSegment objects all have the instructions
listed as a member:

>>> r = gdb.current_recording()
>>> h = r.function_call_history
>>> h[1]
<gdb.RecordFunctionSegment object at 0x7f76b4587210>
>>> h[1].instructions
<gdb.BtraceObjectList object at 0x7f76ac0abb30>
>>> h[1].instructions[7]
<gdb.RecordInstruction object at 0x7f769c257a50>
>>> h[1].instructions[8]
<gdb.RecordAuxiliary object at 0x7f76b455c8d0>

So a user can still access all the same information. In GDB code, we basically do
the same, just that in the CLI variant of record function-call-history GDB also
searches the insn list for aux objects, and prints them. In Python the user has to
do that himself right now.

I don't think putting aux objects also in the function_call_history would be a
good idea. That would duplicate things.

Anyway, I will change it as we agreed on above.

Felix