binutils-gdb/gdb/process-stratum-target.c
Pedro Alves 2954dd2b73 thread_info::executing+resumed -> thread_info::internal_state
While working on Windows non-stop support, I ran into a
very-hard-to-track-down bug.

The problem turned out to be that
infrun.c:proceed_resume_thread_checked resumed an already-executing
thread because the thread was marked as "executing=true,
resumed=false", and that function only skips resuming threads that are
marked resumed=true.  The consequence was that GDB corrupted the
registers of the Windows DLL loader threads, eventually leading to a
GDB+inferior deadlock.

Originally, the "resumed" flag was only ever set when infrun decided
is was ready to process a thread's pending wait status.  infrun has
since evolved to set the resumed flag when we set a thread's executing
flag too.  We are not always consistent throughout in guaranteeing
that a thread is marked resumed=true whenever it is marked
executing=true, though.  For instance, no target code that supports
non-stop mode (linux-nat, remote, and windows-nat with this series) is
making sure that new threads are marked resumed=true when they are
added to the thread list.  They are only marked as {state=running,
executing=true}, the "resumed" flag is not touched.

Making proceed_resume_thread_checked check thr->executing() in
addition to thr->resumed(), feels like papering over a combination of
states that shouldn't happen nowadays.

OTOH, having to have the target backends mark new threads as
resumed=true just feels like too many different states (three) to set:

  add_thread (...);
  set_running (...);
  set_executing (...);
  set_resumed (...);

Yuck.  I think we can do better.

We really have too many "state tracking" flags in a thread.
Basically:

 - whether a thread is "running/stopped/exited" (from the user's
   perspective).  This is the thread_info::state field.

 - whether a thread is "executing" (infrun asked the target to set the
   thread executing).  This is thread_info::executing().

 - whether a thread is "resumed" (infrun wants the thread to be
   resumed, but maybe can't yet because the thread has a pending wait
   status).  This is thread_info::resumed()

"running", "executing", and "resumed" are almost synonyms, so this can
be highly confusing English-wise too.

For "running" vs "executing", in comments, we tipically need to
explain that "running/stopped/exited" is for the user/frontend
perspective, while "executing true/false" is for gdb's internal run
control.

(Also, "executing or not" can also mean something else in GDB's
codebase -- "target has execution" does not mean that threads are
actually running right now -- it's a test for whether we have a live
process vs a core dump!)

One simplification we can do that avoids this running vs executing
ambiguity is to replace the "executing" field with an "internal_state"
field, similar to the thread_info::state field, and make that new
internal_state field reuse the same enum thread_state type that is
used by thread_info::state.  Like:

  struct thread_info
  {
  ...
    /* Frontend/public/external/user view of the thread state.  */
    enum thread_state m_state = THREAD_STOPPED;

    /* The thread's internal state.  When the thread is stopped
       internally while handling an internal event, like a software
       single-step breakpoint, the internal state will be
       THREAD_STOPPED, but the external state will still be
       THREAD_RUNNING.  */
    enum thread_state m_internal_state = THREAD_STOPPED;
  };

(Assume we'd add state() and internal_state() getters.)

With that, every check for thr->executing() is replaced with a
'thr->internal_state() == THREAD_RUNNING' check, and the code is
clearer by design.  There is no confusion between "running" vs
"executing" any more, because they now mean the exact same thing.
Instead, we say e.g., 'thread has (user) state "running", and internal
state "stopped"'.  Or simpler, 'thread is running (from the user's
perspective), but internally stopped'.  That is after all what we
would way in comments today already.

That still leaves the 'resumed' flag, though.  That's the least
obvious one.  Turns out we can get rid of it, and make it a new state
tracked by thread_info::internal_state.  That is, we make
internal_state have its own enumeration type (decoupled from
thread_info::state's type), and convert the resumed true/false flag to
a new enumerator of this new enumeration.  Like so:

  enum thread_int_state
  {
    THREAD_INT_STOPPED,
    THREAD_INT_RUNNING,
 +   THREAD_INT_RESUMED_PENDING_STATUS,
    THREAD_INT_EXITED,
  };

That is what this patch does.  So in summary, we go from:

 thread_info::state {THREAD_STOPPED, THREAD_RUNNING, THREAD_EXITED}
 thread_info::executing {false, true}
 thread_info::resumed {false, true}

to:

 thread_info::state {THREAD_STOPPED, THREAD_RUNNING, THREAD_EXITED}
 thread_info::internal_state {THREAD_INT_STOPPED, THREAD_INT_RUNNING,
                              THREAD_INT_RESUMED_PENDING_STATUS,
			      THREAD_INT_EXITED}

The patch adds getters/setters for both (user) state and
internal_state, and adds assertions around state transitions, ensuring
that internal_state doesn't get out of sync with
thread::have_pending_wait_status().

The code that adds/removes threads from the proc_target's
resumed_with_pending_wait_status list is all centralized within
thread_info::set_internal_state, when we switch to/from the
resumed-pending-status state.  With the assertions in place, it should
be impossible to end up with a THREAD_INT_RUNNING thread with a
pending status.

The thread.c:set_running, thread.c:set_executing, thread.c:set_resumed
global functions are all gone, replaced with new thread.c:set_state
and thread.c:set_internal_state functions.

Tested on x86_64-linux-gnu, native and gdbserver.

Change-Id: I4f5097d68f4694d44e1ae23fea3e9bce45fb078c
commit-id:42ba97d4
2026-04-06 18:50:19 +01:00

221 lines
5.9 KiB
C

/* Abstract base class inherited by all process_stratum targets
Copyright (C) 2018-2026 Free Software Foundation, Inc.
This file is part of GDB.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "process-stratum-target.h"
#include "inferior.h"
#include <algorithm>
process_stratum_target::~process_stratum_target ()
{
}
struct gdbarch *
process_stratum_target::thread_architecture (ptid_t ptid)
{
inferior *inf = find_inferior_ptid (this, ptid);
gdb_assert (inf != NULL);
return inf->arch ();
}
bool
process_stratum_target::has_all_memory ()
{
/* If no inferior selected, then we can't read memory here. */
return inferior_ptid != null_ptid;
}
bool
process_stratum_target::has_memory ()
{
/* If no inferior selected, then we can't read memory here. */
return inferior_ptid != null_ptid;
}
bool
process_stratum_target::has_stack ()
{
/* If no inferior selected, there's no stack. */
return inferior_ptid != null_ptid;
}
bool
process_stratum_target::has_registers ()
{
/* Can't read registers from no inferior. */
return inferior_ptid != null_ptid;
}
bool
process_stratum_target::has_execution (inferior *inf)
{
/* If there's a process running already, we can't make it run
through hoops. */
return inf->pid != 0;
}
/* See process-stratum-target.h. */
void
process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid,
const char *execd_pathname)
{
inferior *orig_inf = current_inferior ();
if (orig_inf != follow_inf)
{
/* Execution continues in a new inferior, push the original inferior's
process target on the new inferior's target stack. The process target
may decide to unpush itself from the original inferior's target stack
after that, at its discretion. */
follow_inf->push_target (orig_inf->process_target ());
thread_info *t = add_thread (follow_inf->process_target (), ptid);
/* Leave the new inferior / thread as the current inferior / thread. */
switch_to_thread (t);
}
}
/* See process-stratum-target.h. */
void
process_stratum_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
target_waitkind fork_kind,
bool follow_child,
bool detach_on_fork)
{
if (child_inf != nullptr)
{
child_inf->push_target (this);
add_thread_silent (this, child_ptid);
}
}
/* See process-stratum-target.h. */
void
process_stratum_target::maybe_add_resumed_with_pending_wait_status
(thread_info *thread)
{
gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ());
if (thread->internal_state () == THREAD_INT_RESUMED_PENDING_STATUS)
{
gdb_assert (thread->has_pending_waitstatus ());
infrun_debug_printf ("adding to resumed threads with event list: %s",
thread->ptid.to_string ().c_str ());
m_resumed_with_pending_wait_status.push_back (*thread);
}
}
/* See process-stratum-target.h. */
void
process_stratum_target::maybe_remove_resumed_with_pending_wait_status
(thread_info *thread)
{
if (thread->internal_state () == THREAD_INT_RESUMED_PENDING_STATUS)
{
gdb_assert (thread->has_pending_waitstatus ());
infrun_debug_printf ("removing from resumed threads with event list: %s",
thread->ptid.to_string ().c_str ());
gdb_assert (thread->resumed_with_pending_wait_status_node.is_linked ());
auto it = m_resumed_with_pending_wait_status.iterator_to (*thread);
m_resumed_with_pending_wait_status.erase (it);
}
else
gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ());
}
/* See process-stratum-target.h. */
thread_info *
process_stratum_target::random_resumed_with_pending_wait_status
(inferior *inf, ptid_t filter_ptid)
{
auto matches = [inf, filter_ptid] (const thread_info &thread)
{
return thread.inf == inf && thread.ptid.matches (filter_ptid);
};
/* First see how many matching events we have. */
const auto &l = m_resumed_with_pending_wait_status;
unsigned int count = std::count_if (l.begin (), l.end (), matches);
if (count == 0)
return nullptr;
/* Now randomly pick a thread out of those that match the criteria. */
int random_selector
= (int) ((count * (double) rand ()) / (RAND_MAX + 1.0));
if (count > 1)
infrun_debug_printf ("Found %u events, selecting #%d",
count, random_selector);
/* Select the Nth thread that matches. */
auto it = std::find_if (l.begin (), l.end (),
[&random_selector, &matches]
(const thread_info &thread)
{
if (!matches (thread))
return false;
return random_selector-- == 0;
});
gdb_assert (it != l.end ());
return &*it;
}
/* See process-stratum-target.h. */
thread_info *
process_stratum_target::find_thread (ptid_t ptid)
{
inferior *inf = find_inferior_ptid (this, ptid);
if (inf == NULL)
return NULL;
return inf->find_thread (ptid);
}
/* See process-stratum-target.h. */
gdb::unordered_set<process_stratum_target *>
all_non_exited_process_targets ()
{
/* Inferiors may share targets. To eliminate duplicates, use a set. */
gdb::unordered_set<process_stratum_target *> targets;
for (inferior *inf : all_non_exited_inferiors ())
targets.insert (inf->process_target ());
return targets;
}
/* See process-stratum-target.h. */
void
switch_to_target_no_thread (process_stratum_target *target)
{
for (inferior *inf : all_inferiors (target))
{
switch_to_inferior_no_thread (inf);
break;
}
}