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-08-30 4:03 p.m., Andrew Burgess wrote:
> This commit was inspired by this comment from Simon:
> 
>   https://sourceware.org/pipermail/gdb-patches/2021-July/180694.html
> 
> The comment is about two thread_info flags m_executing and m_resumed.
> The m_resumed flag indicates that at the infrun level GDB has set the
> thread running, while the m_executing flag indicates that the thread
> is actually running at the target level.
> 
> It is very common that a thread can be m_resumed, but not m_executing,
> that is, core GDB thinks the thread is, or should be, running, but at
> the target level the thread isn't currently running.
> 
> The comment Simon made was in reply to a patch I posted where I found
> a case where a thread was marked m_executing, but not m_resumed, that
> is, at the infrun level GDB thought the thread was stopped, but at the
> target level the thread was actually running.  Simon suggests that
> this feels like an invalid state, and after thinking about it, I
> agree.
> 
> So, this commit adds some new asserts to GDB.  The core idea here is
> that the resumed and executing flags should only change in the
> following pattern, to begin with everything is set false:
> 
>   Resumed: false
>   Executing: false
> 
> Then infrun marks the thread resumed:
> 
>   Resumed: true
>   Executing: false
> 
> Then a target starts the thread executing:
> 
>   Resumed: true
>   Executing: true
> 
> The thread stops, the target spots this and marks the thread no longer
> executing:
> 
>   Resumed: true
>   Executing: false
> 
> And finally, infrun acknowledges that the thread is now no longer
> resumed:
> 
>   Resumed: false
>   Executing: false
> 
> Notice that at no point is resumed false, while executing is true.
> 
> And so we can add some asserts:
> 
>  1. The resumed flag should only change state when the executing flag
>  is false, and
> 
>  2. The executing flag should only change state when the resumed flag
>  is true.
> 
> I added these asserts and ....
> 
> .... it turns out these rules are broken all over the place in GDB, we
> have problems like:
> 
>  (a) When new threads appear they are marked executing, but not
>  resumed, and
> 
>  (b) In some places we mark a single thread as resumed, but then
>  actually set multiple threads executing.
> 
> For (a) it could be argued that this is a legitimate state - this is
> actually the problem I addressed in the original patch that Simon was
> replying too, however, we don't need to support this as a separate
> state, so if we can make this case follow the expected set of state
> transitions then it allows us to reduce the number of states that GDB
> can be in, which I think is a good thing.
> 
> Case (b) seems to just be a bug to me.

Did you consider collapsing the two fields (resumed and executing) into
a single one, with three states?  The names are silly, but as an
example:

enum class thread_resumption_state
{
  NOT_RESUMED,
  RESUMED_BUT_NOT_TARGET_RESUMED,
  RESUMED_AND_TARGET_RESUMED,
};

I think that would end up simpler than two booleans with rules.  But
since it would probably be a pretty big change, I will accept "nope" as
an answer.

> 
> The interesting changes in this commit then are:
> 
>   * gdbthread.h (set_stop_pc): Add assert that the stop_pc can only be
>   set when a thread is neither resumed, or executing.
> 
>   * infcmd.c (post_create_inferior): Ensure thread is non-resumed
>   before clearing the stop_pc.

Should clear_stop_pc have the same assertions as set_stop_pc?  I
guess you tried that and it didn't work?

> @@ -5616,6 +5668,9 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	 execd thread for that case (this is a nop otherwise).  */
>        ecs->event_thread = inferior_thread ();
>  
> +      /* If we did select a new thread, make sure its non-resumed.  */
> +      ecs->event_thread->set_resumed (false);
> +

This one is in the TARGET_WAITKIND_EXECD handling... I'm wondering if it
should be target_ops::follow_exec's contract that any new thread of the
new inferior should be not resumed.  Because follow_exec is always
called in a context where the event thread on entry is not resumed.  So
it's not like new threads that are added in target_ops::wait, for
example.  For those, it makes sense to be added as resumed.

I think it would just mean adding a set_resumed (false) call in
process_stratum_target::follow_exec.  And possibly some assertions in
the follow_exec free function.  For example, I would add to the existing
assertions:

  /* If a new inferior was created, the target must have added at least
     one thread.  If the same inferior was used, we have left one
     thread.  */
  gdb_assert (!inf->thread_list.empty ());

  /* Any added thread must have been added in the not resumed state.  */
  for (thread_info *thread : inf->threads ())
    gdb_assert (!thread->resumed ());

PS: if taking into consideration my comment about the observer seeing
the correct resumed state, in add_thread_silent (below), then perhaps we
would need add_thread_silent to take a "resumed" parameter, so it can
set the right resumed state before calling the observer.

> diff --git a/gdb/thread.c b/gdb/thread.c
> index 10c3dcd6991..aceb233be80 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -262,6 +262,13 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid)
>    tp = new_thread (inf, ptid);
>    gdb::observers::new_thread.notify (tp);
>  
> +  /* All threads are created in a resumed state, that is as soon as GDB
> +     sees a new thread it is expected to be running as far as the core of
> +     GDB is concerned.  At a target level the thread is probably stopped
> +     right now, hence the executing flag is left initialized to false.  */
> +  tp->set_resumed (true);
> +  gdb_assert (!tp->executing ());

I don't know if it matters, but should resumed be set before calling the
new_thread observer?  So that observers see the thread in the state we
intend it to be in?

Simon