Re: [reverse] PATCH: Several interface changes

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 08 October 2008 08:28:05, teawater wrote:
> I think the idea of this patch is good.

Thanks for taking a look!

> But maybe process record still not need it now because p record still
> not support multi-thread. 
> Of course, p record will need it in the feature.

The basic idea and motivation of the patch was to show that we
don't need a target_set_execution_direction method in the
target ops, or have the target_ops implementation query an infrun
global --- the request to go backwards is a property of
the command the user requested.  Much like a single-step range
property is a per-command/per-thread property.  I believe that
a single per-target_ops setting isn't quite the right abstraction.

If a remote stub doesn't need a special indication that
'all operations from now on apply to reverse debugging', and can
sort itself out from the stateless resume requests, a native
target also shouldn't need it.  If we find that a native target
reverse debugging implementation *does require* a
"set_execution_direction" command to work propertly and can't
get it from the resume interface request, then the remote protocol
may prove itself insuficient.  E.g., if "target record" needs it,
then implementing target record in gdbserver would be impossible
with the current remote protocol --- there's always extensions
and new packets, of course.

As I said, this is just a thought, a proof-of-concept, only
for consideration.  It's *not* meant to hold reverse
debugging from going into mainline.  I'm more worried with
the remote protocol being done right (and I manifested my
opinion about that already), than about this, which
is all internal implementation detail that we can change
at any time.

> 
> Michael, how about your target?
> 
> 
> On Wed, Oct 8, 2008 at 00:09, Pedro Alves <pedro@codesourcery.com> wrote:
> > 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