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

[Pedro Alves <pedro@palves.net>]

Hi Markus,

Sorry for the delay.  FWIW, I've tried to review this a number of times, but I keep
getting stuck on not understanding the description fully.  I wonder if it's possible
to reword it to make it clearer.

On 16/03/21 09:34, Markus Metzger via Gdb-patches wrote:
> When trying to step over a breakopint at the end of the trace, the

typo: "breakopint"

> step-over will fail with no-history.  This does not clear step_over_info
> so a subsequent resume will cause GDB to not resume the thread and expect
> a SIGTRAP to complete the step-over.  This will never come causing GDB to
> hang in the wait-for-event poll.
> 
> 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?

Debugging the testcases a bit, I think I see what you mean.  It's:

 That step-over actually completed the step, and so should not have
 returned no-history, but instead a normal single-step stop.  

Correct?

> Fix it by moving the end of execution history check to before we are
> stepping.
> 
> 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)?

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

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.

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

> 
> gdb/ChangeLog:
> 2021-01-14  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdbthread.h (struct thread_control_state) <is_replaying>: New.
> 	* infrun.c (clear_proceed_status_thread): Set
> 	thread_control_state.is_replaying.
> 	(keep_going_pass_signal): Check thread_control_state.is_replaying.
> 	* record-btrace.c (record_btrace_single_step_forward): Move end of
> 	execution history check.
> 
> gdb/testsuite/ChangeLog:
> 2021-01-13  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdb.btrace/cont-hang.exp: New file.
> 	* gdb.btrace/step-hang.exp: New file.
> 	* gdb.btrace/stepn.exp: New file.
> ---
>  gdb/gdbthread.h                        |  3 ++
>  gdb/infrun.c                           | 22 ++++++++++
>  gdb/record-btrace.c                    | 19 ++++-----
>  gdb/testsuite/gdb.btrace/cont-hang.exp | 47 +++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/step-hang.exp | 46 +++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/stepn.exp     | 56 ++++++++++++++++++++++++++
>  6 files changed, 184 insertions(+), 9 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index eef37f79e6a..563b7bd8954 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -159,6 +159,9 @@ struct thread_control_state
>       command.  This is used to decide whether "set scheduler-locking
>       step" behaves like "on" or "off".  */
>    int stepping_command = 0;
> +
> +  /* Whether the thread was replaying when the command was issued.  */
> +  bool is_replaying = false;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a271220b261..591fda93d21 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2658,6 +2658,8 @@ clear_proceed_status_thread (struct thread_info *tp)
>  
>    /* Discard any remaining commands or status from previous stop.  */
>    bpstat_clear (&tp->control.stop_bpstat);
> +
> +  tp->control.is_replaying = target_record_is_replaying (tp->ptid);
>  }
>  
>  void
> @@ -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.

> +      return;
> +    }
> +