Re: [PATCH v9 29/29] record-btrace: add (reverse-)stepping support

[Pedro Alves <palves at redhat dot com>]

On 12/20/2013 02:47 PM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Friday, December 20, 2013 3:37 PM
>> To: Pedro Alves
> 
> 
>>> This really shouldn't be necessary, given target_resume does
>>> it for you.  If you still needed, you're papering over some
>>> problem.
>>
>> If we start replaying in to_wait, we'll call get_current_frame
>> to fix up some stepping related frames.  This will be done on
>> the current PC.
>>
>> When we step later on in record_btrace_step_thread, we change
>> the replay position but not the PC.
>>
>> I guess it will be more clear when I move this into
>> record_btrace_step_thread and change the comment.
> 
> I moved it to record_btrace_start_replaying:

Hmm.

> 
> @@ -1406,15 +1405,15 @@ record_btrace_start_replaying (struct thread_info *tp)
>    /* Restore the previous execution state.  */
>    set_executing (tp->ptid, executing);
>  
> +  /* Invalidate registers again.  If this is called on the to_wait path,
> +     we expect registers still invalid from to_resume.  */
> +  registers_changed_ptid (tp->ptid);
> +

The registers haven't really changed at this point.
It seems like it that may help because it just happens that today,
nothing is currently reading registers before you move the
thread to the next position.  If something does that, you'll get
stale results again.  It'd be safer to do this right after
moving the thread to the next trace position.  So with that
in mind, either indeed put it in record_btrace_step_thread or
just leave it at the place the original code had it already,
but update the comment to sound more determined, something like:

  /* We just moved the thread to a different trace position.
     If anything fetched the thread's registers before, its
     regcache is stale now.  */
  registers_changed_ptid (tp->ptid);

Thanks,
-- 
Pedro Alves