GDB 8.0 release/branching 2017-03-20 update

GDB 8.0 release/branching 2017-03-20 update

[Joel Brobecker]

Hello everyone,

We are now roughly at our target branching date. Looking at the list
of PRs marked for 8.0, we still have:

* gdb/21187  Static linking of libstdc++ and libgcc with GDB leads to
             broken exception handling on AIX platform

	I think Nitish is having a little difficulties with
        the submission process, but the patch is looking cleaner.
        Hopefully it'll be accepted soon.

* gdb/21169     GDBServer on ARM can crash the inferior while single stepping

        Any update on this one? The previous status was:

        | Considered blocking, but only if it doesn't delay the release
        | unreasonably.
        |
        | Antoine is this PR's champion.

* gdb/14441	Need to support DW_TAG_rvalue_reference_type

	I think the part that we need for 8.0 is on the verge of
        getting in. Keith is on top of it, as far as I can remember.
        Keith?

In the "maybe" section, we have:

* [TimWiederhake] new "record" Python API should be finalized before we release
  (otherwise, we might be have a compatibility issue if we don't design
  them well from the start)

        https://sourceware.org/ml/gdb-patches/2017-03/msg00023.html

        Looks like the discussion is still fairly active, and I am starting
        to think that we might be putting too much pressure to get this done
        before 8.0.

        What's the status of this API? Is this something entirely new?
        Or something that replaces an API which was already contributed?
        If it's a replacement, was the older API part of a previous release?
        Looking at the NEWS file, it looks like it's something which
        would be new. So we could decide to not rush this into 8.0,
        and wait for 8.1, when we're more certain of the API we'd like.
        It would also give it another 6 months of use where we can
        easily change the API, before it's part of a release, and therefore
        harder to change.

* [TimWiederhake] Build issue when using Python 2.6.x

        What's the status of that patch? My questions were answered,
        but was the patch reviewed?

        https://www.sourceware.org/ml/gdb-patches/2017-03/msg00053.html

Thanks!
-- 
Joel

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

[Keith Seitz]

On 03/20/2017 01:16 PM, Joel Brobecker wrote:
> * gdb/14441	Need to support DW_TAG_rvalue_reference_type
> 
> 	I think the part that we need for 8.0 is on the verge of
>         getting in. Keith is on top of it, as far as I can remember.
>         Keith?

Everything is approved, and I'm just about to push the whole series.

Keith

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

[Yao Qi]

On Mon, Mar 20, 2017 at 8:16 PM, Joel Brobecker <brobecker@adacore.com> wrote:

Hi Joel,
Tim contacted me off-list that his work site will relocate at the
end of march and he will not be able to respond to mails or
work on the patches.  It may take two weeks, and I am not
sure he has been unreachable from now on.

> * [TimWiederhake] new "record" Python API should be finalized before we release
>   (otherwise, we might be have a compatibility issue if we don't design
>   them well from the start)
>
>         https://sourceware.org/ml/gdb-patches/2017-03/msg00023.html
>
>         Looks like the discussion is still fairly active, and I am starting
>         to think that we might be putting too much pressure to get this done
>         before 8.0.
>
>         What's the status of this API? Is this something entirely new?
>         Or something that replaces an API which was already contributed?

They are new python APIs.

>         If it's a replacement, was the older API part of a previous release?
>         Looking at the NEWS file, it looks like it's something which
>         would be new. So we could decide to not rush this into 8.0,
>         and wait for 8.1, when we're more certain of the API we'd like.
>         It would also give it another 6 months of use where we can
>         easily change the API, before it's part of a release, and therefore
>         harder to change.

I've started reading this new APIs and the code since late last week.
Progress slowly as I know little about GDB python.  I gradually
understand the code, and do some experiments to make sure
my suggestions can be done.  I want to re-design the APIs, to
make them more general, less btrace specific.

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.

>
> * [TimWiederhake] Build issue when using Python 2.6.x
>
>         What's the status of that patch? My questions were answered,
>         but was the patch reviewed?
>
>         https://www.sourceware.org/ml/gdb-patches/2017-03/msg00053.html
>

I've already conditionally approved the patch, with a fix in
doc part, "Python 3" or "@code{Python} 3".  Tim's patch uses
the former but I suggested the latter.  Tim believes "Python 3"
should be used (off-list) so I asked Eli
https://sourceware.org/ml/gdb-patches/2017-03/msg00334.html

-- 
Yao (齐尧)

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

[Yao Qi]

On Mon, Mar 20, 2017 at 10:39 PM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> * [TimWiederhake] Build issue when using Python 2.6.x
>>
>>         What's the status of that patch? My questions were answered,
>>         but was the patch reviewed?
>>
>>         https://www.sourceware.org/ml/gdb-patches/2017-03/msg00053.html
>>
>
> I've already conditionally approved the patch, with a fix in
> doc part, "Python 3" or "@code{Python} 3".  Tim's patch uses
> the former but I suggested the latter.  Tim believes "Python 3"
> should be used (off-list) so I asked Eli
> https://sourceware.org/ml/gdb-patches/2017-03/msg00334.html
>

Just got Eli's reply.  Tim's patch can go in without any changes.
I'll take care of this patch if Tim is unreachable.

-- 
Yao (齐尧)

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

[Joel Brobecker]

Hi Yao,

> Tim contacted me off-list that his work site will relocate at the end
> of march and he will not be able to respond to mails or work on the
> patches.  It may take two weeks, and I am not sure he has been
> unreachable from now on.

Ok, thanks for the heads up.

> > * [TimWiederhake] new "record" Python API should be finalized before we release
> >   (otherwise, we might be have a compatibility issue if we don't design
> >   them well from the start)
> >
> >         https://sourceware.org/ml/gdb-patches/2017-03/msg00023.html
> >
> >         Looks like the discussion is still fairly active, and I am starting
> >         to think that we might be putting too much pressure to get this done
> >         before 8.0.
> >
> >         What's the status of this API? Is this something entirely new?
> >         Or something that replaces an API which was already contributed?
> 
> They are new python APIs.
> 
> >         If it's a replacement, was the older API part of a previous release?
> >         Looking at the NEWS file, it looks like it's something which
> >         would be new. So we could decide to not rush this into 8.0,
> >         and wait for 8.1, when we're more certain of the API we'd like.
> >         It would also give it another 6 months of use where we can
> >         easily change the API, before it's part of a release, and therefore
> >         harder to change.
> 
> I've started reading this new APIs and the code since late last week.
> Progress slowly as I know little about GDB python.  I gradually
> understand the code, and do some experiments to make sure
> my suggestions can be done.  I want to re-design the APIs, to
> make them more general, less btrace specific.
> 
> 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.

Since they are new, and we're still deep in the design phase,
I think it is more reasonable to branch without them. The worse
thing that can happen, really, is for us to rush and then and
realize later on that we should have taken our time. Sounds like
you agree, and with Tim's limited availability, I'll move this
item to the excluded list.

> > * [TimWiederhake] Build issue when using Python 2.6.x
> >
> >         What's the status of that patch? My questions were answered,
> >         but was the patch reviewed?
> >
> >         https://www.sourceware.org/ml/gdb-patches/2017-03/msg00053.html
> >
> 
> I've already conditionally approved the patch, with a fix in
> doc part, "Python 3" or "@code{Python} 3".  Tim's patch uses
> the former but I suggested the latter.  Tim believes "Python 3"
> should be used (off-list) so I asked Eli
> https://sourceware.org/ml/gdb-patches/2017-03/msg00334.html

Looks like Eli just answered! I'm a little tied up today, but
if no one gets to it before tomorrow, I'll try to dig the patch
out, and push it.

Thanks!
-- 
Joel

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

[Yao Qi]

On Mon, Mar 20, 2017 at 10:52 PM, Joel Brobecker <brobecker@adacore.com> wrote:

> Since they are new, and we're still deep in the design phase,
> I think it is more reasonable to branch without them. The worse

What do you mean by "branch without them"?  These patches
were reviewed and committed on Feb.  I didn't have a chance
reading the patches, until recently I want to use these python
APIs to measure the performance/speed of reverse debugging.

> thing that can happen, really, is for us to rush and then and
> realize later on that we should have taken our time. Sounds like
> you agree, and with Tim's limited availability, I'll move this
> item to the excluded list.
>

-- 
Yao (齐尧)

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

[Wiederhake, Tim]

Hi all!

> > Tim contacted me off-list that his work site will relocate at the end
> > of march and he will not be able to respond to mails or work on the
> > patches.  It may take two weeks, and I am not sure he has been
> > unreachable from now on.

That is correct. Tomorrow and /maybe/ Thursday will be the last days for a while I'll be able to react.

> (...)
> > > * [TimWiederhake] Build issue when using Python 2.6.x
> > >
> > >         What's the status of that patch? My questions were answered,
> > >         but was the patch reviewed?
> > >
> > >
> > > https://www.sourceware.org/ml/gdb-patches/2017-03/msg00053.html
> > >
> >
> > I've already conditionally approved the patch, with a fix in doc part,
> > "Python 3" or "@code{Python} 3".  Tim's patch uses the former but I
> > suggested the latter.  Tim believes "Python 3"
> > should be used (off-list) so I asked Eli
> > https://sourceware.org/ml/gdb-patches/2017-03/msg00334.html
> 
> Looks like Eli just answered! I'm a little tied up today, but if no one gets to it
> before tomorrow, I'll try to dig the patch out, and push it.

Pushed a few minutes ago as cee59b3feac9a8f6300a5b788e3db4e15af2a894.

Regards,
Tim
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

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

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

[cc Doug who reviewed the patches v6]

> 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 (齐尧)

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

[Joel Brobecker]

> > Since they are new, and we're still deep in the design phase,
> > I think it is more reasonable to branch without them. The worse
> 
> What do you mean by "branch without them"?  These patches
> were reviewed and committed on Feb.  I didn't have a chance
> reading the patches, until recently I want to use these python
> APIs to measure the performance/speed of reverse debugging.

I misunderstood when you said they were new, and thought
nothing had been checked in yet. But I see that you're now
proposing we temporary back them out, which I agree is probably
the best compromise, at this point.

-- 
Joel

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

[Joel Brobecker]

> > Looks like Eli just answered! I'm a little tied up today, but if no
> > one gets to it before tomorrow, I'll try to dig the patch out, and
> > push it.
> 
> Pushed a few minutes ago as cee59b3feac9a8f6300a5b788e3db4e15af2a894.

Thanks, Tim. I updated the TODO in the release's wiki page
Acordingly.

-- 
Joel

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

[Joel Brobecker]

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

As said elsewhere, I think this is the best course of action,
as this is the kind of thing best not to rush. But since we did
accept the patches before, I think it would be also fair to assure
Tim short review cycles when he is back to working on this. Given
how invested Yao has been, I assume this isn't going to be an issue?

-- 
Joel

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

[Yao Qi]

Joel Brobecker <brobecker@adacore.com> writes:

> Hello everyone,
>
> We are now roughly at our target branching date. Looking at the list
> of PRs marked for 8.0, we still have:

Hi Joel,
There was a regression in some gdb.cp/ tests introduced by a change in
demangler, caught by buildbot
https://sourceware.org/ml/gdb-testers/2017-q1/msg04955.html

-- 
Yao (齐尧)

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

[Pedro Alves]

On 03/21/2017 02:00 PM, Yao Qi wrote:
> Joel Brobecker <brobecker@adacore.com> writes:
> 
>> Hello everyone,
>>
>> We are now roughly at our target branching date. Looking at the list
>> of PRs marked for 8.0, we still have:
> 
> Hi Joel,
> There was a regression in some gdb.cp/ tests introduced by a change in
> demangler, caught by buildbot
> https://sourceware.org/ml/gdb-testers/2017-q1/msg04955.html
> 

Probably fixed by this follow up patch:

 https://sourceware.org/ml/gdb-patches/2017-03/msg00204.html

Thanks,
Pedro Alves

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

[Metzger, Markus T]

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

Hi, Markus,
Thanks for your input.

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

IMO, SAL only makes sense to RecordInstruction.  I don't see why must put
SAL into Instruction.

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

Can we also extend Instruction for record target to add SAL there?  Then
it becomes,

  Instruction
  {
    pc, data, decode, size;
  };

  RecordInstruction : Instruction
  {
    sal;
    is_speculative;
  };

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

All the user visible stuff should be covered in this discussion.  We
need to mention that "gdb.RecordInstruction extends gdb.Instruction" in
the document, so that we don't have to document gdb.Instruction
attributes again in gdb.RecordInstruction.

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

The duck typing is about writing python code, but I raise my concerns in
this thread from the view of controlling the API *interface* for
different record methods by means of inheritance.  With or without
inheritance, the python script still can do duck typing, no problem, but
we will end up having completely different ways of organizing these
interface.  Suppose we have three record methods, "btrace", "full" and
"bar", in current approach, we need to document all the attributes for
these three methods, although some of them are in common.  Why don't we
put these common attributes into a base class, and only document the
base class *once*?

Further, with such base class, we can guarantee that the client python
scripts only access base class attributes are right even with the new
record method may be added in the future.  In current approach, we write
such python code "print(hex(i.pc), i.sal, i.decoded)" just because
BtraceInstruction and FullInstruction *happen* to have these
attributes.  What if a new record method "bar" have instruction trace
which doesn't have attribute "decoded"?

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

I like this idea.

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

Anything can make these APIs more general are great!

-- 
Yao (齐尧)

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

[Metzger, Markus T]

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Wednesday, March 22, 2017 6:09 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org; Wiederhake, Tim
> <tim.wiederhake@intel.com>; xdje42@gmail.com; Joel Brobecker
> <brobecker@adacore.com>
> Subject: Re: GDB 8.0 release/branching 2017-03-20 update

Hello Yao,

> >>   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.
> 
> IMO, SAL only makes sense to RecordInstruction.  I don't see why must put
> SAL into Instruction.

It is certainly not a must.  It might make things easier (or just more convenient)
but if we want a minimal Instruction base class, we would only need pc, decode
(rename to disassembly?), and size.

Record instructions will add data and sal and at least record-btrace will add
is_speculative.  In order to not complicate things too much I would add
is_speculative also to record-full (even if it is always false) and put everything
into a single RecordInstruction class for all record targets.  OK?


> > 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?
> 
> All the user visible stuff should be covered in this discussion.  We
> need to mention that "gdb.RecordInstruction extends gdb.Instruction" in
> the document, so that we don't have to document gdb.Instruction
> attributes again in gdb.RecordInstruction.
> 
> >
> > 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?
> >
> 
> The duck typing is about writing python code, but I raise my concerns in
> this thread from the view of controlling the API *interface* for
> different record methods by means of inheritance.  With or without
> inheritance, the python script still can do duck typing, no problem, but
> we will end up having completely different ways of organizing these
> interface.  Suppose we have three record methods, "btrace", "full" and
> "bar", in current approach, we need to document all the attributes for
> these three methods, although some of them are in common.  Why don't we
> put these common attributes into a base class, and only document the
> base class *once*?

Makes sense.  Is this how it is usually implemented in Python?

I liked about the design that we could have a record-(target-)specific internal
representation.  IIRC we only store the thread's ptid and the instruction number
in that thread's instruction history.  An Instruction object provided by some
future gdb.disassemble() function will need a different internal representation.
Will we be able to do this with a class hierarchy?


> Further, with such base class, we can guarantee that the client python
> scripts only access base class attributes are right even with the new
> record method may be added in the future.  In current approach, we write
> such python code "print(hex(i.pc), i.sal, i.decoded)" just because
> BtraceInstruction and FullInstruction *happen* to have these
> attributes.  What if a new record method "bar" have instruction trace
> which doesn't have attribute "decoded"?

They'd get a not-implemented exception.

I don't see how we could prevent that, though.  If we added another
recording method that didn't provide one of the functions the old
recording method's instruction object provided, it will have to fail
somehow.

And we don't want scripts to restrict themselves to the Instruction
base class.  If they are using record-btrace we do want them to get
the sal from the instruction object, for example.

Thanks,
Markus.

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> It is certainly not a must.  It might make things easier (or just more
> convenient)
> but if we want a minimal Instruction base class, we would only need pc, decode
> (rename to disassembly?), and size.
>
> Record instructions will add data and sal and at least record-btrace will add
> is_speculative.  In order to not complicate things too much I would add
> is_speculative also to record-full (even if it is always false) and
> put everything
> into a single RecordInstruction class for all record targets.  OK?
>

I hate the over-complicated design as well.  That is fine by me.

>
>> > 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?
>> 
>> All the user visible stuff should be covered in this discussion.  We
>> need to mention that "gdb.RecordInstruction extends gdb.Instruction" in
>> the document, so that we don't have to document gdb.Instruction
>> attributes again in gdb.RecordInstruction.
>> 
>> >
>> > 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?
>> >
>> 
>> The duck typing is about writing python code, but I raise my concerns in
>> this thread from the view of controlling the API *interface* for
>> different record methods by means of inheritance.  With or without
>> inheritance, the python script still can do duck typing, no problem, but
>> we will end up having completely different ways of organizing these
>> interface.  Suppose we have three record methods, "btrace", "full" and
>> "bar", in current approach, we need to document all the attributes for
>> these three methods, although some of them are in common.  Why don't we
>> put these common attributes into a base class, and only document the
>> base class *once*?
>
> Makes sense.  Is this how it is usually implemented in Python?
>

I don't know.

> I liked about the design that we could have a record-(target-)specific internal
> representation.  IIRC we only store the thread's ptid and the instruction number
> in that thread's instruction history.  An Instruction object provided by some
> future gdb.disassemble() function will need a different internal representation.
> Will we be able to do this with a class hierarchy?
>

What do you mean by "internal representation"?  They are not internal,
all of them are externally visible.

>> Further, with such base class, we can guarantee that the client python
>> scripts only access base class attributes are right even with the new
>> record method may be added in the future.  In current approach, we write
>> such python code "print(hex(i.pc), i.sal, i.decoded)" just because
>> BtraceInstruction and FullInstruction *happen* to have these
>> attributes.  What if a new record method "bar" have instruction trace
>> which doesn't have attribute "decoded"?
>
> They'd get a not-implemented exception.
>
> I don't see how we could prevent that, though.  If we added another
> recording method that didn't provide one of the functions the old
> recording method's instruction object provided, it will have to fail
> somehow.
>
> And we don't want scripts to restrict themselves to the Instruction
> base class.  If they are using record-btrace we do want them to get
> the sal from the instruction object, for example.

We don't restrict them to the base class.  They are still free using
the class of their preferred record methods.  Again, my
suggestion/design doesn't apply any restrictions to using these python
APIs.  Instead, I intended to apply the restrictions to the python APIs.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> >> > 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?
> >>
> >> All the user visible stuff should be covered in this discussion.  We
> >> need to mention that "gdb.RecordInstruction extends gdb.Instruction" in
> >> the document, so that we don't have to document gdb.Instruction
> >> attributes again in gdb.RecordInstruction.
> >>
> >> >
> >> > 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?
> >> >
> >>
> >> The duck typing is about writing python code, but I raise my concerns in
> >> this thread from the view of controlling the API *interface* for
> >> different record methods by means of inheritance.  With or without
> >> inheritance, the python script still can do duck typing, no problem, but
> >> we will end up having completely different ways of organizing these
> >> interface.  Suppose we have three record methods, "btrace", "full" and
> >> "bar", in current approach, we need to document all the attributes for
> >> these three methods, although some of them are in common.  Why don't we
> >> put these common attributes into a base class, and only document the
> >> base class *once*?
> >
> > Makes sense.  Is this how it is usually implemented in Python?
> >
> 
> I don't know.
> 
> > I liked about the design that we could have a record-(target-)specific internal
> > representation.  IIRC we only store the thread's ptid and the instruction
> number
> > in that thread's instruction history.  An Instruction object provided by some
> > future gdb.disassemble() function will need a different internal representation.
> > Will we be able to do this with a class hierarchy?
> >
> 
> What do you mean by "internal representation"?  They are not internal,
> all of them are externally visible.

I meant that's all we store internally.  Everything else, the pc, the instruction's bytes,
the disassembly, even the is_speculative flag, is computed when the respective
function is called (or field is accessed).

I think we already agreed to remove the ptid from the API.  I'm also not really sure
we need the number in Python.  It is needed by the CLI to refer to an instruction
but in Python we can use an instruction object directly.

Once we drop those, the internal representation will be completely different from the
external one.  And it will be record-(btrace-)specific.  I like that a lot since computing
the disassembly is rather expensive and the string is relatively big.  When a script is
iterating over a few million instructions and all it needs is the pc or the sal or the
is_speculative flag to find a TSX region, we don't want to disassemble the instruction.


> >> Further, with such base class, we can guarantee that the client python
> >> scripts only access base class attributes are right even with the new
> >> record method may be added in the future.  In current approach, we write
> >> such python code "print(hex(i.pc), i.sal, i.decoded)" just because
> >> BtraceInstruction and FullInstruction *happen* to have these
> >> attributes.  What if a new record method "bar" have instruction trace
> >> which doesn't have attribute "decoded"?
> >
> > They'd get a not-implemented exception.
> >
> > I don't see how we could prevent that, though.  If we added another
> > recording method that didn't provide one of the functions the old
> > recording method's instruction object provided, it will have to fail
> > somehow.
> >
> > And we don't want scripts to restrict themselves to the Instruction
> > base class.  If they are using record-btrace we do want them to get
> > the sal from the instruction object, for example.
> 
> We don't restrict them to the base class.  They are still free using
> the class of their preferred record methods.  Again, my
> suggestion/design doesn't apply any restrictions to using these python
> APIs.  Instead, I intended to apply the restrictions to the python APIs.

It makes perfect sense to agree on the fields the various record targets
provide.

Regards,
Markus.
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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> I meant that's all we store internally.  Everything else, the pc, the
> instruction's bytes,
> the disassembly, even the is_speculative flag, is computed when the respective
> function is called (or field is accessed).
>

We can also compute them lazily in gdb.disassemble ().

> I think we already agreed to remove the ptid from the API.  I'm also
> not really sure
> we need the number in Python.  It is needed by the CLI to refer to an
> instruction
> but in Python we can use an instruction object directly.
>
> Once we drop those, the internal representation will be completely
> different from the
> external one.  And it will be record-(btrace-)specific.  I like that a
> lot since computing

AFAICS, The internal representation of instruction object provided by
gdb.disassemble () can be a base class, and record-(target-) specific
instruction object can extend it, but doesn't have to.  It is an
implementation decision, and user invisible, right?

> the disassembly is rather expensive and the string is relatively big.
> When a script is
> iterating over a few million instructions and all it needs is the pc
> or the sal or the
> is_speculative flag to find a TSX region, we don't want to disassemble
> the instruction.

I agree.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > I meant that's all we store internally.  Everything else, the pc, the
> > instruction's bytes,
> > the disassembly, even the is_speculative flag, is computed when the respective
> > function is called (or field is accessed).
> >
> 
> We can also compute them lazily in gdb.disassemble ().
> 
> > I think we already agreed to remove the ptid from the API.  I'm also
> > not really sure
> > we need the number in Python.  It is needed by the CLI to refer to an
> > instruction
> > but in Python we can use an instruction object directly.
> >
> > Once we drop those, the internal representation will be completely
> > different from the
> > external one.  And it will be record-(btrace-)specific.  I like that a
> > lot since computing
> 
> AFAICS, The internal representation of instruction object provided by
> gdb.disassemble () can be a base class, and record-(target-) specific
> instruction object can extend it, but doesn't have to.  It is an
> implementation decision, and user invisible, right?

Instruction would have to be an abstract class.  Then we could have two
derived classes DisassembleInstruction for the disassemble command
that likely wants to store the pc and maybe the inferior id - or even the
disassembled string.
And we have Record(Btrace)Instruction that wants to store the ptid
and the number in the recorded instruction-history.

What won't work is that we simply extend an Instruction base class by
adding new functions and data members.  We need to overwrite every
function in the base class and each derived class will provide its own set
of data members to work on.

Thanks,
Markus.

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> Instruction would have to be an abstract class.  Then we could have two
> derived classes DisassembleInstruction for the disassemble command
> that likely wants to store the pc and maybe the inferior id - or even the
> disassembled string.
> And we have Record(Btrace)Instruction that wants to store the ptid
> and the number in the recorded instruction-history.

Instruction doesn't have to be an abstract class, instead, it can be a
base class which has attribute pc, data, and disassembled string.  All
of them can be used in Record(Btrace)Instruction too, but "data" and
"disassembled string" can be got lazily.  Record(Btrace)Instruction
extends Instruction, and add its own stuff.  In the future, we can add
new RecordFooInstruction, extends Instruction, and add its own stuff too.

>
> What won't work is that we simply extend an Instruction base class by
> adding new functions and data members.  We need to overwrite every
> function in the base class and each derived class will provide its own set
> of data members to work on.

I don't expect the change like that.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > Instruction would have to be an abstract class.  Then we could have two
> > derived classes DisassembleInstruction for the disassemble command
> > that likely wants to store the pc and maybe the inferior id - or even the
> > disassembled string.
> > And we have Record(Btrace)Instruction that wants to store the ptid
> > and the number in the recorded instruction-history.
> 
> Instruction doesn't have to be an abstract class, instead, it can be a
> base class which has attribute pc, data, and disassembled string.  All
> of them can be used in Record(Btrace)Instruction too, but "data" and
> "disassembled string" can be got lazily.  Record(Btrace)Instruction
> extends Instruction, and add its own stuff.  In the future, we can add
> new RecordFooInstruction, extends Instruction, and add its own stuff too.

RecordInstruction wants to store ptid and insn# and compute pc, data, and
disassembled string from those.  It may need to read memory from binaries
that have already been unmapped.  But we only want to read memory if we
really have to, so we can't just store the memory in the Instruction object.

DisassembleInstruction can't use ptid and insn# since insn# doesn't mean
anything outside of record targets.  It will need to store different information
from which it can compute pc, data, and the disassembled string.

While they provide the same interface (i.e. member functions) to the user, they
store different information internally and they use different algorithms.  In C++
I would model this with an abstract base class.  How would one model this in Python?


> > What won't work is that we simply extend an Instruction base class by
> > adding new functions and data members.  We need to overwrite every
> > function in the base class and each derived class will provide its own set
> > of data members to work on.
> 
> I don't expect the change like that.

I didn't understand your comment.  Would you please rephrase it?

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

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

[Antoine Tremblay]

Joel Brobecker writes:

> * gdb/21169     GDBServer on ARM can crash the inferior while single stepping
>
>         Any update on this one? The previous status was:
>
>         | Considered blocking, but only if it doesn't delay the release
>         | unreasonably.
>         |
>         | Antoine is this PR's champion.

Just a head's up, I'm back from leave and I'm working on this now, I
expect it will take around 2 weeks to get a proper patch...

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> RecordInstruction wants to store ptid and insn# and compute pc, data, and
> disassembled string from those.  It may need to read memory from binaries
> that have already been unmapped.  But we only want to read memory if we
> really have to, so we can't just store the memory in the Instruction object.
>

They are all about internal representation, aren't they?  Let me be
clear, "internal representation" is about the C code, not python one.

> DisassembleInstruction can't use ptid and insn# since insn# doesn't mean
> anything outside of record targets.  It will need to store different information
> from which it can compute pc, data, and the disassembled string.
>
> While they provide the same interface (i.e. member functions) to the user, they

"the same interface" is python interface or c interface?  I assume you
meant python interface.

> store different information internally and they use different
> algorithms.  In C++
> I would model this with an abstract base class.  How would one model
> this in Python?
>

We can have the same interfaces in Python but with different
implementations in CPython.  Suppose we have a base class Instruction in
python, and it has attributes, pc, size, data and decoded.  Its CPython
implementation is what current BtraceInstruction does, which only gets
contents when they are accessed.  However, we are free to change the
implementation to store all needed contents when the object is created.
Classes Instruction, BtraceInstruction, and FullInstruction can have
their own functions to access these four attributes in different ways.
The key point is that record.instruction_history (in Python) return a
list of objects which has these four attributes in common.  It doesn't
matter in Python on how to get their value/contents.

>
>> > What won't work is that we simply extend an Instruction base class by
>> > adding new functions and data members.  We need to overwrite every
>> > function in the base class and each derived class will provide its own set
>> > of data members to work on.
>> 
>> I don't expect the change like that.
>
> I didn't understand your comment.  Would you please rephrase it?

What I meant is that I don't expect adding *new* functions or data members
in the base class, but I don't see why can't override function in the
base class.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > RecordInstruction wants to store ptid and insn# and compute pc, data, and
> > disassembled string from those.  It may need to read memory from binaries
> > that have already been unmapped.  But we only want to read memory if we
> > really have to, so we can't just store the memory in the Instruction object.
> >
> 
> They are all about internal representation, aren't they?  Let me be
> clear, "internal representation" is about the C code, not python one.

Correct.  I am only talking about the internal representation.  This was to clarify
that we need to overwrite all inherited functions and that we will have a different
set of data members for each variant.


> > DisassembleInstruction can't use ptid and insn# since insn# doesn't mean
> > anything outside of record targets.  It will need to store different information
> > from which it can compute pc, data, and the disassembled string.
> >
> > While they provide the same interface (i.e. member functions) to the user,
> they
> 
> "the same interface" is python interface or c interface?  I assume you
> meant python interface.

Yes, I was referring to the Python interface.


> > store different information internally and they use different
> > algorithms.  In C++
> > I would model this with an abstract base class.  How would one model
> > this in Python?
> >
> 
> We can have the same interfaces in Python but with different
> implementations in CPython.  Suppose we have a base class Instruction in
> python, and it has attributes, pc, size, data and decoded.  Its CPython
> implementation is what current BtraceInstruction does, which only gets
> contents when they are accessed.  However, we are free to change the
> implementation to store all needed contents when the object is created.
> Classes Instruction, BtraceInstruction, and FullInstruction can have
> their own functions to access these four attributes in different ways.
> The key point is that record.instruction_history (in Python) return a
> list of objects which has these four attributes in common.  It doesn't
> matter in Python on how to get their value/contents.

That sounds good.  We want to provide a fixed set of functions and we want
to be free to choose the best internal representation for each variant.

How would this look in our python implementation?


> >> > What won't work is that we simply extend an Instruction base class by
> >> > adding new functions and data members.  We need to overwrite every
> >> > function in the base class and each derived class will provide its own set
> >> > of data members to work on.
> >>
> >> I don't expect the change like that.
> >
> > I didn't understand your comment.  Would you please rephrase it?
> 
> What I meant is that I don't expect adding *new* functions or data members
> in the base class, but I don't see why can't override function in the
> base class.

Thanks.  In addition to overwriting base class functions, BtraceInstruction will
add a sal function.  My point was that this won't be enough and it looks like we
agree on this.

Regards,
Markus.

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> That sounds good.  We want to provide a fixed set of functions and we want
> to be free to choose the best internal representation for each variant.
>

Exactly.

> How would this look in our python implementation?

I am not sure.  One approach in my mind is that sub-class can overwrite
by defining its own getset.

struct PyGetSetDef py_insn_getset[] =
 {
  { "data", py_insn_data, NULL, "raw instruction data", NULL},
  { "decoded", py_insn_decode, NULL, "decoded instruction", NULL},
  { "size", py_insn_size, NULL, "instruction size in byte", NULL},
  { "pc", py_insn_pc, NULL, "instruction address", NULL },
  {NULL}
};

struct PyGetSetDef btpy_insn_getset[] =
{
  { "data", btpy_insn_data, NULL, "raw instruction data", NULL},
  { "decoded", btpy_insn_decode, NULL, "decoded instruction", NULL},
  { "size", btpy_insn_size, NULL, "instruction size in byte", NULL},
  { "pc", btpy_insn_pc, NULL, "instruction address", NULL },

  { "number", btpy_number, NULL, "instruction number", NULL},
  { "sal", btpy_sal, NULL, "instruction number", NULL},
  {NULL}
};

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > How would this look in our python implementation?
> 
> I am not sure.  One approach in my mind is that sub-class can overwrite
> by defining its own getset.
> 
> struct PyGetSetDef py_insn_getset[] =
>  {
>   { "data", py_insn_data, NULL, "raw instruction data", NULL},
>   { "decoded", py_insn_decode, NULL, "decoded instruction", NULL},
>   { "size", py_insn_size, NULL, "instruction size in byte", NULL},
>   { "pc", py_insn_pc, NULL, "instruction address", NULL },
>   {NULL}
> };

This is for the (abstract) base-class, I assume.

We don't store any data in the base class so the Python object would
contain the PyObject header and nothing else, correct?

And the above functions would throw an exception or return None.
Correct?


> struct PyGetSetDef btpy_insn_getset[] =
> {
>   { "data", btpy_insn_data, NULL, "raw instruction data", NULL},
>   { "decoded", btpy_insn_decode, NULL, "decoded instruction", NULL},
>   { "size", btpy_insn_size, NULL, "instruction size in byte", NULL},
>   { "pc", btpy_insn_pc, NULL, "instruction address", NULL },
> 
>   { "number", btpy_number, NULL, "instruction number", NULL},
>   { "sal", btpy_sal, NULL, "instruction number", NULL},
>   {NULL}
> };

This is for the BtraceInstruction derived class, I assume.  That's essentially
what Tim implemented.  Correct?

This doesn't look too far away from what we have in GDB today.

Thanks,
Markus.

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

>> struct PyGetSetDef py_insn_getset[] =
>>  {
>>   { "data", py_insn_data, NULL, "raw instruction data", NULL},
>>   { "decoded", py_insn_decode, NULL, "decoded instruction", NULL},
>>   { "size", py_insn_size, NULL, "instruction size in byte", NULL},
>>   { "pc", py_insn_pc, NULL, "instruction address", NULL },
>>   {NULL}
>> };
>
> This is for the (abstract) base-class, I assume.
>

Yes, it is the base class, but it is the implementation detail about the
class is abstract or not.  I haven't think about it yet.  Again, I only
care about python interface at this stage.

> We don't store any data in the base class so the Python object would
> contain the PyObject header and nothing else, correct?
>
> And the above functions would throw an exception or return None.
> Correct?
>

I didn't go that far yet :)

>
>> struct PyGetSetDef btpy_insn_getset[] =
>> {
>>   { "data", btpy_insn_data, NULL, "raw instruction data", NULL},
>>   { "decoded", btpy_insn_decode, NULL, "decoded instruction", NULL},
>>   { "size", btpy_insn_size, NULL, "instruction size in byte", NULL},
>>   { "pc", btpy_insn_pc, NULL, "instruction address", NULL },
>> 
>>   { "number", btpy_number, NULL, "instruction number", NULL},
>>   { "sal", btpy_sal, NULL, "instruction number", NULL},
>>   {NULL}
>> };
>
> This is for the BtraceInstruction derived class, I assume.  That's essentially
> what Tim implemented.  Correct?
>
> This doesn't look too far away from what we have in GDB today.

I requested to change the python interface in Tim's patch, not the
implementation.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> >> struct PyGetSetDef py_insn_getset[] =
> >>  {
> >>   { "data", py_insn_data, NULL, "raw instruction data", NULL},
> >>   { "decoded", py_insn_decode, NULL, "decoded instruction", NULL},
> >>   { "size", py_insn_size, NULL, "instruction size in byte", NULL},
> >>   { "pc", py_insn_pc, NULL, "instruction address", NULL },
> >>   {NULL}
> >> };
> >
> > This is for the (abstract) base-class, I assume.
> >
> 
> Yes, it is the base class, but it is the implementation detail about the
> class is abstract or not.  I haven't think about it yet.  Again, I only
> care about python interface at this stage.
> 
> > We don't store any data in the base class so the Python object would
> > contain the PyObject header and nothing else, correct?
> >
> > And the above functions would throw an exception or return None.
> > Correct?
> >
> 
> I didn't go that far yet :)

What else could the do?  We agreed that the base class will be abstract and
let the derived classes chose the data members to best implement the functions.



> >> struct PyGetSetDef btpy_insn_getset[] =
> >> {
> >>   { "data", btpy_insn_data, NULL, "raw instruction data", NULL},
> >>   { "decoded", btpy_insn_decode, NULL, "decoded instruction", NULL},
> >>   { "size", btpy_insn_size, NULL, "instruction size in byte", NULL},
> >>   { "pc", btpy_insn_pc, NULL, "instruction address", NULL },
> >>
> >>   { "number", btpy_number, NULL, "instruction number", NULL},
> >>   { "sal", btpy_sal, NULL, "instruction number", NULL},
> >>   {NULL}
> >> };
> >
> > This is for the BtraceInstruction derived class, I assume.  That's essentially
> > what Tim implemented.  Correct?
> >
> > This doesn't look too far away from what we have in GDB today.
> 
> I requested to change the python interface in Tim's patch, not the
> implementation.

Now I'm a bit lost.  What exactly do you want changed at this point to
establish the Python interface?

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

Hi Markus,

> Now I'm a bit lost.  What exactly do you want changed at this point to
> establish the Python interface?

I requested the change to Python interface, as described
https://sourceware.org/ml/gdb-patches/2017-03/msg00366.html and the
python interface is implementable, nothing odd or fancy there.  I want:

 1, we agree on that new python interface,
 2, decide we either change the python interface according to my
 proposal before 8.0 release or release 8.0 without these record
 (btrace) python interfaces.

I've never suggest anything on the implementation of the CPython code
for the python interface.  Anything reasonable is good to me, and we are
free to change them because they are internal things.  How to establish
such python interface is a little bit out of the scope of this
discussion, but let me try again to make it clear.

In my proposal above, we have Python classes,

Instruction
{
   pc, data, decode, size;
};

RecordInstruction : Instruction
{
  sal;
  is_speculative;
}

Record.instruction_history returns a list of RecordInstruction objects.
Instruction can be used for disassemble in Python.  Note that how does
these attributes get data is the implementation (C code) details.  We
have two approaches,

 a) copy all the data into the internal structure when the object is
 created, and the "get" function just returns the data in the internal
 structure,
 b) get the data from gdb when the attribute is accessed,

Now, let us dive into C code, and py_insn_getset is for Python class
Instruction,

 struct PyGetSetDef py_insn_getset[] =
  {
   { "data", py_insn_data, NULL, "raw instruction data", NULL},
   { "decoded", py_insn_decode, NULL, "decoded instruction", NULL},
   { "size", py_insn_size, NULL, "instruction size in byte", NULL},
   { "pc", py_insn_pc, NULL, "instruction address", NULL },
   {NULL}
 };

We are free to choose approach a) or b) for functions py_insn_data and
py_insn_pc.  Python class RecordInstruction has its own getset.
If the py_insn_data's behavior is what RecordInstruction expects, just
use it, otherwise, overwrite its own function.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > Now I'm a bit lost.  What exactly do you want changed at this point to
> > establish the Python interface?
> 
> I requested the change to Python interface, as described
> https://sourceware.org/ml/gdb-patches/2017-03/msg00366.html and the
> python interface is implementable, nothing odd or fancy there.  I want:
> 
>  1, we agree on that new python interface,
>  2, decide we either change the python interface according to my
>  proposal before 8.0 release or release 8.0 without these record
>  (btrace) python interfaces.
> 
> I've never suggest anything on the implementation of the CPython code
> for the python interface.  Anything reasonable is good to me, and we are
> free to change them because they are internal things.  How to establish
> such python interface is a little bit out of the scope of this
> discussion, but let me try again to make it clear.
> 
> In my proposal above, we have Python classes,
> 
> Instruction
> {
>    pc, data, decode, size;
> };
> 
> RecordInstruction : Instruction
> {
>   sal;
>   is_speculative;
> }

We agreed on this interface.

What I'm less clear about is what changes you are requesting in order to
establish that interface.

Should we just document it that way?
Should we implement an Instruction base class that throws in all functions?
Should we implement an Instruction base class that returns None in all functions?

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> What I'm less clear about is what changes you are requesting in order to
> establish that interface.
>
> Should we just document it that way?

Yes, I think so.  (,Record)Instruction.{pc, data, decode, size} should
behave as expected, .pc should return the address of instruction, and
.data should return the bytes of instruction, .decode should return a
string of asm code.

From documentation point of view, these four attributes are documented
in Instruction, and RecordInstruction extends Instruction.

> Should we implement an Instruction base class that throws in all functions?
> Should we implement an Instruction base class that returns None in all
> functions?

This is the C implementation decision, not related to Python interface nor
document.  Python script can only get RecordInstruction objects via
Record.instruction_history, and RecordInstruction.{pc, data, decode,
size} should behave as we documented for Instruction.  So far, that is
no way in python script to get Instruction objects.  We can either throw
exceptions or return None. (I personally prefer exceptions, if you
really want me to choose one).

Suppose, one day, we add a new python interface like
gdb.Inferior.read_insn (start_pc,) returns Instruction objects.  The These
objects should behave as we already documented.  In C code, we can
change Instruction's getset functions to do something meaningful rather
than throwing exception.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > What I'm less clear about is what changes you are requesting in order to
> > establish that interface.
> >
> > Should we just document it that way?
> 
> Yes, I think so.  (,Record)Instruction.{pc, data, decode, size} should
> behave as expected, .pc should return the address of instruction, and
> .data should return the bytes of instruction, .decode should return a
> string of asm code.
> 
> From documentation point of view, these four attributes are documented
> in Instruction, and RecordInstruction extends Instruction.

OK.  That shouldn't take long.  Also the other items we discussed shouldn't
take long to implement.  We agreed to remove ptid and number from
RecordBtraceInstruction and rename it to RecordInstruction.

When we do this for record-full, we need different functions and we may also
want to store different data in the python object.

Anticipating such a change, I would document class RecordInstruction as we
discussed but keep RecordBtraceInstruction.  We may introduce yet another
abstract base class RecordInstruction.  OK?

We may not need to document class RecordBtraceInstruction since it does not
really add anything to class RecordInstruction.  The interface consists of classes
Instruction and RecordInstruction.  OK?


> > Should we implement an Instruction base class that throws in all functions?
> > Should we implement an Instruction base class that returns None in all
> > functions?
> 
> This is the C implementation decision, not related to Python interface nor
> document.  Python script can only get RecordInstruction objects via
> Record.instruction_history, and RecordInstruction.{pc, data, decode,
> size} should behave as we documented for Instruction.  So far, that is
> no way in python script to get Instruction objects.  We can either throw
> exceptions or return None. (I personally prefer exceptions, if you
> really want me to choose one).
> 
> Suppose, one day, we add a new python interface like
> gdb.Inferior.read_insn (start_pc,) returns Instruction objects.  The These
> objects should behave as we already documented.  In C code, we can
> change Instruction's getset functions to do something meaningful rather
> than throwing exception.

We will need to introduce another derived class DisassembleInstruction
since it will want to store data in the python object and we don't want that
data in RecordInstruction objects, so we can't add it to the Instruction base
class.

That's again talking about the implementation.  If we can model this in
Python without introducing new classes, I'm fine, as well.  We should also
do it like this for classes RecordInstruction and RecordBtraceInstruction in
that case.

Regards,
Markus.
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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> Anticipating such a change, I would document class RecordInstruction as we
> discussed but keep RecordBtraceInstruction.  We may introduce yet another
> abstract base class RecordInstruction.  OK?

Why do we still keep RecordBtraceInstruction in Python?

> We may not need to document class RecordBtraceInstruction since it does not
> really add anything to class RecordInstruction.  The interface
> consists of classes
> Instruction and RecordInstruction.  OK?

These two paragraphs are self-contradictory.  We don't need to keep
RecordBtraceInstruction in Python, just use RecordInstruction.  We've
already agreed that "put everything into a single RecordInstruction
class for all record targets", as you said
https://sourceware.org/ml/gdb-patches/2017-03/msg00420.html

>> > Should we implement an Instruction base class that throws in all functions?
>> > Should we implement an Instruction base class that returns None in all
>> > functions?
>> 
>> This is the C implementation decision, not related to Python interface nor
>> document.  Python script can only get RecordInstruction objects via
>> Record.instruction_history, and RecordInstruction.{pc, data, decode,
>> size} should behave as we documented for Instruction.  So far, that is
>> no way in python script to get Instruction objects.  We can either throw
>> exceptions or return None. (I personally prefer exceptions, if you
>> really want me to choose one).
>> 
>> Suppose, one day, we add a new python interface like
>> gdb.Inferior.read_insn (start_pc,) returns Instruction objects.  The These
>> objects should behave as we already documented.  In C code, we can
>> change Instruction's getset functions to do something meaningful rather
>> than throwing exception.
>
> We will need to introduce another derived class DisassembleInstruction
> since it will want to store data in the python object and we don't want that
> data in RecordInstruction objects, so we can't add it to the Instruction base
> class.
>
> That's again talking about the implementation.  If we can model this in

What is "this"?

> Python without introducing new classes, I'm fine, as well.  We should
> also

"without introducing new classes" in C?  What are the "new classes",
Disassembleinstruction?

> do it like this for classes RecordInstruction and RecordBtraceInstruction in
> that case.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

> > Anticipating such a change, I would document class RecordInstruction as we
> > discussed but keep RecordBtraceInstruction.  We may introduce yet another
> > abstract base class RecordInstruction.  OK?
> 
> Why do we still keep RecordBtraceInstruction in Python?
> 
> > We may not need to document class RecordBtraceInstruction since it does not
> > really add anything to class RecordInstruction.  The interface
> > consists of classes
> > Instruction and RecordInstruction.  OK?
> 
> These two paragraphs are self-contradictory.  We don't need to keep
> RecordBtraceInstruction in Python, just use RecordInstruction.  We've
> already agreed that "put everything into a single RecordInstruction
> class for all record targets", as you said
> https://sourceware.org/ml/gdb-patches/2017-03/msg00420.html

This may be an interface/implementation confusion again.

I thought we need it because we may get RecordFullInstruction in the future
that needs to store different data and install different functions.

They will behave identical but will have different implementations.

Can they pretend to be the same type in Python?


> >> > Should we implement an Instruction base class that throws in all functions?
> >> > Should we implement an Instruction base class that returns None in all
> >> > functions?
> >>
> >> This is the C implementation decision, not related to Python interface nor
> >> document.  Python script can only get RecordInstruction objects via
> >> Record.instruction_history, and RecordInstruction.{pc, data, decode,
> >> size} should behave as we documented for Instruction.  So far, that is
> >> no way in python script to get Instruction objects.  We can either throw
> >> exceptions or return None. (I personally prefer exceptions, if you
> >> really want me to choose one).
> >>
> >> Suppose, one day, we add a new python interface like
> >> gdb.Inferior.read_insn (start_pc,) returns Instruction objects.  The These
> >> objects should behave as we already documented.  In C code, we can
> >> change Instruction's getset functions to do something meaningful rather
> >> than throwing exception.
> >
> > We will need to introduce another derived class DisassembleInstruction
> > since it will want to store data in the python object and we don't want that
> > data in RecordInstruction objects, so we can't add it to the Instruction base
> > class.
> >
> > That's again talking about the implementation.  If we can model this in
> 
> What is "this"?
> 
> > Python without introducing new classes, I'm fine, as well.  We should
> > also
> 
> "without introducing new classes" in C?  What are the "new classes",
> Disassembleinstruction?
> 
> > do it like this for classes RecordInstruction and RecordBtraceInstruction in
> > that case.

This is the same as the above. 

Regards,
Markus.

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

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

[Yao Qi]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> I thought we need it because we may get RecordFullInstruction in the future
> that needs to store different data and install different functions.
>
> They will behave identical but will have different implementations.

Do you have some concrete reasons that we may change
Record{Full,Btrace,...}Instruction in C?  I can't figure out the reason
in the future we do the change, so I don't worry it now.

>
> Can they pretend to be the same type in Python?

Suppose one day in the future, we really need such change.  We can still
have different C structs for different record methods, different
PyTypeObject but with the same .tp_name "gdb.RecordInstruction" and
different .tp_getset.  I hacked GDB code, and find python doesn't
complain that I call PyType_Ready with different PyTypeObject, but they
have the same .tp_name.  Everything works fine.

If the change I do above is hacky, then we have to create sub-types of
RecordInstruction for different record methods in C
(gdb.priv.RecordFullinstruction for example?), but Python interface is
still documented as returning type gdb.RecordInstruction.

 r = gdb.current_recording()
 insn = r.instruction_history
 print insn[0].pc
 print insn[0].sal

and the in C code for .instruction_history,

  if (obj->method == RECORD_METHOD_BTRACE)
    return recpy_bt_instruction_history (obj->ptid, closure);
  else if (obj->method == RECORD_METHOD_FULL)
    {
      gdbpy_ref<> list (PyList_New (0));
      fullpy_insn_object * const obj = PyObject_New (fullpy_insn_object,
						     &fullpy_insn_type);

      if (obj == NULL)
	return NULL;

      obj->insn.insn.pc = 11;
      obj->insn.insn.size = 22;
      PyList_Append (list.get (), (PyObject *) obj);

      return PyList_AsTuple (list.get ());
    }

  return PyErr_Format (PyExc_NotImplementedError, _("Not implemented."));

Python code still uses insn[0] as the base type RecordInstruction, and
Python code doesn't need to know the sub-types of RecordInstruction C
code creates.  We deliberately don't document these sub-types in GDB if
we add them.  IMO, not every python types we added in C are public types.

-- 
Yao (齐尧)

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

[Metzger, Markus T]

Hello Yao,

Thanks for working with Tim and me on this.

> > I thought we need it because we may get RecordFullInstruction in the future
> > that needs to store different data and install different functions.
> >
> > They will behave identical but will have different implementations.
> 
> Do you have some concrete reasons that we may change
> Record{Full,Btrace,...}Instruction in C?  I can't figure out the reason
> in the future we do the change, so I don't worry it now.

They may want to store different data in the python object.  Disassemble and
record-btrace will definitely want to store different data. 


> > Can they pretend to be the same type in Python?
> 
> Suppose one day in the future, we really need such change.  We can still
> have different C structs for different record methods, different
> PyTypeObject but with the same .tp_name "gdb.RecordInstruction" and
> different .tp_getset.  I hacked GDB code, and find python doesn't
> complain that I call PyType_Ready with different PyTypeObject, but they
> have the same .tp_name.  Everything works fine.
> 
[...]
> 
> Python code still uses insn[0] as the base type RecordInstruction, and
> Python code doesn't need to know the sub-types of RecordInstruction C
> code creates.  We deliberately don't document these sub-types in GDB if
> we add them.  IMO, not every python types we added in C are public types.

If that's how GDB does it in other cases, too, I'm fine.  I am not familiar with
GDB's Python bindings or Python bindings in general.

Tim, are you OK with the result of this discussion, as well?

Joel, do we have a week or so for Tim to send a patch?

Thanks,
Markus.

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

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

[Joel Brobecker]

> Tim, are you OK with the result of this discussion, as well?
> 
> Joel, do we have a week or so for Tim to send a patch?

We do (I am going to be away all of next week).  After that,
let's cut the branch no matter what, and backport if necessary.
I'd like to start soon, so as to give time for stabilization
if needed.

-- 
Joel

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

[Wiederhake, Tim]

Hi all,

> Tim, are you OK with the result of this discussion, as well?

I'm back and have read the mails you all wrote about this topic.  Let me
summarize to make sure I understand everything correctly.

The Python interface shall look like this (from Python's perspective):

gdb:
	Record		start_recording([str method], [str format])
	Record		current_recording()
	None		stop_recording()

	Unchanged.

gdb.Record:
	str		method
	str		format
	RecordInstruction	begin
	RecordInstruction	end
	RecordInstruction	replay_position
	RecordInstruction[]	instruction_history
	RecordFunctionSegment[]	function_call_history
	None		goto(RecordInstruction insn)

	gdb.Record loses the "ptid" attribute.  "instruction_history" and 
	"function_call_history" actually returns a "list" or "list-like object"
	as there is no way to enforce the type of elements in this list on a
	language level in Python.  Practically, these list will always contain
	"RecordInstruction" / "RecordFunctionSegment" objects since we control
	their creation.  "*_history" may return "None" if the current recording
	method cannot provide such a list.

gdb.Instruction:
	int		pc
	buffer		data
	str		decode
	int		size
	
	gdb.Instruction is meant to be an abstract base class.  The user will
	never receive a raw gdb.Instruction object and accessing any attribute
	in this class will throw a "NotImplementedError" (from what I
	understand, that's the preferred way to handle this kind of situation
	in Python).

gdb.RecordInstruction (gdb.Instruction):
	int		pc	<inherited from gdb.Instruction>
	buffer		data	<inherited from gdb.Instruction>
	str		decode	<inherited from gdb.Instruction>
	int		size	<inherited from gdb.Instruction>
	int		error
	gdb.Symtab_and_line	sal
	bool		is_speculative
	
	gdb.RecordInstruction is a sub class of gdb.Instruction. It loses
	the "number" attribute.  "error" will be "None" if there is no error.

gdb.<Whatever>Instruction (gdb.Instruction):
	int		pc	<inherited from gdb.Instruction>
	buffer		data	<inherited from gdb.Instruction>
	str		decode	<inherited from gdb.Instruction>
	int		size	<inherited from gdb.Instruction>
	...
	
	Created by other Python interfaces, e.g. a function that dissasembles
	target memory.

gdb.RecordFunctionSegment:
	gdb.Symbol	symbol
	int		level
	gdb.RecordInstruction[]	instructions
	gdb.RecordFunctionSegment	up
	gdb.RecordFunctionSegment	prev
	gdb.RecordFunctionSegment	next
	
	Renamed from "gdb.BtraceFunctionCall" to better fit the general scheme.
	Loses the "number" attribute.  As above, "instructions" actually returns
	a list / list-like object.  "prev_sibling" and "next_sibling" are
	renamed to "prev" and "next" for simplicity (discussed with Markus
	off-list).
	
Correct so far?

Initially I supported the idea of losing the "number" attributes in
"gdb.RecordInstruction" and "gdb.RecordFunctionSegment", but I see a
problem with that now:  If an instruction is executed multiple times (e.g. in a
loop), all user visible attributes for these gdb.RecordInstruction objects are
the same, nevertheless a comparison with "==" does not yield "True" because they
represent, well, two different instances of execution of that instruction.
Keeping the "number" attribute in would show the user, why those objects are not
equal.  Therefore I propose to retain the "number" attribute in
"gdb.RecordInstruction" and for symmetry in "gdb.RecordFunctionSegment" as well.

Markus and I further discussed how we handle gaps or other errors in the trace
from the Python point of view.  We came to the conclusion that it would be
beneficial for the user if we replaced the definition of "gdb.RecordInstruction"
above with the following two:

gdb.RecordInstruction (gdb.Instruction):
	int		pc	<inherited from gdb.Instruction>
	buffer		data	<inherited from gdb.Instruction>
	str		decode	<inherited from gdb.Instruction>
	int		size	<inherited from gdb.Instruction>
	int		number
	gdb.Symtab_and_line	sal
	bool		is_speculative
	
gdb.RecordGap:
	int		number
	str		error_message
	int		error_code

	Does NOT inherit from gdb.Instruction.

gdb.Record.instruction_history would then not "return a list of
RecordInstructions" but instead "return a list of RecordInstructions and
(potentially) RecordGaps".

The user needs to distinguish between instructions and gaps somehow anyway, and
this solution would let them do so quite nicely.  Example code:

 r = gdb.current_recording()
 for insn in r.instruction_history:
	try:
		print insn.pc, insn.sal
	except:
		# It's a gap!
		print insn.error_message

Please let me know if you agree with this so I can get to work as soon as
possible.

Regards,
Tim

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Friday, March 31, 2017 6:03 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: Yao Qi <qiyaoltc@gmail.com>; Wiederhake, Tim
> <tim.wiederhake@intel.com>; gdb-patches@sourceware.org;
> xdje42@gmail.com
> Subject: Re: GDB 8.0 release/branching 2017-03-20 update
> 
> > Tim, are you OK with the result of this discussion, as well?
> >
> > Joel, do we have a week or so for Tim to send a patch?
> 
> We do (I am going to be away all of next week).  After that, let's cut the
> branch no matter what, and backport if necessary.
> I'd like to start soon, so as to give time for stabilization if needed.
> 
> --
> Joel
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

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

[Yao Qi]

"Wiederhake, Tim" <tim.wiederhake@intel.com> writes:

Hi Tim,
I was off yesterday.  Thanks for the summary.

> The Python interface shall look like this (from Python's perspective):
>
> gdb:
> 	Record		start_recording([str method], [str format])
> 	Record		current_recording()
> 	None		stop_recording()
>
> 	Unchanged.
>
> gdb.Record:
> 	str		method
> 	str		format
> 	RecordInstruction	begin
> 	RecordInstruction	end
> 	RecordInstruction	replay_position
> 	RecordInstruction[]	instruction_history
> 	RecordFunctionSegment[]	function_call_history
> 	None		goto(RecordInstruction insn)
>
> 	gdb.Record loses the "ptid" attribute.  "instruction_history" and 
> 	"function_call_history" actually returns a "list" or "list-like object"
> 	as there is no way to enforce the type of elements in this list on a
> 	language level in Python.  Practically, these list will always contain
> 	"RecordInstruction" / "RecordFunctionSegment" objects since we control
> 	their creation.  "*_history" may return "None" if the current recording
> 	method cannot provide such a list.
>

> gdb.Instruction:
> 	int		pc
> 	buffer		data
> 	str		decode
> 	int		size
> 	
> 	gdb.Instruction is meant to be an abstract base class.  The user will
> 	never receive a raw gdb.Instruction object and accessing any attribute
> 	in this class will throw a "NotImplementedError" (from what I
> 	understand, that's the preferred way to handle this kind of situation
> 	in Python).

From the implementation point of view, it can be an abstract base class,
but we don't have to mention it in Python/Document.  Likewise, we don't
apply the restriction that "user will never receive a raw
gdb.Instruction object" on the Python interface.

>
> gdb.RecordInstruction (gdb.Instruction):
> 	int		pc	<inherited from gdb.Instruction>
> 	buffer		data	<inherited from gdb.Instruction>
> 	str		decode	<inherited from gdb.Instruction>
> 	int		size	<inherited from gdb.Instruction>
> 	int		error
> 	gdb.Symtab_and_line	sal
> 	bool		is_speculative
> 	
> 	gdb.RecordInstruction is a sub class of gdb.Instruction. It loses
> 	the "number" attribute.  "error" will be "None" if there is no error.
>

> gdb.<Whatever>Instruction (gdb.Instruction):
> 	int		pc	<inherited from gdb.Instruction>
> 	buffer		data	<inherited from gdb.Instruction>
> 	str		decode	<inherited from gdb.Instruction>
> 	int		size	<inherited from gdb.Instruction>
> 	...
> 	
> 	Created by other Python interfaces, e.g. a function that dissasembles
> 	target memory.

Right, to be clear, we focus on record/btrace instruction.

>
> gdb.RecordFunctionSegment:
> 	gdb.Symbol	symbol
> 	int		level
> 	gdb.RecordInstruction[]	instructions
> 	gdb.RecordFunctionSegment	up
> 	gdb.RecordFunctionSegment	prev
> 	gdb.RecordFunctionSegment	next
> 	
> 	Renamed from "gdb.BtraceFunctionCall" to better fit the general scheme.

"Segment" is less clear than "Call".  Does this object represent
something other than a function call?

> 	Loses the "number" attribute.  As above, "instructions" actually returns
> 	a list / list-like object.  "prev_sibling" and "next_sibling" are
> 	renamed to "prev" and "next" for simplicity (discussed with Markus
> 	off-list).

It is a good renaming to me.

> 	
> Correct so far?
>

Yes, I think so :)

> Initially I supported the idea of losing the "number" attributes in
> "gdb.RecordInstruction" and "gdb.RecordFunctionSegment", but I see a
> problem with that now:  If an instruction is executed multiple times (e.g. in a
> loop), all user visible attributes for these gdb.RecordInstruction objects are
> the same, nevertheless a comparison with "==" does not yield "True" because they
> represent, well, two different instances of execution of that instruction.
> Keeping the "number" attribute in would show the user, why those objects are not
> equal.  Therefore I propose to retain the "number" attribute in
> "gdb.RecordInstruction" and for symmetry in "gdb.RecordFunctionSegment" as well.
>

As far as I can see, it is not a problem to me.  The multiple instances
of the same instruction are different, because the same instruction
executed multiple times.  IOW, gdb.RecordInstruction from different
slots of .instruction_history are different.

> Markus and I further discussed how we handle gaps or other errors in the trace
> from the Python point of view.  We came to the conclusion that it would be
> beneficial for the user if we replaced the definition of "gdb.RecordInstruction"
> above with the following two:
>
> gdb.RecordInstruction (gdb.Instruction):
> 	int		pc	<inherited from gdb.Instruction>
> 	buffer		data	<inherited from gdb.Instruction>
> 	str		decode	<inherited from gdb.Instruction>
> 	int		size	<inherited from gdb.Instruction>
> 	int		number
> 	gdb.Symtab_and_line	sal
> 	bool		is_speculative
> 	
> gdb.RecordGap:
> 	int		number
> 	str		error_message
> 	int		error_code
>
> 	Does NOT inherit from gdb.Instruction.
>
> gdb.Record.instruction_history would then not "return a list of
> RecordInstructions" but instead "return a list of RecordInstructions and
> (potentially) RecordGaps".

Yeah, I can see the motivation of this change, because it is strange to
have a field "error" in gdb.RecordInstruction to indicate this.

>
> The user needs to distinguish between instructions and gaps somehow anyway, and
> this solution would let them do so quite nicely.  Example code:
>
>  r = gdb.current_recording()
>  for insn in r.instruction_history:
> 	try:
> 		print insn.pc, insn.sal
> 	except:
> 		# It's a gap!
> 		print insn.error_message
>

I don't like using exception for control flow.  If I understand "gap"
correctly, it is caused something interrupt the tracing.  I'd like to
change the interface like this,

gdb.InstructionHistory
  a list of gdb.RecordInstruction
  gdb.RecordGap (or gdb.RecordStopReason)

It saves a list of instructions and why the record is stopped or
interrupted.  It can be different reasons, like hardware limitation,
or user preference (user only wants to record/trace instructions
executed in current function foo, so any function calls in foo will
cause a gap in the history.  In this way, gdb.RecordGap don't have to be
an error).

gdb.Record.instruction_history returns a list of gdb.InstructionHistory.

-- 
Yao (齐尧)

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

[Wiederhake, Tim]

Hi Yao,

> -----Original Message-----
> From: Yao Qi [mailto:qiyaoltc@gmail.com]
> Sent: Friday, April 7, 2017 10:11 AM
> To: Wiederhake, Tim <tim.wiederhake@intel.com>
> Cc: Joel Brobecker <brobecker@adacore.com>; Metzger, Markus T
> <markus.t.metzger@intel.com>; gdb-patches@sourceware.org; xdje42@gmail.com
> Subject: Re: GDB 8.0 release/branching 2017-03-20 update
> 
> "Wiederhake, Tim" <tim.wiederhake@intel.com> writes:
> 
> Hi Tim,
> I was off yesterday.  Thanks for the summary.
> 
> > The Python interface shall look like this (from Python's perspective):
> >
> > gdb:
> > 	Record		start_recording([str method], [str format])
> > 	Record		current_recording()
> > 	None		stop_recording()
> >
> > 	Unchanged.
> >
> > gdb.Record:
> > 	str		method
> > 	str		format
> > 	RecordInstruction	begin
> > 	RecordInstruction	end
> > 	RecordInstruction	replay_position
> > 	RecordInstruction[]	instruction_history
> > 	RecordFunctionSegment[]	function_call_history
> > 	None		goto(RecordInstruction insn)
> >
> > 	gdb.Record loses the "ptid" attribute.  "instruction_history" and
> > 	"function_call_history" actually returns a "list" or "list-like
> > 	object"
> > 	as there is no way to enforce the type of elements in this list on a
> > 	language level in Python.  Practically, these list will always
> > 	contain
> > 	"RecordInstruction" / "RecordFunctionSegment" objects since we
> > 	control
> > 	their creation.  "*_history" may return "None" if the current
> > 	recording
> > 	method cannot provide such a list.
> >
> > gdb.Instruction:
> > 	int		pc
> > 	buffer		data
> > 	str		decode
> > 	int		size
> >
> > 	gdb.Instruction is meant to be an abstract base class.  The user
> > 	will
> > 	never receive a raw gdb.Instruction object and accessing any
> > 	attribute
> > 	in this class will throw a "NotImplementedError" (from what I
> > 	understand, that's the preferred way to handle this kind of
> > 	situation
> > 	in Python).
> From the implementation point of view, it can be an abstract base class,
> but we don't have to mention it in Python/Document.  Likewise, we don't
> apply the restriction that "user will never receive a raw
> gdb.Instruction object" on the Python interface.

You are correct, this is not meant to end up in the documentation.  Instead,
this is just the explanation to see if I understood the purpose of
gdb.Instruction correctly.

> >
> > gdb.RecordInstruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	int		error
> > 	gdb.Symtab_and_line	sal
> > 	bool		is_speculative
> >
> > 	gdb.RecordInstruction is a sub class of gdb.Instruction. It loses
> > 	the "number" attribute.  "error" will be "None" if there is no
> > 	error.
> >
> > gdb.<Whatever>Instruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	...
> >
> > 	Created by other Python interfaces, e.g. a function that
> > 	dissasembles
> > 	target memory.
> Right, to be clear, we focus on record/btrace instruction.

Correct.  I mentioned this only to showcase how gdb.Instruction would be used by
other Python interfaces.

> > gdb.RecordFunctionSegment:
> > 	gdb.Symbol	symbol
> > 	int		level
> > 	gdb.RecordInstruction[]	instructions
> > 	gdb.RecordFunctionSegment	up
> > 	gdb.RecordFunctionSegment	prev
> > 	gdb.RecordFunctionSegment	next
> >
> > 	Renamed from "gdb.BtraceFunctionCall" to better fit the general
> scheme.
> 
> "Segment" is less clear than "Call".  Does this object represent
> something other than a function call?

Imagine you have some code like this:

 void
 a ()
 {
   /* some code */
 }
 
 void
 b ()
 {
   /* some code */
   a ();
   /* some code */
 }

Then the trace for a call to function "b" would generate three function call
segments: One for the code in "b" before the call to "a", one for the code in
"a" and one for the code in "b" after the call to "a" returned.

The "next" attribute in the first function segment points to the third function
segment and likewise the "prev" attribute in the third function segment points
to the first function segment.  For the second function segment the "up"
attribute points to the first function segment.

"Segment" is used instead of "Call" throughout the btrace code to avoid the
impression that one function segment contains the whole recorded data for a
function call whereas it only contains a part of it.  I believe we should
continue this terminology in Python.
 
> > 	Loses the "number" attribute.  As above, "instructions" actually
> returns
> > 	a list / list-like object.  "prev_sibling" and "next_sibling" are
> > 	renamed to "prev" and "next" for simplicity (discussed with Markus
> > 	off-list).
> 
> It is a good renaming to me.
> 
> >
> > Correct so far?
> >
> 
> Yes, I think so :)

Glad to hear :)

> > Initially I supported the idea of losing the "number" attributes in
> > "gdb.RecordInstruction" and "gdb.RecordFunctionSegment", but I see a
> > problem with that now:  If an instruction is executed multiple times
> > (e.g. in a
> > loop), all user visible attributes for these gdb.RecordInstruction
> > objects are
> > the same, nevertheless a comparison with "==" does not yield "True"
> > because they
> > represent, well, two different instances of execution of that
> > instruction.
> > Keeping the "number" attribute in would show the user, why those objects
> > are not
> > equal.  Therefore I propose to retain the "number" attribute in
> > "gdb.RecordInstruction" and for symmetry in "gdb.RecordFunctionSegment"
> > as well.
> 
> As far as I can see, it is not a problem to me.  The multiple instances
> of the same instruction are different, because the same instruction
> executed multiple times.  IOW, gdb.RecordInstruction from different
> slots of .instruction_history are different.

Then it's decided, and the "number" attribute may stay.

> > Markus and I further discussed how we handle gaps or other errors in the
> > trace
> > from the Python point of view.  We came to the conclusion that it would
> > be
> > beneficial for the user if we replaced the definition of
> > "gdb.RecordInstruction"
> > above with the following two:
> >
> > gdb.RecordInstruction (gdb.Instruction):
> > 	int		pc	<inherited from gdb.Instruction>
> > 	buffer		data	<inherited from gdb.Instruction>
> > 	str		decode	<inherited from gdb.Instruction>
> > 	int		size	<inherited from gdb.Instruction>
> > 	int		number
> > 	gdb.Symtab_and_line	sal
> > 	bool		is_speculative
> >
> > gdb.RecordGap:
> > 	int		number
> > 	str		error_message
> > 	int		error_code
> >
> > 	Does NOT inherit from gdb.Instruction.
> >
> > gdb.Record.instruction_history would then not "return a list of
> > RecordInstructions" but instead "return a list of RecordInstructions and
> > (potentially) RecordGaps".
> 
> Yeah, I can see the motivation of this change, because it is strange to
> have a field "error" in gdb.RecordInstruction to indicate this.

Exactly.  And it goes even further:  We don't have a "pc" or "size" for a gap,
so what do we return in this case?  That's why the gdb.RecordGap type does not
contain these fields that don't make sense in that case.  A gap is not an
instruction and we believe it makes things clearer if "gap" therefore does not
pretend to be an instruction.

> > The user needs to distinguish between instructions and gaps somehow
> > anyway, and
> > this solution would let them do so quite nicely.  Example code:
> >
> >  r = gdb.current_recording()
> >  for insn in r.instruction_history:
> > 	try:
> > 		print insn.pc, insn.sal
> > 	except:
> > 		# It's a gap!
> > 		print insn.error_message
> >
> I don't like using exception for control flow.

The above code with the "error" attribute still in place would look like this:

 r = gdb.current_recording()
 for insn in r.instruction_history:
   if not insn.error:
     print insn.pc, insn.sal
   else:
     print insn.error_message

The user has to take care about the rare possibility of a gap occurring in the
trace.  In the latter case, they have to explicitly check that the instruction
they look at is not a gap, which is easy to forget and will lead to strange
results as the "pc", "sal", "data", "size" and so on attributes of a gap do not
contain meaningful data.

In the former example, the exception is raised by accessing "insn.pc", an
attribute that does not exist for a gap.  I expect the usual code for the
"except" path to just contain a "pass" or "continue" statement.  I just printed
the error_message in the example to showcase how this situation would be handled
if a user desired so.

If you wanted to write this example without handling exceptions, you could do
so as well:

 r = gdb.current_recording()
 for insn in r.instruction_history:
	print getattr(insn, "pc", "has no address"), getattr(insn, "sal", "has no sal")

And even if the user simply wrote:

 r = gdb.current_recording()
 for insn in r.instruction_history:
	print insn.pc, insn.sal

there are two cases: The trace contains no gaps (common), or the trace contains
gaps (uncommon).  If the trace did not contain any gaps, the code works as
expected.  If the trace contained gaps, Python will raise an exception pointing
in the right direction of the problem.

If instead we continued to offer the "error" attribute and the user forgot to
check, the code would silently fail and produce wrong results.  I believe the
option where we fail loud and clear is favorable over the silently wrong result.

> If I understand "gap"
> correctly, it is caused something interrupt the tracing.

There might be other causes as well, such as overflows, decoding errors, etc.
Again, ideally the trace contains no gaps at all, but rarely it may contain one
or more gaps interleaved with the regular instructions.

>  I'd like to
> change the interface like this,
> 
> gdb.InstructionHistory
>   a list of gdb.RecordInstruction
>   gdb.RecordGap (or gdb.RecordStopReason)
> 
> It saves a list of instructions and why the record is stopped or
> interrupted.  It can be different reasons, like hardware limitation,
> or user preference (user only wants to record/trace instructions
> executed in current function foo, so any function calls in foo will
> cause a gap in the history.  In this way, gdb.RecordGap don't have to be
> an error).
> 
> gdb.Record.instruction_history returns a list of gdb.InstructionHistory.

Just to make sure: You propose that gdb.Record.instruction_history returns a
list of partial lists, and each partial list contains the reason why that
particular partial list was interrupted / stopped, correct?

Generating this list of lists would be quite expensive as we have to go over the
whole list of instructions (that at least for btrace internally /is/ a list of
instructions interleaved with gap objects), count the gaps and save their
position to know the length of this list of lists and the start and end of the
individual sub lists.  With the explanation above how to do this without having
to care about exceptions, would you agree on keeping the list one-dimensional,
with instructions and rarely gaps interleaved as-is?

Regards,
Tim
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

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

[Yao Qi]

"Wiederhake, Tim" <tim.wiederhake@intel.com> writes:

> Imagine you have some code like this:
>
>  void
>  a ()
>  {
>    /* some code */
>  }
>  
>  void
>  b ()
>  {
>    /* some code */
>    a ();
>    /* some code */
>  }
>
> Then the trace for a call to function "b" would generate three function call
> segments: One for the code in "b" before the call to "a", one for the code in
> "a" and one for the code in "b" after the call to "a" returned.
>
> The "next" attribute in the first function segment points to the third function
> segment and likewise the "prev" attribute in the third function segment points
> to the first function segment.  For the second function segment the "up"
> attribute points to the first function segment.
>
> "Segment" is used instead of "Call" throughout the btrace code to avoid the
> impression that one function segment contains the whole recorded data for a
> function call whereas it only contains a part of it.  I believe we should
> continue this terminology in Python.
>  

OK, "Segment" is fine with me.

>> > The user needs to distinguish between instructions and gaps somehow
>> > anyway, and
>> > this solution would let them do so quite nicely.  Example code:
>> >
>> >  r = gdb.current_recording()
>> >  for insn in r.instruction_history:
>> > 	try:
>> > 		print insn.pc, insn.sal
>> > 	except:
>> > 		# It's a gap!
>> > 		print insn.error_message
>> >
>> I don't like using exception for control flow.
>
> The above code with the "error" attribute still in place would look like this:
>
>  r = gdb.current_recording()
>  for insn in r.instruction_history:
>    if not insn.error:
>      print insn.pc, insn.sal
>    else:
>      print insn.error_message
>
> The user has to take care about the rare possibility of a gap occurring in the
> trace.  In the latter case, they have to explicitly check that the instruction
> they look at is not a gap, which is easy to forget and will lead to strange
> results as the "pc", "sal", "data", "size" and so on attributes of a gap do not
> contain meaningful data.
>
> In the former example, the exception is raised by accessing "insn.pc", an
> attribute that does not exist for a gap.  I expect the usual code for the
> "except" path to just contain a "pass" or "continue" statement.  I just printed
> the error_message in the example to showcase how this situation would be handled
> if a user desired so.
>
> If you wanted to write this example without handling exceptions, you could do
> so as well:
>
>  r = gdb.current_recording()
>  for insn in r.instruction_history:
> 	print getattr(insn, "pc", "has no address"), getattr(insn, "sal", "has no sal")
>
> And even if the user simply wrote:
>
>  r = gdb.current_recording()
>  for insn in r.instruction_history:
> 	print insn.pc, insn.sal
>
> there are two cases: The trace contains no gaps (common), or the trace contains
> gaps (uncommon).  If the trace did not contain any gaps, the code works as
> expected.  If the trace contained gaps, Python will raise an exception pointing
> in the right direction of the problem.
>
> If instead we continued to offer the "error" attribute and the user forgot to
> check, the code would silently fail and produce wrong results.  I believe the
> option where we fail loud and clear is favorable over the silently wrong result.
>
>> If I understand "gap"
>> correctly, it is caused something interrupt the tracing.
>
> There might be other causes as well, such as overflows, decoding errors, etc.
> Again, ideally the trace contains no gaps at all, but rarely it may contain one
> or more gaps interleaved with the regular instructions.
>

Is it possible that gap is not caused by error?  As I said in previous
mail, user may want to trace a block of instructions, instructions out
of this range are regarded as gap.  My point is that we need to define a
general reason in gap instead of error.

>>  I'd like to
>> change the interface like this,
>> 
>> gdb.InstructionHistory
>>   a list of gdb.RecordInstruction
>>   gdb.RecordGap (or gdb.RecordStopReason)
>> 
>> It saves a list of instructions and why the record is stopped or
>> interrupted.  It can be different reasons, like hardware limitation,
>> or user preference (user only wants to record/trace instructions
>> executed in current function foo, so any function calls in foo will
>> cause a gap in the history.  In this way, gdb.RecordGap don't have to be
>> an error).
>> 
>> gdb.Record.instruction_history returns a list of gdb.InstructionHistory.
>
> Just to make sure: You propose that gdb.Record.instruction_history returns a
> list of partial lists, and each partial list contains the reason why that
> particular partial list was interrupted / stopped, correct?

Yes.

>
> Generating this list of lists would be quite expensive as we have to go over the
> whole list of instructions (that at least for btrace internally /is/ a list of
> instructions interleaved with gap objects), count the gaps and save their
> position to know the length of this list of lists and the start and end of the
> individual sub lists.  With the explanation above how to do this without having
> to care about exceptions, would you agree on keeping the list one-dimensional,
> with instructions and rarely gaps interleaved as-is?

OK, that is fine to me.

-- 
Yao (齐尧)