[binutils-gdb] Don't pretend infcalls don't set the inferior running (PR gdb/34082)

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=519774805a14850f13064cc6fcc4179c5b700631

commit 519774805a14850f13064cc6fcc4179c5b700631
Author: Pedro Alves <[email protected]>
Date:   Tue Apr 21 21:08:11 2026 +0100

    Don't pretend infcalls don't set the inferior running (PR gdb/34082)
    
    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 the code to skip set_running (nowadays 'set_state(...,
      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.
    
    - MI still suppresses *running
    
      The running state could potentially be observed on frontends, with
      e.g., MI's *running => *stopped transitions, but if that is a
      problem, I think it should be handled by the interpreter layer not
      emiting the notifications, instead of hacking the threads's core
      state.  I.e., make it a presentation detail.
    
      I don't think it would be a problem for frontends to see *running
      during infcalls, but in any case, MI is already suppressing such
      notifications when they are caused by an infcall.  See
      mi_interp::on_target_resumed.
    
      So e.g., if we let the threads transition to THREAD_RUNNING, with
      MI, we still see no *running/*stopped:
    
        (gdb)
        p malloc(0)
        &"p malloc(0)\n"
        ~"$2 = (void *) 0x555555560320\n"
        ^done
        (gdb)
    
      I don't know if it'd be a problem for DAP, but I assume not too.
    
      Note how the code in proceed that skips setting threads to
      THREAD_RUNNING only applies to already-known threads.  Any new
      thread that appears while the infcall is ongoing will end up marked
      THREAD_RUNNING, causing frontend notifications.  This shows how
      hiding the running state doesn't really hide it completely.
    
    So this is what this commit does.  It lets threads transition to
    THREAD_RUNNING even during infcalls, fixing the problem described
    above, as now there will be proper THREAD_RUNNING => THREAD_STOPPED
    transitions when the infcall finishes.
    
    It also tweaks the testcase to spawn 10 threads per infcall instead of
    one.  This makes it much more likely to reproduce the problem on my
    machine.  Without it, the test still passes for me, which is why I
    didn't see the problem before merging 2954dd2b73.
    
    Tested on x86-64 Linux, native and gdbserver.
    
    Approved-By: Andrew Burgess <[email protected]>
    Change-Id: I1bdc733ac865102d3f7bf0a4f7f56e6f7d75d457
    commit-id:d2abd130

Diff:
---
 gdb/infrun.c                                     |  9 +++------
 gdb/testsuite/gdb.threads/hand-call-new-thread.c | 16 +++++++++++-----
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6401df78e0a..0e359f0ed74 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3688,11 +3688,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   /* Even if RESUME_PTID is a wildcard, and we end up resuming fewer
      threads (e.g., we might need to set threads stepping over
      breakpoints first), from the user/frontend's point of view, all
-     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);
+     threads in RESUME_PTID are now running.  */
+  set_state (resume_target, resume_ptid, THREAD_RUNNING);
 
   infrun_debug_printf ("addr=%s, signal=%s, resume_ptid=%s",
 		       paddress (gdbarch, addr),
@@ -6636,7 +6633,7 @@ restart_threads (struct thread_info *event_thread, inferior *inf)
 	  continue;
 	}
 
-      if (!(tp.state () == THREAD_RUNNING || tp.control.in_infcall))
+      if (tp.state () != THREAD_RUNNING)
 	{
 	  infrun_debug_printf ("restart threads: [%s] not meant to be running",
 			       tp.ptid.to_string ().c_str ());
diff --git a/gdb/testsuite/gdb.threads/hand-call-new-thread.c b/gdb/testsuite/gdb.threads/hand-call-new-thread.c
index 620322fff10..04154576351 100644
--- a/gdb/testsuite/gdb.threads/hand-call-new-thread.c
+++ b/gdb/testsuite/gdb.threads/hand-call-new-thread.c
@@ -34,14 +34,20 @@ thread_function (void *arg)
     foo ();
 }
 
+#define NTHREADS 10
+
 void
 new_thread (void)
 {
-  pthread_t thread;
-  int res;
-
-  res = pthread_create (&thread, NULL, thread_function, NULL);
-  assert (res == 0);
+  pthread_t thread[NTHREADS];
+  int i;
+
+  for (i = 0; i < NTHREADS; i++)
+    {
+      int res;
+      res = pthread_create (&thread[i], NULL, thread_function, NULL);
+      assert (res == 0);
+    }
 }
 
 int