Re: [PATCH] Don't pretend infcalls don't set the inferior running (PR gdb/34082)

[Andrew Burgess <aburgess@redhat.com>]

Pedro,

Thanks for fixing this.  No real problems, one trivial nit, and a
question, see below...


Pedro Alves <pedro@palves.net> writes:

> Commit 2954dd2b73 ("thread_info::executing+resumed ->
> thread_info::internal_state"), caused a regression in
> gdb.threads/hand-call-new-thread.exp:
>
>  ...
>  (gdb) PASS: gdb.threads/hand-call-new-thread.exp: iter 1: no thread marked running
>  p new_thread ()
>  .../src/gdb/infrun.c:3742: internal-error: proceed: Assertion `!thread_is_in_step_over_chain (&tp)' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>  ----- Backtrace -----
>  FAIL: gdb.threads/hand-call-new-thread.exp: iter 2: gdb-command<p new_thread ()> (GDB internal error)
>  ...
>
> This commit fixes it.
>
> Let's say we have three threads, 1, 2, and 3.  User does:
>
>  (gdb) continue
>
> This makes GDB switch all three threads to THREAD_RUNNING.
>
> If some of those threads, now running, spawns a new thread, that
> thread is also set to state THREAD_RUNNING.  We end up with four
> threads marked THREAD_RUNNING.
>
> If e.g., threads 2 and 3 both hit a breakpoint that needs to be
> stepped over (e.g., condition evals false), and there is only one
> displaced-stepping slot, then one thread starts a displaced stepping
> sequence, while the other is put in the step-over queue, waiting for
> its turn.
>
> Now, if meanwhile thread 1 hits a user-visible stop, GDB stops all
> threads, and transitions all their states to THREAD_STOPPED.  Any
> thread that was still waiting for its turn in the step-over queue is
> removed from the queue.  That happens in the THREAD_RUNNING =>
> THREAD_STOPPED transition, here:
>
>   thread_state
>   thread_info::set_state (thread_state state, bool suppress_notification)
>   {
>   ...
>     switch (m_state)
>       {
>       case THREAD_STOPPED:
>         if (thread_is_in_step_over_chain (this))
>           global_thread_step_over_chain_remove (this);
>
> The next time the user continues execution, if the breakpoint is still
> inserted, proceed() sets them stepping the breakpoint again.  And
> again, if there is more than one thread that needs to step-over, and
> there aren't enough slots, some threads may end up in the step-over
> queue.  Rinse, repeat.
>
> All this works well with normal resumption commands, like continue,
> step, next, etc.
>
> The problem exposed by gdb.threads/hand-call-new-thread.exp is if you
> resume execution with an infcall instead of a normal execution
> command.  In that case, proceed() skips transitioning (pre-existing)
> threads to THREAD_RUNNING, here:
>
>   proceed (CORE_ADDR addr, enum gdb_signal siggnal)
>   {
>    ...
>       /* Even if RESUME_PTID is a wildcard, and we end up resuming fewer
> 	 threads in RESUME_PTID are now running.  Unless we're calling an
> 	 inferior function, as in that case we pretend the inferior
> 	 doesn't run at all.  */
>       if (!cur_thr->control.in_infcall)
> 	set_state (resume_target, resume_ptid, THREAD_RUNNING);
>
> So later, when the call finishes for any reason (normal call finish,
> or some other user-visible stop happens), and GDB transitions all
> threads to THREAD_STOPPED, we hit the early return in
> thread_info::set_state:
>
>   thread_state
>   thread_info::set_state (thread_state state, bool suppress_notification)
>   {
>     thread_state prev_state = m_state;
>     if (prev_state == state)
>       return prev_state;          // <== EARLY RETURN
>
>     m_state = state;
>     switch (m_state)
>       {
>       case THREAD_STOPPED:
>         if (thread_is_in_step_over_chain (this))
>           global_thread_step_over_chain_remove (this);  // NOT REACHED
>         break;
>       ...
>       }
>   }
>
> ... and so if any thread had been put in the step-over queue since the
> last proceed(), it will incorrectly be left still in the step-over
> queue, with THREAD_STOPPED state.
>
> If/when the user re-resumes the program again, we trip the assertion
> in proceed:
>
>   (gdb) p new_thread ()
>   ../../src/gdb/infrun.c:3742: internal-error: proceed: Assertion `!thread_is_in_step_over_chain (&tp)' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>
> Before commit 2954dd2b73 ("thread_info::executing+resumed ->
> thread_info::internal_state"), this didn't happen because
> set_running_thread(..., running=false) would remove threads from the
> step-over queue unconditionally, even if they were already marked
> stopped.
>
> I think the right fix is to stop pretending that infcalls don't set
> the target running.  I can't think of a reason we do that.  It really
> does run.  Some thoughts:
>
> - I added to code to skip set_running (nowadays 'set_state(...,

typo: "I added to code" -> "I added THE code" ?

>   THREAD_RUNNING))' for infcalls back in commit 4d9d9d0423 over 10
>   years ago, but I honestly don't recall why.  My guess is that it
>   must have been to keep backwards compatibility with something, and
>   the code has probably changed sufficiently since then making it no
>   longer necessary.
>
> - infcalls are always synchronous, so the intermediate running state
>   can't be observed with commands.

I assume you mean by this that infcalls are something the user initiates
from the prompt, and so while the infcall is running the user cannot
also ask to inspect the thread state.

I don't disagree with any of your conclusions, but, if an infcall is
made as part of a breakpoint condition, then than could run in the
background, asynchronously, while the user is also issuing other
commands.

Not that this changes anything.  In this situation the user believes
that the inferior is running in the background, so seeing the threads as
running is absolutely fine.  I'm just not convinced that the claims made
in this point "infcalls are always asynchronously" and "the intermediate
running state can' be observed" are correct.

But the patch itself makes a lot of sense, and looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew