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

[Andrew Burgess <andrew.burgess@embecosm.com>]

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

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.

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.

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.

Thanks for all your other feedback.   I'll go through and try and
address all your points and post a new version of the patch.

Thanks,
Andrew


> 
> Note that I'm still ok with how you are doing things, I'm just
> discussing this to know whether we would want this as follow-up work or
> not (if somebody is interested in implementing it).
> 
> >  - The add_thread* calls now take a flag, and threads can be started
> >    as resumed or non-resumed.
> > 
> >  - Some of the state changes, like in ::follow_exec, are now pushed
> >    back to where the thread is created, as a result, some places where
> >    in V1 I was changing the state, I'm now asserting the state.
> > 
> >  - Added some asserts to clear_stop_pc
> > 
> > One big consequence of this patch is that we now expect that threads
> > are created in the "correct" resumed/non-resumed state.  A place where
> > this is definitely now wrong is during the initial creation of a
> > process to debug - threads are created in the default resumed state,
> > which is not correct.
> > 
> > I've fixed this for the native Linux and remote targets, but for all
> > other targets this will likely be incorrect.
> > 
> > As I'm not able to test these targets I made the choice to, instead of
> > asserting that GDB has the correct state, fix the state.  This can be
> > seen in gdb_startup_inferior (fork-child.c) and one in run_command_1
> > (infcmd.c), where I've added comments about this.
> > 
> > I don't really see a way around this.  If I make the default thread
> > state be non-resumed then the inferior creation code would work, but
> > any target specific code to handle creating new threads would
> > potentially be wrong (as threads would be created non-resumed when
> > they should be resumed).
> 
> I'm not a fan of the default parameter values in add_thread and friends.
> I'd prefer having to pass an explicit value at each call site (there
> aren't that many), that would force thinking about what to pass at each
> call site.
> 
> For targets you can't build and test... I'd say do a best effort to
> update them (update calls to add_thread* functions) and then use
> assertions.  I much prefer a clear and consistent set of
> preconditions/postconditions to the target interface than a state where
> some targets do one thing and other targets do something else.  That's
> easier to work with in the long run.  And ideally, these would be
> documented in the target interface.  For example, the documentation for
> target_ops::attach should say "threads created by the target during
> attach should be marked as (resumed|non-resumed)".  If somebody hits one
> of your assertions, because a target is not respecting this
> postcondition, it will be quite obvious.
> 
> 
> > @@ -1172,6 +1179,11 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
> >    gdb_assert (current_inferior () == inf);
> >    gdb_assert (current_program_space == inf->pspace);
> >  
> > +  /* The inferior (and so thread) might have changed depending on which
> > +     path we took through the above code.  However, the currently selected
> > +     thread should not be in a resumed state.  */
> > +  gdb_assert (!inferior_thread ()->resumed ());
> 
> Here, I suggested using a loop over all of current inferior's threads.
> It sounds a bit silly, but I'm thinking that some weird platform some
> day might have processes with two threads when coming out of an exec.
> Alternatively, we could assert that the current inferior contains
> exactly one thread.
> 
> > +
> >    /* Attempt to open the exec file.  SYMFILE_DEFER_BP_RESET is used
> >       because the proper displacement for a PIE (Position Independent
> >       Executable) main symbol file will only be computed by
> > @@ -2100,6 +2112,52 @@ internal_resume_ptid (int user_step)
> >      return user_visible_resume_ptid (user_step);
> >  }
> >  
> > +/* Before calling into target code to set inferior threads executing we
> > +   must mark all threads as resumed.  If an exception is thrown while
> > +   trying to set the threads executing then we should mark the threads as
> > +   non-resumed.
> > +
> > +   Create an instance of this struct before */
> 
> Seems like this comment was not finished.
> 
> > +struct scoped_mark_thread_resumed
> > +{
> > +  /* Constructor.  All threads matching PTID will be marked as resumed.  */
> > +  scoped_mark_thread_resumed (process_stratum_target *targ, ptid_t ptid)
> > +    : m_target (targ), m_ptid (ptid)
> > +  {
> > +    gdb_assert (m_target != nullptr);
> > +    set_resumed (m_target, m_ptid, true);
> > +  }
> > +
> > +  /* Destructor.  If M_TARGET is not nullptr then mark all threads matching
> > +     M_PTID as no longer being resumed.  The expectation is that on the
> > +     exception path this will be called with M_TARGET still set to a valid
> > +     target.  If however, the threads were successfully set executing then
> > +     this->commit() will have been called, and M_TARGET will now be
> > +     nullptr.  */
> 
> I'd say "If the object was not committed" instead of "If M_TARGET is not
> nullptr".  M_TARGET being nullptr to represent the commit is an internal
> detail, and this comment is supposed to describe the interface.
> 
> > +  ~scoped_mark_thread_resumed ()
> > +  {
> > +    if (m_target != nullptr)
> > +      set_resumed (m_target, m_ptid, false);
> > +  }
> > +
> > +  /* Called once all of the threads have successfully be set executing (by
> > +     calling into the target code).  Clears M_TARGET as an indication that,
> > +     when this object is destructed, we should leave all matching threads
> > +     as being marked resumed.  */
> 
> Same idea here.  You could put the "Clear M_TARGET..." part inside the
> method body (to describe the implementation), while the comment above
> the method can describe the interface (e.g. "Calling commit makes it so
> threads are left as resumed on destruction.").
> 
> Simon