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

[Simon Marchi <simark@simark.ca>]

On 3/31/23 06:58, Willgerodt, Felix wrote:
>> -----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.

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"

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

Which list are you referring to?

Simon