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: Dienstag, 25. Juli 2023 15:30
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org;
> simark@simark.ca
> Subject: RE: [PATCH v10 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> Hello Felix,
> 
> >-  btrace_find_insn_by_number (&iter, &tinfo->btrace, number);
> >+  if (btrace_find_insn_by_number (&iter, &tinfo->btrace, number) == 0)
> 
> Nit: how about storing the result in a local variable to avoid side-effects
> in the if expression?

What side effects do you have in mind? I checked the function and couldn't find
any. And I just copied this from the existing btrace_insn_from_recpy_insn.

> >+  if (Py_TYPE (self) != &recpy_aux_type)
> >+    {
> >+      PyErr_Format (gdbpy_gdb_error, _("Must be a gdb.Auxiliary."));
> >+      return NULL;
> >+    }
> >+
> >+  obj = (const recpy_element_object *) self;
> >+  tinfo = obj->thread;
> >+
> >+  if (tinfo == NULL || btrace_is_empty (tinfo))
> >+    {
> >+      PyErr_Format (gdbpy_gdb_error, _("No such auxiliary object."));
> >+      return NULL;
> >+    }
> >+
> >+  if (btrace_find_insn_by_number (&iter, &tinfo->btrace, obj->number) == 0)
> >+    {
> >+      PyErr_Format (gdbpy_gdb_error, _("No such auxiliary object."));
> >+      return NULL;
> >+    }
> >+
> >+  insn = btrace_insn_get (&iter);
> >+  if (insn == NULL)
> >+    {
> >+      PyErr_Format (gdbpy_gdb_error, _("Not a valid auxiliary object."));
> >+      return NULL;
> >+    }
> >+
> >+  return PyUnicode_FromString
> >+    (iter.btinfo->aux_data.at (insn->aux_data_index).c_str ());
> 
> Shouldn't we check the insn class, too?

We checked the python object type above already.
So this would only catch cases in which something went horribly wrong
and the python trace doesn't match the internal trace.
I think right now that can never be the case, but it might be a good
sanity check, so I will add it.

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

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