gdb: make the remote target track its own thread resume state
authorSimon Marchi <simon.marchi@efficios.com>
Thu, 14 Jan 2021 01:20:43 +0000 (20:20 -0500)
committerSimon Marchi <simon.marchi@polymtl.ca>
Thu, 14 Jan 2021 01:20:43 +0000 (20:20 -0500)
commitc9d220893e9c3128ca78e613a532d5cc6998c332
tree5c80e4e49f23781d576a417f0218b2c6bbd5bc48
parent3eeabe12c3b1550f2341ea114e939e7122109ce5
gdb: make the remote target track its own thread resume state

The next patch moves the target commit_resume method to be a
process_stratum_target-only method.  The only non-process targets that
currently implement the commit_resume method are the btrace and full
record targets.  The only reason they need to do so is to prevent a
commit resume from reaching the beneath (process) target if they are
currently replaying.

This is important if a record target is used on top of the remote target
(the only process target implementing the commit_resume method).
Currently, the remote target checks the `thread_info::executing` flag of
a thread to know if it should commit resume that thread:

    if (!tp->executing || remote_thr->vcont_resumed)
      continue;

The `tp->executing` flag is set by infrun when it has asked the target
stack to resume the thread, and therefore if the thread is executing,
from its point of view.  It _not_ equivalent to whether the remote
target was asked to resume this thread.

Indeed, if infrun asks the target stack to resume some thread while the
record target is replaying, the record target won't forward the resume
request the remote target beneath, because we don't actually want to
resume the thread on the execution target.  But the `tp->executing` flag
is still set, because from the point of view of infrun, the thread
executes.  So, if the commit_resume call wasn't intercepted by the
record target as it is today and did reach the remote target, the remote
target would say "Oh, this thread should be executing and I haven't
vCont-resumed it!  I must vCont-resume it!".  But that would be wrong,
because it was never asked to resume this thread, the resume request did
not reach it.  This is why the record targets currently need to
implement commit_resume: to prevent the beneath target from
commit_resuming threads it wasn't asked to resume.

Since commit_resume will become a method on process_stratum_target in
the following patch, record targets won't have a chance to intercept the
calls and that would result in the remote target commit_resuming threads
it shouldn't.  To avoid this, this patch makes the remote target track
its own thread resumption state.  That means, tracking which threads it
was asked to resume via target_ops::resume.  Regardless of the context
of this patch, I think this change makes it easier to understand how
resume / commit_resume works in the remote target.  It makes the target
more self-contained, as it only depends on what it gets asked to do via
the target methods, and not on tp->executing, which is a flag maintained
from the point of view of infrun.

I initially made it so this state was only used when the remote target
operates in non-stop mode, since commit_resume is only used when the
target is non-stop.  However, it's more consistent and it can be useful
to maintain this state even in all-stop too.  In all-stop, receiving a
stop notification for one thread means all threads of the target are
considered stopped.

From the point of view of the remote target, there are three states a
thread can be in:

 1. not resumed
 2. resumed but pending vCont-resume
 3. resumed

State 2 only exists when the target is non-stop.

As of this patch, valid state transitions are:

 - 1 -> 2 (through the target resume method if in non-stop)
 - 2 -> 3 (through the target commit_resume method if in non-stop)
 - 1 -> 3 (through the target resume method if in all-stop)
 - 3 -> 1 (through a remote stop notification / reporting an event to the
   event loop)

A subsequent patch will make it possible to go from 2 to 1, in case
infrun asks to stop a thread that was resumed but not commit-resumed
yet.  I don't think it can happen as of now.

In terms of code, this patch replaces the vcont_resumed field with an
enumeration that explicitly represents the three states described above.
The last_resume_sig and last_resume_step fields are moved to a structure
which is clearly identified as only used when the thread is in the
"resumed but pending vCont-resume" state.

gdb/ChangeLog:

* remote.c (enum class resume_state): New.
(struct resumed_pending_vcont_info): New.
(struct remote_thread_info) <resume_state, set_not_resumed,
set_resumed_pending_vcont, resumed_pending_vcont_info,
set_resumed, m_resume_state, m_resumed_pending_vcont_info>:
New.
<last_resume_step, last_resume_sig, vcont_resumed>: Remove.
(remote_target::remote_add_thread): Adjust.
(remote_target::process_initial_stop_replies): Adjust.
(remote_target::resume): Adjust.
(remote_target::commit_resume): Rely on state in
remote_thread_info and not on tp->executing.
(remote_target::process_stop_reply): Adjust.

Change-Id: I10480919ccb4552faa62575e447a36dbe7c2d523
gdb/ChangeLog
gdb/remote.c