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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----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
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928