[RFA] Reverse Debugging, 1/5

[RFA] Reverse Debugging, 1/5

[Michael Snyder]

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



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


This first patch of the reverse debugging sequence affects
only the target_ops interface (adding several target methods
and some enums and such).  

Adding this patch alone does not affect gdb's behavior, and 
I have run testsuites under RHEL with no change in results.


2008-09-30  Michael Snyder  <msnyder@vmware.com>
	Target interface for reverse debugging.
	* 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.c (target_get_execdir): New generic method.
	(target_set_execdir): Ditto.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.184
diff -u -p -r1.184 target.c
--- target.c	22 Sep 2008 15:21:30 -0000	1.184
+++ target.c	30 Sep 2008 18:19:00 -0000
@@ -455,6 +455,8 @@ update_current_target (void)
       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);
       /* Do not inherit to_read_description.  */
       /* Do not inherit to_search_memory.  */
       INHERIT (to_magic, t);
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.130
diff -u -p -r1.130 target.h
--- target.h	19 Aug 2008 13:22:14 -0000	1.130
+++ target.h	30 Sep 2008 18:19:00 -0000
@@ -128,7 +128,11 @@ enum target_waitkind
        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
+    TARGET_WAITKIND_IGNORE,
+
+    /* The target has run out of history information,
+       and cannot run backward any further.  */
+    TARGET_WAITKIND_NO_HISTORY
   };
 
 struct target_waitstatus
@@ -147,6 +151,14 @@ struct target_waitstatus
     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
@@ -523,6 +535,11 @@ struct target_ops
 			     const gdb_byte *pattern, ULONGEST pattern_len,
 			     CORE_ADDR *found_addrp);
 
+    /* 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?
      */
@@ -1127,6 +1144,18 @@ extern int target_stopped_data_address_p
 #define target_watchpoint_addr_within_range(target, addr, start, length) \
   (*target.to_watchpoint_addr_within_range) (target, addr, start, length)
 
+/* 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)
+
+
 extern const struct target_desc *target_read_description (struct target_ops *);
 
 /* Utility implementation of searching memory.  */

Re: [RFA] Reverse Debugging, 1/5

[Doug Evans]

On Wed, Oct 1, 2008 at 12:17 PM, Michael Snyder <msnyder@vmware.com> wrote:
>
>
> This first patch of the reverse debugging sequence affects
> only the target_ops interface (adding several target methods
> and some enums and such).
>
> Adding this patch alone does not affect gdb's behavior, and
> I have run testsuites under RHEL with no change in results.
>
>
> 2008-09-30  Michael Snyder  <msnyder@vmware.com>
>        Target interface for reverse debugging.
>        * 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.c (target_get_execdir): New generic method.
>        (target_set_execdir): Ditto.
>
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.184
> diff -u -p -r1.184 target.c
> --- target.c    22 Sep 2008 15:21:30 -0000      1.184
> +++ target.c    30 Sep 2008 18:19:00 -0000
> @@ -455,6 +455,8 @@ update_current_target (void)
>       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);
>       /* Do not inherit to_read_description.  */
>       /* Do not inherit to_search_memory.  */
>       INHERIT (to_magic, t);
> Index: target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.h,v
> retrieving revision 1.130
> diff -u -p -r1.130 target.h
> --- target.h    19 Aug 2008 13:22:14 -0000      1.130
> +++ target.h    30 Sep 2008 18:19:00 -0000
> @@ -128,7 +128,11 @@ enum target_waitkind
>        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
> +    TARGET_WAITKIND_IGNORE,
> +
> +    /* The target has run out of history information,
> +       and cannot run backward any further.  */
> +    TARGET_WAITKIND_NO_HISTORY
>   };
>
>  struct target_waitstatus
> @@ -147,6 +151,14 @@ struct target_waitstatus
>     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
> @@ -523,6 +535,11 @@ struct target_ops
>                             const gdb_byte *pattern, ULONGEST pattern_len,
>                             CORE_ADDR *found_addrp);
>
> +    /* 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?
>      */
> @@ -1127,6 +1144,18 @@ extern int target_stopped_data_address_p
>  #define target_watchpoint_addr_within_range(target, addr, start, length) \
>   (*target.to_watchpoint_addr_within_range) (target, addr, start, length)
>
> +/* 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)
> +
> +
>  extern const struct target_desc *target_read_description (struct target_ops
> *);
>
>  /* Utility implementation of searching memory.  */

Hi.

Nit: Can all occurrences of "execdir" be replaced with
"exec_direction"?  I look at "execdir" and think "execution
directory".  [I realize often the context provides enough clues that
prevent this misinterpretation, but that's not always true.]

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Doug Evans wrote:
> On Wed, Oct 1, 2008 at 12:17 PM, Michael Snyder <msnyder@vmware.com> wrote:
>>
>> This first patch of the reverse debugging sequence affects
>> only the target_ops interface (adding several target methods
>> and some enums and such).
>>
>> Adding this patch alone does not affect gdb's behavior, and
>> I have run testsuites under RHEL with no change in results.

> Hi.
> 
> Nit: Can all occurrences of "execdir" be replaced with
> "exec_direction"?  I look at "execdir" and think "execution
> directory".  [I realize often the context provides enough clues that
> prevent this misinterpretation, but that's not always true.]

Sure, if no one else objects, I'm fine with it.

Any other nits with this first part of the patch?  Anyone?

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> 2008-09-30  Michael Snyder  <msnyder@vmware.com>
> 	Target interface for reverse debugging.
> 	* 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.c (target_get_execdir): New generic method.
> 	(target_set_execdir): Ditto.

One of the questions I'm asking myself is why having the get_execdir
method? It seems that, once we have called "target_sets_execdir" and
it hasn't returned ERROR, core GDB should know what the execution
direction is, no? Is there a situation where a round-trip to the
target would be necessary?  Otherwise, we'll end up with the target
code all doing the same thing, which is caching the current value
of the same thing.

One thing that crossed my mind while thinking about it is whether
we want to make this property global to all inferiors or specific
to each inferior. Ahem, shall we say global?

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Joel Brobecker wrote:
>> 2008-09-30  Michael Snyder  <msnyder@vmware.com>
>>       Target interface for reverse debugging.
>>       * 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.c (target_get_execdir): New generic method.
>>       (target_set_execdir): Ditto.
> 
> One of the questions I'm asking myself is why having the get_execdir
> method? It seems that, once we have called "target_sets_execdir" and
> it hasn't returned ERROR, core GDB should know what the execution
> direction is, no? Is there a situation where a round-trip to the
> target would be necessary?  Otherwise, we'll end up with the target
> code all doing the same thing, which is caching the current value
> of the same thing.
> 
> One thing that crossed my mind while thinking about it is whether
> we want to make this property global to all inferiors or specific
> to each inferior. Ahem, shall we say global?

It was a design choice.

There were two choices:
1) modify target_resume (ops->to_resume), to add a direction
parameter.
2) Add a to_set_direction target method.

The first would have required modifying all existing targets,
so I chose the second.

Once you have a "set" method, it seems logical to have a
"get" method.


Making no assumptions about HOW the target sets the direction,
it seems likely that at least *some* targets will have to
remember this state locally.  Whereas there is no reason
for core-gdb to have to remember the state locally, if it can
always get it from the target.

It seems a worse duplication to keep the same state information
simultaneously in the target and in the core, since now you
have to worry about them getting out of sync.

At worst, a target will need to maintain an int's worth of state
locally, and so long as we're never running two targets at the
same time, there's no synchronization issue.

Re: [RFA] Reverse Debugging, 1/5

[Daniel Jacobowitz]

On Mon, Oct 06, 2008 at 02:00:37PM -0700, Michael Snyder wrote:
> It was a design choice.
>
> There were two choices:
> 1) modify target_resume (ops->to_resume), to add a direction
> parameter.
> 2) Add a to_set_direction target method.
>
> The first would have required modifying all existing targets,
> so I chose the second.

The problem is, we're now talking about a multi-process GDB.  It's a
small step from there to one using multiple targets automatically.  Is
core GDB going to have to do the juggling / bookkeeping to keep
direction in sync for all of them?  The granularity will depend on the
backend...

If it's just a matter of changing the existing targets, then avoiding
that on a branch makes sense - but updating everything in the mainline
version makes sense too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Oct 06, 2008 at 02:00:37PM -0700, Michael Snyder wrote:
>> It was a design choice.
>>
>> There were two choices:
>> 1) modify target_resume (ops->to_resume), to add a direction
>> parameter.
>> 2) Add a to_set_direction target method.
>>
>> The first would have required modifying all existing targets,
>> so I chose the second.
> 
> The problem is, we're now talking about a multi-process GDB.  It's a
> small step from there to one using multiple targets automatically.  Is
> core GDB going to have to do the juggling / bookkeeping to keep
> direction in sync for all of them?  The granularity will depend on the
> backend...
> 
> If it's just a matter of changing the existing targets, then avoiding
> that on a branch makes sense - but updating everything in the mainline
> version makes sense too.

I guess I could be persuaded.  But I'm not, yet...

You're going to send "to_set_direction" to the target,
and then you're going to assume that you know what the
state is later on, rather than asking the target?  What
if the target gets it wrong?  Now you're out of sync.

If we do keep the direction-state in the core, where would
we put it?  In the ecs?  In a global variable in infrun?

I still feel as if it's only the target that KNOWS what
its direction state is -- it should tell gdb, rather than
have gdb make assumptions.

Re: [RFA] Reverse Debugging, 1/5

[Daniel Jacobowitz]

On Mon, Oct 06, 2008 at 02:17:25PM -0700, Michael Snyder wrote:
> You're going to send "to_set_direction" to the target,
> and then you're going to assume that you know what the
> state is later on, rather than asking the target?  What
> if the target gets it wrong?  Now you're out of sync.

Huh?  No, part of every resume.

> If we do keep the direction-state in the core, where would
> we put it?  In the ecs?  In a global variable in infrun?

In infrun, the same as any other user supplied execution state.
For instance the scheduler-locking setting.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Oct 06, 2008 at 02:17:25PM -0700, Michael Snyder wrote:
>> You're going to send "to_set_direction" to the target,
>> and then you're going to assume that you know what the
>> state is later on, rather than asking the target?  What
>> if the target gets it wrong?  Now you're out of sync.
> 
> Huh?  No, part of every resume.

It's not part of resume.  That was the very first design
decision.  We didn't modify resume, because it would require
modifying every existing target.  Instead, we added a set
execution direction method, which has to be called before
resume (and after, depending).

So right now there are two ways this gets invoked:

1) By a one-shot reverse command, reverse-xxx (where
xxx is step/stepi/next/nexti/continue/finish/until).
In that case, we do:
     target_set_exec_dir (reverse);
     step/next/finish/continue/???
     target_set_exec_dir (forward);
2) By the user command set exec-direction, in which case
we leave it in the user-supplied state until the user
changes it again.

All resumes get the implied direction that was specified
in the last target_set_exec_dir.  Resume doesn't have
an extra parameter, but if you do reverse-step, for
instance, there may be many resumes between the two
calls to target_set_exec_dir.



>> If we do keep the direction-state in the core, where would
>> we put it?  In the ecs?  In a global variable in infrun?
> 
> In infrun, the same as any other user supplied execution state.
> For instance the scheduler-locking setting.

I'm ok with that, if that's what we decide.

But understand -- the target HAS to remember this state,
so now we are duplicating state.  Unles we go back and
reverse that very first design decision and add a parameter
to resume -- which will be a lot of work.

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> >In infrun, the same as any other user supplied execution state.
> >For instance the scheduler-locking setting.
> 
> I'm ok with that, if that's what we decide.
> 
> But understand -- the target HAS to remember this state,
> so now we are duplicating state.  Unles we go back and
> reverse that very first design decision and add a parameter
> to resume -- which will be a lot of work.

I don't think that adding a parameter to resume will be beneficial
at this point.  But I think it would be beneficial to have it in
infrun instead of having the extra target method - the "target"
in GDB's sense of the target layer doesn't have to remember the
direction, since it can query infrun.  What you're saying is that
the actual target (be it some device behind the remote protocol,
or a native target, etc) will also have to remember. But I think
that's fine, and I think it's better to have this really small
duplication rather than taking the chance of sending a packet
over a slow line.  In fact, you already realized this, since you
cached the direction in the remote target layer. ;-).

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Joel Brobecker wrote:
>>> In infrun, the same as any other user supplied execution state.
>>> For instance the scheduler-locking setting.
>> I'm ok with that, if that's what we decide.
>>
>> But understand -- the target HAS to remember this state,
>> so now we are duplicating state.  Unles we go back and
>> reverse that very first design decision and add a parameter
>> to resume -- which will be a lot of work.
> 
> I don't think that adding a parameter to resume will be beneficial
> at this point.  But I think it would be beneficial to have it in
> infrun instead of having the extra target method - the "target"
> in GDB's sense of the target layer doesn't have to remember the
> direction, since it can query infrun.  

All right, I know when I'm licked.  ;-)

Before I actually implement this, let me see if we're all
on the same page (Daniel, Joel, Pedro...)

If I put the "exec_direction" state variable in infrun,
then there's no point in having EITHER target vector
(target_set_execdir or target_get_execdir).  It'll just
be an ordinary gdb "set" command.

But then I ought to add a new target vector, something like
"to_can_go_backward"  (ok, to_can_reverse), because I've
presently overloaded this functionality onto the
target_get/set_execdir methods.

Then, on "set exec-dir reverse", infrun will call
target_can_reverse, and reject accordingly.

The target_ops, such as remote.c, instead of
checking its own local state variable, will call
infrun_get_exec_direction every time target_resume
is called.

Right?

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> Before I actually implement this, let me see if we're all
> on the same page (Daniel, Joel, Pedro...)

My own view was a little simpler: Delete the target_set_execdir method,
and replace all the calls with a reference to the infrun global. I would
keep the target_set_execdir more or less as is; otherwise, you'll need
some kind of observer to notice when the execdir changes. The
"to_can_go_backwards" is an interesting idea, but in my opinion only
makes sense if the target_set_execdir method is removed.  Otherwise,
we can treat target_set_execdir == NULL as cannot-go-backwards.

These are just my opionions, I don't feel strongly about them.

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Joel Brobecker wrote:
>> Before I actually implement this, let me see if we're all
>> on the same page (Daniel, Joel, Pedro...)
> 
> My own view was a little simpler: Delete the target_set_execdir method,

I'm guessing you meant "get" here?

> and replace all the calls with a reference to the infrun global. I would
> keep the target_set_execdir more or less as is; otherwise, you'll need
> some kind of observer to notice when the execdir changes. The
> "to_can_go_backwards" is an interesting idea, but in my opinion only
> makes sense if the target_set_execdir method is removed.  Otherwise,
> we can treat target_set_execdir == NULL as cannot-go-backwards.

OK, so you're saying that "target_set_execdir" will set the
global infrun variable, not a target-defined variable?

I would have thought somebody would object to that on semantic grounds.

Then the only reason to put it into the target vector is so that
we can check for "cannot-go-backwards", and surely someone will
come back and tell me "well that is what you should call it then".

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> >My own view was a little simpler: Delete the target_set_execdir method,
> 
> I'm guessing you meant "get" here?

Ooops! yes, that' right.

> 
> >and replace all the calls with a reference to the infrun global. I would
> >keep the target_set_execdir more or less as is; otherwise, you'll need
> >some kind of observer to notice when the execdir changes. The
> >"to_can_go_backwards" is an interesting idea, but in my opinion only
> >makes sense if the target_set_execdir method is removed.  Otherwise,
> >we can treat target_set_execdir == NULL as cannot-go-backwards.
> 
> OK, so you're saying that "target_set_execdir" will set the
> global infrun variable, not a target-defined variable?

Not quite, actually. Roughly, when the user changes the exec direction,
we call target_set_execdir and, if the call succeeded, then update the
direction known in infrun. Or perhaps you or others have better ideas?

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Joel Brobecker wrote:
>>> My own view was a little simpler: Delete the target_set_execdir method,
>> I'm guessing you meant "get" here?
> 
> Ooops! yes, that' right.
> 
>>> and replace all the calls with a reference to the infrun global. I would
>>> keep the target_set_execdir more or less as is; otherwise, you'll need
>>> some kind of observer to notice when the execdir changes. The
>>> "to_can_go_backwards" is an interesting idea, but in my opinion only
>>> makes sense if the target_set_execdir method is removed.  Otherwise,
>>> we can treat target_set_execdir == NULL as cannot-go-backwards.
>> OK, so you're saying that "target_set_execdir" will set the
>> global infrun variable, not a target-defined variable?
> 
> Not quite, actually. Roughly, when the user changes the exec direction,
> we call target_set_execdir and, if the call succeeded, then update the
> direction known in infrun. Or perhaps you or others have better ideas?

Just, well, then target_set_execdir does not actually
set the exec direction -- instead it sort of tells you
if it's *ok* to set the exec direction -- which is what
I would think of as target_can_reverse.

I'm putting this together, and will have a new patch submission shortly.

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> Just, well, then target_set_execdir does not actually
> set the exec direction -- instead it sort of tells you
> if it's *ok* to set the exec direction -- which is what
> I would think of as target_can_reverse.

I think that the overloading of the word target is making it very
hard to understand each other.

Again, target_set_exec_dir would tell the inferior that, from now on,
the exec direction for all future resumes will be (forward|reverse).
If the target method is unset, then we know the feature is unsupported.
If it is set, and it fails, then refuse the change of direction -
feature not supported either.  Otherwise, update the infrun direction
with the new direction.

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Joel Brobecker wrote:
>> Just, well, then target_set_execdir does not actually
>> set the exec direction -- instead it sort of tells you
>> if it's *ok* to set the exec direction -- which is what
>> I would think of as target_can_reverse.
> 
> I think that the overloading of the word target is making it very
> hard to understand each other.

Well, *something* is, that's for sure.
Might just be early senility on my part...   ;-)

> Again, target_set_exec_dir would tell the inferior that, from now on,
> the exec direction for all future resumes will be (forward|reverse).

But... Oh, I get it.  You're right.  It is the overloading of "target".

Joel, the inferior (in the sense of eg. gdbserver) doesn't have any
memory of the exec direction.  It's stateless.  Each message (eg.
'c' or 'bc') is unaffected by the previous one.

So if we remove the memory of the exec state from the target_ops
layer and transfer it to the infrun layer, then target_set_execdir
will have no semantics at all except "does this work".

> If the target method is unset, then we know the feature is unsupported.
> If it is set, and it fails, then refuse the change of direction -
> feature not supported either.  Otherwise, update the infrun direction
> with the new direction.
> 
> --
> Joel

Re: [RFA] Reverse Debugging, 1/5

[Joel Brobecker]

> So if we remove the memory of the exec state from the target_ops
> layer and transfer it to the infrun layer, then target_set_execdir
> will have no semantics at all except "does this work".

OK, somehow, I thought things would work differently. Thanks for breaking
through my thick skull. I think that a to_can_... method would indeed
be a good solution.

-- 
Joel

Re: [RFA] Reverse Debugging, 1/5

[Daniel Jacobowitz]

On Mon, Oct 06, 2008 at 02:43:42PM -0700, Michael Snyder wrote:
> But understand -- the target HAS to remember this state,
> so now we are duplicating state.  Unles we go back and
> reverse that very first design decision and add a parameter
> to resume -- which will be a lot of work.

What I was trying to say is that on a branch, it's a lot of work.  In
a patch, it's not.  It's a one line change to maybe fifteen files -
more lines if you add errors for unsupported direction, but still
less work than we've spent discussing it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse Debugging, 1/5

[Pedro Alves]

On Monday 06 October 2008 22:00:37, Michael Snyder wrote:

Certainly you've been through this much more than all of us,
so, I'm just going to give a knee-jerk like reaction to this.
It's not really a profoundly thought about opinion, so wear
sunglasses...

> Making no assumptions about HOW the target sets the direction,
> it seems likely that at least *some* targets will have to
> remember this state locally.  Whereas there is no reason
> for core-gdb to have to remember the state locally, if it can
> always get it from the target.

If we consider the packets you're introducing to the remote protocol,
it looks a bit that these abstractions colide.  That is,
if you have a way to set the direction on the target, then you
wouldn't need a special step-backwards packet.  You'd just
pass down the direction and issue a normal step...

A per-target property may seems to make sense on
single-threaded,single-inferior targets, but when you add support
for multi-inferiors per target (e.g., extended-remote has some of it now,
and I'm going to push more of it), or multi-threaded support, the
per-target setting may not make sense anymore --- explicit requests
at the target resume interface (just like your new packets) may make
more sense.  Imagine forward execution non-stop debugging in all threads
but one, which the user is examining in reverse.  What's the target
direction in this case?

> It seems a worse duplication to keep the same state information
> simultaneously in the target and in the core, since now you
> have to worry about them getting out of sync.
> 
> At worst, a target will need to maintain an int's worth of state
> locally, and so long as we're never running two targets at the
> same time, there's no synchronization issue.
> 

The question to me is --- when/why does the target (as in, the debug
API abstraction) ever need to know about the current direction that
it couldn't get from the core's request?

-- 
Pedro Alves

Re: [RFA] Reverse Debugging, 1/5

[Michael Snyder]

Pedro Alves wrote:
> On Monday 06 October 2008 22:00:37, Michael Snyder wrote:
> 
> Certainly you've been through this much more than all of us,
> so, I'm just going to give a knee-jerk like reaction to this.
> It's not really a profoundly thought about opinion, so wear
> sunglasses...

"With Reverse Debugging, the future's so bright,
ya gotta wear shades!"    ;-)

>> Making no assumptions about HOW the target sets the direction,
>> it seems likely that at least *some* targets will have to
>> remember this state locally.  Whereas there is no reason
>> for core-gdb to have to remember the state locally, if it can
>> always get it from the target.
> 
> If we consider the packets you're introducing to the remote protocol,
> it looks a bit that these abstractions colide.  That is,
> if you have a way to set the direction on the target, then you
> wouldn't need a special step-backwards packet.  You'd just
> pass down the direction and issue a normal step...

Sorry, verbal ambiguity on my part.

Where I've been saying "target" in this thread, I
have been meaning "the target_ops implementation",
eg. (but not limited to) remote.c

I believe you're thinking "target" as in for instance
"gdbserver" or VirtuTech/Simics simulator.

Two different API's.

The API for the target_ops target is
   target_set_exec_dir
   target_resume
   target_wait

This has an explicit persistant state -- the exec_dir.

The API for remote targets is 'c', 'bc', 's', 'bs'.
In this, there is no persistant state.

But it's important to remember that GDB proper does not
know anything about the remote protocol API.  Only
one or more target_ops implementations know about that,
and there is no particular reason for the two APIs to
resemble each other.  The API for (eg.) a native target
is likely to be completely different.


> A per-target property may seems to make sense on
> single-threaded,single-inferior targets, but when you add support
> for multi-inferiors per target (e.g., extended-remote has some of it now,
> and I'm going to push more of it), or multi-threaded support, the
> per-target setting may not make sense anymore --- explicit requests
> at the target resume interface (just like your new packets) may make
> more sense.  Imagine forward execution non-stop debugging in all threads
> but one, which the user is examining in reverse.  What's the target
> direction in this case?

Yakkk!!!

OK, I don't have a story for multi-thread reverse yet,
at all, and I certainly don't have one for multi-thread
in which some threads are running forward and others
are running in reverse!

Nor do I have a story for multi-process or multi-target.

Planning ahead is great, but at some point, you have to
say "that's for a later patch".


>> It seems a worse duplication to keep the same state information
>> simultaneously in the target and in the core, since now you
>> have to worry about them getting out of sync.
>>
>> At worst, a target will need to maintain an int's worth of state
>> locally, and so long as we're never running two targets at the
>> same time, there's no synchronization issue.
>>
> 
> The question to me is --- when/why does the target (as in, the debug
> API abstraction) ever need to know about the current direction that
> it couldn't get from the core's request?

At this interface layer, the core's requests look like:

   target_set_exec_dir
   target_resume
   target_wait
   [repeat]
   target_set_exec_dir

So there may be many resume/wait calls between calls to set_exec_dir.
This means that the target_ops module MUST remember the state, whether
or not the core remembers it also.

Re: [RFA] Reverse Debugging, 1/5

[Pedro Alves]

On Monday 06 October 2008 23:11:14, Michael Snyder wrote:

> Where I've been saying "target" in this thread, I
> have been meaning "the target_ops implementation",
> eg. (but not limited to) remote.c

"target" is such an overloaded word in GDB lingo ...

> > The question to me is --- when/why does the target (as in, the debug
> > API abstraction) ever need to know about the current direction that
> > it couldn't get from the core's request?
> 

> At this interface layer, the core's requests look like:
> 
>    target_set_exec_dir
>    target_resume
>    target_wait
>    [repeat]
>    target_set_exec_dir
> 
> So there may be many resume/wait calls between calls to set_exec_dir.
> This means that the target_ops module MUST remember the state, whether
> or not the core remembers it also.

That's all nice and pretty, for sync.  For async, you'll
have:

<event loop>
    command
      target_set_exec_dir
      target_resume
<event loop>
    target_wait
    target_resume
<event loop>
    target_wait
    target_resume
    target_set_exec_dir
<event loop>

At some point, if you support more than one inferior behind the
target_ops interface, you'll start asking yourself "why didn't I
make the interfaces fully complete and go rely on global state?"

Or even, in the single-inferior case:

<event loop>
    command
      target_set_exec_dir
      target_resume
<event loop>
    target_wait
    target_resume
    target_set_exec_dir
<event loop>
    target_wait
    handle_inferior_event
      error >>>>>>>>>> what's the target execution
            >>>>>>>>>> state after this, where was it stored?
<event loop>

But it's OK, we can always come back to this later, because
you're making the remote protocol stateless, which is good
enough for me for now.  ;-)

-- 
Pedro Alves