[PATCH 1/4] gdb: make remote target track resumed but no vCont-resumed threads

[Simon Marchi]

On 2020-12-23 7:09 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-12-23 17:40:25 -0500]:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> 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 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.
>>
>> 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 and already vCont-resumed
>>
>> As of this patch, valid state transitions are:
>>
>>  - 1 -> 2 (through the target resume method)
>>  - 2 -> 3 (through the target commit_resume method)
>>  - 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, but 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.
> 
> If I understand this patch correctly, this new state tracking only
> applies for the commit_resume use case, which is only used in a
> specific set of modes?
> 
> I ask because of this patch:
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-December/174277.html
> 
> in which I propose adding a new check of the thread->executing flag.
> 
> If I understand this correctly then I don't think the new state
> tracking would actually help my case, right?

Indeed, this new state tracking only applies when the remote target
operates in non-stop mode.

The tp->executing check you add is probably fine, because in practice,
if you have a record target on top of the remote target, I don't think
you can have some threads being replayed and some threads executing
for real.  And if you receive a stop reply, it's because you had
something executing for real.  So you won't fall into the case where
tp->executing is true, but the thread isn't resumed on the remote target.

However, it's perhaps confusing that the remote target uses this new
state tracking, but only in non-stop mode.  And it would be confusing if
in some places, to check whether a thread is running, we check
tp->executing, and in other places we check remote_thread->resumed_state.
It would be much nicer if the remote target could always trust
remote_thread->resumed_state.

So perhaps the thread's resume state should be tracked by the remote
target in all modes.  The enum would be:

enum class resume_state
{
  /* Not resumed - we haven't been asked to resume this thread.  */
  NOT_RESUMED,

  /* We have been asked to resume this thread, but haven't sent a vCont action
     for it yet.  We'll need to consider it next time commit_resume is
     called.  This state is only used when the remote target is in non-stop
     mode.  */
  RESUMED_PENDING_VCONT,

  /* Fully resumed.  */
  RESUMED,
};

In all-stop, threads would go from NOT_RESUMED to RESUMED directly.  In
non-stop, they would go through the intermediary RESUMED_PENDING_VCONT
state.  With this, your patch could check whether
remote_thread->resume_state is resume_state::RESUMED.

I haven't read the explanation in your patch completely though, so take
that with a grain of salt.

> 
> Also I spotted a typo, see below.
> 
> Thanks,
> Andrew
> 
>>
>> gdb/ChangeLog:
>>
>>         * remote.c (enum class resume_state): New.
>>         (struct pending_vcont_resume_info): New.
>>         (struct remote_thread_info) <resume_state, set_not_resumed,
>> 	set_pending_vcont_resume, pending_vcont_resume_info,
>> 	set_vcont_resumed, m_resume_state, m_pending_vcont_resume_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/remote.c | 151 +++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 115 insertions(+), 36 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 71f814efb365..1b5e5f5727b3 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -1050,6 +1050,38 @@ static struct cmd_list_element *remote_show_cmdlist;
>>  
>>  static bool use_range_stepping = true;
>>  
>> +/* From the remote target's point of view, each thread is in one of these three
>> +   states.  */
>> +enum class resume_state
>> +{
>> +  /* Not resumed - we haven't been asked to resume this thread.  */
>> +  NOT_RESUMED,
>> +
>> +  /* We have been asked to resume this thread, but haven't sent a vCont action
>> +     for it yet.  We'll need to consider it next time commit_resume is
>> +     called.  */
>> +  PENDING_VCONT_RESUME,
>> +
>> +  /* We have been asked to resume this thread, and we have sent a vCont action
>> +     for it.  */
>> +  VCONT_RESUMED,
>> +};
>> +
>> +/* Information about a thread's pending vCont-resume.  Used when a thread is in
>> +   the remote_resume_state::PENDING_VCONT_RESUME state.  remote_target::resume
>> +   stores this information which is them picked up by
>> +   remote_target::commit_resume to know which is the propert action for this
> 
> Typo 'propert' ?

Fixed to "proper", thanks.

Simon