Re: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-09-29 4:09 a.m., Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2021-09-23 13:14:33 -0400]:
>
>>> I did think about this, but I guess I'm not quite sure how this ends
>>> up being any simpler than the two booleans with rules.
>>>
>>> My assumption is that we'd want to keep the API as
>>> executing/set_executing and resumed/set_resumed as this represents the
>>> questions we want to ask.
>>>
>>> As such something like set_executing would become:
>>>
>>>   void
>>>   thread_info::set_executing (bool e)
>>>   {
>>>     if (m_state == RESUMED_AND_TARGET_RESUMED)
>>>       return;
>>>
>>>     gdb_assert (m_state == RESUMED_BUT_NOT_TARGET_RESUMED);
>>>
>>>     m_state = RESUMED_AND_TARGET_RESUMED;
>>>   }
>>>
>>> Which doesn't feel any less complex to me.
>>>
>>> Of course, we could change the API so the user sets the state
>>> directly, like:
>>>
>>>   void
>>>   thread_info_::set_executing_resumed_state (enum state_type s)
>>>   {
>>>     /* ... */
>>>   }
>>>
>>> But then we'd _still_ end up having to assert the correct state
>>> transitions.
>>
>> The later is what I was thinking.
>>
>> What do you mean "assert the correct state transitions"?  With the enum
>> all possible transitions are valid.  I'm not sure if
>> RESUMED_AND_TARGET_RESUMED -> RESUMED_BUT_NOT_TARGET_RESUMED is actually
>> used, but it's not something prevented by your patch either.
>
> The RESUMED_AND_TARGET_RESUMED -> RESUMED_BUT_NOT_TARGET_RESUMED
> transition is definitely used, and is, I believe required with my
> patch.
>
> I don't know if there are many places in GDB that we actually sit in
> the RESUMED_BUT_NOT_TARGET_RESUMED state for long (that would
> currently be resumed=true,executing=false, right?) but my intention is
> that we have to go back through that state, i.e. we can't set resumed
> to false while executing is true.

The most prominent case I know of is when resuming threads that have a
pending status (an event that was pulled out of the target, but not
consumed, instead saved in the thread_info structure).  When resuming
such a thread, infrun just pretends to resume it: it marks it as
resumed=true, but does not call target_ops::resume on it, and therefore
leaves it at executing=false (this is where I think the terminology is
confusing).  We will go back to the event loop, and infrun will go "oh!
a thread just stopped!" and consume that event.  That will make the
thread go back to resumed=false,executing=false.

> From you original proposed states:
>
>   enum class thread_resumption_state
>   {
>     NOT_RESUMED,			/* 1 */
>     RESUMED_BUT_NOT_TARGET_RESUMED,	/* 2 */
>     RESUMED_AND_TARGET_RESUMED,		/* 3 */
>   };
>
> Under the current scheme the transition order should always be:
>
>   1 -> 2 -> 3 -> 2 -> 1
>
> Anything else should trigger an assertion.

I think that would be fine, but I don't see the value.  The 3 -> 1
transition happens when target_ops::wait returns an event for a
thread.  The thread is no longer executing on the target, and no longer
resumed in the eyes of infrun.  So we could go 3 -> 2 -> 1 but that just
seems like an extra step.

> Ideally, if I transitioned to the new scheme I'd (at least initially)
> want to keep assertions that force that transition scheme, so no
> transitions 1->3 or 3->1.
>
> However, there are places where set_resumed and set_executing are
> right next to each other.
>
> So maybe they would be merged into a single set_state call, which
> means the 1->3 or 3->1 transitions would be legal.  So, then we remove
> the asserts and allow any state transitions.
>
> But then, if we have a bug which GDB skips the logic that used to be
> responsible for "set_resumed" we'd never know, right?  When we hit the
> old set_executing call, this will now just set the state as if the
> resumed had happened...
>
> ...this seems less good to me.

If resumed and executing are kept as separate states, then yeah the
transition needs to go through 2, as it's not possible to go from
resumed=true,executing=true to resume=false,executing=false atomically.

> I guess given you've said you're not against what I have then for now
> I'd prefer to stick with the separate resumed/executing.  We can
> always change later - I think it would be best done in a separate
> patch anyway - I just want to try and explain my thinking.

Yeah, that's fine with me.  Of course, I'd like if Pedro could take a
look, but he has a gazillion things to do.

And, food for future thoughts / changes, while reading about the 3 -> 2
-> 1 vs 3 -> 1 transition, I thought: it makes sense for infrun to
manage the resumed flag, because it's the "owner" of that flag.  That
flag means whether infrun considers this thread is running (regardless
of whether this thread is actually executing on target or infrun just
pretends it is running, for its own internal pending status logic).  But
the executing flag means "has the target been asked to resume execution
for this thread and hasn't yet reported an event implying that this
thread's execution on the target has halted".  So it seems to me that
the management of the executing flag could be done entirely in the
target_ops API frontier, in target_resume and target_wait.

target_resume already sets executing=true, which makes sense: we ask the
target to resume execution of the thread.  I think that the mirror
operation could be done in target_wait.  Let's say a target reports a
TARGET_WAITKIND_STOPPED for a thread, target_wait would mark this thread
as executing=false, because the target has reported an event that
implies that execution of that thread has stopped.  Basically, I'm
saying that the job of mark_non_executing_threads should maybe be done
by target_wait, except the set_resumed call at the end (that is still
infrun's job).  That would avoid the possibility of pulling an event out
of the target and forgetting to set executing=false, leading to problems
later.  And if doing so, the transition would always naturally be 3 -> 2
-> 1, because 3 -> 2 would be done by the target API, and later 2 ->
1 would be done by infrun.

Simon