Re: [reverse] PATCH: Several interface changes

[teawater <teawater@gmail.com>]

I think the idea of this patch is good.
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.

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
>