RE: GDB 8.0 release/branching 2017-03-20 update

Metzger, Markus T
2017-03-22T13:58:20Z

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Yao Qi
> Sent: Tuesday, March 21, 2017 2:08 PM
> To: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org; Wiederhake, Tim
> <tim.wiederhake@intel.com>; xdje42@gmail.com
> Subject: Re: GDB 8.0 release/branching 2017-03-20 update

Hello Yao,

Thanks for your review and for your feedback.

> 1. BtraceInstruction.sal is not necessary. It can be got via
> gdb.find_pc_line(BtraceInstruction.pc).

I asked Tim to put this into the Instruction object in order to support
future extensions.

At the moment, we decode from live memory, and there, you're right
that we can get the SAL from the PC. If we wanted to support exec()
or dlclose() in the future, the trace would refer to instructions that are
no longer available in memory and GDB would not know about them.

Having users get the SAL from the Instruction object gives us more
freedom to model this. We could, for example, decode from binary
files and leave GDB to represent the current program state. Not sure
whether this would suffice - probably not. But it sounded like a good
idea to have users get such information via the Instruction object from
the beginning. That's also why the Instruction object has a data function
to get the raw bytes of an instruction rather than having users read
it from memory.

> 2. BtraceInstruction has some attributes (pc, data, decoded, and size,)
> which are not btrace related.
> They should be moved to a new class gdb.Instruction and BtraceInstruction
> extends it.
>
> Instruction
> {
> pc, data, decode, size;
> };

That makes sense if we're considering a Python API for GDB's disassembler.
I would extend it to include SAL for the above reasons so users don't need
to know from where they got the Instruction object.

And we would extend it for record targets to add the speculative execution
indication and a means for interleaving decode errors with decoded instructions.

I'm wondering how much of this discussion is really a question of sub-classing
and how much of it is about documentation. Would I even be able to tell the
difference in my python scripts that use the API?

The duck typing argument makes sense to me and I'm not familiar enough
with implementing Python to have an opinion on how this should be done.
Do we have an expert on this in the GDB community?

> 3. Why does Record have an attribute ptid?
> Other record method may have process-wide record/trace. I'd like to put
> common attributes in Record, and extend Record for each specific record
> method.
>
> Record
> {
> method, format;
> instruction_history;
> };

Agreed. The ptid seems to be an implementation detail. Tim, would you be
OK to remove it?

I would further put the function-call history into the Record base-class. It is
merely another view that can be computed from the instruction-history.
Record targets that don't support this view can return None.

Also the replay_position is common to all record targets that support reverse/
replay. And record targets that don't will always remain at the end of the
recorded trace. I don't see why a record target would not be able to provide
it. Currently, all record targets support it.

If you and Tim agree with this, we won't need a btrace sub-class, anymore and
this should be a simple renaming.

Thanks,
Markus.

> > I'll post my design draft tomorrow, then people can compare
> > the current design to mine. If we think we need to improve the
> > current design, it is unlikely to get it done by 8.0.
>
> Python APIs of record and btrace
>
> This summarizes the new added record and btrace Python APIs, in order
> to facilitate the discussion that whether we need to release them in
> 8.0.
>
> * Current status and design
>
> These new python APIs and classes are added to master already.
>
> BtraceInstruction
> {
> int number;
> int error;
> gdb.Symtab_and_line sal;
> pc, data, decoded, size;
> is_speculative;
> };
>
> BtraceFunctionCall
> {
> int number;
> gdb.Symbol symbol;
> int level;
> BtraceInstruction instructions;
> BtraceFunctionCall up, prev_sibling, next_sibling;
> };
>
> gdb.current_recording (), gdb.start_recording and
> gdb.stop_recording (),
>
> Record
> {
> ptid;
> format;
> begin, end;
> replay_position;
> instruction_history, function_call_history;
> };
>
> * Issues in the APIs and suggestions
>
> 1. BtraceInstruction.sal is not necessary. It can be got via
> gdb.find_pc_line(BtraceInstruction.pc).
>
> 2. BtraceInstruction has some attributes (pc, data, decoded, and size,)
> which are not btrace related.
> They should be moved to a new class gdb.Instruction and BtraceInstruction
> extends it.
>
> Instruction
> {
> pc, data, decode, size;
> };
>
> BtraceInstruction : Instruction
> {
> int number;
> int error;
> is_speculative;
> };
>
> gdb.Instruction can be used in somewhere else, for example, we can add
> a function Inferior.read_code (address), which returns gdb.Instruction.
> The record of other methods, like "full", can get its own instructions,
> but they have to be extended from gdb.Instruction. Just like
> gdb.FinishBreakpoint extends gdb.Breakpoint.
>
> 3. Why does Record have an attribute ptid?
> Other record method may have process-wide record/trace. I'd like to put
> common attributes in Record, and extend Record for each specific record
> method.
>
> Record
> {
> method, format;
> instruction_history;
> };
>
> BtraceRecord : Record
> {
> ptid;
> ...
> };
>
> FullRecord : Record
> {
> };
>
> gdb.current_recording () can be a factory returning the right Record
> object according to target_record_method (inferior_ptid).
>
> * Actions
>
> According to my analysis above, these APIs still need some adjustments
> and re-designs, so I request to revert these commits below,
>
> cee59b3 Fix break on Python 2
> 0a0faf9 Add documentation for new record Python bindings.
> 714aa61 python: Add tests for record Python bindings
> 75c0bdf python: Implement btrace Python bindings for record history.
> 4726b2d python: Create Python bindings for record history.
>
> and add them back gradually after 8.0 release. To be clear, I can offer
> some times review the new design and patches later, but I don't take
> over this project, Tim is still the owner.
>
> --
> Yao (齐尧)
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928