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

[Michael Snyder <msnyder@redhat.com>]

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).