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

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

On 6/4/19 2:35 PM, Metzger, Markus T wrote:
> Hello Felix,
>
>> Auxiliary instructions are no real instructions and get their own object
>> class, similar to gaps. gdb.Record.instruction_history is now possibly a
>> list of gdb.RecordInstruction, gdb.RecordGap or gdb.RecordAuxiliary
>> objects.
>>
>> This patch is in preparation for the new ptwrite feature, which is based on
>> auxiliary instructions.
>>
>> 2019-05-29  Felix Willgerodt  <felix.willgerodt@intel.com>
>>
>> gdb/ChangeLog:
>> * py-record-btrace.c (btpy_insn_or_gap_new): Removed.
>> (btpy_item_new): New function.
>> (btpy_list_item): Call btpy_item_new instead of recpy_insn_new.
>> (recpy_bt_replay_position): Call btpy_item_new instead of
>> btpy_insn_or_gap_new.
>> (recpy_bt_begin): Call btpy_item_new instead of btpy_insn_or_gap_new.
>> (recpy_bt_end): Call btpy_item_new instead of btpy_insn_or_gap_new.
>> * py-record.c (recpy_aux_type): New static object.
>> (recpy_aux_object): New typedef.
>> (recpy_aux_new, recpy_aux_number, recpy_aux_data): New function.
>> (recpy_aux_getset): New static object.
>> (gdbpy_initialize_record): Initialize gdb.RecordAuxiliary type.
>>
>> gdb/doc/ChangeLog:
>> * python.texi (gdb.RecordAuxiliary): New documentation.
>>
>> ---
>>   gdb/doc/python.texi           | 13 +++++++
>>   gdb/python/py-record-btrace.c | 22 +++++++----
>>   gdb/python/py-record.c        | 73 ++++++++++++++++++++++++++++++++++-
>>   gdb/python/py-record.h        |  3 ++
>>   4 files changed, 103 insertions(+), 8 deletions(-)
>
> Looks good to me but I can't approve it.
>
>> @@ -172,6 +173,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);

Markus> This must be true because we checked btrace_insn_get_error () before.  To make it
Markus> more obvious, we could rewrite the function to call btrace_insn_get () first and check
Markus> the returned btrace_insn pointer.  There's no need to check the error code, I think.
Markus> In the worst case, we'll end up with a gap that says 'success'.

You are right, removed it.

>> @@ -470,7 +479,7 @@ 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);
>> +    return btpy_item_new (obj->thread, number);
>>     else
>>       return recpy_func_new (obj->thread, RECORD_METHOD_BTRACE, number);
>>   }

Markus> Isn't this code just creating list item objects?

Yes, but when creating the list we now don't want to add RecordAuxiliary
objects as RecordInstruction objects. The only list objects created are of type
recpy_insn_type or recpy_func_type.


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