[RFA] Submit process record and replay fourth time, 0/8

[RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi guys,

After third time submit, there are a lot of change with process record
and replay.  So I submit fourth time to make it clear to review.

For this time, Most of changes were updated follow cvs head and a lot
of format fixes.  Patch for target.c was removed.  Record.c was
updated a lot of parts according to the ideas of Pedro (Much
appreciated).
Thanks for help of everybody in the maillist.

Process record and replay make gdb can record inferior execute log and
replay (include reverse debug).
Now, it support I386-Linux single-thread inferior native debug.

I've divided this patch into eight sections, for ease of review.
They group as:
1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
2) New stratum of strata in target layer (target.h).
3) Process record and replay target (record.c, record.h, Makefile.in).
4) Process record and replay for Linux (linux-record.c,
linux-record.h, Makefile.in, configure.tgt).
5) Event handling (infrun.c).
6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
8) User interface and documentation.

For the NEWS:
* Process record and replay

 In a architecture environment that supports ``process record and
 replay'', ``process record and replay'' target can record a log of
 the process execution, and replay it with both forward and reverse
 execute commands.

These patches be tested with testsuite gdb.twreverse in branch
reverse-20081226-branch.

Attachment is the compressed patches package to make get all patches easy.

Thanks,
Hui

[-- Attachment #2: prec40.tar.bz2 --]
[-- Type: application/x-bzip2, Size: 25119 bytes --]

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

Some update.

On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
> Hi guys,
>
> After third time submit, there are a lot of change with process record
> and replay.  So I submit fourth time to make it clear to review.
>
> For this time, Most of changes were updated follow cvs head and a lot
> of format fixes.  Patch for target.c was removed.  Record.c was
> updated a lot of parts according to the ideas of Pedro (Much
> appreciated).
> Thanks for help of everybody in the maillist.
>
> Process record and replay make gdb can record inferior execute log and
> replay (include reverse debug).
> Now, it support I386-Linux single-thread inferior native debug.
>
> I've divided this patch into eight sections, for ease of review.
> They group as:
> 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
> 2) New stratum of strata in target layer (target.h).
> 3) Process record and replay target (record.c, record.h, Makefile.in).
> 4) Process record and replay for Linux (linux-record.c,
> linux-record.h, Makefile.in, configure.tgt).
> 5) Event handling (infrun.c).
> 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
> 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
> 8) User interface and documentation.
>
> For the NEWS:
> * Process record and replay
>
>  In a architecture environment that supports ``process record and
>  replay'', ``process record and replay'' target can record a log of
>  the process execution, and replay it with both forward and reverse
>  execute commands.
>
> These patches be tested with testsuite gdb.twreverse in branch
> reverse-20081226-branch.
>
> Attachment is the compressed patches package to make get all patches easy.
>
> Thanks,
> Hui
>

[-- Attachment #2: prec41.tar.bz2 --]
[-- Type: application/x-bzip2, Size: 25302 bytes --]

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

Hi guys,

I am not sure your are reviewing the p record patches or done.

Do you think all of them are ok to in?  :)


Thanks,
Hui


On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
> Hi guys,
>
> After third time submit, there are a lot of change with process record
> and replay.  So I submit fourth time to make it clear to review.
>
> For this time, Most of changes were updated follow cvs head and a lot
> of format fixes.  Patch for target.c was removed.  Record.c was
> updated a lot of parts according to the ideas of Pedro (Much
> appreciated).
> Thanks for help of everybody in the maillist.
>
> Process record and replay make gdb can record inferior execute log and
> replay (include reverse debug).
> Now, it support I386-Linux single-thread inferior native debug.
>
> I've divided this patch into eight sections, for ease of review.
> They group as:
> 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
> 2) New stratum of strata in target layer (target.h).
> 3) Process record and replay target (record.c, record.h, Makefile.in).
> 4) Process record and replay for Linux (linux-record.c,
> linux-record.h, Makefile.in, configure.tgt).
> 5) Event handling (infrun.c).
> 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
> 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
> 8) User interface and documentation.
>
> For the NEWS:
> * Process record and replay
>
>  In a architecture environment that supports ``process record and
>  replay'', ``process record and replay'' target can record a log of
>  the process execution, and replay it with both forward and reverse
>  execute commands.
>
> These patches be tested with testsuite gdb.twreverse in branch
> reverse-20081226-branch.
>
> Attachment is the compressed patches package to make get all patches easy.
>
> Thanks,
> Hui
>

RE: [RFA] Submit process record and replay fourth time, 0/8

[Marc Khouzam]

Hi Hui,

as you know, I am a big fan of your RecordAndReplay enhancements.
In fact, I just demoed it at EclipseCon to show how we can do Reverse
Debugging in the CDT.  People were very very interested!

However, I think there are still a few bugs.  These bugs may be hard
to notice when working from the command line, but when working in Eclipse
they can be seen easily.  Those bugs are mostly unexpected behavior such
as jumping too far backwards.  They also happen more easily when using
a program that has a bit of complexity.  For example, the recursive
bug that I found last week.

Do you have access to eclipse?  We can work together to have you try
the reverse debugging that I added to CDT, so that you can work with
your patches more intensely and test them even better.

I would really like to see your patches in GDB 7.0, so maybe using
Eclipse to test it can help move forward.  What do you think?

Marc


> -----Original Message-----
> From: Hui Zhu [mailto:teawater@gmail.com] 
> Sent: Monday, March 30, 2009 1:34 AM
> To: Pedro Alves; Marc Khouzam; Michael Snyder; Thiago Jung 
> Bauermann; Eli Zaretskii; Mark Kettenis
> Cc: gdb-patches@sourceware.org
> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
> 
> Hi guys,
> 
> I am not sure your are reviewing the p record patches or done.
> 
> Do you think all of them are ok to in?  :)
> 
> 
> Thanks,
> Hui
> 
> 
> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
> > Hi guys,
> >
> > After third time submit, there are a lot of change with 
> process record
> > and replay.  So I submit fourth time to make it clear to review.
> >
> > For this time, Most of changes were updated follow cvs head 
> and a lot
> > of format fixes.  Patch for target.c was removed.  Record.c was
> > updated a lot of parts according to the ideas of Pedro (Much
> > appreciated).
> > Thanks for help of everybody in the maillist.
> >
> > Process record and replay make gdb can record inferior 
> execute log and
> > replay (include reverse debug).
> > Now, it support I386-Linux single-thread inferior native debug.
> >
> > I've divided this patch into eight sections, for ease of review.
> > They group as:
> > 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
> > 2) New stratum of strata in target layer (target.h).
> > 3) Process record and replay target (record.c, record.h, 
> Makefile.in).
> > 4) Process record and replay for Linux (linux-record.c,
> > linux-record.h, Makefile.in, configure.tgt).
> > 5) Event handling (infrun.c).
> > 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
> > 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
> > 8) User interface and documentation.
> >
> > For the NEWS:
> > * Process record and replay
> >
> >  In a architecture environment that supports ``process record and
> >  replay'', ``process record and replay'' target can record a log of
> >  the process execution, and replay it with both forward and reverse
> >  execute commands.
> >
> > These patches be tested with testsuite gdb.twreverse in branch
> > reverse-20081226-branch.
> >
> > Attachment is the compressed patches package to make get 
> all patches easy.
> >
> > Thanks,
> > Hui
> >
> 

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

Hi Marc,

I think most of issue that you got is about the reverse debug control
part.  This is not in the p record patches.  The reverse debug include
reverse debug control part already in cvs-head.
For example, the 3 bugs that you sent to maillist are all for reverse
debug control part. But not for p record.

Process record just a target that support reverse debug function.  It
depend on reverse debug function.
Actually, process record target was designed and developed together
with reverse debug (Michael and me).

And I think this is a good reason for let process record in now.
Reverse debug function had already checked in cvs-head a lot of month.
 It will have a lot of customers after 7.0 release.  I think it need a
big and clear test.
Now, remote target support reverse debug in cvs-head.  The gdb stubs
support it are vmware, simics and gdbreplay.
So, just process record can use reverse debug in host directly.  And
there is a testsuite for reverse debug and process record called
gdb.twreverse (This a temp name) in branch reverse-20081226-branch.
It can be very easy to porting to cvs-head.  And we can add more test
(include for MI and eclipse) to increase the testsuit, p record and
reverse debug.

So, I think let process record in can help move forward.  :)

Thanks,
Hui



On Mon, Mar 30, 2009 at 21:50, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
> Hi Hui,
>
> as you know, I am a big fan of your RecordAndReplay enhancements.
> In fact, I just demoed it at EclipseCon to show how we can do Reverse
> Debugging in the CDT.  People were very very interested!
>
> However, I think there are still a few bugs.  These bugs may be hard
> to notice when working from the command line, but when working in Eclipse
> they can be seen easily.  Those bugs are mostly unexpected behavior such
> as jumping too far backwards.  They also happen more easily when using
> a program that has a bit of complexity.  For example, the recursive
> bug that I found last week.
>
> Do you have access to eclipse?  We can work together to have you try
> the reverse debugging that I added to CDT, so that you can work with
> your patches more intensely and test them even better.
>
> I would really like to see your patches in GDB 7.0, so maybe using
> Eclipse to test it can help move forward.  What do you think?
>
> Marc
>
>
>> -----Original Message-----
>> From: Hui Zhu [mailto:teawater@gmail.com]
>> Sent: Monday, March 30, 2009 1:34 AM
>> To: Pedro Alves; Marc Khouzam; Michael Snyder; Thiago Jung
>> Bauermann; Eli Zaretskii; Mark Kettenis
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
>>
>> Hi guys,
>>
>> I am not sure your are reviewing the p record patches or done.
>>
>> Do you think all of them are ok to in?  :)
>>
>>
>> Thanks,
>> Hui
>>
>>
>> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
>> > Hi guys,
>> >
>> > After third time submit, there are a lot of change with
>> process record
>> > and replay.  So I submit fourth time to make it clear to review.
>> >
>> > For this time, Most of changes were updated follow cvs head
>> and a lot
>> > of format fixes.  Patch for target.c was removed.  Record.c was
>> > updated a lot of parts according to the ideas of Pedro (Much
>> > appreciated).
>> > Thanks for help of everybody in the maillist.
>> >
>> > Process record and replay make gdb can record inferior
>> execute log and
>> > replay (include reverse debug).
>> > Now, it support I386-Linux single-thread inferior native debug.
>> >
>> > I've divided this patch into eight sections, for ease of review.
>> > They group as:
>> > 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
>> > 2) New stratum of strata in target layer (target.h).
>> > 3) Process record and replay target (record.c, record.h,
>> Makefile.in).
>> > 4) Process record and replay for Linux (linux-record.c,
>> > linux-record.h, Makefile.in, configure.tgt).
>> > 5) Event handling (infrun.c).
>> > 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
>> > 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
>> > 8) User interface and documentation.
>> >
>> > For the NEWS:
>> > * Process record and replay
>> >
>> >  In a architecture environment that supports ``process record and
>> >  replay'', ``process record and replay'' target can record a log of
>> >  the process execution, and replay it with both forward and reverse
>> >  execute commands.
>> >
>> > These patches be tested with testsuite gdb.twreverse in branch
>> > reverse-20081226-branch.
>> >
>> > Attachment is the compressed patches package to make get
>> all patches easy.
>> >
>> > Thanks,
>> > Hui
>> >
>>
>

RE: [RFA] Submit process record and replay fourth time, 0/8

[Marc Khouzam]

I see.  I've only tried reverse with your patches so I don't know
if the bugs I see also show up when using vmware, simics or gdbreplay...
But if those bugs are already in HEAD, it would be good if we could
get them fixed before 7.0.

If anyone wants to try reverse with Eclipse to do more thourough testing,
I'll help.

Marc


> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Hui Zhu
> Sent: Monday, March 30, 2009 11:17 AM
> To: Marc Khouzam
> Cc: Pedro Alves; Michael Snyder; Thiago Jung Bauermann; Eli 
> Zaretskii; Mark Kettenis; gdb-patches@sourceware.org
> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
> 
> Hi Marc,
> 
> I think most of issue that you got is about the reverse debug control
> part.  This is not in the p record patches.  The reverse debug include
> reverse debug control part already in cvs-head.
> For example, the 3 bugs that you sent to maillist are all for reverse
> debug control part. But not for p record.
> 
> Process record just a target that support reverse debug function.  It
> depend on reverse debug function.
> Actually, process record target was designed and developed together
> with reverse debug (Michael and me).
> 
> And I think this is a good reason for let process record in now.
> Reverse debug function had already checked in cvs-head a lot of month.
>  It will have a lot of customers after 7.0 release.  I think it need a
> big and clear test.
> Now, remote target support reverse debug in cvs-head.  The gdb stubs
> support it are vmware, simics and gdbreplay.
> So, just process record can use reverse debug in host directly.  And
> there is a testsuite for reverse debug and process record called
> gdb.twreverse (This a temp name) in branch reverse-20081226-branch.
> It can be very easy to porting to cvs-head.  And we can add more test
> (include for MI and eclipse) to increase the testsuit, p record and
> reverse debug.
> 
> So, I think let process record in can help move forward.  :)
> 
> Thanks,
> Hui
> 
> 
> 
> On Mon, Mar 30, 2009 at 21:50, Marc Khouzam 
> <marc.khouzam@ericsson.com> wrote:
> > Hi Hui,
> >
> > as you know, I am a big fan of your RecordAndReplay enhancements.
> > In fact, I just demoed it at EclipseCon to show how we can 
> do Reverse
> > Debugging in the CDT.  People were very very interested!
> >
> > However, I think there are still a few bugs.  These bugs may be hard
> > to notice when working from the command line, but when 
> working in Eclipse
> > they can be seen easily.  Those bugs are mostly unexpected 
> behavior such
> > as jumping too far backwards.  They also happen more easily 
> when using
> > a program that has a bit of complexity.  For example, the recursive
> > bug that I found last week.
> >
> > Do you have access to eclipse?  We can work together to have you try
> > the reverse debugging that I added to CDT, so that you can work with
> > your patches more intensely and test them even better.
> >
> > I would really like to see your patches in GDB 7.0, so maybe using
> > Eclipse to test it can help move forward.  What do you think?
> >
> > Marc
> >
> >
> >> -----Original Message-----
> >> From: Hui Zhu [mailto:teawater@gmail.com]
> >> Sent: Monday, March 30, 2009 1:34 AM
> >> To: Pedro Alves; Marc Khouzam; Michael Snyder; Thiago Jung
> >> Bauermann; Eli Zaretskii; Mark Kettenis
> >> Cc: gdb-patches@sourceware.org
> >> Subject: Re: [RFA] Submit process record and replay fourth 
> time, 0/8
> >>
> >> Hi guys,
> >>
> >> I am not sure your are reviewing the p record patches or done.
> >>
> >> Do you think all of them are ok to in?  :)
> >>
> >>
> >> Thanks,
> >> Hui
> >>
> >>
> >> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
> >> > Hi guys,
> >> >
> >> > After third time submit, there are a lot of change with
> >> process record
> >> > and replay.  So I submit fourth time to make it clear to review.
> >> >
> >> > For this time, Most of changes were updated follow cvs head
> >> and a lot
> >> > of format fixes.  Patch for target.c was removed.  Record.c was
> >> > updated a lot of parts according to the ideas of Pedro (Much
> >> > appreciated).
> >> > Thanks for help of everybody in the maillist.
> >> >
> >> > Process record and replay make gdb can record inferior
> >> execute log and
> >> > replay (include reverse debug).
> >> > Now, it support I386-Linux single-thread inferior native debug.
> >> >
> >> > I've divided this patch into eight sections, for ease of review.
> >> > They group as:
> >> > 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
> >> > 2) New stratum of strata in target layer (target.h).
> >> > 3) Process record and replay target (record.c, record.h,
> >> Makefile.in).
> >> > 4) Process record and replay for Linux (linux-record.c,
> >> > linux-record.h, Makefile.in, configure.tgt).
> >> > 5) Event handling (infrun.c).
> >> > 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
> >> > 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
> >> > 8) User interface and documentation.
> >> >
> >> > For the NEWS:
> >> > * Process record and replay
> >> >
> >> >  In a architecture environment that supports ``process record and
> >> >  replay'', ``process record and replay'' target can 
> record a log of
> >> >  the process execution, and replay it with both forward 
> and reverse
> >> >  execute commands.
> >> >
> >> > These patches be tested with testsuite gdb.twreverse in branch
> >> > reverse-20081226-branch.
> >> >
> >> > Attachment is the compressed patches package to make get
> >> all patches easy.
> >> >
> >> > Thanks,
> >> > Hui
> >> >
> >>
> >
> 

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

Hi Marc,

Could you send me the bugs that you got?  If it just can reproduce in
Eclipse, I will try it.

About these bugs, I think we need deal with them one by one and step
by step like before.  :)

Thanks,
Hui

On Mon, Mar 30, 2009 at 23:28, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
> I see.  I've only tried reverse with your patches so I don't know
> if the bugs I see also show up when using vmware, simics or gdbreplay...
> But if those bugs are already in HEAD, it would be good if we could
> get them fixed before 7.0.
>
> If anyone wants to try reverse with Eclipse to do more thourough testing,
> I'll help.
>
> Marc
>
>
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org
>> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Hui Zhu
>> Sent: Monday, March 30, 2009 11:17 AM
>> To: Marc Khouzam
>> Cc: Pedro Alves; Michael Snyder; Thiago Jung Bauermann; Eli
>> Zaretskii; Mark Kettenis; gdb-patches@sourceware.org
>> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
>>
>> Hi Marc,
>>
>> I think most of issue that you got is about the reverse debug control
>> part.  This is not in the p record patches.  The reverse debug include
>> reverse debug control part already in cvs-head.
>> For example, the 3 bugs that you sent to maillist are all for reverse
>> debug control part. But not for p record.
>>
>> Process record just a target that support reverse debug function.  It
>> depend on reverse debug function.
>> Actually, process record target was designed and developed together
>> with reverse debug (Michael and me).
>>
>> And I think this is a good reason for let process record in now.
>> Reverse debug function had already checked in cvs-head a lot of month.
>>  It will have a lot of customers after 7.0 release.  I think it need a
>> big and clear test.
>> Now, remote target support reverse debug in cvs-head.  The gdb stubs
>> support it are vmware, simics and gdbreplay.
>> So, just process record can use reverse debug in host directly.  And
>> there is a testsuite for reverse debug and process record called
>> gdb.twreverse (This a temp name) in branch reverse-20081226-branch.
>> It can be very easy to porting to cvs-head.  And we can add more test
>> (include for MI and eclipse) to increase the testsuit, p record and
>> reverse debug.
>>
>> So, I think let process record in can help move forward.  :)
>>
>> Thanks,
>> Hui
>>
>>
>>
>> On Mon, Mar 30, 2009 at 21:50, Marc Khouzam
>> <marc.khouzam@ericsson.com> wrote:
>> > Hi Hui,
>> >
>> > as you know, I am a big fan of your RecordAndReplay enhancements.
>> > In fact, I just demoed it at EclipseCon to show how we can
>> do Reverse
>> > Debugging in the CDT.  People were very very interested!
>> >
>> > However, I think there are still a few bugs.  These bugs may be hard
>> > to notice when working from the command line, but when
>> working in Eclipse
>> > they can be seen easily.  Those bugs are mostly unexpected
>> behavior such
>> > as jumping too far backwards.  They also happen more easily
>> when using
>> > a program that has a bit of complexity.  For example, the recursive
>> > bug that I found last week.
>> >
>> > Do you have access to eclipse?  We can work together to have you try
>> > the reverse debugging that I added to CDT, so that you can work with
>> > your patches more intensely and test them even better.
>> >
>> > I would really like to see your patches in GDB 7.0, so maybe using
>> > Eclipse to test it can help move forward.  What do you think?
>> >
>> > Marc
>> >
>> >
>> >> -----Original Message-----
>> >> From: Hui Zhu [mailto:teawater@gmail.com]
>> >> Sent: Monday, March 30, 2009 1:34 AM
>> >> To: Pedro Alves; Marc Khouzam; Michael Snyder; Thiago Jung
>> >> Bauermann; Eli Zaretskii; Mark Kettenis
>> >> Cc: gdb-patches@sourceware.org
>> >> Subject: Re: [RFA] Submit process record and replay fourth
>> time, 0/8
>> >>
>> >> Hi guys,
>> >>
>> >> I am not sure your are reviewing the p record patches or done.
>> >>
>> >> Do you think all of them are ok to in?  :)
>> >>
>> >>
>> >> Thanks,
>> >> Hui
>> >>
>> >>
>> >> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
>> >> > Hi guys,
>> >> >
>> >> > After third time submit, there are a lot of change with
>> >> process record
>> >> > and replay.  So I submit fourth time to make it clear to review.
>> >> >
>> >> > For this time, Most of changes were updated follow cvs head
>> >> and a lot
>> >> > of format fixes.  Patch for target.c was removed.  Record.c was
>> >> > updated a lot of parts according to the ideas of Pedro (Much
>> >> > appreciated).
>> >> > Thanks for help of everybody in the maillist.
>> >> >
>> >> > Process record and replay make gdb can record inferior
>> >> execute log and
>> >> > replay (include reverse debug).
>> >> > Now, it support I386-Linux single-thread inferior native debug.
>> >> >
>> >> > I've divided this patch into eight sections, for ease of review.
>> >> > They group as:
>> >> > 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
>> >> > 2) New stratum of strata in target layer (target.h).
>> >> > 3) Process record and replay target (record.c, record.h,
>> >> Makefile.in).
>> >> > 4) Process record and replay for Linux (linux-record.c,
>> >> > linux-record.h, Makefile.in, configure.tgt).
>> >> > 5) Event handling (infrun.c).
>> >> > 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
>> >> > 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
>> >> > 8) User interface and documentation.
>> >> >
>> >> > For the NEWS:
>> >> > * Process record and replay
>> >> >
>> >> >  In a architecture environment that supports ``process record and
>> >> >  replay'', ``process record and replay'' target can
>> record a log of
>> >> >  the process execution, and replay it with both forward
>> and reverse
>> >> >  execute commands.
>> >> >
>> >> > These patches be tested with testsuite gdb.twreverse in branch
>> >> > reverse-20081226-branch.
>> >> >
>> >> > Attachment is the compressed patches package to make get
>> >> all patches easy.
>> >> >
>> >> > Thanks,
>> >> > Hui
>> >> >
>> >>
>> >
>>
>

RE: [RFA] Submit process record and replay fourth time, 0/8

[Marc Khouzam]

> Could you send me the bugs that you got?  If it just can reproduce in
> Eclipse, I will try it.
> 
> About these bugs, I think we need deal with them one by one and step
> by step like before.  :)

I was suggesting using Eclipse, because it takes me time to isolate a bug
and reproduce it in a smaller, command-line example.
But when I have a little time, I will send whatever I find

Marc

> 
> Thanks,
> Hui
> 
> On Mon, Mar 30, 2009 at 23:28, Marc Khouzam 
> <marc.khouzam@ericsson.com> wrote:
> > I see.  I've only tried reverse with your patches so I don't know
> > if the bugs I see also show up when using vmware, simics or 
> gdbreplay...
> > But if those bugs are already in HEAD, it would be good if we could
> > get them fixed before 7.0.
> >
> > If anyone wants to try reverse with Eclipse to do more 
> thourough testing,
> > I'll help.
> >
> > Marc
> >
> >
> >> -----Original Message-----
> >> From: gdb-patches-owner@sourceware.org
> >> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Hui Zhu
> >> Sent: Monday, March 30, 2009 11:17 AM
> >> To: Marc Khouzam
> >> Cc: Pedro Alves; Michael Snyder; Thiago Jung Bauermann; Eli
> >> Zaretskii; Mark Kettenis; gdb-patches@sourceware.org
> >> Subject: Re: [RFA] Submit process record and replay fourth 
> time, 0/8
> >>
> >> Hi Marc,
> >>
> >> I think most of issue that you got is about the reverse 
> debug control
> >> part.  This is not in the p record patches.  The reverse 
> debug include
> >> reverse debug control part already in cvs-head.
> >> For example, the 3 bugs that you sent to maillist are all 
> for reverse
> >> debug control part. But not for p record.
> >>
> >> Process record just a target that support reverse debug 
> function.  It
> >> depend on reverse debug function.
> >> Actually, process record target was designed and developed together
> >> with reverse debug (Michael and me).
> >>
> >> And I think this is a good reason for let process record in now.
> >> Reverse debug function had already checked in cvs-head a 
> lot of month.
> >>  It will have a lot of customers after 7.0 release.  I 
> think it need a
> >> big and clear test.
> >> Now, remote target support reverse debug in cvs-head.  The 
> gdb stubs
> >> support it are vmware, simics and gdbreplay.
> >> So, just process record can use reverse debug in host 
> directly.  And
> >> there is a testsuite for reverse debug and process record called
> >> gdb.twreverse (This a temp name) in branch reverse-20081226-branch.
> >> It can be very easy to porting to cvs-head.  And we can 
> add more test
> >> (include for MI and eclipse) to increase the testsuit, p record and
> >> reverse debug.
> >>
> >> So, I think let process record in can help move forward.  :)
> >>
> >> Thanks,
> >> Hui
> >>
> >>
> >>
> >> On Mon, Mar 30, 2009 at 21:50, Marc Khouzam
> >> <marc.khouzam@ericsson.com> wrote:
> >> > Hi Hui,
> >> >
> >> > as you know, I am a big fan of your RecordAndReplay enhancements.
> >> > In fact, I just demoed it at EclipseCon to show how we can
> >> do Reverse
> >> > Debugging in the CDT.  People were very very interested!
> >> >
> >> > However, I think there are still a few bugs.  These bugs 
> may be hard
> >> > to notice when working from the command line, but when
> >> working in Eclipse
> >> > they can be seen easily.  Those bugs are mostly unexpected
> >> behavior such
> >> > as jumping too far backwards.  They also happen more easily
> >> when using
> >> > a program that has a bit of complexity.  For example, 
> the recursive
> >> > bug that I found last week.
> >> >
> >> > Do you have access to eclipse?  We can work together to 
> have you try
> >> > the reverse debugging that I added to CDT, so that you 
> can work with
> >> > your patches more intensely and test them even better.
> >> >
> >> > I would really like to see your patches in GDB 7.0, so 
> maybe using
> >> > Eclipse to test it can help move forward.  What do you think?
> >> >
> >> > Marc
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Hui Zhu [mailto:teawater@gmail.com]
> >> >> Sent: Monday, March 30, 2009 1:34 AM
> >> >> To: Pedro Alves; Marc Khouzam; Michael Snyder; Thiago Jung
> >> >> Bauermann; Eli Zaretskii; Mark Kettenis
> >> >> Cc: gdb-patches@sourceware.org
> >> >> Subject: Re: [RFA] Submit process record and replay fourth
> >> time, 0/8
> >> >>
> >> >> Hi guys,
> >> >>
> >> >> I am not sure your are reviewing the p record patches or done.
> >> >>
> >> >> Do you think all of them are ok to in?  :)
> >> >>
> >> >>
> >> >> Thanks,
> >> >> Hui
> >> >>
> >> >>
> >> >> On Sat, Mar 21, 2009 at 23:58, Hui Zhu 
> <teawater@gmail.com> wrote:
> >> >> > Hi guys,
> >> >> >
> >> >> > After third time submit, there are a lot of change with
> >> >> process record
> >> >> > and replay.  So I submit fourth time to make it clear 
> to review.
> >> >> >
> >> >> > For this time, Most of changes were updated follow cvs head
> >> >> and a lot
> >> >> > of format fixes.  Patch for target.c was removed.  
> Record.c was
> >> >> > updated a lot of parts according to the ideas of Pedro (Much
> >> >> > appreciated).
> >> >> > Thanks for help of everybody in the maillist.
> >> >> >
> >> >> > Process record and replay make gdb can record inferior
> >> >> execute log and
> >> >> > replay (include reverse debug).
> >> >> > Now, it support I386-Linux single-thread inferior 
> native debug.
> >> >> >
> >> >> > I've divided this patch into eight sections, for ease 
> of review.
> >> >> > They group as:
> >> >> > 1) Architecture support layer (gdbarch.sh, gdbarch.c, 
> gbarch.h).
> >> >> > 2) New stratum of strata in target layer (target.h).
> >> >> > 3) Process record and replay target (record.c, record.h,
> >> >> Makefile.in).
> >> >> > 4) Process record and replay for Linux (linux-record.c,
> >> >> > linux-record.h, Makefile.in, configure.tgt).
> >> >> > 5) Event handling (infrun.c).
> >> >> > 6) Intel 386 target-dependent stuff (i386-tdep.c, 
> i386-tdep.h).
> >> >> > 7) Target-dependent code for GNU/Linux i386 
> (i386-linux-tdep.c).
> >> >> > 8) User interface and documentation.
> >> >> >
> >> >> > For the NEWS:
> >> >> > * Process record and replay
> >> >> >
> >> >> >  In a architecture environment that supports 
> ``process record and
> >> >> >  replay'', ``process record and replay'' target can
> >> record a log of
> >> >> >  the process execution, and replay it with both forward
> >> and reverse
> >> >> >  execute commands.
> >> >> >
> >> >> > These patches be tested with testsuite gdb.twreverse in branch
> >> >> > reverse-20081226-branch.
> >> >> >
> >> >> > Attachment is the compressed patches package to make get
> >> >> all patches easy.
> >> >> >
> >> >> > Thanks,
> >> >> > Hui
> >> >> >
> >> >>
> >> >
> >>
> >
> 

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

Hi guys,

Could you please help me with it?
Thanks a lot.  :)

Hui

On Mon, Mar 30, 2009 at 13:33, Hui Zhu <teawater@gmail.com> wrote:
> Hi guys,
>
> I am not sure your are reviewing the p record patches or done.
>
> Do you think all of them are ok to in?  :)
>
>
> Thanks,
> Hui
>
>
> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
>> Hi guys,
>>
>> After third time submit, there are a lot of change with process record
>> and replay.  So I submit fourth time to make it clear to review.
>>
>> For this time, Most of changes were updated follow cvs head and a lot
>> of format fixes.  Patch for target.c was removed.  Record.c was
>> updated a lot of parts according to the ideas of Pedro (Much
>> appreciated).
>> Thanks for help of everybody in the maillist.
>>
>> Process record and replay make gdb can record inferior execute log and
>> replay (include reverse debug).
>> Now, it support I386-Linux single-thread inferior native debug.
>>
>> I've divided this patch into eight sections, for ease of review.
>> They group as:
>> 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
>> 2) New stratum of strata in target layer (target.h).
>> 3) Process record and replay target (record.c, record.h, Makefile.in).
>> 4) Process record and replay for Linux (linux-record.c,
>> linux-record.h, Makefile.in, configure.tgt).
>> 5) Event handling (infrun.c).
>> 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
>> 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
>> 8) User interface and documentation.
>>
>> For the NEWS:
>> * Process record and replay
>>
>>  In a architecture environment that supports ``process record and
>>  replay'', ``process record and replay'' target can record a log of
>>  the process execution, and replay it with both forward and reverse
>>  execute commands.
>>
>> These patches be tested with testsuite gdb.twreverse in branch
>> reverse-20081226-branch.
>>
>> Attachment is the compressed patches package to make get all patches easy.
>>
>> Thanks,
>> Hui
>>
>

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

Hi guys,

Could you please help me with it?
Thanks a lot.  :)

Hui

On Wed, Apr 1, 2009 at 12:53, Hui Zhu <teawater@gmail.com> wrote:
> Hi guys,
>
> Could you please help me with it?
> Thanks a lot.  :)
>
> Hui
>
> On Mon, Mar 30, 2009 at 13:33, Hui Zhu <teawater@gmail.com> wrote:
>> Hi guys,
>>
>> I am not sure your are reviewing the p record patches or done.
>>
>> Do you think all of them are ok to in?  :)
>>
>>
>> Thanks,
>> Hui
>>
>>
>> On Sat, Mar 21, 2009 at 23:58, Hui Zhu <teawater@gmail.com> wrote:
>>> Hi guys,
>>>
>>> After third time submit, there are a lot of change with process record
>>> and replay.  So I submit fourth time to make it clear to review.
>>>
>>> For this time, Most of changes were updated follow cvs head and a lot
>>> of format fixes.  Patch for target.c was removed.  Record.c was
>>> updated a lot of parts according to the ideas of Pedro (Much
>>> appreciated).
>>> Thanks for help of everybody in the maillist.
>>>
>>> Process record and replay make gdb can record inferior execute log and
>>> replay (include reverse debug).
>>> Now, it support I386-Linux single-thread inferior native debug.
>>>
>>> I've divided this patch into eight sections, for ease of review.
>>> They group as:
>>> 1) Architecture support layer (gdbarch.sh, gdbarch.c, gbarch.h).
>>> 2) New stratum of strata in target layer (target.h).
>>> 3) Process record and replay target (record.c, record.h, Makefile.in).
>>> 4) Process record and replay for Linux (linux-record.c,
>>> linux-record.h, Makefile.in, configure.tgt).
>>> 5) Event handling (infrun.c).
>>> 6) Intel 386 target-dependent stuff (i386-tdep.c, i386-tdep.h).
>>> 7) Target-dependent code for GNU/Linux i386 (i386-linux-tdep.c).
>>> 8) User interface and documentation.
>>>
>>> For the NEWS:
>>> * Process record and replay
>>>
>>>  In a architecture environment that supports ``process record and
>>>  replay'', ``process record and replay'' target can record a log of
>>>  the process execution, and replay it with both forward and reverse
>>>  execute commands.
>>>
>>> These patches be tested with testsuite gdb.twreverse in branch
>>> reverse-20081226-branch.
>>>
>>> Attachment is the compressed patches package to make get all patches easy.
>>>
>>> Thanks,
>>> Hui
>>>
>>
>

Re: [RFA] Submit process record and replay fourth time, 0/8

[Pedro Alves]

On Wednesday 25 March 2009 07:23:45, Hui Zhu wrote:
> Some update.

Thanks.  I'm not sure I'm looking at the most up-to-date version.
Here's a few review comments.

* 1-gdbarch.txt

  This one's OK.

* 2-target_record_stratum.txt

  This one's OK.

* 3-record_target.txt

> +record_list_release (record_t * rec)

                                  ^

drop the space between '*' and rec, here and elsewhere.

Is record_t supposed to be an opaque type?  Throughout
gdb, we tend to only "_t" struct typedef types if they're
going to be value-like, passed by type, and opaque.
Otherwise, we just use something like:

 record_list_release (struct record_entry *rec)

that is, no typedef.

> +/* Add a record_end type record_t to record_arch_list.  */
> +int
> +record_arch_list_add_end (void)

  Add a blank line between comment and function.


> +	      q = yquery (_("Do you want to auto delete previous execution "
> +			    "log entries when record/replay buffer becomes "
> +			    "full (record-stop-at-limit)?"));


  What do you mean here by "when"?  Didn't the user just hit
the limit *now*, and that is why you're asking what to do?


> +/* Before inferior step (when GDB record the running message, inferior
> +   only can step), GDB will call this function to record the values to
> +   record_list.  This function will call gdbarch_process_record to
> +   record the running message of inferior and set them to
> +   record_arch_list, and add it to record_list.  */
> +
(...)
> +
> +static int
> +record_message (void *args)
> +{
> +  int ret;
> +  struct gdbarch *gdbarch = args;
> +  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
> +
> +  record_arch_list_head = NULL;
> +  record_arch_list_tail = NULL;
> +
> +  /* Check record_insn_num.  */
> +  record_check_insn_num (1);
> +
> +  record_regcache = get_current_regcache ();

  - When I read the first time, my question is: 'What is a "record message"'?

  - ARGS is a `struct gdbarch', yet you access the current regcache.
    I see you pass current_gdbarch to do_record_message.  We're trying
    to eliminate the current_gdbarch global.

>      + if (do_record_message (current_gdbarch))

    You can get at the correct gdbarch with get_regcache_arch.
While we're there, what is the point of the record_regcache global?
We *are* trying to move away from global state, but I don't see how
this new global is any better than accessing get_current_regcache
directly.  The alternative would be to pass the regcache as parameter
instead.

> +static void
> +record_sig_handler (int signo)
> +{
> +  if (record_debug)
> +    fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n");
> +
> +  /* It will break the running inferior in replay mode.  */
> +  record_resume_step = 1;

  I still don't understand this comment.  Please explain *why* you
need to set this here.

> +
> +  /* It will let record_wait set inferior status to get the signal
> +     SIGINT.  */
> +  record_get_sig = 1;
> +}

> +struct cleanup *
> +record_gdb_operation_disable_set (void)
> +{
> +  struct cleanup *old_cleanups = NULL;
> +
> +  if (!record_gdb_operation_disable)
> +    {
> +      old_cleanups =
> +        make_cleanup_restore_integer (&record_gdb_operation_disable);
> +      record_gdb_operation_disable = 1;
> +    }
> +
> +  return old_cleanups;
> +}

This returns a NULL cleanup if record_gdb_operation_disable is
already set, but make_cleanup also returns a legitimate
NULL, and it is not an error.  It returns NULL when for the
first cleanup put in the chain.  See make_my_cleanup2.

You could make use of "make_cleanup (null_cleanup, NULL)", but,
simply:

 struct cleanup *
 record_gdb_operation_disable_set (void)
 {
   struct cleanup *old_cleanups = NULL;

   old_cleanups =
     make_cleanup_restore_integer (&record_gdb_operation_disable);
   record_gdb_operation_disable = 1;

   return old_cleanups;
 }

... should do.  make_cleanup_restore_integer restores the
previous value, so nested calls are safe.

> +static int
> +record_insert_breakpoint (struct bp_target_info *bp_tgt)
> +{
> +  if (!RECORD_IS_REPLAY)
> +    {
> +      struct cleanup *old_cleanups = record_gdb_operation_disable_set ();
> +      int ret = record_beneath_to_insert_breakpoint (bp_tgt);
> +
> +      if (old_cleanups)
> +        do_cleanups (old_cleanups);

So here, this cleanup handling is wrong.  Don't check for NULL
old_cleanups.  Call do_cleanups unconditionally.  Here and
and elsewhere you used this idiom.

> +static void
> +record_wait_cleanups (void *ignore)
> +{
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      if (record_list->next)
> +	record_list = record_list->next;
> +    }
> +  else
> +    record_list = record_list->prev;
> +}

> +static ptid_t
> +record_wait (struct target_ops *ops,
> +              ptid_t ptid, struct target_waitstatus *status)
(...)
> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);

This cleanup looks suspiciously broken to me.  It appears that
is will do weird things depending on when an exception is thrown.
But then again, record_wait's nesting and use of goto's makes
my eyes bleed.  :P


Please delete this comment:

> +  /* XXX: I try to use some simple commands such as "disconnect" and
> +     "detach" to support this functions.  But these commands all have
> +     other affect to GDB such as call function "no_shared_libraries".
> +     So I add special commands to GDB.  */


I was looking at the commands record, delrecord, stoprecord,
record-stop-at-limit, record-insn-number-max, record-insn-number,
and wondering what would you think if we made all record commands
under a "record" prefix?

 record
 record stop
 record delete
 set record stop-at-limit
 set record insn-number-max
 info record insn-number


* 4-linux-record.txt

> +   Copyright (C) 2008 Free Software Foundation, Inc.

     2009.

> +#include <stdint.h>

Remove this.  defs.h includes stdint.h.

+	uint32_t addr, count;
+	regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr);

This (and many more similar instances) is assuming
host-endianness == target-endianess, that the registers are 32-bit, and
probably that signed<->unsigned bit representation is equal.  Is
this what you want?  When you get to 64-bit, most of this will break,
it seems.

* 5-infrun.txt

infun.c:

> + #include "record.h"

> +         && current_target.to_stratum != record_stratum);

Sigh, I've spent to much time trying to explain why this was
wrong.  Let's at least leave this behind the macro you used to
have.

-- 
Pedro Alves

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 7327 bytes --]

Hi Pedro,

The attachment is the compress for all the patches of precord.
And I will post each patch to maillist too.

Please help me review it.


The following is what I updated according to your comment:
>
>> +record_list_release (record_t * rec)
>>                                  ^
>
> drop the space between '*' and rec, here and elsewhere.

It was fixed in 3.

>
> Is record_t supposed to be an opaque type?  Throughout
> gdb, we tend to only "_t" struct typedef types if they're
> going to be value-like, passed by type, and opaque.
> Otherwise, we just use something like:
>
>  record_list_release (struct record_entry *rec)
>

record_t is changed to record_entry in 3.

>
>  Add a blank line between comment and function.
>
I fixed it in all the patches of p record.

>> +  record_regcache = get_current_regcache ();
>

>
>  - ARGS is a `struct gdbarch', yet you access the current regcache.
>    I see you pass current_gdbarch to do_record_message.  We're trying
>    to eliminate the current_gdbarch global.
>
>>      + if (do_record_message (current_gdbarch))
record_regcache was removed.  Now, regcache is the argument of a lot
of functions of precord.


> Please delete this comment:
>
>> +  /* XXX: I try to use some simple commands such as "disconnect" and
>> +     "detach" to support this functions.  But these commands all have
>> +     other affect to GDB such as call function "no_shared_libraries".
>> +     So I add special commands to GDB.  */
It was deleted.

> * 4-linux-record.txt
>
>> +   Copyright (C) 2008 Free Software Foundation, Inc.
>
>     2009.
Done.

>> +#include <stdint.h>
> Remove this.  defs.h includes stdint.h.
Done.

> I was looking at the commands record, delrecord, stoprecord,
> record-stop-at-limit, record-insn-number-max, record-insn-number,
> and wondering what would you think if we made all record commands
> under a "record" prefix?
>
>  record
>  record stop
>  record delete
>  set record stop-at-limit
>  set record insn-number-max
>  info record insn-number
Done.

> * 5-infrun.txt
> infun.c:
>
>> + #include "record.h"
>
>> +         && current_target.to_stratum != record_stratum);
>
> Sigh, I've spent to much time trying to explain why this was
> wrong.  Let's at least leave this behind the macro you used to
> have.
OK.  I make it to a macro.





The following is my explain for your comment:
>
>> +           q = yquery (_("Do you want to auto delete previous execution "
>> +                         "log entries when record/replay buffer becomes "
>> +                         "full (record-stop-at-limit)?"));
>
>
>  What do you mean here by "when"?  Didn't the user just hit
> the limit *now*, and that is why you're asking what to do?
>

Because if user said yes, it will auto delete each time.  It will not
ask user again.  So I say when.

>  - When I read the first time, my question is: 'What is a "record message"'?
GDB record the running message.

>> +static void
>> +record_sig_handler (int signo)
>> +{
>> +  if (record_debug)
>> +    fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n");
>> +
>> +  /* It will break the running inferior in replay mode.  */
>> +  record_resume_step = 1;
>
>  I still don't understand this comment.  Please explain *why* you
> need to set this here.
>
		{
		  /* In EXEC_REVERSE mode, this is the record_end of prev
		     instruction.
		     In EXEC_FORWARD mode, this is the record_end of current
		     instruction.  */
		  /* step */
		  if (record_resume_step)
		    {
		      if (record_debug > 1)
			fprintf_unfiltered (gdb_stdlog,
					    "Process record: step.\n");
		      continue_flag = 0;
		    }

		  /* check breakpoint */
		  tmp_pc = regcache_read_pc (regcache);
		  if (breakpoint_inserted_here_p (tmp_pc))
		    {
		      if (record_debug)
			fprintf_unfiltered (gdb_stdlog,
					    "Process record: break "
					    "at 0x%s.\n",
					    paddr_nz (tmp_pc));
		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
			  && execution_direction == EXEC_FORWARD
			  && !record_resume_step)
			regcache_write_pc (regcache,
					   tmp_pc +
					   gdbarch_decr_pc_after_break
					   (get_regcache_arch (regcache)));
		      continue_flag = 0;
		    }
		}
When p record get a sig, I need it stop like single step.  So set
record_resume_step to 1.

>
>> +struct cleanup *
>> +record_gdb_operation_disable_set (void)
>> +{
>> +  struct cleanup *old_cleanups = NULL;
>> +
>> +  if (!record_gdb_operation_disable)
>> +    {
>> +      old_cleanups =
>> +        make_cleanup_restore_integer (&record_gdb_operation_disable);
>> +      record_gdb_operation_disable = 1;
>> +    }
>> +
>> +  return old_cleanups;
>> +}
>
> This returns a NULL cleanup if record_gdb_operation_disable is
> already set, but make_cleanup also returns a legitimate
> NULL, and it is not an error.  It returns NULL when for the
> first cleanup put in the chain.  See make_my_cleanup2.
>

  if (set_cleanups)
    do_cleanups (set_cleanups);
void
do_cleanups (struct cleanup *old_chain)
{
  do_my_cleanups (&cleanup_chain, old_chain);
}
static void
do_my_cleanups (struct cleanup **pmy_chain,
		struct cleanup *old_chain)
{
  struct cleanup *ptr;
  while ((ptr = *pmy_chain) != old_chain)
    {
      *pmy_chain = ptr->next;	/* Do this first incase recursion */
      (*ptr->function) (ptr->arg);
      if (ptr->free_arg)
	(*ptr->free_arg) (ptr->arg);
      xfree (ptr);
    }
}
NULL will make all the chain clean.


>
>> +static void
>> +record_wait_cleanups (void *ignore)
>> +{
>> +  if (execution_direction == EXEC_REVERSE)
>> +    {
>> +      if (record_list->next)
>> +     record_list = record_list->next;
>> +    }
>> +  else
>> +    record_list = record_list->prev;
>> +}
>
>> +static ptid_t
>> +record_wait (struct target_ops *ops,
>> +              ptid_t ptid, struct target_waitstatus *status)
> (...)
>> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>
> This cleanup looks suspiciously broken to me.  It appears that
> is will do weird things depending on when an exception is thrown.
> But then again, record_wait's nesting and use of goto's makes
> my eyes bleed.  :P

This cleanup will make record_list point to the record entry that
execution before this record entry because set this record entry get
fail.
This part is not very easy to make clear.  I make clear it use a lot
of time.  :P
About goto, most of them are deleted.  I just keep one:
      /* Check breakpoint when forward execute.  */
      if (execution_direction == EXEC_FORWARD)
	{
            xxx
            xxx
	      goto replay_out;
	    }
	}

> +       uint32_t addr, count;
> +       regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr);
>
> This (and many more similar instances) is assuming
> host-endianness == target-endianess, that the registers are 32-bit, and
> probably that signed<->unsigned bit representation is equal.  Is
> this what you want?  When you get to 64-bit, most of this will break,
> it seems.
For now, record just support i386 arch.  Please let us keep it to the future.


Thanks,
Hui

[-- Attachment #2: prec20090416.tar.gz --]
[-- Type: application/x-gzip, Size: 31701 bytes --]

Re: [RFA] Submit process record and replay fourth time, 0/8

[Pedro Alves]

On Wednesday 15 April 2009 17:56:34, Hui Zhu wrote:

> >> +struct cleanup *
> >> +record_gdb_operation_disable_set (void)
> >> +{
> >> +  struct cleanup *old_cleanups = NULL;
> >> +
> >> +  if (!record_gdb_operation_disable)
> >> +    {
> >> +      old_cleanups =
> >> +        make_cleanup_restore_integer (&record_gdb_operation_disable);
> >> +      record_gdb_operation_disable = 1;
> >> +    }
> >> +
> >> +  return old_cleanups;
> >> +}
> >
> > This returns a NULL cleanup if record_gdb_operation_disable is
> > already set, but make_cleanup also returns a legitimate
> > NULL, and it is not an error.  It returns NULL when for the
> > first cleanup put in the chain.  See make_my_cleanup2.
> >
> 
>   if (set_cleanups)
>     do_cleanups (set_cleanups);
> void
> do_cleanups (struct cleanup *old_chain)
> {
>   do_my_cleanups (&cleanup_chain, old_chain);
> }
> static void
> do_my_cleanups (struct cleanup **pmy_chain,
> 		struct cleanup *old_chain)
> {
>   struct cleanup *ptr;
>   while ((ptr = *pmy_chain) != old_chain)
>     {
>       *pmy_chain = ptr->next;	/* Do this first incase recursion */
>       (*ptr->function) (ptr->arg);
>       if (ptr->free_arg)
> 	(*ptr->free_arg) (ptr->arg);
>       xfree (ptr);
>     }
> }
> NULL will make all the chain clean.

Yes, and that's the problem with your code.  You should *not*
treat a NULL cleanup any differently from any other cleanup
value.  If there was no cleanup yet installed in the chain
when you called make_cleanup, make_cleanup will return NULL.
If later, you check on the result of make_cleanup

old_chain = make_cleanup (foo, NULL);
^^^^^^^^^
    ^ -  this will return NULL if there's nothing else
         in the chain.  But, after the call there's a new
         cleanup installed (your `foo' cleanup), that you
         do want to run.

if (old_chain)
  do_cleanups (old_chain);
^^^^^^^^^^^^^
    ^ - so this is wrong, because old_chain may be NULL, and
        you still wanted the do_cleanups call to happen.

> >> +static void
> >> +record_wait_cleanups (void *ignore)
> >> +{
> >> +  if (execution_direction == EXEC_REVERSE)
> >> +    {
> >> +      if (record_list->next)
> >> +     record_list = record_list->next;
> >> +    }
> >> +  else
> >> +    record_list = record_list->prev;
> >> +}
> >
> >> +static ptid_t
> >> +record_wait (struct target_ops *ops,
> >> +              ptid_t ptid, struct target_waitstatus *status)
> > (...)
> >> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
> >
> > This cleanup looks suspiciously broken to me.  It appears that
> > is will do weird things depending on when an exception is thrown.
> > But then again, record_wait's nesting and use of goto's makes
> > my eyes bleed.  :P
> 
> This cleanup will make record_list point to the record entry that
> execution before this record entry because set this record entry get
> fail.
> This part is not very easy to make clear.  I make clear it use a lot
> of time.  :P

It looks broken, e.g., because if the cleanup runs before adding a new
entry to the list, you'd undo the wrong thing.  You needed a conditional
on 'record_list->next' being not-NULL --- if the cleanup is always safe
and correct to run, why did you need it?
 
> +static void
> +record_wait_cleanups (void *ignore)
> +{
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      if (record_list->next)
         ^^^^^^^^^^^^^^^^^^^^^^
> +     record_list = record_list->next;
> +    }
> +  else
> +    record_list = record_list->prev;
> +}

I looks like it would be better to build the record entry, and
only when everything is fine and validated, you'd add it to the
record list.  That way, you wouldn't need to (dangerously) undo
a bad list entry.  But I'm not going to make a fuss about this.
Let's keep it that way for now if you'd like.

> > This (and many more similar instances) is assuming
> > host-endianness == target-endianess, that the registers are 32-bit, and
> > probably that signed<->unsigned bit representation is equal.  Is
> > this what you want?  When you get to 64-bit, most of this will break,
> > it seems.
> For now, record just support i386 arch.  Please let us keep it to the future.

Ok.

-- 
Pedro Alves

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

Hi Pedro,

The attachment is the compress for all the patches of precord.
And I will post changed patch to maillist too.

Please help me review it.

>
> if (old_chain)
>  do_cleanups (old_chain);
> ^^^^^^^^^^^^^
>    ^ - so this is wrong, because old_chain may be NULL, and
>        you still wanted the do_cleanups call to happen.
>

Sorry for I didn't understand your prev mail.
Now I got and fixed it.  Thanks for explain it twice.  :)


Thanks,
Hui

[-- Attachment #2: prec-20090422.tar.bz2 --]
[-- Type: application/x-bzip2, Size: 25466 bytes --]

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

Hi guys,

All of the patches of process record was approved by Pedro and Eli.
The attachment is the compress for all the patches of precord.
I will check it in 2009-04-30.

Thanks,
Hui

[-- Attachment #2: prec-20090428.tar.bz2.tar.gz --]
[-- Type: application/x-gzip, Size: 31780 bytes --]

RE: [RFA] Submit process record and replay fourth time, 0/8

[Marc Khouzam]

> -----Original Message-----
> From: Hui Zhu [mailto:teawater@gmail.com] 
> Sent: Tuesday, April 28, 2009 6:24 AM
> To: Pedro Alves; Mark Kettenis; Marc Khouzam; Michael Snyder; 
> Thiago Jung Bauermann; Eli Zaretskii; paawan1982@yahoo.com
> Cc: gdb-patches@sourceware.org
> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
> 
> Hi guys,
> 
> All of the patches of process record was approved by Pedro and Eli.
> The attachment is the compress for all the patches of precord.
> I will check it in 2009-04-30.
> 
> Thanks,
> Hui

This is great news!

I believe there are still three minor patches that haven't been approved
yet.  Those caused bugs in Reverse execution:

http://sourceware.org/ml/gdb-patches/2009-03/msg00428.html
http://sourceware.org/ml/gdb-patches/2009-03/msg00005.html
http://sourceware.org/ml/gdb-patches/2009-01/msg00444.html

Besides that, thanks Hui for the great work, and thanks to everyone that
reviewed!

There was great interest when we presented Eclipse with Reverse
debugging
(using PRecord) at EclipseCon.  I am very glad GDB 7.0 will have this
great feature!

We're presenting again at the GCC summit and at the Linux Symposium.
Maybe we'll see some of you there.

Thanks again

Marc

Re: [RFA] Submit process record and replay fourth time, 0/8

[Hui Zhu]

On Tue, Apr 28, 2009 at 21:49, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
>> -----Original Message-----
>> From: Hui Zhu [mailto:teawater@gmail.com]
>> Sent: Tuesday, April 28, 2009 6:24 AM
>> To: Pedro Alves; Mark Kettenis; Marc Khouzam; Michael Snyder;
>> Thiago Jung Bauermann; Eli Zaretskii; paawan1982@yahoo.com
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
>>
>> Hi guys,
>>
>> All of the patches of process record was approved by Pedro and Eli.
>> The attachment is the compress for all the patches of precord.
>> I will check it in 2009-04-30.
>>
>> Thanks,
>> Hui
>
> This is great news!
>
> I believe there are still three minor patches that haven't been approved
> yet.  Those caused bugs in Reverse execution:
>
> http://sourceware.org/ml/gdb-patches/2009-03/msg00428.html
> http://sourceware.org/ml/gdb-patches/2009-03/msg00005.html
> http://sourceware.org/ml/gdb-patches/2009-01/msg00444.html

I had told with Michael about them.  I think all of them will be done
before branch day.

>
> Besides that, thanks Hui for the great work, and thanks to everyone that
> reviewed!
>
You are welcome.  :)

> There was great interest when we presented Eclipse with Reverse
> debugging
> (using PRecord) at EclipseCon.  I am very glad GDB 7.0 will have this
> great feature!
>
Thanks to your work at EclipseCon and reverse debug.  :)


Thanks,
Hui