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

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

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

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