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: Dienstag, 4. April 2023 16:17
> To: Metzger, Markus T <markus.t.metzger@intel.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> On 4/4/23 02:57, Metzger, Markus T wrote:
> >
> >>>>> +/* 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.
> >>
> >> Then, maybe if you record something after clearing the data?  Something
> >> like:
> >>
> >> 1. Create a gdb.RecordAuxiliary object "b"
> >> 2. Issue "maint btrace clear"
> >> 3. Do one step (to make the trace non-empty again)
> >> 4. Access "b.data"
> >
> > I added the 'maint btrace clear' command to force re-decoding of the trace
> > for debugging purposes.  Any CLI command that uses btrace will trigger a
> > trace fetch and decode.
> 
> Ack.
> 
> > For ptwrite filters, the use-case is to replace the filter and re-decode with
> > the new filter in place (and the old filter removed).
> 
> Yes, that makes sense.
> 
> > I'm not against chaining
> > filters and decorating their aux contribution with the filter name.  IIRC this
> > was considered more complicated and we couldn't really find a good use-
> case.
> 
> Just to be clear, I didn't suggest to add that.
> 
> > The filter needs to know about the source or binary instrumentation that
> > added those ptwrite instructions.  It's job is to aggregate the raw data and
> > the IP of the ptwrite instruction that emitted that data and turn it into
> > something more useful than the raw hex values.
> >
> > The use case for filter chaining would be independent instrumentations
> > active at the same time and interpreted by their respective ptwrite filter.
> 
> Ack.  Or some kind of top-level filter that delegates.
> 
> > To the topic at hand, I'd say that record instruction or aux objects should
> > behave like, say, frame objects.  You cannot clear the frames to force a
> > re-unwind AFAIK but you can step or finish to turn a frame invalid.
> 
> Yes, that is my worry.  It seems possible for the btrace Python objects
> to outlive the data they are wrapping, and I don't see any measures
> taken to avoid accessing the stale data.  Usually that can be done with
> an observer that will mark the Python objects invalid.
> 
> Another case: if you "continue" and a thread exits, is the btrace record
> data for that thread deleted?
> 
> Simon

I looked at it a bit more yesterday.

The gap objects get a string literal (const char *). Which have a guaranteed
lifetime over the full program afaik and therefore pointers are never dangling.
I didn't check if we wouldn't error out first, as I don't know how to reliably
record gaps.

The insn and func objects cannot be accessed after clear.
Any commands will show:

"gdb.error: No such function segment."
or
"gdb.error: No such instruction."

Even if you "save" it in a separate python variable, e.g. like "b" in Simon's list.

For auxiliaries in my patch however, Simon is right. We just pass a pointer
to an element in a vector of strings (aux_data) that is allocated in btrace.c.
We clear that vector with clear. I debugged GDB, in that case we do still point
to the same address and just read a string from it. No matter what is at that
address now:

>>> r = gdb.current_recording()
>>> i = r.instruction_history
>>> a = i[9]
>>> a.data

Thread 1 "gdb-up" hit Breakpoint 1, recpy_aux_data (
    self=<gdb.RecordAuxiliary at remote 0x7fffe8152710>, closure=0x0)
    at gdb/gdb/python/py-record.c:547
547       const recpy_aux_object * const obj = (const recpy_aux_object *) self;
(gdb) n
549       return PyUnicode_FromString (obj->data);
(gdb) p obj.data
$1 = 0x2d1ade0 "42"
(gdb) c
Continuing.
'42'
>>> r.clear()
>>> a.data

Thread 1 "gdb-up" hit Breakpoint 1, recpy_aux_data (
    self=<gdb.RecordAuxiliary at remote 0x7fffe8152710>, closure=0x0)
    at gdb/gdb/python/py-record.c:547
547       const recpy_aux_object * const obj = (const recpy_aux_object *) self;
(gdb) n
549       return PyUnicode_FromString (obj->data);
(gdb) p obj.data
$2 = 0x2d1ade0 "42"
(gdb) c
Continuing.
'42'

So I should really create a copy or error out if the trace
was cleared instead. Probably the latter to be consistent.

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