[RFA] Reverse Debugging, 2/5

[RFA] Reverse Debugging, 2/5

[Michael Snyder]

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



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


This patch (#2) adds a target implementation of reverse debugging
(for the "remote" target), implementing the new target_ops methods
and changing remote_wait and remote_resume to make use of them.

Adding this patch on top of patch #1 causes no change in gdb
behavior, since nothing invokes the new methods.  Tested under
RHEL native with no change in results.


2008-09-30  Michael Snyder  <msnyder@vmware.com>
	Remote interface for reverse debugging.
	* remote.c (remote_get_execdir, remote_set_execdir): New methods.
	(remote_vcont_resume): Jump out if attempting reverse execution.
	(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).
	(_initialize_remote): Add new methods to remote target vector.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.310
diff -u -p -r1.310 remote.c
--- remote.c	22 Sep 2008 15:21:30 -0000	1.310
+++ remote.c	30 Sep 2008 18:23:30 -0000
@@ -3385,7 +3385,15 @@ remote_resume (ptid_t ptid, int step, en
     set_continue_thread (ptid);
 
   buf = rs->buf;
-  if (siggnal != TARGET_SIGNAL_0)
+  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);
@@ -3651,8 +3659,15 @@ remote_wait (ptid_t ptid, struct target_
 	  /* We're out of sync with the target now.  Did it continue or not?
 	     Not is more likely, so report a stop.  */
 	  warning (_("Remote failure reply: %s"), buf);
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_0;
+	  if (buf[1] == '0' && buf[2] == '6')
+	    {
+	      status->kind = TARGET_WAITKIND_NO_HISTORY;
+	    }
+	  else
+	    {
+	      status->kind = TARGET_WAITKIND_STOPPED;
+	      status->value.sig = TARGET_SIGNAL_0;
+	    }
 	  goto got_status;
 	case 'F':		/* File-I/O request.  */
 	  remote_fileio_request (buf);
@@ -7545,6 +7560,35 @@ remote_command (char *args, int from_tty
   help_list (remote_cmdlist, "remote ", -1, gdb_stdout);
 }
 
+/* Reverse execution.
+   TODO: 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");
+
+  /* TODO: 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)
 {
@@ -7593,6 +7637,8 @@ 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_get_execdir = remote_get_execdir;
+  remote_ops.to_set_execdir = remote_set_execdir;
   remote_ops.to_magic = OPS_MAGIC;
   remote_ops.to_memory_map = remote_memory_map;
   remote_ops.to_flash_erase = remote_flash_erase;

Re: [RFA] Reverse Debugging, 2/5

[Pedro Alves]

On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
> +  /* TODO: check target for capability.  */

Can we address this?  If you want to be able to query for support,
it would be a matter of defining a new qSupported feature.

On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_0;
> +         if (buf[1] == '0' && buf[2] == '6')
> +           {
> +             status->kind = TARGET_WAITKIND_NO_HISTORY;
> +           }
> +         else
> +           {
> +             status->kind = TARGET_WAITKIND_STOPPED;
> +             status->value.sig = TARGET_SIGNAL_0;
> +           }

This isn't really an error, it's a defined reply, so it
looks a bit strange to me to be using an error number.

Is there a reason this can't be reported with a T stop reply and
a special "register", like "library" -> TARGET_WAITKIND_LOADED is?

AFAICT, nothing else in the remote implementation relies
on defined error numbers currently --- annoying at times, but
doesn't seem to apply here.

+
+static enum exec_direction_kind remote_get_execdir (void)

Function name on the first column please.

+{
+  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;
+}

This should be made i18n aware.

Similarly in remote_set_execdir.

No new vCont packets -> no plans on reverse + multi-threading ?  :-)

-- 
Pedro Alves

Re: [RFA] Reverse Debugging, 2/5

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
>> +  /* TODO: check target for capability.  */
> 
> Can we address this?  If you want to be able to query for support,
> it would be a matter of defining a new qSupported feature.

OK -- but what about existing targets that support reverse,
but don't know about the qSupported query?

When I put that comment in, I probably intended an implied
question-mark -- that is, I wasn't asserting that a query
would be useful, just wondering aloud...


> On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
>> -         status->kind = TARGET_WAITKIND_STOPPED;
>> -         status->value.sig = TARGET_SIGNAL_0;
>> +         if (buf[1] == '0' && buf[2] == '6')
>> +           {
>> +             status->kind = TARGET_WAITKIND_NO_HISTORY;
>> +           }
>> +         else
>> +           {
>> +             status->kind = TARGET_WAITKIND_STOPPED;
>> +             status->value.sig = TARGET_SIGNAL_0;
>> +           }
> 
> This isn't really an error, it's a defined reply, so it
> looks a bit strange to me to be using an error number.
> 
> Is there a reason this can't be reported with a T stop reply and
> a special "register", like "library" -> TARGET_WAITKIND_LOADED is?
> 
> AFAICT, nothing else in the remote implementation relies
> on defined error numbers currently --- annoying at times, but
> doesn't seem to apply here.

Yeah, I hear ya -- I'm not crazy about it either, and I
don't think I knew about the idea of adding new tags onto
the "T" packet two years ago.

But... the discussion about the remote protocol for this
happened back in '06.  There are now targets out in the field
that implement it this way.  It would be bad form to break them...

We could add a new T packet tag, and then threaten to
deprecate support for the error reply at some future time.


> +
> +static enum exec_direction_kind remote_get_execdir (void)
> 
> Function name on the first column please.
> 
> +{
> +  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;
> +}
> 
> This should be made i18n aware.
> 
> Similarly in remote_set_execdir.

You got it.  ;-)

> No new vCont packets -> no plans on reverse + multi-threading ?  :-)

Noooo... it means no story *now* about reverse and multi-threading.
Can always be added in later...

Re: [RFA] Reverse Debugging, 2/5

[Daniel Jacobowitz]

On Mon, Oct 06, 2008 at 02:10:18PM -0700, Michael Snyder wrote:
> Pedro Alves wrote:
>> On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
>>> +  /* TODO: check target for capability.  */
>>
>> Can we address this?  If you want to be able to query for support,
>> it would be a matter of defining a new qSupported feature.
>
> OK -- but what about existing targets that support reverse,
> but don't know about the qSupported query?
>
> When I put that comment in, I probably intended an implied
> question-mark -- that is, I wasn't asserting that a query
> would be useful, just wondering aloud...

All qSupported probes can be overridden by a manual setting.  I don't
feel particularly bad about forcing people to update, if there's a
workaround - that's part of getting protocol changes merged :-)

However, I'm not completely sure it's necessary in this case.  When do
we check for capability?  If it's only at the appropriate run/continue
command, then probing is OK - though this would make it hard to,
e.g., automatically enable IDE buttons.

> Yeah, I hear ya -- I'm not crazy about it either, and I
> don't think I knew about the idea of adding new tags onto
> the "T" packet two years ago.
>
> But... the discussion about the remote protocol for this
> happened back in '06.  There are now targets out in the field
> that implement it this way.  It would be bad form to break them...

I'm pretty sure nothing about this error was in that discussion.  At
least, I think I would have objected at the time.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse Debugging, 2/5

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Oct 06, 2008 at 02:10:18PM -0700, Michael Snyder wrote:
>> Pedro Alves wrote:
>>> On Wednesday 01 October 2008 20:17:54, Michael Snyder wrote:
>>>> +  /* TODO: check target for capability.  */
>>> Can we address this?  If you want to be able to query for support,
>>> it would be a matter of defining a new qSupported feature.
>> OK -- but what about existing targets that support reverse,
>> but don't know about the qSupported query?
>>
>> When I put that comment in, I probably intended an implied
>> question-mark -- that is, I wasn't asserting that a query
>> would be useful, just wondering aloud...
> 
> All qSupported probes can be overridden by a manual setting.  I don't
> feel particularly bad about forcing people to update, if there's a
> workaround - that's part of getting protocol changes merged :-)

I see.  So we would just make them type "set reverse-supported on"
or something like that.

> However, I'm not completely sure it's necessary in this case.  When do
> we check for capability?  If it's only at the appropriate run/continue
> command, then probing is OK - though this would make it hard to,
> e.g., automatically enable IDE buttons.

Well, MI isn't in there yet, though I've heard from a
possible contributor.  How about if we put qSupported
into a later patch as well?

Nothing wrong with the present target implementers being
supported in the first version...


>> Yeah, I hear ya -- I'm not crazy about it either, and I
>> don't think I knew about the idea of adding new tags onto
>> the "T" packet two years ago.
>>
>> But... the discussion about the remote protocol for this
>> happened back in '06.  There are now targets out in the field
>> that implement it this way.  It would be bad form to break them...
> 
> I'm pretty sure nothing about this error was in that discussion.  At
> least, I think I would have objected at the time.

You're probably right.  But it was certainly in my earlier
patch submissions.