On Monday 06 October 2008 23:11:14, Michael Snyder wrote:
> Pedro Alves wrote:
> > 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!!!

:-)  Here's an alternative interface I was considering and envisioning
when I mentioned the above.  Consider this just a suggestion.  If it
looks bad, let's quickly forget about it.

> > 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?

So, after last night's discussion, I came up with the attached to
see how it would look like.  The major change is that I consider the
reverse/forward-direction thing a property or the command the user
requested, and as such, belongs together with the other thread
stepping state we keep in struct thread_info, and the
target_ops implementation, adjusts itself to the direction GDB
requests with target_resume.  I've extended target_resume's interface
to accept this instead of a `step' boolean:

 enum target_resume_kind
   {
     /* Continue forward.  */
     rk_continue,

     /* Step forward.  */
     rk_step,

     /* Continue in the reverse direction.  */
     rk_continue_reverse,

     /* Step in the reverse direction.  */
     rk_step_reverse,
   };

(notice that the order of the things in the enum allows me to
miss some conversions --- I'm lazy).

  I can't say if I like this new target_resume interface or
not.  I just tried doing it to see how it would look.

(I can imagine that we're in the future going to extend the
target_resume interface to be more like gdbserver's, but, well,
that's another issue.)

So, the interface at the command level implementation is just
like it was before:

 1)  call clear_proceed_status ();

 2)  /* construct the step/continue request */

 3)  call proceed (...);

Where in #2, you can set the thread to go backwards by
doing:

     thread->reverse = 1;

The attached patch applies against the reverse-20080930-branch.

Other things I've done in the patch:

   * Added support for a "Tnn nohistory" stop reply that translates
    to TARGET_WAITKIND_NO_HISTORY.  When going multi-threaded, or
    multi-process this will allow things like "T05;thread:pPID.TID;nohistory"
    for free.  I absolutelly didn't test this.  I've no reverse aware target
    at hand.

   * At places, error out if async + reverse or non-stop + reverse
     was requested.

   * Added a target_can_reverse_p method, so infcmd.c can check if the
     target supports reverse execution before calling into the target.  Not
     strictly necessary, though, but I thought this was nicer this way.

I checked that I can use the record target on x86 (actually x86_64
with -m32) as good as without the patch, but it's quite possible I
broke things badly.

-- 
Pedro Alves