RE: [PATCH 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Thanks for the review, Pedro.

>> That step-over failed after actually completing the step.  This is wrong.
>
>These last two sentences confused me, it is not clear to me what you're
>saying is wrong.  I.e., what exactly does "fail" mean here?  And what exactly
>is wrong?

It is wrong that we complete the step and then fail.  The step itself should
have failed and we should not have completed the step.  I rephrased the
commit message.


>> This exposes another issue, however.  When completing a step-over at the
>> end of the execution history, we implicitly stop replaying that thread.
>
>OOC, where does that happen (implicitly stop replaying)?

In record_btrace_stop_replaying_at_end().


>> A
>> continue command would resume after the step-over and, since we're no
>> longer replaying, would continue recording.
>>
>
>This approach relies on the reverse debugging / replaying being handled by GDB.
>
>I mean, remote targets that support reverse debugging themselves, via the "bc",
>"bs"
>packets (backwards continue/step), and "replaylog" stop reply
>(TARGET_WAITKIND_NO_HISTORY)
>are out of luck since the remote target has no way to implement
>target_record_is_replaying.

Why is that?  The functionality solely depends on the replay state of threads
inside that target.

There's also target_record_will_replay() that takes the stepping command's
execution direction into account.


>Thus I'm wondering whether target_record_is_replaying() existing is a design
>mistake, and
>I wonder -- should it be the target that is guaranteed to reply with no-history
>before
>implicitly stopping replay?  IOW, don't stop replay until a no-history event is
>returned.

We need some mechanism to switch between replay mode and record mode.
E.g. if the current thread is not replaying but other threads are, in all-stop mode
we need to either a) abort the mover command or b) silently stop replaying
other threads.

We could move the decision and hence the code down into the target such
that the move request to the target would fail or silently stop replaying others.

Record-btrace as well as record-full, however, implement replay in GDB and
not in the target.  This keeps gdbserver more light-weight as we wouldn't
need to decode the trace on the target itself, nor store a huge amount of
trace data.  It also saves a lot of round-trips.  I wouldn't change that design.

I don't see, however, why this shouldn't be able to co-exist with record/replay
being implemented on the target side.


> IOW, don't stop replay until a no-history event is returned.

I implicitly stop replaying to make 'rsi, si, cont' work.  Otherwise, the user
would either need to repeatedly continue until he gets no-history or do
'record goto end'.

I also implicitly stop replaying other threads to not have the user iterate
over all threads and do 'record goto end' to stop replaying each thread
before he can resume.


>> Fix that by recording the replay state in the thread's control state and
>> failing with no-history in keep_going if we're switching from replay to
>> recording.
>
>This affects target record just as well, right?  Would it be possible for
>the testcases to be made to test that target as well?

We kept tests separate, so far, since record btrace doesn't support data
and record full doesn't support multi-threading.


>> @@ -7813,6 +7815,26 @@ keep_going_pass_signal (struct
>execution_control_state *ecs)
>>    gdb_assert (ecs->event_thread->ptid == inferior_ptid);
>>    gdb_assert (!ecs->event_thread->resumed);
>>
>> +  /* When a thread reaches the end of its execution history, it automatically
>> +     stops replaying.  This is so the user doesn't need to explicitly stop it
>> +     with a separate command.
>> +
>> +     We do not want a single command (e.g. continue) to transition from
>> +     replaying to recording, though, e.g. when starting from a breakpoint we
>> +     needed to step over at the end of the trace.  When we reach the end of the
>> +     execution history during stepping, stop with no-history.
>> +
>> +     The other direction is fine.  When we're at the end of the execution
>> +     history, we may reverse-continue to start replaying.  */
>> +  if (ecs->event_thread->control.is_replaying
>> +      && !target_record_is_replaying (ecs->event_thread->ptid))
>> +    {
>> +      gdb::observers::no_history.notify ();
>> +      stop_waiting (ecs);
>> +      normal_stop ();
>
>normal_stop looks at the last target status to decide what to do.  So here we
>should call
>set_last_target_status before calling normal_stop, I'd think.

I'm doing that in patch 3.  I moved the code into this patch.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928