[PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

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

Following discussion, this patch adds feature capability handling
(enable, disable, and automatic "qSupported" query) for the reverse
execution packets "bs" (backward step) and "bc" (backward continue
in the gdb remote protocol.

Cc:ing Jakob and Greg, whose remote targets may be affected.
What you guys will want to do is have your remote targets
recognize the "qSupported" query from gdb, and respond with:

	ReverseContinue+;ReverseStep+

This will tell gdb that your targets support those two commands.

Otherwise, they default to "disabled", and a user would need
to enable them with these commands (which might be added to a
.gdbinit file):

	set remote reverse-continue on
	set remote reverse-step on

Pedro, does this look like what you expected?

Michael


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

2009-08-31  Michael Snyder  <msnyder@vmware.com>

	* remote.c (PACKET_bc, PACKET_bs): New enums.
	(remote_protocol_features): Add ReverseStep, ReverseContinue.
	(remote_resume): Check for reverse capability.
	(_initialize_remote): Add packet config for "bs" and "bc" packets.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.370
diff -u -p -r1.370 remote.c
--- remote.c	18 Aug 2009 16:17:16 -0000	1.370
+++ remote.c	31 Aug 2009 19:55:21 -0000
@@ -1000,6 +1000,8 @@ enum {
   PACKET_qXfer_siginfo_write,
   PACKET_qAttached,
   PACKET_ConditionalTracepoints,
+  PACKET_bc,
+  PACKET_bs,
   PACKET_MAX
 };
 
@@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_siginfo_write },
   { "ConditionalTracepoints", PACKET_DISABLE, remote_cond_tracepoint_feature,
     PACKET_ConditionalTracepoints },
+  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bc },
+  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bs },
 };
 
 static void
@@ -3818,6 +3824,14 @@ remote_resume (struct target_ops *ops,
       if (info_verbose && siggnal != TARGET_SIGNAL_0)
 	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
 		 siggnal);
+
+      if (step &&
+	  remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
+	error ("Remote reverse-step not supported.");
+      if (!step &&
+	  remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
+	error ("Remote reverse-continue not supported.");
+
       strcpy (buf, step ? "bs" : "bc");
     }
   else if (siggnal != TARGET_SIGNAL_0)
@@ -9165,6 +9179,12 @@ Show the maximum size of the address (in
 			 "qGetTLSAddr", "get-thread-local-storage-address",
 			 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
+			 "bc", "reverse-continue", 0);
+
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
+			 "bs", "reverse-step", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
 			 "qSupported", "supported-packets", 0);
 

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Hui Zhu]

Should we need update them?
static int remote_target_can_reverse = 1;

static int
remote_can_execute_reverse (void)
{
  return remote_target_can_reverse;
}

Thanks,
Hui

On Tue, Sep 1, 2009 at 03:56, Michael Snyder<msnyder@vmware.com> wrote:
> Following discussion, this patch adds feature capability handling
> (enable, disable, and automatic "qSupported" query) for the reverse
> execution packets "bs" (backward step) and "bc" (backward continue
> in the gdb remote protocol.
>
> Cc:ing Jakob and Greg, whose remote targets may be affected.
> What you guys will want to do is have your remote targets
> recognize the "qSupported" query from gdb, and respond with:
>
>        ReverseContinue+;ReverseStep+
>
> This will tell gdb that your targets support those two commands.
>
> Otherwise, they default to "disabled", and a user would need
> to enable them with these commands (which might be added to a
> .gdbinit file):
>
>        set remote reverse-continue on
>        set remote reverse-step on
>
> Pedro, does this look like what you expected?
>
> Michael
>
>
> 2009-08-31  Michael Snyder  <msnyder@vmware.com>
>
>        * remote.c (PACKET_bc, PACKET_bs): New enums.
>        (remote_protocol_features): Add ReverseStep, ReverseContinue.
>        (remote_resume): Check for reverse capability.
>        (_initialize_remote): Add packet config for "bs" and "bc" packets.
>
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.370
> diff -u -p -r1.370 remote.c
> --- remote.c    18 Aug 2009 16:17:16 -0000      1.370
> +++ remote.c    31 Aug 2009 19:55:21 -0000
> @@ -1000,6 +1000,8 @@ enum {
>   PACKET_qXfer_siginfo_write,
>   PACKET_qAttached,
>   PACKET_ConditionalTracepoints,
> +  PACKET_bc,
> +  PACKET_bs,
>   PACKET_MAX
>  };
>
> @@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
>     PACKET_qXfer_siginfo_write },
>   { "ConditionalTracepoints", PACKET_DISABLE,
> remote_cond_tracepoint_feature,
>     PACKET_ConditionalTracepoints },
> +  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_bc },
> +  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_bs },
>  };
>
>  static void
> @@ -3818,6 +3824,14 @@ remote_resume (struct target_ops *ops,
>       if (info_verbose && siggnal != TARGET_SIGNAL_0)
>        warning (" - Can't pass signal %d to target in reverse: ignored.\n",
>                 siggnal);
> +
> +      if (step &&
> +         remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
> +       error ("Remote reverse-step not supported.");
> +      if (!step &&
> +         remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
> +       error ("Remote reverse-continue not supported.");
> +
>       strcpy (buf, step ? "bs" : "bc");
>     }
>   else if (siggnal != TARGET_SIGNAL_0)
> @@ -9165,6 +9179,12 @@ Show the maximum size of the address (in
>                         "qGetTLSAddr", "get-thread-local-storage-address",
>                         0);
>
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
> +                        "bc", "reverse-continue", 0);
> +
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
> +                        "bs", "reverse-step", 0);
> +
>   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
>                         "qSupported", "supported-packets", 0);
>
>
>

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

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

Hui Zhu wrote:
> Should we need update them?
> static int remote_target_can_reverse = 1;
> 
> static int
> remote_can_execute_reverse (void)
> {
>   return remote_target_can_reverse;
> }

Right!  I forgot about it -- thanks for reminding me.
New diff.


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

2009-08-31  Michael Snyder  <msnyder@vmware.com>

	* remote.c (PACKET_bc, PACKET_bs): New enums.
	(remote_protocol_features): Add ReverseStep, ReverseContinue.
	(remote_resume): Check for reverse capability.
	(remote_can_execute_reverse): Use packet config variables.
	(_initialize_remote): Add packet config for "bs" and "bc" packets.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.370
diff -u -p -r1.370 remote.c
--- remote.c	18 Aug 2009 16:17:16 -0000	1.370
+++ remote.c	1 Sep 2009 04:01:04 -0000
@@ -1000,6 +1000,8 @@ enum {
   PACKET_qXfer_siginfo_write,
   PACKET_qAttached,
   PACKET_ConditionalTracepoints,
+  PACKET_bc,
+  PACKET_bs,
   PACKET_MAX
 };
 
@@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_siginfo_write },
   { "ConditionalTracepoints", PACKET_DISABLE, remote_cond_tracepoint_feature,
     PACKET_ConditionalTracepoints },
+  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bc },
+  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bs },
 };
 
 static void
@@ -3818,6 +3824,14 @@ remote_resume (struct target_ops *ops,
       if (info_verbose && siggnal != TARGET_SIGNAL_0)
 	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
 		 siggnal);
+
+      if (step &&
+	  remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
+	error ("Remote reverse-step not supported.");
+      if (!step &&
+	  remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
+	error ("Remote reverse-continue not supported.");
+
       strcpy (buf, step ? "bs" : "bc");
     }
   else if (siggnal != TARGET_SIGNAL_0)
@@ -8730,12 +8744,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;
+  if (remote_protocol_packets[PACKET_bs].support == PACKET_ENABLE
+      || remote_protocol_packets[PACKET_bc].support == PACKET_ENABLE)
+    return 1;
+  else
+    return 0;
 }
 
 static int
@@ -9165,6 +9181,12 @@ Show the maximum size of the address (in
 			 "qGetTLSAddr", "get-thread-local-storage-address",
 			 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
+			 "bc", "reverse-continue", 0);
+
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
+			 "bs", "reverse-step", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
 			 "qSupported", "supported-packets", 0);
 

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Hui Zhu]

On Tue, Sep 1, 2009 at 12:02, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Should we need update them?
>> static int remote_target_can_reverse = 1;
>>
>> static int
>> remote_can_execute_reverse (void)
>> {
>>  return remote_target_can_reverse;
>> }
>
> Right!  I forgot about it -- thanks for reminding me.
> New diff.
>
>
Thanks.  I think this version is better.

Hui

> 2009-08-31  Michael Snyder  <msnyder@vmware.com>
>
>        * remote.c (PACKET_bc, PACKET_bs): New enums.
>        (remote_protocol_features): Add ReverseStep, ReverseContinue.
>        (remote_resume): Check for reverse capability.
>        (remote_can_execute_reverse): Use packet config variables.
>        (_initialize_remote): Add packet config for "bs" and "bc" packets.
>
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.370
> diff -u -p -r1.370 remote.c
> --- remote.c    18 Aug 2009 16:17:16 -0000      1.370
> +++ remote.c    1 Sep 2009 04:01:04 -0000
> @@ -1000,6 +1000,8 @@ enum {
>   PACKET_qXfer_siginfo_write,
>   PACKET_qAttached,
>   PACKET_ConditionalTracepoints,
> +  PACKET_bc,
> +  PACKET_bs,
>   PACKET_MAX
>  };
>
> @@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
>     PACKET_qXfer_siginfo_write },
>   { "ConditionalTracepoints", PACKET_DISABLE,
> remote_cond_tracepoint_feature,
>     PACKET_ConditionalTracepoints },
> +  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_bc },
> +  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_bs },
>  };
>
>  static void
> @@ -3818,6 +3824,14 @@ remote_resume (struct target_ops *ops,
>       if (info_verbose && siggnal != TARGET_SIGNAL_0)
>        warning (" - Can't pass signal %d to target in reverse: ignored.\n",
>                 siggnal);
> +
> +      if (step &&
> +         remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
> +       error ("Remote reverse-step not supported.");
> +      if (!step &&
> +         remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
> +       error ("Remote reverse-continue not supported.");
> +
>       strcpy (buf, step ? "bs" : "bc");
>     }
>   else if (siggnal != TARGET_SIGNAL_0)
> @@ -8730,12 +8744,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;
> +  if (remote_protocol_packets[PACKET_bs].support == PACKET_ENABLE
> +      || remote_protocol_packets[PACKET_bc].support == PACKET_ENABLE)
> +    return 1;
> +  else
> +    return 0;
>  }
>
>  static int
> @@ -9165,6 +9181,12 @@ Show the maximum size of the address (in
>                         "qGetTLSAddr", "get-thread-local-storage-address",
>                         0);
>
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
> +                        "bc", "reverse-continue", 0);
> +
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
> +                        "bs", "reverse-step", 0);
> +
>   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
>                         "qSupported", "supported-packets", 0);
>
>
>

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Pedro Alves]

On Monday 31 August 2009 20:56:03, Michael Snyder wrote:
> Following discussion, this patch adds feature capability handling
> (enable, disable, and automatic "qSupported" query) for the reverse
> execution packets "bs" (backward step) and "bc" (backward continue
> in the gdb remote protocol.
> 
> Cc:ing Jakob and Greg, whose remote targets may be affected.
> What you guys will want to do is have your remote targets
> recognize the "qSupported" query from gdb, and respond with:
> 
> 	ReverseContinue+;ReverseStep+
> 
> This will tell gdb that your targets support those two commands.
> 
> Otherwise, they default to "disabled", and a user would need
> to enable them with these commands (which might be added to a
> .gdbinit file):
> 
> 	set remote reverse-continue on
> 	set remote reverse-step on
> 
> Pedro, does this look like what you expected?

Yeah, something like that.  Thanks!  Hui already mentioned
target_can_execute_reverse (thanks!).  Initialy I was actually
thinking of the target reporting a single "CanReverse" feature, not a
feature for each packet.  I mean, the core of gdb only
knows about target_can_execute_reverse() your idea works for me too,
if it makes sense to you.

While you're at it, how about making remote_resume skip calling
remote_vcont_resume if GDB is requesting a reverse resume?

(I was hoping that someday we'd add reverse support to
vCont packets, so gdb could make use of
say: "vCont?" -> "vCont;c;s;C;S;rs;rc".  Something like vCont
will be necessary if gdb is to control (non-transparently) simultaneous
multiple sort-of-independently reversible devices like Jakob
seems to indicate his target can.)

Your patch also needs docs and NEWS entries, BTW.

-- 
Pedro Alves

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Pedro Alves]

[replying to the updated patch itself]

> +
> +      if (step &&
> +         remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
> +       error ("Remote reverse-step not supported.");
> +      if (!step &&
> +         remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
> +       error ("Remote reverse-continue not supported.");
> +

'&&' at beginning of new line, not at end of previous line.  The
error strings need wrapping in _() for i18n.

Don't forget docs and NEWS.  :-)

-- 
Pedro Alves

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Tue, 1 Sep 2009 16:44:13 +0100
> Cc: Michael Snyder <msnyder@vmware.com>,  Jakob Engblom <jakob@virtutech.com>,  Greg Law <glaw@undo-software.com>
> 
> Your patch also needs docs and NEWS entries, BTW.

And a patch for the manual documenting the new packets, no?

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

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

Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@codesourcery.com>
>> Date: Tue, 1 Sep 2009 16:44:13 +0100
>> Cc: Michael Snyder <msnyder@vmware.com>,  Jakob Engblom <jakob@virtutech.com>,  Greg Law <glaw@undo-software.com>
>>
>> Your patch also needs docs and NEWS entries, BTW.
> 
> And a patch for the manual documenting the new packets, no?

New diff incorporating comments and adding docs and NEWS.



[-- Attachment #2: qSupported-3.txt --]
[-- Type: text/plain, Size: 4664 bytes --]

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.370
diff -u -p -r1.370 remote.c
--- remote.c	18 Aug 2009 16:17:16 -0000	1.370
+++ remote.c	6 Sep 2009 03:33:51 -0000
@@ -1000,6 +1000,8 @@ enum {
   PACKET_qXfer_siginfo_write,
   PACKET_qAttached,
   PACKET_ConditionalTracepoints,
+  PACKET_bc,
+  PACKET_bs,
   PACKET_MAX
 };
 
@@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_siginfo_write },
   { "ConditionalTracepoints", PACKET_DISABLE, remote_cond_tracepoint_feature,
     PACKET_ConditionalTracepoints },
+  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bc },
+  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bs },
 };
 
 static void
@@ -3818,6 +3824,14 @@ remote_resume (struct target_ops *ops,
       if (info_verbose && siggnal != TARGET_SIGNAL_0)
 	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
 		 siggnal);
+
+      if (step 
+	  && remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
+	error ("Remote reverse-step not supported.");
+      if (!step
+	  && remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
+	error ("Remote reverse-continue not supported.");
+
       strcpy (buf, step ? "bs" : "bc");
     }
   else if (siggnal != TARGET_SIGNAL_0)
@@ -8730,12 +8744,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;
+  if (remote_protocol_packets[PACKET_bs].support == PACKET_ENABLE
+      || remote_protocol_packets[PACKET_bc].support == PACKET_ENABLE)
+    return 1;
+  else
+    return 0;
 }
 
 static int
@@ -9165,6 +9181,12 @@ Show the maximum size of the address (in
 			 "qGetTLSAddr", "get-thread-local-storage-address",
 			 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
+			 "bc", "reverse-continue", 0);
+
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
+			 "bs", "reverse-step", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
 			 "qSupported", "supported-packets", 0);
 
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.328
diff -u -p -r1.328 NEWS
--- NEWS	31 Aug 2009 20:18:45 -0000	1.328
+++ NEWS	6 Sep 2009 03:33:51 -0000
@@ -309,6 +309,14 @@ show remote write-siginfo-object
   Control use of remote protocol `qXfer:siginfo:write' (write-siginfo-object)
   packet.
 
+set remote reverse-continue
+show remote reverse-continue
+  Control use of remote protocol 'bc' (reverse-continue) packet.
+
+set remote reverse-step
+show remote reverse-step
+  Control use of remote protocol 'bs' (reverse-step) packet.
+
 set displaced-stepping
 show displaced-stepping
   Control displaced stepping mode.  Displaced stepping is a way to
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.620
diff -u -p -r1.620 gdb.texinfo
--- doc/gdb.texinfo	1 Sep 2009 18:48:58 -0000	1.620
+++ doc/gdb.texinfo	6 Sep 2009 03:33:51 -0000
@@ -27493,16 +27493,18 @@ 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
+@anchor{bc}
+@item bc
 Backward continue.  Execute the target system in reverse.  No parameter.
 @xref{Reverse Execution}, for more information.
 
 Reply:
 @xref{Stop Reply Packets}, for the reply specifications.
 
-@item bs
 @cindex @samp{bs} packet
+@anchor{bs}
+@item bs
 Backward single step.  Execute one instruction in reverse.  No parameter.
 @xref{Reverse Execution}, for more information.
 
@@ -28746,6 +28748,16 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab No
 
+@item @samp{ReverseContinue}
+@tab No
+@tab @samp{+}
+@tab No
+
+@item @samp{ReverseStep}
+@tab No
+@tab @samp{+}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -28827,6 +28839,14 @@ The remote stub understands the @samp{qX
 The remote stub accepts and implements conditional expressions defined
 for tracepoints (@pxref{Tracepoint Conditions}).
 
+@item ReverseContinue
+The remote stub accepts and implements the reverse continue packet
+(@pxref{bc}).
+
+@item ReverseStep
+The remote stub accepts and implements the reverse step packet
+(@pxref{bs}).
+
 @end table
 
 @item qSymbol::

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Eli Zaretskii]

> Date: Sat, 05 Sep 2009 20:36:22 -0700
> From: Michael Snyder <msnyder@vmware.com>
> CC: Pedro Alves <pedro@codesourcery.com>, 
>  "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
>  "jakob@virtutech.com" <jakob@virtutech.com>, 
>  "glaw@undo-software.com" <glaw@undo-software.com>
> 
> New diff incorporating comments and adding docs and NEWS.

Thanks.  The patches for the manual and NEWS are fine with me.

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Pedro Alves]

On Sunday 06 September 2009 04:36:22, Michael Snyder wrote:
> Eli Zaretskii wrote:
> >> From: Pedro Alves <pedro@codesourcery.com>
> >> Date: Tue, 1 Sep 2009 16:44:13 +0100
> >> Cc: Michael Snyder <msnyder@vmware.com>,  Jakob Engblom <jakob@virtutech.com>,  Greg Law <glaw@undo-software.com>
> >>
> >> Your patch also needs docs and NEWS entries, BTW.
> > 
> > And a patch for the manual documenting the new packets, no?
> 
> New diff incorporating comments and adding docs and NEWS.

What about the i18n comments?
What about the vCont (the one about not sending vcont
if requesting a reverse resume) comments?

-- 
Pedro Alves

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

Pedro Alves wrote:
> On Sunday 06 September 2009 04:36:22, Michael Snyder wrote:
>> Eli Zaretskii wrote:
>>>> From: Pedro Alves <pedro@codesourcery.com>
>>>> Date: Tue, 1 Sep 2009 16:44:13 +0100
>>>> Cc: Michael Snyder <msnyder@vmware.com>,  Jakob Engblom <jakob@virtutech.com>,  Greg Law <glaw@undo-software.com>
>>>>
>>>> Your patch also needs docs and NEWS entries, BTW.
>>> And a patch for the manual documenting the new packets, no?
>> New diff incorporating comments and adding docs and NEWS.
> 
> What about the i18n comments?

Oof, sorry, forgot.  You just mean the two error msgs, right?
See revised diff.

> What about the vCont (the one about not sending vcont
> if requesting a reverse resume) comments?

Are you sure?  I guess, like you, I hoped it would eventually
be added.  Works fine as it is, it probes and fails, but if
you want it, ok...  added below.

I have one final question to raise.

You may notice (though nobody has commented), that I made the
two new supported-probed-packets (bs and bc) default to "enabled".

This sort of defeats the purpose (if the purpose is that we can
decide whether to run a testsuite on a remote target) -- but I
was just reluctant to default them to "disabled", because it
would mean that anybody with a deployed target that doesn't yet
support the new "qSupported" probe would have to make his users
enable them by hand.

(why I cc:ed Jakob and Greg.)

So now that I've mentioned it, what do you think?
Enabled, or disabled by default?

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

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

Michael Snyder wrote:
> Pedro Alves wrote:
>> On Sunday 06 September 2009 04:36:22, Michael Snyder wrote:
>>> Eli Zaretskii wrote:
>>>>> From: Pedro Alves <pedro@codesourcery.com>
>>>>> Date: Tue, 1 Sep 2009 16:44:13 +0100
>>>>> Cc: Michael Snyder <msnyder@vmware.com>,  Jakob Engblom <jakob@virtutech.com>,  Greg Law <glaw@undo-software.com>
>>>>>
>>>>> Your patch also needs docs and NEWS entries, BTW.
>>>> And a patch for the manual documenting the new packets, no?
>>> New diff incorporating comments and adding docs and NEWS.
>> What about the i18n comments?
> 
> Oof, sorry, forgot.  You just mean the two error msgs, right?
> See revised diff.
> 
>> What about the vCont (the one about not sending vcont
>> if requesting a reverse resume) comments?
> 
> Are you sure?  I guess, like you, I hoped it would eventually
> be added.  Works fine as it is, it probes and fails, but if
> you want it, ok...  added below.
> 
> I have one final question to raise.
> 
> You may notice (though nobody has commented), that I made the
> two new supported-probed-packets (bs and bc) default to "enabled".
> 
> This sort of defeats the purpose (if the purpose is that we can
> decide whether to run a testsuite on a remote target) -- but I
> was just reluctant to default them to "disabled", because it
> would mean that anybody with a deployed target that doesn't yet
> support the new "qSupported" probe would have to make his users
> enable them by hand.
> 
> (why I cc:ed Jakob and Greg.)
> 
> So now that I've mentioned it, what do you think?
> Enabled, or disabled by default?

Arrr, patch attached this time.




[-- Attachment #2: qSupported-4.txt --]
[-- Type: text/plain, Size: 5648 bytes --]

2009-09-07  Michael Snyder  <msnyder@vmware.com>

	* remote.c (PACKET_bc, PACKET_bs): New enums.
	(remote_protocol_features): Add ReverseStep, ReverseContinue.
	(remote_resume): Check for reverse capability.
	(_initialize_remote): Add packet config for "bs" and "bc" packets.
	* NEWS (new options): Mention set/show for "bs" and "bc" packets.

2009-09-07  Michael Snyder  <msnyder@vmware.com>

	* gdb.texinfo (qSupported): Mention new ReverseContinue and
	ReverseStep replies to the qSupported query.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.370
diff -u -p -r1.370 remote.c
--- remote.c	18 Aug 2009 16:17:16 -0000	1.370
+++ remote.c	7 Sep 2009 22:10:56 -0000
@@ -1000,6 +1000,8 @@ enum {
   PACKET_qXfer_siginfo_write,
   PACKET_qAttached,
   PACKET_ConditionalTracepoints,
+  PACKET_bc,
+  PACKET_bs,
   PACKET_MAX
 };
 
@@ -3051,6 +3053,10 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_siginfo_write },
   { "ConditionalTracepoints", PACKET_DISABLE, remote_cond_tracepoint_feature,
     PACKET_ConditionalTracepoints },
+  { "ReverseContinue", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bc },
+  { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
+    PACKET_bs },
 };
 
 static void
@@ -3801,8 +3807,10 @@ remote_resume (struct target_ops *ops,
   remote_pass_signals ();
 
   /* The vCont packet doesn't need to specify threads via Hc.  */
-  if (remote_vcont_resume (ptid, step, siggnal))
-    goto done;
+  /* No reverse support (yet) for vCont.  */
+  if (execution_direction != EXEC_REVERSE)
+    if (remote_vcont_resume (ptid, step, siggnal))
+      goto done;
 
   /* All other supported resume packets do use Hc, so set the continue
      thread.  */
@@ -3818,6 +3826,14 @@ remote_resume (struct target_ops *ops,
       if (info_verbose && siggnal != TARGET_SIGNAL_0)
 	warning (" - Can't pass signal %d to target in reverse: ignored.\n",
 		 siggnal);
+
+      if (step 
+	  && remote_protocol_packets[PACKET_bs].support == PACKET_DISABLE)
+	error (_("Remote reverse-step not supported."));
+      if (!step
+	  && remote_protocol_packets[PACKET_bc].support == PACKET_DISABLE)
+	error ("_(Remote reverse-continue not supported."));
+
       strcpy (buf, step ? "bs" : "bc");
     }
   else if (siggnal != TARGET_SIGNAL_0)
@@ -8730,12 +8746,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;
+  if (remote_protocol_packets[PACKET_bs].support == PACKET_ENABLE
+      || remote_protocol_packets[PACKET_bc].support == PACKET_ENABLE)
+    return 1;
+  else
+    return 0;
 }
 
 static int
@@ -9165,6 +9183,12 @@ Show the maximum size of the address (in
 			 "qGetTLSAddr", "get-thread-local-storage-address",
 			 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bc],
+			 "bc", "reverse-continue", 0);
+
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_bs],
+			 "bs", "reverse-step", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
 			 "qSupported", "supported-packets", 0);
 
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.328
diff -u -p -r1.328 NEWS
--- NEWS	31 Aug 2009 20:18:45 -0000	1.328
+++ NEWS	7 Sep 2009 22:10:57 -0000
@@ -309,6 +309,14 @@ show remote write-siginfo-object
   Control use of remote protocol `qXfer:siginfo:write' (write-siginfo-object)
   packet.
 
+set remote reverse-continue
+show remote reverse-continue
+  Control use of remote protocol 'bc' (reverse-continue) packet.
+
+set remote reverse-step
+show remote reverse-step
+  Control use of remote protocol 'bs' (reverse-step) packet.
+
 set displaced-stepping
 show displaced-stepping
   Control displaced stepping mode.  Displaced stepping is a way to
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.620
diff -u -p -r1.620 gdb.texinfo
--- doc/gdb.texinfo	1 Sep 2009 18:48:58 -0000	1.620
+++ doc/gdb.texinfo	7 Sep 2009 22:10:57 -0000
@@ -27493,16 +27493,18 @@ 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
+@anchor{bc}
+@item bc
 Backward continue.  Execute the target system in reverse.  No parameter.
 @xref{Reverse Execution}, for more information.
 
 Reply:
 @xref{Stop Reply Packets}, for the reply specifications.
 
-@item bs
 @cindex @samp{bs} packet
+@anchor{bs}
+@item bs
 Backward single step.  Execute one instruction in reverse.  No parameter.
 @xref{Reverse Execution}, for more information.
 
@@ -28746,6 +28748,16 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab No
 
+@item @samp{ReverseContinue}
+@tab No
+@tab @samp{+}
+@tab No
+
+@item @samp{ReverseStep}
+@tab No
+@tab @samp{+}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -28827,6 +28839,14 @@ The remote stub understands the @samp{qX
 The remote stub accepts and implements conditional expressions defined
 for tracepoints (@pxref{Tracepoint Conditions}).
 
+@item ReverseContinue
+The remote stub accepts and implements the reverse continue packet
+(@pxref{bc}).
+
+@item ReverseStep
+The remote stub accepts and implements the reverse step packet
+(@pxref{bs}).
+
 @end table
 
 @item qSymbol::

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Pedro Alves]

On Monday 07 September 2009 23:19:09, Michael Snyder wrote:
> Michael Snyder wrote:
> > Pedro Alves wrote:
> >> What about the i18n comments?
> > 
> > Oof, sorry, forgot.  You just mean the two error msgs, right?
> > See revised diff.

Yep.  Thanks.

> >> What about the vCont (the one about not sending vcont
> >> if requesting a reverse resume) comments?
> > 
> > Are you sure?  I guess, like you, I hoped it would eventually
> > be added.  Works fine as it is, it probes and fails, but if
> > you want it, ok...  added below.

That's because your target doesn't support vCont, otherwise, the target
will run forward instead of backwards.  Granted, the
target_can_support_reverse check at a higher layer will prevent getting
here if neither bc or bs are supported, but, there may be targets that
end up supporting both (forward) vCont and bs+bc.

> > I have one final question to raise.
> > 
> > You may notice (though nobody has commented), that I made the
> > two new supported-probed-packets (bs and bc) default to "enabled".
> > 
> > This sort of defeats the purpose (if the purpose is that we can
> > decide whether to run a testsuite on a remote target) -- but I
> > was just reluctant to default them to "disabled", because it
> > would mean that anybody with a deployed target that doesn't yet
> > support the new "qSupported" probe would have to make his users
> > enable them by hand.
> > 
> > (why I cc:ed Jakob and Greg.)
> > 
> > So now that I've mentioned it, what do you think?
> > Enabled, or disabled by default?
> 

Oh, I totally missed that.  When would they be set to
disabled without user intervention then?  E.g., what happens
against current gdbserver?  Does it still misbehave when trying
to reverse step?

Given there's an easy workaround (set remote foo-packet on),
I vote disabled by default.

-- 
Pedro Alves

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Greg Law]

Michael Snyder wrote:
> [...]  it
> would mean that anybody with a deployed target that doesn't yet
> support the new "qSupported" probe would have to make his users
> enable them by hand.
> 
> (why I cc:ed Jakob and Greg.)

 From our side we're fine with it defaulting to off and we'll add support
for new probe (we haven't released anything yet, so it's fine).

Cheers,

Greg
-- 
Greg Law, Undo Software                       http://undo-software.com/

RE: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Jakob Engblom]

> > [...]  it
> > would mean that anybody with a deployed target that doesn't yet
> > support the new "qSupported" probe would have to make his users
> > enable them by hand.
> >
> > (why I cc:ed Jakob and Greg.)
> 
>  From our side we're fine with it defaulting to off and we'll add support
> for new probe (we haven't released anything yet, so it's fine).

I agree that it makes sense to default to off. 

To support gdb7, we will need to update our side anyway. So there is no point in
trying to be nice. Better to fail early and fail often, as far as I am
concerned.

/jakob

Re: [PATCH] Add 'reverse' capability query to remote protocol (qSupported).

[Michael Snyder]

Jakob Engblom wrote:
>>> [...]  it
>>> would mean that anybody with a deployed target that doesn't yet
>>> support the new "qSupported" probe would have to make his users
>>> enable them by hand.
>>>
>>> (why I cc:ed Jakob and Greg.)
>>  From our side we're fine with it defaulting to off and we'll add support
>> for new probe (we haven't released anything yet, so it's fine).
> 
> I agree that it makes sense to default to off. 
> 
> To support gdb7, we will need to update our side anyway. So there is no point in
> trying to be nice. Better to fail early and fail often, as far as I am
> concerned.

So be it.
Committed.