[RFA] Resubmit, reverse debugging [0/5]

[RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

OK, after incorporating various review suggestions,
this patch supercedes the patch of 10/01/2008.

The following change log entries are not part of the patch,
but are provided to show what has changed since last time,
mostly in response to review suggestions.


2008-10-07  Michael Snyder  <msnyder@vmware.com>

         * target.h (to_set_exec_direction, to_get_exec_direction): Remove.
         (to_can_execute_reverse): New method.
         (enum exec_direction_kind): Move to inferior.h.
         * target.c (update_current_target): Inherit to_can_execute_reverse.
         Remove to_set_exec_direction, to_get_exec_direction.
         * inferior.h (enum exec_direction_kind): Move from target.h.

         * infrun.c (set_exec_direction_func): Move here from reverse.c.
         (show_exec_direction_func): Ditto.
         (proceed): Consult global execution_direction instead of
         target method.
         (handle_inferior_event): Ditto.

         * reverse.c (set_exec_direction_func): Move to infrun.c
         (show_exec_direction_func): Ditto.
         (exec_direction_default): Set infrun global variable.
         (exec_reverse_once): Consult infrun global direction variable.

         * infcmd.c (step_1): Consult infrun global direction variable.
         (step_once): Ditto.
         (until_next_command): Ditto.
         (finish_command): Ditto.

         * record.h (record_exec_direction): Delete.
         (RECORD_IS_REPLAY): Consult infrun global direction variable.

         * record.c: (record_wait_cleanups): Use infrun state variable.
         (record_wait): Ditto.
         (record_get_exec_direction, record_set_exec_direction): Remove.
         (record_can_execute_reverse): New target method.

         * remote.c (remote_resume): Use infrun state variable.
         (remote_get_exec_direction, remote_set_exec_direction): Remove.
         (remote_can_execute_reverse): New target method.

2008-10-07  Michael Snyder  <msnyder@vmware.com>

         * infrun.c (stepped_into_function): Rename 
handle_step_into_function.
         (stepped_into_function_backward):
         Rename handle_step_into_function_backward.

         * reverse.c: Update copyright.
         (exec_reverse_once): Add function header comment.
         (_initialize_reverse): Use i18n macros for set exec-direction.
2008-10-06  Michael Snyder  <msnyder@vmware.com>

         * breakpoint.c (breakpoint_silence): Rename to 
make_breakpoint_silent.
         * breakpoint.h: Ditto.
         * infcmd.c (finish_backward): Call make_breakpoint_silent
         instead of breakpoint_silence.

         * infcmd.c (finish_command): Reject async in reverse.
         * infcmd.c (finish_command): Use i18n macros.
         * infcmd.c: Minor function reordering.
         * infrun.c (step_into_function): Rename to stepped_into_function.
         Split into two versions (normal (forward), and reverse).
         (handle_inferior_event): Call stepped_into_function or
         stepped_into_function_backward, depending on exec_direction.
         * infrun.c (handle_inferior_event): Comment rewording.
         * remote.c (remote_get_exec_direction): Use i18n macros.
         (remote_set_exec_direction): Ditto.
         * infrun.c (handle_inferior_event, step_into_function): Formatting.
         * infrun.c (handle_inferior_event): Formatting, spelling fix.

         * infrun.c (handle_inferior_event): Add special case for
         "next" in reverse.
2008-10-05  Michael Snyder  <msnyder@promb-2s-dhcp59.eng.vmware.com>

         * reverse.c (exec_reverse_once): Call do_cleanups explicitly.
         * infrun.c (handle_inferior_event): Fix typo in comment.

2008-10-04  Michael Snyder  <msnyder@vmware.com>

         * target.c, target.h: Rename execdir to exec_direction.
         * record.c, record.h: Ditto.
         * reverse.c: Ditto.
         * remote.c: Ditto.

         * reverse.c (show_exec_direction_func): Don't error, just inform.

         * reverse.c (reverse-continue): Remove a comma from docs string,
         to avoid confusing output from 'apropos'.

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Has the discussion converged?

Wait until Monday and then commit?


Michael Snyder wrote:
> OK, after incorporating various review suggestions,
> this patch supercedes the patch of 10/01/2008.
> 
> The following change log entries are not part of the patch,
> but are provided to show what has changed since last time,
> mostly in response to review suggestions.
> 
> 
> 2008-10-07  Michael Snyder  <msnyder@vmware.com>
> 
>          * target.h (to_set_exec_direction, to_get_exec_direction): Remove.
>          (to_can_execute_reverse): New method.
>          (enum exec_direction_kind): Move to inferior.h.
>          * target.c (update_current_target): Inherit to_can_execute_reverse.
>          Remove to_set_exec_direction, to_get_exec_direction.
>          * inferior.h (enum exec_direction_kind): Move from target.h.
> 
>          * infrun.c (set_exec_direction_func): Move here from reverse.c.
>          (show_exec_direction_func): Ditto.
>          (proceed): Consult global execution_direction instead of
>          target method.
>          (handle_inferior_event): Ditto.
> 
>          * reverse.c (set_exec_direction_func): Move to infrun.c
>          (show_exec_direction_func): Ditto.
>          (exec_direction_default): Set infrun global variable.
>          (exec_reverse_once): Consult infrun global direction variable.
> 
>          * infcmd.c (step_1): Consult infrun global direction variable.
>          (step_once): Ditto.
>          (until_next_command): Ditto.
>          (finish_command): Ditto.
> 
>          * record.h (record_exec_direction): Delete.
>          (RECORD_IS_REPLAY): Consult infrun global direction variable.
> 
>          * record.c: (record_wait_cleanups): Use infrun state variable.
>          (record_wait): Ditto.
>          (record_get_exec_direction, record_set_exec_direction): Remove.
>          (record_can_execute_reverse): New target method.
> 
>          * remote.c (remote_resume): Use infrun state variable.
>          (remote_get_exec_direction, remote_set_exec_direction): Remove.
>          (remote_can_execute_reverse): New target method.
> 
> 2008-10-07  Michael Snyder  <msnyder@vmware.com>
> 
>          * infrun.c (stepped_into_function): Rename
> handle_step_into_function.
>          (stepped_into_function_backward):
>          Rename handle_step_into_function_backward.
> 
>          * reverse.c: Update copyright.
>          (exec_reverse_once): Add function header comment.
>          (_initialize_reverse): Use i18n macros for set exec-direction.
> 2008-10-06  Michael Snyder  <msnyder@vmware.com>
> 
>          * breakpoint.c (breakpoint_silence): Rename to
> make_breakpoint_silent.
>          * breakpoint.h: Ditto.
>          * infcmd.c (finish_backward): Call make_breakpoint_silent
>          instead of breakpoint_silence.
> 
>          * infcmd.c (finish_command): Reject async in reverse.
>          * infcmd.c (finish_command): Use i18n macros.
>          * infcmd.c: Minor function reordering.
>          * infrun.c (step_into_function): Rename to stepped_into_function.
>          Split into two versions (normal (forward), and reverse).
>          (handle_inferior_event): Call stepped_into_function or
>          stepped_into_function_backward, depending on exec_direction.
>          * infrun.c (handle_inferior_event): Comment rewording.
>          * remote.c (remote_get_exec_direction): Use i18n macros.
>          (remote_set_exec_direction): Ditto.
>          * infrun.c (handle_inferior_event, step_into_function): Formatting.
>          * infrun.c (handle_inferior_event): Formatting, spelling fix.
> 
>          * infrun.c (handle_inferior_event): Add special case for
>          "next" in reverse.
> 2008-10-05  Michael Snyder  <msnyder@promb-2s-dhcp59.eng.vmware.com>
> 
>          * reverse.c (exec_reverse_once): Call do_cleanups explicitly.
>          * infrun.c (handle_inferior_event): Fix typo in comment.
> 
> 2008-10-04  Michael Snyder  <msnyder@vmware.com>
> 
>          * target.c, target.h: Rename execdir to exec_direction.
>          * record.c, record.h: Ditto.
>          * reverse.c: Ditto.
>          * remote.c: Ditto.
> 
>          * reverse.c (show_exec_direction_func): Don't error, just inform.
> 
>          * reverse.c (reverse-continue): Remove a comma from docs string,
>          to avoid confusing output from 'apropos'.
> 
> 
> 

Re: [RFA] Resubmit, reverse debugging [0/5]

[Daniel Jacobowitz]

On Fri, Oct 10, 2008 at 10:40:53AM -0700, Michael Snyder wrote:
> Has the discussion converged?

I can't see that it has, but a discussion like this (especially one
with a lot of "ok, I've changed that") is very hard to follow.

The thing that makes me the most uncomfortable, and I think Pedro too,
is the remote protocol changes.  Most other things are easy to fix
later.  For instance, I think that assigning magic meaning to "E06" is
a design mistake.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Fri, Oct 10, 2008 at 10:40:53AM -0700, Michael Snyder wrote:
>> Has the discussion converged?
> 
> I can't see that it has, but a discussion like this (especially one
> with a lot of "ok, I've changed that") is very hard to follow.
> 
> The thing that makes me the most uncomfortable, and I think Pedro too,
> is the remote protocol changes.  Most other things are easy to fix
> later.  For instance, I think that assigning magic meaning to "E06" is
> a design mistake.

OK, I agree -- but it's in use, in the field.

Wouldn't the practice be to deprecate it, and announce
the intention of removing it at some future time, rather
than just suddenly take it away?

I realize that it's never been in the main branch, but
the folks who have it in the field (eg. Virtutech) are
not to blame for that.  They tried to submit their
changes over two years ago.

And if we're going to create a better mechanism,
and deprecate this one, could that not be done in
a later patch, rather than holding back this one
(which has been waiting now for over 2 years)?

Can we not expect to maintain and improve this
functionality better once it's actually in?

Re: [RFA] Resubmit, reverse debugging [0/5]

[Daniel Jacobowitz]

On Fri, Oct 10, 2008 at 11:35:17AM -0700, Michael Snyder wrote:
> OK, I agree -- but it's in use, in the field.
>
> Wouldn't the practice be to deprecate it, and announce
> the intention of removing it at some future time, rather
> than just suddenly take it away?

Sorry, but it's hard to be sympathetic to this.  I go through this on
a regular basis, so do other GDB contributors: it's the risk you take
for deploying things before they're merged.  If you don't get at least
the protocol documentation committed to the master repository, then
this is what happens.

Speaking of documentation, is there a documentation part for the
remote protocol changes?  I haven't seen one in the most recent patch
set, and I don't want any new protocol extensions without docs (for
the obvious reason).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

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

Daniel Jacobowitz wrote:
> On Fri, Oct 10, 2008 at 11:35:17AM -0700, Michael Snyder wrote:
>> OK, I agree -- but it's in use, in the field.
>>
>> Wouldn't the practice be to deprecate it, and announce
>> the intention of removing it at some future time, rather
>> than just suddenly take it away?
> 
> Sorry, but it's hard to be sympathetic to this.  I go through this on
> a regular basis, so do other GDB contributors: it's the risk you take
> for deploying things before they're merged.  If you don't get at least
> the protocol documentation committed to the master repository, then
> this is what happens.

Fine, but we want to encourage outside contribution, don't we?
VirtuTech were the original contributors of this functionality,
(albeit they wouldn't recognize more than a line or two of code
by now, this being one of them).  Can you blame them for not
waiting two years to distribute it?  In this industry, that is
the equivalent of several lifetimes.

Can't we compromise, and be nice to them, to thank them
for the contribution?

> Speaking of documentation, is there a documentation part for the
> remote protocol changes?  I haven't seen one in the most recent patch
> set, and I don't want any new protocol extensions without docs (for
> the obvious reason).

OK, here's a patch that adds
   1) remote protocol documentation
   2) The requested extension to the 'T' reply to replace E06
   3) A big honking "deprecated" warning for E06.

Tested using gdb-freeplay.
Is this good?


[-- Attachment #2: remotelog.txt --]
[-- Type: text/plain, Size: 3524 bytes --]

2008-10-10  Michael Snyder  <msnyder@vmware.com>

	* remote.c (remote_wait): Recognize "replaylog" reply as part 
	of 'T' stop event (NO_HISTORY).
	Declare "end of replay history" error reply as deprecated.

2008-10-10  Michael Snyder  <msnyder@vmware.com>

	* gdb.texinfo: Document the 'bc' and 'bs' (reverse) commands,
	and the 'replaylog' annotation of the 'T' stop event message.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.310.2.4
diff -u -p -r1.310.2.4 remote.c
--- remote.c	8 Oct 2008 00:26:28 -0000	1.310.2.4
+++ remote.c	10 Oct 2008 20:37:48 -0000
@@ -3616,6 +3616,7 @@ remote_wait (ptid_t ptid, struct target_
   ptid_t event_ptid = null_ptid;
   ULONGEST addr;
   int solibs_changed = 0;
+  int replay_event = 0;
 
   status->kind = TARGET_WAITKIND_EXITED;
   status->value.integer = 0;
@@ -3661,6 +3662,7 @@ remote_wait (ptid_t ptid, struct target_
 	  warning (_("Remote failure reply: %s"), buf);
 	  if (buf[1] == '0' && buf[2] == '6')
 	    {
+	      warning (_("Use of 'E6' to indicate NO_HISTORY is deprecated, and will go away in a future GDB release."));
 	      status->kind = TARGET_WAITKIND_NO_HISTORY;
 	    }
 	  else
@@ -3734,6 +3736,16 @@ Packet: '%s'\n"),
 			solibs_changed = 1;
 			p = p_temp;
 		      }
+		    else if (strncmp (p, "replaylog", p1 - 1) == 0)
+		      {
+			/* NO_HISTORY event.
+			   p1 will indicate "begin" or "end", but
+			   it makes no difference for now, so ignore it.  */
+			replay_event = 1;
+			p_temp = strchr (p1 + 1, ';');
+			if (p_temp)
+			  p = p_temp;
+		      }
 		    else
  		      {
  			/* Silently skip unknown optional info.  */
@@ -3779,6 +3791,8 @@ Packet: '%s'\n"),
 	case 'S':		/* Old style status, just signal only.  */
 	  if (solibs_changed)
 	    status->kind = TARGET_WAITKIND_LOADED;
+	  else if (replay_event)
+	    status->kind = TARGET_WAITKIND_NO_HISTORY;
 	  else
 	    {
 	      status->kind = TARGET_WAITKIND_STOPPED;
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.525.2.3
diff -u -p -r1.525.2.3 gdb.texinfo
--- doc/gdb.texinfo	4 Oct 2008 19:11:51 -0000	1.525.2.3
+++ doc/gdb.texinfo	10 Oct 2008 20:37:49 -0000
@@ -24611,6 +24611,20 @@ breakpoint at @var{addr}.
 Don't use this packet.  Use the @samp{Z} and @samp{z} packets instead
 (@pxref{insert breakpoint or watchpoint packet}).
 
+@item bc
+@cindex @samp{bc} packet
+Backward continue.  Execute the target system in reverse.  No parameter.
+
+Reply:
+@xref{Stop Reply Packets}, for the reply specifications.
+
+@item bs
+@cindex @samp{bs} packet
+Backward single step.  Execute one instruction in reverse.  No parameter.
+
+Reply:
+@xref{Stop Reply Packets}, for the reply specifications.
+
 @item c @r{[}@var{addr}@r{]}
 @cindex @samp{c} packet
 Continue.  @var{addr} is address to resume.  If @var{addr} is omitted,
@@ -25228,6 +25242,14 @@ hex.
 The packet indicates that the loaded libraries have changed.
 @value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
 list of loaded libraries.  @var{r} is ignored.
+
+@cindex replay log events, remote reply
+@item replaylog
+The packet indicates that the target cannot continue replaying 
+logged execution events, because it has reached the end (or the
+beginning when executing backward) of the log.  The value of @var{r}
+will be either @samp{begin} or @samp{end}.
+
 @end table
 
 @item W @var{AA}

Re: [RFA] Resubmit, reverse debugging [0/5]

[Eli Zaretskii]

> Date: Fri, 10 Oct 2008 14:58:06 -0700
> From: Michael Snyder <msnyder@vmware.com>
> 
> OK, here's a patch that adds
>    1) remote protocol documentation
>    2) The requested extension to the 'T' reply to replace E06
>    3) A big honking "deprecated" warning for E06.

Thanks.  The documentation part is approved, with 2 comments:

> 2008-10-10  Michael Snyder  <msnyder@vmware.com>
> 
> 	* gdb.texinfo: Document the 'bc' and 'bs' (reverse) commands,
> 	and the 'replaylog' annotation of the 'T' stop event message.

Please mention the names of the nodes where you made the changes.  In
doc/ChangeLog, node names are treated like functions in the code, for
this purpose, i.e. they should appear in parens before the description
of the change itself.

> +@item bc
> +@cindex @samp{bc} packet
> +Backward continue.  Execute the target system in reverse.  No parameter.

Please add here an xref to the place that describes the reverse
execution feature.  Not everybody will remember that by heart when
they read the words "backward continue".

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Eli Zaretskii wrote:
>> Date: Fri, 10 Oct 2008 14:58:06 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> OK, here's a patch that adds
>>    1) remote protocol documentation
>>    2) The requested extension to the 'T' reply to replace E06
>>    3) A big honking "deprecated" warning for E06.
> 
> Thanks.  The documentation part is approved, with 2 comments:
> 
>> 2008-10-10  Michael Snyder  <msnyder@vmware.com>
>>
>>       * gdb.texinfo: Document the 'bc' and 'bs' (reverse) commands,
>>       and the 'replaylog' annotation of the 'T' stop event message.
> 
> Please mention the names of the nodes where you made the changes.  In
> doc/ChangeLog, node names are treated like functions in the code, for
> this purpose, i.e. they should appear in parens before the description
> of the change itself.
> 
>> +@item bc
>> +@cindex @samp{bc} packet
>> +Backward continue.  Execute the target system in reverse.  No parameter.
> 
> Please add here an xref to the place that describes the reverse
> execution feature.  Not everybody will remember that by heart when
> they read the words "backward continue".

Right to both.  Thanks.

RE: [RFA] Resubmit, reverse debugging [0/5]

[Jakob Engblom]

Hi!

This is Virtutech weighing in, since our use and implementation of reversibility
in gbd-serial has come up.  We are perfectly fine with the commands changing a
little, it is as people have noted not a particularly big deal what the precise
gdb-serial commands are, as long as they get decided on and remain fixed.  Which
is exactly why we like the current process of finally seeing reverse execution
become part of the gdb mainline. 

"bc" and "bs" are tried and tested as far as we are concerned, and they appear
to be still in place from what I can see. 

I have another question here: is there a way to check if the remote target does
support reverse execution? Not all targets will, in our experience. So having
some way to detect that property of the target is quite useful. 

Best regards,

/jakob

_______________________________________________________

Jakob Engblom, PhD, Technical Marketing Manager

Virtutech                   Direct: +46 8 690 07 47    
Drottningholmsvägen 14      Mobile: +46 709 242 646   
11243 Stockholm             Web:    www.virtutech.com  
Sweden
________________________________________________________
 

Re: [RFA] Resubmit, reverse debugging [0/5]

[teawater]

On Tue, Oct 14, 2008 at 15:55, Jakob Engblom <jakob@virtutech.com> wrote:
> Hi!
>
> This is Virtutech weighing in, since our use and implementation of reversibility
> in gbd-serial has come up.  We are perfectly fine with the commands changing a
> little, it is as people have noted not a particularly big deal what the precise
> gdb-serial commands are, as long as they get decided on and remain fixed.  Which
> is exactly why we like the current process of finally seeing reverse execution
> become part of the gdb mainline.
>
> "bc" and "bs" are tried and tested as far as we are concerned, and they appear
> to be still in place from what I can see.
>
> I have another question here: is there a way to check if the remote target does
> support reverse execution? Not all targets will, in our experience. So having
> some way to detect that property of the target is quite useful.

I don't think we need it.
Cause if remote stub don't support "bc" and "bs", it can return empty
rsp package to GDB mean that it don't support this command. GDB can
output warning to user that fail.


>
> Best regards,
>
> /jakob
>
> _______________________________________________________
>
> Jakob Engblom, PhD, Technical Marketing Manager
>
> Virtutech                   Direct: +46 8 690 07 47
> Drottningholmsvägen 14      Mobile: +46 709 242 646
> 11243 Stockholm             Web:    www.virtutech.com
> Sweden
> ________________________________________________________
>
>
>
>

Re: [RFA] Resubmit, reverse debugging [0/5]

[Daniel Jacobowitz]

On Tue, Oct 14, 2008 at 05:06:10PM +0800, teawater wrote:
> I don't think we need it.
> Cause if remote stub don't support "bc" and "bs", it can return empty
> rsp package to GDB mean that it don't support this command. GDB can
> output warning to user that fail.

It's useful for other things - for instance, an IDE might display
different buttons if it was supported.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Resubmit, reverse debugging [0/5]

[Daniel Jacobowitz]

On Fri, Oct 10, 2008 at 02:58:06PM -0700, Michael Snyder wrote:
> OK, here's a patch that adds
>   1) remote protocol documentation
>   2) The requested extension to the 'T' reply to replace E06
>   3) A big honking "deprecated" warning for E06.

I'm even fine with the warning being replaced by a comment.  The most
important part, IMO, is having a non-hackish version of the protocol
implemented and documented.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Fri, Oct 10, 2008 at 02:58:06PM -0700, Michael Snyder wrote:
>> OK, here's a patch that adds
>>   1) remote protocol documentation
>>   2) The requested extension to the 'T' reply to replace E06
>>   3) A big honking "deprecated" warning for E06.
> 
> I'm even fine with the warning being replaced by a comment.  The most
> important part, IMO, is having a non-hackish version of the protocol
> implemented and documented.

All right, so that's in.

We also discussed earlier the idea of a capability check,
or like a qSupported message.  Are we going to make that a
requirement before this patch can go in?  Or can it be added
as a subsequent, incremental improvement?

And are there any more requirements before this patch can go in?

Re: [RFA] Resubmit, reverse debugging [0/5]

[Daniel Jacobowitz]

On Wed, Oct 15, 2008 at 11:16:23AM -0700, Michael Snyder wrote:
> We also discussed earlier the idea of a capability check,
> or like a qSupported message.  Are we going to make that a
> requirement before this patch can go in?  Or can it be added
> as a subsequent, incremental improvement?

I don't have an opinion either way.

> And are there any more requirements before this patch can go in?

If Pedro has no further comments - he had several before - then it
should be fine.  I don't know what "this patch" covers, though, so
it's hard to be sure.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Wed, Oct 15, 2008 at 11:16:23AM -0700, Michael Snyder wrote:
>> We also discussed earlier the idea of a capability check,
>> or like a qSupported message.  Are we going to make that a
>> requirement before this patch can go in?  Or can it be added
>> as a subsequent, incremental improvement?
> 
> I don't have an opinion either way.
> 
>> And are there any more requirements before this patch can go in?
> 
> If Pedro has no further comments - he had several before - then it
> should be fine.  I don't know what "this patch" covers, though, so
> it's hard to be sure.

Pedro?  Did I address your concerns, and/or are the remaining
ones addressable incrementally?

Re: [RFA] Resubmit, reverse debugging [0/5]

[Pedro Alves]

On Wednesday 15 October 2008 19:38:46, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> > On Wed, Oct 15, 2008 at 11:16:23AM -0700, Michael Snyder wrote:

> > If Pedro has no further comments - he had several before - then it
> > should be fine.  I don't know what "this patch" covers, though, so
> > it's hard to be sure.
> 
> Pedro?  Did I address your concerns, and/or are the remaining
> ones addressable incrementally?

Sure, certainly.  Thanks.

-- 
Pedro Alves

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 15 October 2008 19:38:46, Michael Snyder wrote:
>> Daniel Jacobowitz wrote:
>>> On Wed, Oct 15, 2008 at 11:16:23AM -0700, Michael Snyder wrote:
> 
>>> If Pedro has no further comments - he had several before - then it
>>> should be fine.  I don't know what "this patch" covers, though, so
>>> it's hard to be sure.
>> Pedro?  Did I address your concerns, and/or are the remaining
>> ones addressable incrementally?
> 
> Sure, certainly.  Thanks.

Great!  So last chance, any further comments before I commit?

Since this is a significant change and the discussion has been long,
I will wait two more days (till Friday pm).

Re: [RFA] Resubmit, reverse debugging [0/5]

[Joel Brobecker]

> OK, I agree -- but it's in use, in the field.

I don't see this as a factor. If people want to use the version of the
protocol that has been deployed, then they can stick with the debugger
provided by the associated vendors. If they want to use the FSF GDB,
then they have to migrate.  We're not suddenly dropping a feature that
we used to support. If we look at Ada as an example, there are several
features that AdaCore contributed which got redesigned as the result of
the review process.  That meant that people using GPS (AdaCore's GUI)
would have some compatibility issues with the FSF GDB for a while, until
we enhanced GDB to recognize the way we implemented the given feature
for the FSF. We have done that a few times, now.

Personally, after having had protocol compatibility issues between
a vendor-supplied gdbserver and our GDB, putting a new packet only to
remove it later is, IMO, asking for the same trouble again.

-- 
Joel

Re: [RFA] Resubmit, reverse debugging [0/5]

[Michael Snyder]

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

Joel Brobecker wrote:
>> OK, I agree -- but it's in use, in the field.
> 
> I don't see this as a factor. If people want to use the version of the
> protocol that has been deployed, then they can stick with the debugger
> provided by the associated vendors. If they want to use the FSF GDB,
> then they have to migrate.  We're not suddenly dropping a feature that
> we used to support. If we look at Ada as an example, there are several
> features that AdaCore contributed which got redesigned as the result of
> the review process.  That meant that people using GPS (AdaCore's GUI)
> would have some compatibility issues with the FSF GDB for a while, until
> we enhanced GDB to recognize the way we implemented the given feature
> for the FSF. We have done that a few times, now.
> 
> Personally, after having had protocol compatibility issues between
> a vendor-supplied gdbserver and our GDB, putting a new packet only to
> remove it later is, IMO, asking for the same trouble again.

OK, thanks guys -- I'll remove it.
Revised patch attached.

Is this perchance the final issue?



[-- Attachment #2: remote.e6.txt --]
[-- Type: text/plain, Size: 3228 bytes --]

2008-10-13  Michael Snyder  <msnyder@vmware.com>

	* remote.c (remote_wait): Remove handling of "E6" no_history error.
	Recognize "replaylog" reply as part of 'T' stop event
	(NO_HISTORY).  Declare "end of replay history" error reply
	as deprecated.

	Remote interface for reverse debugging.
	* remote.c (remote_can_execute_reverse): New target method.
	(remote_resume): Check for reverse exec direction, and send 
	appropriate command to target.
	(remote_wait): Check target response for NO_HISTORY status.
	Also check for empty reply (target doesn't understand "bs" or "bc).
	(remote_vcont_resume): Jump out if attempting reverse execution.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.316
diff -u -p -r1.316 remote.c
--- remote.c	10 Oct 2008 14:46:31 -0000	1.316
+++ remote.c	13 Oct 2008 19:06:21 -0000
@@ -3405,7 +3405,15 @@ remote_resume (ptid_t ptid, int step, en
     set_continue_thread (ptid);
 
   buf = rs->buf;
-  if (siggnal != TARGET_SIGNAL_0)
+  if (execution_direction == EXEC_REVERSE)
+    {
+      /* We don't pass signals to the target in reverse exec mode.  */
+      if (info_verbose && siggnal != TARGET_SIGNAL_0)
+	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
+		 siggnal);
+      strcpy (buf, step ? "bs" : "bc");
+    }
+  else if (siggnal != TARGET_SIGNAL_0)
     {
       buf[0] = step ? 'S' : 'C';
       buf[1] = tohex (((int) siggnal >> 4) & 0xf);
@@ -3634,6 +3642,7 @@ remote_wait_as (ptid_t ptid, struct targ
   ptid_t event_ptid = null_ptid;
   ULONGEST addr;
   int solibs_changed = 0;
+  int replay_event = 0;
   char *buf, *p;
 
   status->kind = TARGET_WAITKIND_IGNORE;
@@ -3749,6 +3758,16 @@ Packet: '%s'\n"),
 		    solibs_changed = 1;
 		    p = p_temp;
 		  }
+		else if (strncmp (p, "replaylog", p1 - p) == 0)
+		  {
+		    /* NO_HISTORY event.
+		       p1 will indicate "begin" or "end", but
+		       it makes no difference for now, so ignore it.  */
+		    replay_event = 1;
+		    p_temp = strchr (p1 + 1, ';');
+		    if (p_temp)
+		      p = p_temp;
+		  }
 		else
 		  {
 		    /* Silently skip unknown optional info.  */
@@ -3794,6 +3813,8 @@ Packet: '%s'\n"),
     case 'S':		/* Old style status, just signal only.  */
       if (solibs_changed)
 	status->kind = TARGET_WAITKIND_LOADED;
+      else if (replay_event)
+            status->kind = TARGET_WAITKIND_NO_HISTORY;
       else
 	{
 	  status->kind = TARGET_WAITKIND_STOPPED;
@@ -7615,6 +7636,14 @@ remote_command (char *args, int from_tty
   help_list (remote_cmdlist, "remote ", -1, gdb_stdout);
 }
 
+static int remote_target_can_reverse = 1;
+
+static int
+remote_can_execute_reverse (void)
+{
+  return remote_target_can_reverse;
+}
+
 static void
 init_remote_ops (void)
 {
@@ -7663,6 +7692,7 @@ Specify the serial device it is connecte
   remote_ops.to_has_registers = 1;
   remote_ops.to_has_execution = 1;
   remote_ops.to_has_thread_control = tc_schedlock;	/* can lock scheduler */
+  remote_ops.to_can_execute_reverse = remote_can_execute_reverse;
   remote_ops.to_magic = OPS_MAGIC;
   remote_ops.to_memory_map = remote_memory_map;
   remote_ops.to_flash_erase = remote_flash_erase;