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

Willgerodt, Felix via Gdb-patches
2023-08-07T09:08:59Z

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