Re: [RFA] Reverse debugging, part 1/3: target interface

Re: [RFA] Reverse debugging, part 1/3: target interface

[Eli Zaretskii]

> Date: Fri, 31 Mar 2006 14:17:20 -0800
> From: Michael Snyder <msnyder@redhat.com>
> 
> OK, folks, ready to submit this for real.

Thanks!

> + /* Forward/reverse execution direction.  
> +    These will only be implemented by a target that supports reverse execution.
> + */
> + #define target_get_execution_direction() \
> +     (current_target.to_get_execdir ? \
> +      (*current_target.to_get_execdir) () : EXEC_ERROR)

Isn't it better if the default will be EXEC_FORWARD, not EXEC_ERROR?

> + /* Reverse execution.  
> +    FIXME: set up as a capability.  */
> + static enum exec_direction_kind remote_execdir = EXEC_FORWARD;
> + 
> + static enum exec_direction_kind remote_get_execdir (void)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("remote execdir is %s\n", 
> + 		     remote_execdir == EXEC_FORWARD ? "forward" :
> + 		     remote_execdir == EXEC_REVERSE ? "reverse" :
> + 		     "unknown");
> +   return remote_execdir;
> + }
> + 
> + static int remote_set_execdir (enum exec_direction_kind dir)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("Set remote execdir: %s\n",
> + 		     dir == EXEC_FORWARD ? "forward" :
> + 		     dir == EXEC_REVERSE ? "reverse" :
> + 		     "bad direction");
> + 
> +   /* FIXME: check target for capability.  */
> +   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
> +     return (remote_execdir = dir);
> +   else
> +     return EXEC_ERROR;
> + }

I don't understand these two methods--they don't seem to do anything
that their names imply.  What am I missing?

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Two week ping...  there having been no objections to this patch,
shall I check it in?

Michael Snyder wrote:
> OK, folks, ready to submit this for real.
> 
> As a preface, there *is* an open-source target that will
> support reverse execution, and will work with this patch.
> A patch to the SID simulator exists in-house, and will
> hopefully be ready for public submission "real soon now".
> 
> Although the gdb patch is not huge, I've decided to split
> it into three parts, both for simplicity of review, and to
> emphasize that the three parts are pretty orthogonal.
> They are:
> 
> 1) Target interface, including a working instance (remote)
> 2) Internals (the "handle_inferior_event" interface), and
> 3) User interface.
> 
> This patch, #1, can be applied independantly of the other two,
> and passes test suites (no new failures).  This patch is also
> fully compatible with the simics target.
> 
> Numbers 2 and 3 depend on #1, but not on each other.
> 
> In particular, if somebody really hates the UI, it can
> be tossed out and completely replaced without affecting
> the other two patches in any way.
> 
> Eli, I've included the docs patch with the user interface.
> 
> 
> ------------------------------------------------------------------------
> 
> 2006-03-31  Michael Snyder  <msnyder@redhat.com>
> 
> 	Target interface for reverse execution.
> 	* target.h (enum target_waitkind): 
> 	Add new wait event, TARGET_WAITKIND_NO_HISTORY.
> 	(enum exec_direction_kind): New enum.
> 	(struct target_ops): New methods to_set_execdir, to_get_execdir.
> 	(target_set_execution_direction): New macro.
> 	(target_get_execution_direction): New macro.
> 	* target.c (update_current_target): Inherit new execdir methods.
> 
> 	* remote.c (remote_get_execdir, remote_set_execdir): New methods.
> 	(_initialize_remote): Add new methods to remote target vector.
> 	(remote_resume): Check for reverse exec direction, and send
> 	appropriate command to target.
> 	(remote_wait): Check target response for NO_HISTORY status.
> 
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.117
> diff -p -r1.117 target.c
> *** target.c	17 Mar 2006 00:30:34 -0000	1.117
> --- target.c	31 Mar 2006 21:46:54 -0000
> *************** update_current_target (void)
> *** 457,462 ****
> --- 457,464 ----
>         INHERIT (to_find_memory_regions, t);
>         INHERIT (to_make_corefile_notes, t);
>         INHERIT (to_get_thread_local_address, t);
> +       INHERIT (to_get_execdir, t);
> +       INHERIT (to_set_execdir, t);
>         INHERIT (to_magic, t);
>       }
>   #undef INHERIT
> Index: target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.h,v
> retrieving revision 1.81
> diff -p -r1.81 target.h
> *** target.h	21 Feb 2006 18:22:26 -0000	1.81
> --- target.h	31 Mar 2006 21:46:55 -0000
> *************** enum target_waitkind
> *** 129,135 ****
>          inferior, rather than being stuck in the remote_async_wait()
>          function. This way the event loop is responsive to other events,
>          like for instance the user typing.  */
> !     TARGET_WAITKIND_IGNORE
>     };
>   
>   struct target_waitstatus
> --- 129,139 ----
>          inferior, rather than being stuck in the remote_async_wait()
>          function. This way the event loop is responsive to other events,
>          like for instance the user typing.  */
> !     TARGET_WAITKIND_IGNORE,
> ! 
> !     /* The target has run out of history information,
> !        and cannot run backward any further.  */
> !     TARGET_WAITKIND_NO_HISTORY
>     };
>   
>   struct target_waitstatus
> *************** struct target_waitstatus
> *** 148,153 ****
> --- 152,165 ----
>       value;
>     };
>   
> + /* Reverse execution.  */
> + enum exec_direction_kind 
> +   {
> +     EXEC_FORWARD,
> +     EXEC_REVERSE,
> +     EXEC_ERROR
> +   };
> + 
>   /* Possible types of events that the inferior handler will have to
>      deal with.  */
>   enum inferior_event_type
> *************** struct target_ops
> *** 423,428 ****
> --- 435,445 ----
>   				gdb_byte *readbuf, const gdb_byte *writebuf,
>   				ULONGEST offset, LONGEST len);
>   
> +     /* Set execution direction (forward/reverse).  */
> +     int (*to_set_execdir) (enum exec_direction_kind);
> +     /* Get execution direction (forward/reverse).  */
> +     enum exec_direction_kind (*to_get_execdir) (void);
> + 
>       int to_magic;
>       /* Need sub-structure for target machine related rather than comm related?
>        */
> *************** extern int target_stopped_data_address_p
> *** 1074,1079 ****
> --- 1091,1108 ----
>   #define target_stopped_data_address_p(CURRENT_TARGET) (1)
>   #endif
>   
> + /* Forward/reverse execution direction.  
> +    These will only be implemented by a target that supports reverse execution.
> + */
> + #define target_get_execution_direction() \
> +     (current_target.to_get_execdir ? \
> +      (*current_target.to_get_execdir) () : EXEC_ERROR)
> + 
> + #define target_set_execution_direction(DIR) \
> +     (current_target.to_set_execdir ? \
> +      (*current_target.to_set_execdir) (DIR) : EXEC_ERROR)
> +     
> + 
>   /* This will only be defined by a target that supports catching vfork events,
>      such as HP-UX.
>   
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.205
> diff -p -r1.205 remote.c
> *** remote.c	30 Mar 2006 16:56:30 -0000	1.205
> --- remote.c	31 Mar 2006 21:46:58 -0000
> *************** remote_resume (ptid_t ptid, int step, en
> *** 2423,2429 ****
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (siggnal != TARGET_SIGNAL_0)
>       {
>         buf[0] = step ? 'S' : 'C';
>         buf[1] = tohex (((int) siggnal >> 4) & 0xf);
> --- 2423,2437 ----
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (target_get_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);
> *************** remote_wait (ptid_t ptid, struct target_
> *** 2697,2702 ****
> --- 2705,2715 ----
>         switch (buf[0])
>   	{
>   	case 'E':		/* Error of some sort.  */
> + 	  if (buf[1] == '0' && buf[2] == '6')
> + 	    {
> + 	      status->kind = TARGET_WAITKIND_NO_HISTORY;
> + 	      goto got_status;
> + 	    }
>   	  warning (_("Remote failure reply: %s"), buf);
>   	  continue;
>   	case 'F':		/* File-I/O request.  */
> *************** remote_get_thread_local_address (ptid_t 
> *** 5229,5234 ****
> --- 5242,5276 ----
>     return 0;
>   }
>   
> + /* Reverse execution.  
> +    FIXME: set up as a capability.  */
> + static enum exec_direction_kind remote_execdir = EXEC_FORWARD;
> + 
> + static enum exec_direction_kind remote_get_execdir (void)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("remote execdir is %s\n", 
> + 		     remote_execdir == EXEC_FORWARD ? "forward" :
> + 		     remote_execdir == EXEC_REVERSE ? "reverse" :
> + 		     "unknown");
> +   return remote_execdir;
> + }
> + 
> + static int remote_set_execdir (enum exec_direction_kind dir)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("Set remote execdir: %s\n",
> + 		     dir == EXEC_FORWARD ? "forward" :
> + 		     dir == EXEC_REVERSE ? "reverse" :
> + 		     "bad direction");
> + 
> +   /* FIXME: check target for capability.  */
> +   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
> +     return (remote_execdir = dir);
> +   else
> +     return EXEC_ERROR;
> + }
> + 
>   static void
>   init_remote_ops (void)
>   {
> *************** Specify the serial device it is connecte
> *** 5276,5281 ****
> --- 5318,5325 ----
>     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_get_execdir = remote_get_execdir;
> +   remote_ops.to_set_execdir = remote_set_execdir;
>     remote_ops.to_magic = OPS_MAGIC;
>   }
>   

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Mon, Apr 17, 2006 at 04:37:43PM -0700, Michael Snyder wrote:
> Two week ping...  there having been no objections to this patch,
> shall I check it in?

For something this major, I think we should make sure someone else at
least looks over it first.  I haven't had time, and I was hoping that
someone else would take the time; but if no one does, I will
definitely get to it soon.

> >As a preface, there *is* an open-source target that will
> >support reverse execution, and will work with this patch.
> >A patch to the SID simulator exists in-house, and will
> >hopefully be ready for public submission "real soon now".

Any news on this, by the way?  It'd be nice to play with it.
I'll have to see about getting it implemented in qemu...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Tue, Apr 18, 2006 at 08:58:36AM -0400, Daniel Jacobowitz wrote:
> On Mon, Apr 17, 2006 at 04:37:43PM -0700, Michael Snyder wrote:
> > Two week ping...  there having been no objections to this patch,
> > shall I check it in?
> 
> For something this major, I think we should make sure someone else at
> least looks over it first.  I haven't had time, and I was hoping that
> someone else would take the time; but if no one does, I will
> definitely get to it soon.

Oh, I see one thing in a first look: you are adding to the remote
protocol without adding documentation for the new packets or error
codes.  Please document them!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Apr 17, 2006 at 04:37:43PM -0700, Michael Snyder wrote:
> 
>>Two week ping...  there having been no objections to this patch,
>>shall I check it in?
> 
> 
> For something this major, I think we should make sure someone else at
> least looks over it first.  I haven't had time, and I was hoping that
> someone else would take the time; but if no one does, I will
> definitely get to it soon.

OK -- well if you have to budget your time, I would suggest
that the parts in infrun and infcmd (part 2/3) could use
review the most.


>>>As a preface, there *is* an open-source target that will
>>>support reverse execution, and will work with this patch.
>>>A patch to the SID simulator exists in-house, and will
>>>hopefully be ready for public submission "real soon now".
> 
> 
> Any news on this, by the way?  It'd be nice to play with it.
> I'll have to see about getting it implemented in qemu...

That'd be great!  The guy who needs to submit the SID
implementation is getting close to finishing the other
project that's keeping him busy.  I hope he'll be able
to submit it soon.  FYI, I've been using it to test gdb,
and it works pretty darn well!

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Tue, Apr 18, 2006 at 08:58:36AM -0400, Daniel Jacobowitz wrote:
> 
>>On Mon, Apr 17, 2006 at 04:37:43PM -0700, Michael Snyder wrote:
>>
>>>Two week ping...  there having been no objections to this patch,
>>>shall I check it in?
>>
>>For something this major, I think we should make sure someone else at
>>least looks over it first.  I haven't had time, and I was hoping that
>>someone else would take the time; but if no one does, I will
>>definitely get to it soon.
> 
> 
> Oh, I see one thing in a first look: you are adding to the remote
> protocol without adding documentation for the new packets or error
> codes.  Please document them!

Good point.  If anyone wants to modify them, now's the time.

How's this, at least for a start?

@@ -22554,6 +22660,16 @@
  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
+Continue execution in reverse (if capable).
+@xref{Reverse Execution, ,Running programs backward}.
+
+@item bs
+@cindex @samp{bs} packet
+Single step in reverse (if capable).
+@xref{Reverse Execution, ,Running programs backward}.
+
  @item c @var{addr}
  @cindex @samp{c} packet
  Continue.  @var{addr} is address to resume.  If @var{addr} is omitted,

Re: [RFA] Reverse debugging, part 1/3: target interface

[Eli Zaretskii]

> Date: Tue, 18 Apr 2006 15:08:22 -0700
> From: Michael Snyder <msnyder@redhat.com>
> CC: GDB Patches <gdb-patches@sources.redhat.com>
> 
> > Oh, I see one thing in a first look: you are adding to the remote
> > protocol without adding documentation for the new packets or error
> > codes.  Please document them!
> 
> Good point.  If anyone wants to modify them, now's the time.
> 
> How's this, at least for a start?

It's okay, but other packets' descriptions say how to reply to them.
Can we include that for these two as well?

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

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

Eli Zaretskii wrote:
>>Date: Tue, 18 Apr 2006 15:08:22 -0700
>>From: Michael Snyder <msnyder@redhat.com>
>>CC: GDB Patches <gdb-patches@sources.redhat.com>
>>
>>>Oh, I see one thing in a first look: you are adding to the remote
>>>protocol without adding documentation for the new packets or error
>>>codes.  Please document them!
>>
>>Good point.  If anyone wants to modify them, now's the time.
>>
>>How's this, at least for a start?
> 
> 
> It's okay, but other packets' descriptions say how to reply to them.
> Can we include that for these two as well?
> 

Good point.  How about this?


[-- Attachment #2: packets --]
[-- Type: text/plain, Size: 953 bytes --]

Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.322
diff -u -r1.322 gdb.texinfo
--- gdb.texinfo	14 Apr 2006 18:31:32 -0000	1.322
+++ gdb.texinfo	19 Apr 2006 18:24:30 -0000
@@ -22554,6 +22660,22 @@
 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
+Continue execution in reverse (if capable).  
+@xref{Reverse Execution, ,Running programs backward}.
+
+Reply:
+@xref{Stop Reply Packets}, for the reply specifications.
+
+@item bs
+@cindex @samp{bs} packet
+Single step in reverse (if capable).  
+@xref{Reverse Execution, ,Running programs backward}.
+
+Reply:
+@xref{Stop Reply Packets}, for the reply specifications.
+
 @item c @var{addr}
 @cindex @samp{c} packet
 Continue.  @var{addr} is address to resume.  If @var{addr} is omitted,

Re: [RFA] Reverse debugging, part 1/3: target interface

[Eli Zaretskii]

> Date: Wed, 19 Apr 2006 11:26:11 -0700
> From: Michael Snyder <msnyder@redhat.com>
> CC: drow@false.org, gdb-patches@sources.redhat.com
> 
> > It's okay, but other packets' descriptions say how to reply to them.
> > Can we include that for these two as well?
> > 
> 
> Good point.  How about this?

It's fine, thanks.

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Wed, Apr 19, 2006 at 11:26:11AM -0700, Michael Snyder wrote:
> +@item bc
> +@cindex @samp{bc} packet
> +Continue execution in reverse (if capable).  
> +@xref{Reverse Execution, ,Running programs backward}.
> +
> +Reply:
> +@xref{Stop Reply Packets}, for the reply specifications.
> +
> +@item bs
> +@cindex @samp{bs} packet
> +Single step in reverse (if capable).  
> +@xref{Reverse Execution, ,Running programs backward}.
> +
> +Reply:
> +@xref{Stop Reply Packets}, for the reply specifications.
> +

You designed the target interface so that it could return EXEC_ERROR,
"I don't support reverse execution".  But here you've got only bc and
bs; you've got no way to probe the remote target for reverse execution
support.

I realize you've got at least one already shipping stub for this; can
we still fix it, or should we assume that there will be at least one
target where the only way to find out is to try?  This seems to be
one of the stray FIXMEs in this code which really ought to be fixed
before we merge it:

+   /* FIXME: check target for capability.  */

I'm guessing you haven't tried this with a remote target that didn't
support it.  It looks like it would fail messily.


Are there other hard cases that should be discussed in the remote
protocol documentation, or handled by the protocol?  The first one that
comes to mind is "what do we do with threads" - is it ever going to be
relevant to do this in a multi-threaded system, and if so, are we going
to reverse all threads at once?


Oh, and refering to Stop Reply Packets is not complete for the reply;
that doesn't cover error codes.  For instance, you have one error code
designated to mean "no more history".  That should be in the
documentation, please.  I've only skimmed the patch; are there others?



-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

About the patch itself...

On Fri, Mar 31, 2006 at 02:17:20PM -0800, Michael Snyder wrote:
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.117
> diff -p -r1.117 target.c
> *** target.c	17 Mar 2006 00:30:34 -0000	1.117
> --- target.c	31 Mar 2006 21:46:54 -0000
> *************** update_current_target (void)
> *** 457,462 ****
> --- 457,464 ----
>         INHERIT (to_find_memory_regions, t);
>         INHERIT (to_make_corefile_notes, t);
>         INHERIT (to_get_thread_local_address, t);
> +       INHERIT (to_get_execdir, t);
> +       INHERIT (to_set_execdir, t);
>         INHERIT (to_magic, t);
>       }
>   #undef INHERIT

Need "set debug target 1" support for the new target ops.  Also, it
might be nice to use the new style rather than using INHERIT (search
the vector at call time; see the "do not inherit" comments); that makes
the debugging output nicer, too.

> +     /* Set execution direction (forward/reverse).  */
> +     int (*to_set_execdir) (enum exec_direction_kind);
> +     /* Get execution direction (forward/reverse).  */
> +     enum exec_direction_kind (*to_get_execdir) (void);
> + 

We've got these execdir methods...

> + /* Forward/reverse execution direction.  
> +    These will only be implemented by a target that supports reverse execution.
> + */

(comment formatting; wrap before the */ please?)

> + #define target_get_execution_direction() \
> +     (current_target.to_get_execdir ? \
> +      (*current_target.to_get_execdir) () : EXEC_ERROR)

... but these _execution_direction methods.  Please choose one or the
other; it makes it simpler to find the uses and implementations at the
same time.

> *************** remote_resume (ptid_t ptid, int step, en
> *** 2423,2429 ****
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (siggnal != TARGET_SIGNAL_0)
>       {
>         buf[0] = step ? 'S' : 'C';
>         buf[1] = tohex (((int) siggnal >> 4) & 0xf);
> --- 2423,2437 ----
>     else
>       set_thread (pid, 0);	/* Run this thread.  */
>   
> !   if (target_get_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);

You're below this bit:

  /* The vCont packet doesn't need to specify threads via Hc.  */
  if (remote_vcont_resume (ptid, step, siggnal))
    return;

So if the remote target supports vCont, you'll go forwards by accident
instead of reverse!  Talk about a traffic accident.

> + static int remote_set_execdir (enum exec_direction_kind dir)
> + {
> +   if (remote_debug && info_verbose)
> +     printf_filtered ("Set remote execdir: %s\n",
> + 		     dir == EXEC_FORWARD ? "forward" :
> + 		     dir == EXEC_REVERSE ? "reverse" :
> + 		     "bad direction");
> + 
> +   /* FIXME: check target for capability.  */

I think that's a pretty big FIXME.

> +   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
> +     return (remote_execdir = dir);

And please don't do that - no wonder Eli was confused by this
function.  If you want an assignment, put it on its own line :-)

> +   remote_ops.to_get_execdir = remote_get_execdir;
> +   remote_ops.to_set_execdir = remote_set_execdir;

What about the other remote ops vectors?  At least one isn't copied
from here.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Wed, Apr 19, 2006 at 11:26:11AM -0700, Michael Snyder wrote:
> 
>>+@item bc
>>+@cindex @samp{bc} packet
>>+Continue execution in reverse (if capable).  
>>+@xref{Reverse Execution, ,Running programs backward}.
>>+
>>+Reply:
>>+@xref{Stop Reply Packets}, for the reply specifications.
>>+
>>+@item bs
>>+@cindex @samp{bs} packet
>>+Single step in reverse (if capable).  
>>+@xref{Reverse Execution, ,Running programs backward}.
>>+
>>+Reply:
>>+@xref{Stop Reply Packets}, for the reply specifications.
>>+
> 
> 
> You designed the target interface so that it could return EXEC_ERROR,
> "I don't support reverse execution".  But here you've got only bc and
> bs; you've got no way to probe the remote target for reverse execution
> support.

Yes, I think I remarked on the need for that in comments.

> I realize you've got at least one already shipping stub for this; can
> we still fix it, or should we assume that there will be at least one
> target where the only way to find out is to try? 

No problem -- it's not actually shipping, or even promised.
This has been a rare, redhat-funded pure research project.   ;-)

Yes, I appreciate how lucky I am.  Thank you, oh employer mine.
[not that we don't hope to make money from it eventually...]

> This seems to be one of the stray FIXMEs in this code which really
 > ought to be fixed before we merge it:
> 
> +   /* FIXME: check target for capability.  */
> 
> I'm guessing you haven't tried this with a remote target that didn't
> support it.  It looks like it would fail messily.

Hmmmmm... ok, your guess is correct.

I propose a deal -- I'll try it, and if it does fail messily,
I will fix it.  If it doesn't, will you let me check it in
with the expectation of future enhancement?  I feel that for
a major new functionality, useability rather than completeness
should be the mark.

It's not like we're not going to add to what's been
implemented so far.

> Are there other hard cases that should be discussed in the remote
> protocol documentation, or handled by the protocol?  The first one that
> comes to mind is "what do we do with threads" - is it ever going to be
> relevant to do this in a multi-threaded system, and if so, are we going
> to reverse all threads at once?

I think in previous discussion, we have recognized that this is a
hard problem, and I have simply deferred addressing it.  I don't
even claim to know if it *can* be addressed.  I'd like to say,
we can now use this feature on the non-trivial subset of non-
threaded programs, and put threaded programs down as a hoped-
for future enhancement.


> Oh, and refering to Stop Reply Packets is not complete for the reply;
> that doesn't cover error codes.  For instance, you have one error code
> designated to mean "no more history".  That should be in the
> documentation, please. 

Well, yes, I thought about that -- but in fact, AFAICT there is
*no* documentation for error codes, and I didn't feel like starting
it.  Yes, I think we need some, but it's probably worth having a
discussion of how we would like to do it.

And, self-interest not withstanding, I don't think there's a good
reason for holding up this patch for lack of something that is
lacking throughout.

 > I've only skimmed the patch; are there others?

None that I'm deliberately withholding.   ;-)
I'm counting on you reviewers to help find any that I've overlooked.

Thanks for your review and feedback.

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> Need "set debug target 1" support for the new target ops. 

Oh yeah.  Thanks, will do.

 > Also, it
> might be nice to use the new style rather than using INHERIT (search
> the vector at call time; see the "do not inherit" comments); that makes
> the debugging output nicer, too.

OK, I think I know what you mean (the ->beneath business) --
but why is this better?


>>+     /* Set execution direction (forward/reverse).  */
>>+     int (*to_set_execdir) (enum exec_direction_kind);
>>+     /* Get execution direction (forward/reverse).  */
>>+     enum exec_direction_kind (*to_get_execdir) (void);
>>+ 
> 
> We've got these execdir methods...
> 
>>+ /* Forward/reverse execution direction.  
>>+    These will only be implemented by a target that supports reverse execution.
>>+ */
> 
> (comment formatting; wrap before the */ please?)
OK

> 
>>+ #define target_get_execution_direction() \
>>+     (current_target.to_get_execdir ? \
>>+      (*current_target.to_get_execdir) () : EXEC_ERROR)
> 
> 
> ... but these _execution_direction methods.  Please choose one or the
> other; it makes it simpler to find the uses and implementations at the
> same time.

... are you referring to the choices of names?

I made the names of the macros more verbose because they're
more visible, and used in a context where any aid to understanding
is desireable.  For the struct members themselves, I used shorter
names because they're *not* all that visible, are used in a context
where it's not all that critical to understand precisely what they
do, and longer names are cumbersome.  For instance, they would make
the declarations in target.h really long.

Do you still think I should change it?

  > You're below this bit:
> 
>   /* The vCont packet doesn't need to specify threads via Hc.  */
>   if (remote_vcont_resume (ptid, step, siggnal))
>     return;
> 
> So if the remote target supports vCont, you'll go forwards by accident
> instead of reverse!  Talk about a traffic accident.

Oops, thanks for the catch.  I'm not that familiar with vCont.

> I think that's a pretty big FIXME.

Acknowledged, discussed in another sub-thread.

>>+   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
>>+     return (remote_execdir = dir);
> 
> 
> And please don't do that - no wonder Eli was confused by this
> function.  If you want an assignment, put it on its own line :-)

Oh, alright...   ;-)


>>+   remote_ops.to_get_execdir = remote_get_execdir;
>>+   remote_ops.to_set_execdir = remote_set_execdir;
> 
> 
> What about the other remote ops vectors?  At least one isn't copied
> from here.

Right, well, I figured get it accepted in one vector, then
worry about extending it into others.  Basically, as of this
patch, it will work with "target remote" only (and I guess
only in non-async mode).

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Thu, Apr 20, 2006 at 12:22:37PM -0700, Michael Snyder wrote:
> >This seems to be one of the stray FIXMEs in this code which really
> > ought to be fixed before we merge it:
> >
> >+   /* FIXME: check target for capability.  */
> >
> >I'm guessing you haven't tried this with a remote target that didn't
> >support it.  It looks like it would fail messily.
> 
> Hmmmmm... ok, your guess is correct.
> 
> I propose a deal -- I'll try it, and if it does fail messily,
> I will fix it.  If it doesn't, will you let me check it in
> with the expectation of future enhancement?  I feel that for
> a major new functionality, useability rather than completeness
> should be the mark.
> 
> It's not like we're not going to add to what's been
> implemented so far.

I'm happy to be flexible about what gets committed, for new features;
but this is a protocol issue.  It'll change what other projects have to
implement to take advantage of this new functionality.  I'm less happy
being flexible about that, having had to handle backwards compatibility
for beta protocol proposals a time or two :-)

Let's see what it does.  Not hard at all; patched, built, and a handy
gdbserver which doesn't support reversing...

/space/fsf/commit/src/gdb/infcmd.c:1392: warning: 'breakpoint' may be
used uninitialized in this function

It's right, too.  The logic in finish_backward is broken.

With that fixed I can try it:

(gdb) tar rem :1234
Remote debugging using :1234
0x00002aaaaaaaba80 in ?? ()
(gdb) rsi
0x00002aaaaaaaba80 in ?? ()
(gdb)
0x00002aaaaaaaba83 in ?? ()

OK, that's two bugs.  One is the vCont bug I mentioned in another
message; it's stepping forward instead of backwards.  The other has
nothing to do with this at all; apparently PTRACE_STEP isn't working
for the first instruction in the program and we're taking two steps to
go one instruction.  OK, let's avoid the vCont bug and try again:

(gdb) set remote verbose-resume-packet 0
(gdb) rsi
Sending packet: $m2aaaaaab7320,1#70...Ack
Packet received: f3
Sending packet: $M2aaaaaab7320,1:cc#50...Ack
Packet received: OK
Sending packet: $Hc0#db...Ack
Packet received: E01
Sending packet: $bs#d5...Ack
Packet received:
warning: Invalid remote reply:
*hang*

It thinks that the attempt to step has succeeded and that the program
is now running.  That's pretty messy failing.

> >Are there other hard cases that should be discussed in the remote
> >protocol documentation, or handled by the protocol?  The first one that
> >comes to mind is "what do we do with threads" - is it ever going to be
> >relevant to do this in a multi-threaded system, and if so, are we going
> >to reverse all threads at once?
> 
> I think in previous discussion, we have recognized that this is a
> hard problem, and I have simply deferred addressing it.  I don't
> even claim to know if it *can* be addressed.  I'd like to say,
> we can now use this feature on the non-trivial subset of non-
> threaded programs, and put threaded programs down as a hoped-
> for future enhancement.

Sure, but, a certain amount of future proofing is still in order. 
Let's at least say that their behavior for threaded programs is not yet
defined?  If we want something more complex, maybe vCont can help
later.

> >Oh, and refering to Stop Reply Packets is not complete for the reply;
> >that doesn't cover error codes.  For instance, you have one error code
> >designated to mean "no more history".  That should be in the
> >documentation, please. 
> 
> Well, yes, I thought about that -- but in fact, AFAICT there is
> *no* documentation for error codes, and I didn't feel like starting
> it.  Yes, I think we need some, but it's probably worth having a
> discussion of how we would like to do it.
> 
> And, self-interest not withstanding, I don't think there's a good
> reason for holding up this patch for lack of something that is
> lacking throughout.

You misunderstand me.  Right now, GDB doesn't generally assign meaning
to the error numbers. They just mean "error" and they're all treated
the same.  This isn't like that.  You've set things up so that the
target is supposed to return "E06" when it runs out of history.  That's
protocol; it should be in the protocol documentation!

Other errors, in general, sure.  They're undocumented.  We'll have to
contend with that at a later date.  Is rc/rs allowed to return other
errors (whatever they may mean)?  If so, the packet description ought
to say "Enn: Another error has occured".

> > I've only skimmed the patch; are there others?
> 
> None that I'm deliberately withholding.   ;-)

Other error numbers, I meant.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Thu, Apr 20, 2006 at 12:41:58PM -0700, Michael Snyder wrote:
> > Also, it
> >might be nice to use the new style rather than using INHERIT (search
> >the vector at call time; see the "do not inherit" comments); that makes
> >the debugging output nicer, too.
> 
> OK, I think I know what you mean (the ->beneath business) --
> but why is this better?

It's easier to deal with because we can figure out which target is
implementing the method (e.g. if it wants to get at target-specific
data).  Also, "set debug target 1" takes effect right away if you do
this - not the next time you connect.
> >... but these _execution_direction methods.  Please choose one or the
> >other; it makes it simpler to find the uses and implementations at the
> >same time.
> 
> ... are you referring to the choices of names?
> 
> I made the names of the macros more verbose because they're
> more visible, and used in a context where any aid to understanding
> is desireable.  For the struct members themselves, I used shorter
> names because they're *not* all that visible, are used in a context
> where it's not all that critical to understand precisely what they
> do, and longer names are cumbersome.  For instance, they would make
> the declarations in target.h really long.
> 
> Do you still think I should change it?

I do.  The problem is, what are people going to call the target
implementations of these?  They implement
target_get_execution_direction, but they get assigned to
to_get_execdir.  So whichever one you pick, some of the people
searching for it aren't going to find it.

I don't really care which one you use, though.

> >>+   remote_ops.to_get_execdir = remote_get_execdir;
> >>+   remote_ops.to_set_execdir = remote_set_execdir;
> >
> >
> >What about the other remote ops vectors?  At least one isn't copied
> >from here.
> 
> Right, well, I figured get it accepted in one vector, then
> worry about extending it into others.  Basically, as of this
> patch, it will work with "target remote" only (and I guess
> only in non-async mode).

I guess.  I really want to kill the extra vectors; but we can leave
this as one more inconsistency to clean up between them.  But the bits
you're adding to remote.c don't seem that they'd need anything new to
handle async.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Daniel Jacobowitz wrote:

> With that fixed I can try it:
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> 0x00002aaaaaaaba80 in ?? ()
> (gdb) rsi
> 0x00002aaaaaaaba80 in ?? ()
> (gdb)
> 0x00002aaaaaaaba83 in ?? ()
> 
> OK, that's two bugs.  One is the vCont bug I mentioned in another
> message; it's stepping forward instead of backwards.  The other has
> nothing to do with this at all; apparently PTRACE_STEP isn't working
> for the first instruction in the program and we're taking two steps to
> go one instruction.  OK, let's avoid the vCont bug and try again:
> 
> (gdb) set remote verbose-resume-packet 0
> (gdb) rsi
> Sending packet: $m2aaaaaab7320,1#70...Ack
> Packet received: f3
> Sending packet: $M2aaaaaab7320,1:cc#50...Ack
> Packet received: OK
> Sending packet: $Hc0#db...Ack
> Packet received: E01
> Sending packet: $bs#d5...Ack
> Packet received:
> warning: Invalid remote reply:
> *hang*
> 
> It thinks that the attempt to step has succeeded and that the program
> is now running.  That's pretty messy failing.

Agreed, of course.  Well, this code in remote_wait...

1.1 (shebs 16-Apr-99):      default:
1.1 (shebs 16-Apr-99):        warning ("Invalid remote reply: %s", buf);
1.1 (shebs 16-Apr-99):        continue;
1.1 (shebs 16-Apr-99):      }

*... has been essentially untouched since the public repository was
created.  I could trace it back further, but my gut feeling is that
it is basically untested.  I mean, if we say "continue" here,
of course it's going to hang...  we've just consumed the target's
reply, so it's not going to send us anything else.  Generally, you
send one command, you get one reply.

My inclination is to call error here, instead of continue.
But of course I know of no way to test the effects, except
for the experiment you've just done.

How does that strike you?



* Of course the code is not really Stan's -- he just created the repository.

Re: [RFA] Reverse debugging, part 1/3: target interface

[Daniel Jacobowitz]

On Thu, Apr 20, 2006 at 04:53:29PM -0700, Michael Snyder wrote:
> >It thinks that the attempt to step has succeeded and that the program
> >is now running.  That's pretty messy failing.
> 
> Agreed, of course.  Well, this code in remote_wait...
> 
> 1.1 (shebs 16-Apr-99):      default:
> 1.1 (shebs 16-Apr-99):        warning ("Invalid remote reply: %s", buf);
> 1.1 (shebs 16-Apr-99):        continue;
> 1.1 (shebs 16-Apr-99):      }
> 
> *... has been essentially untouched since the public repository was
> created.  I could trace it back further, but my gut feeling is that
> it is basically untested.  I mean, if we say "continue" here,
> of course it's going to hang...  we've just consumed the target's
> reply, so it's not going to send us anything else.  Generally, you
> send one command, you get one reply.
> 
> My inclination is to call error here, instead of continue.
> But of course I know of no way to test the effects, except
> for the experiment you've just done.
> 
> How does that strike you?

Well, I've got no problem with that, but I don't think it's enough to
settle this.  "error: Invalid remote reply: " (the buffer will be
empty) is not a very informative error message.  We want to say that
reverse execution is not supported.  See my other message about when.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 1/3: target interface

[Michael Snyder]

Eli writes:
>> + /* Forward/reverse execution direction.
>> +    These will only be implemented by a target that supports reverse
execution.
>> + */
>> + #define target_get_execution_direction() \
>> +     (current_target.to_get_execdir ? \
>> +      (*current_target.to_get_execdir) () : EXEC_ERROR)
>
> Isn't it better if the default will be EXEC_FORWARD, not EXEC_ERROR?

Well, I wanted a way to probe and find out whether the target
implements these methods at all.  If you call get_execdir and
get back "error", you can report "sorry, this target doesn't support it".

I can see your argument, that you could simply report "yes,
you're going forward" -- but then the only way to probe
would be to call target_SET_execdir, and that seems too
intrusive for a probe.

Of course, I could just add a third target vector element,
but that seemed like overkill.  Perhaps I'm trying to be
too clever?

>> + /* Reverse execution.
>> +    FIXME: set up as a capability.  */
>> + static enum exec_direction_kind remote_execdir = EXEC_FORWARD;
>> +
>> + static enum exec_direction_kind remote_get_execdir (void)
>> + {
>> +   if (remote_debug && info_verbose)
>> +     printf_filtered ("remote execdir is %s\n",
>> +        remote_execdir == EXEC_FORWARD ? "forward" :
>> +        remote_execdir == EXEC_REVERSE ? "reverse" :
>> +        "unknown");
>> +   return remote_execdir;
>> + }
>> +
>> + static int remote_set_execdir (enum exec_direction_kind dir)
>> + {
>> +   if (remote_debug && info_verbose)
>> +     printf_filtered ("Set remote execdir: %s\n",
>> +        dir == EXEC_FORWARD ? "forward" :
>> +        dir == EXEC_REVERSE ? "reverse" :
>> +        "bad direction");
>> +
>> +   /* FIXME: check target for capability.  */
>> +   if (dir == EXEC_FORWARD || dir == EXEC_REVERSE)
>> +     return (remote_execdir = dir);
>> +   else
>> +     return EXEC_ERROR;
>> + }
>
> I don't understand these two methods--they don't seem to do anything
> that their names imply.  What am I missing?

It's deceptively simple.   ;-)

The two methods just get and set the value of a local mode variable,
remote_execdir.  That's all.  Then that variable is used by remote_wait
and remote_resume.