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

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

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 15:28
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > @@ -163,6 +165,14 @@ btpy_insn_or_gap_new (thread_info *tinfo,
> Py_ssize_t number)
> >        return recpy_gap_new (err_code, err_string, number);
> >      }
> >
> > +  const struct btrace_insn *insn = btrace_insn_get (&iter);
> > +  gdb_assert (insn != nullptr);
> > +
> > +  if (insn->iclass == BTRACE_INSN_AUX)
> > +    return recpy_aux_new
> > +      (iter.btinfo->aux_data.at (insn->aux_data_index).c_str (), number);
> 
> Not really important, but recpy_aux_new could take a
> `const std::string &`, it would simplify callers a bit.

Thanks, I will do that.

> > +
> > +
> 
> Remove one newline.

Done.

> > +/* Implementation of Auxiliary.data [str].  */
> > +
> > +static PyObject *
> > +recpy_aux_data (PyObject *self, void *closure)
> > +{
> > +  const recpy_aux_object * const obj = (const recpy_aux_object *) self;
> > +
> > +  return PyUnicode_FromString (obj->data);
> > +}
> 
> Nothing new with this patch, since this is the same pattern used for
> other object, but just wondering: obj->data seems to be borrowed from
> the btrace backend.  Are there some lifetime issues if you do:
> 
> 1. Create a gdb.RecordAuxiliary object "b"
> 2. Issue a gdb command to clear the btrace data
> 3. Access "b.data"
> 
> ?  It seems to me like obj->data might point to freed data.

About the pattern:
There is already "maint btrace clear" so I could test and debug this with
master. On master GDB prints an error before accessing any of these objects,
as we check if the trace is empty before accessing anything.

With my patch I could however see a nullptr access. But not at this part of the code.
I think I need to rethink the modelling of Auxiliary objects as new objects.
We model them as a class of instruction in btrace.c.
And the python lists we put them in are designed to be a list of a specific type of object.
So I can either change the way lists are for btrace recordings or model Auxiliaries
as an instruction. I will think about it and post a new revision that addresses this.

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