Re: [reverse] PATCH: Several interface changes

[Michael Snyder <msnyder@vmware.com>]

teawater wrote:
> 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?

No multi-thread support, yet.

Pedro, future enhancement?

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