Re: [PATCH 1/3] gdb: make thread_info::executing private

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

* Simon Marchi <simon.marchi@polymtl.ca> [2021-09-01 09:53:31 -0400]:

> > @@ -841,24 +851,11 @@ set_running (process_stratum_target *targ, ptid_t ptid, bool running)
> >      gdb::observers::target_resumed.notify (ptid);
> >  }
> >  
> > -
> > -/* Helper for set_executing.  Set's the thread's 'executing' field
> > -   from EXECUTING, and if EXECUTING is true also clears the thread's
> > -   stop_pc.  */
> 
> That comment should probably be moved to the set_executing
> declaration, in the thread_info class.  The part about clearing stop_pc
> is relevant to have there, I think.

Thanks.  I made that change, and also added a comment onto the
declaration of set_resumed.

I pushed just this patch from this series, as this seems like a
reasonable cleanup on its own.

Below is what I pushed.

Thanks,
Andrew

---

commit 611841bb1afc689becfab6dd490e1799aabf547d
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Aug 10 11:20:44 2021 +0100

    gdb: make thread_info::executing private
    
    Rename thread_info::executing to thread_info::m_executing, and make it
    private.  Add a new get/set member functions, and convert GDB to make
    use of these.
    
    The only real change of interest in this patch is in thread.c where I
    have deleted the helper function set_executing_thread, and now just
    use the new set function thread_info::set_executing.  However, the old
    helper function set_executing_thread included some code to reset the
    thread's stop_pc, so I moved this code into the new function
    thread_info::set_executing.  However, I don't believe there is
    anywhere that this results in a change of behaviour, previously the
    executing flag was always set true through a call to
    set_executing_thread anyway.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f6c9683aecf..10b28c97be7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1661,7 +1661,7 @@ watchpoint_in_thread_scope (struct watchpoint *b)
   return (b->pspace == current_program_space
 	  && (b->watchpoint_thread == null_ptid
 	      || (inferior_ptid == b->watchpoint_thread
-		  && !inferior_thread ()->executing)));
+		  && !inferior_thread ()->executing ())));
 }
 
 /* Set watchpoint B to disp_del_at_next_stop, even including its possible
@@ -4512,7 +4512,7 @@ get_bpstat_thread ()
     return NULL;
 
   thread_info *tp = inferior_thread ();
-  if (tp->state == THREAD_EXITED || tp->executing)
+  if (tp->state == THREAD_EXITED || tp->executing ())
     return NULL;
   return tp;
 }
diff --git a/gdb/frame.c b/gdb/frame.c
index 4d7505f7ae3..d28944075ed 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1789,7 +1789,7 @@ has_stack_frames ()
 	return false;
 
       /* ... or from a spinning thread.  */
-      if (tp->executing)
+      if (tp->executing ())
 	return false;
     }
 
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9c178f531d9..e6f383cca61 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -287,15 +287,19 @@ class thread_info : public refcounted_object,
      if the thread does not have a user-given name.  */
   char *name = NULL;
 
-  /* True means the thread is executing.  Note: this is different
-     from saying that there is an active target and we are stopped at
-     a breakpoint, for instance.  This is a real indicator whether the
-     thread is off and running.  */
-  bool executing = false;
+  bool executing () const
+  { return m_executing; }
+
+  /* Set the thread's 'm_executing' field from EXECUTING, and if EXECUTING
+     is true also clears the thread's stop_pc.  */
+  void set_executing (bool executing);
 
   bool resumed () const
   { return m_resumed; }
 
+  /* Set the thread's 'm_resumed' field from RESUMED.  The thread may also
+     be added to (when RESUMED is true), or removed from (when RESUMED is
+     false), the list of threads with a pending wait status.  */
   void set_resumed (bool resumed);
 
   /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
@@ -488,6 +492,12 @@ class thread_info : public refcounted_object,
      the thread run.  */
   bool m_resumed = false;
 
+  /* True means the thread is executing.  Note: this is different
+     from saying that there is an active target and we are stopped at
+     a breakpoint, for instance.  This is a real indicator whether the
+     thread is off and running.  */
+  bool m_executing = false;
+
   /* State of inferior thread to restore after GDB is done with an inferior
      call.  See `struct thread_suspend_state'.  */
   thread_suspend_state m_suspend;
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c183b60e81a..e6ee49bc43d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2371,7 +2371,7 @@ proceed_after_attach (inferior *inf)
   scoped_restore_current_thread restore_thread;
 
   for (thread_info *thread : inf->non_exited_threads ())
-    if (!thread->executing
+    if (!thread->executing ()
 	&& !thread->stop_requested
 	&& thread->stop_signal () == GDB_SIGNAL_0)
       {
@@ -2644,7 +2644,7 @@ notice_new_inferior (thread_info *thr, bool leave_running, int from_tty)
   /* When we "notice" a new inferior we need to do all the things we
      would normally do if we had just attached to it.  */
 
-  if (thr->executing)
+  if (thr->executing ())
     {
       struct inferior *inferior = current_inferior ();
 
diff --git a/gdb/inflow.c b/gdb/inflow.c
index 74dda702a8a..bcab66487b0 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -525,7 +525,7 @@ child_interrupt (struct target_ops *self)
   thread_info *resumed = NULL;
   for (thread_info *thr : all_non_exited_threads ())
     {
-      if (thr->executing)
+      if (thr->executing ())
 	{
 	  resumed = thr;
 	  break;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 694bbe665f4..8e778576eab 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -862,7 +862,7 @@ static void
 proceed_after_vfork_done (thread_info *thread)
 {
   if (thread->state == THREAD_RUNNING
-      && !thread->executing
+      && !thread->executing ()
       && !thread->stop_requested
       && thread->stop_signal () == GDB_SIGNAL_0)
     {
@@ -1885,7 +1885,7 @@ start_step_over (void)
 
       if (tp->control.trap_expected
 	  || tp->resumed ()
-	  || tp->executing)
+	  || tp->executing ())
 	{
 	  internal_error (__FILE__, __LINE__,
 			  "[%s] has inconsistent state: "
@@ -1893,7 +1893,7 @@ start_step_over (void)
 			  target_pid_to_str (tp->ptid).c_str (),
 			  tp->control.trap_expected,
 			  tp->resumed (),
-			  tp->executing);
+			  tp->executing ());
 	}
 
       infrun_debug_printf ("resuming [%s] for step-over",
@@ -3197,7 +3197,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
 	      {
 		infrun_debug_printf ("[%s] resumed",
 				     target_pid_to_str (tp->ptid).c_str ());
-		gdb_assert (tp->executing || tp->has_pending_waitstatus ());
+		gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
 		continue;
 	      }
 
@@ -3329,7 +3329,7 @@ infrun_thread_stop_requested (ptid_t ptid)
     {
       if (tp->state != THREAD_RUNNING)
 	continue;
-      if (tp->executing)
+      if (tp->executing ())
 	continue;
 
       /* Remove matching threads from the step-over queue, so
@@ -3778,7 +3778,7 @@ prepare_for_detach (void)
 	{
 	  if (thr->displaced_step_state.in_progress ())
 	    {
-	      if (thr->executing)
+	      if (thr->executing ())
 		{
 		  if (!thr->stop_requested)
 		    {
@@ -4806,7 +4806,7 @@ handle_one (const wait_one_event &event)
 	t = add_thread (event.target, event.ptid);
 
       t->stop_requested = 0;
-      t->executing = 0;
+      t->set_executing (false);
       t->set_resumed (false);
       t->control.may_range_step = 0;
 
@@ -4947,7 +4947,7 @@ stop_all_threads (void)
 	      if (!target_is_non_stop_p ())
 		continue;
 
-	      if (t->executing)
+	      if (t->executing ())
 		{
 		  /* If already stopping, don't request a stop again.
 		     We just haven't seen the notification yet.  */
@@ -5091,7 +5091,7 @@ handle_no_resumed (struct execution_control_state *ecs)
 
   for (thread_info *thread : all_non_exited_threads ())
     {
-      if (swap_terminal && thread->executing)
+      if (swap_terminal && thread->executing ())
 	{
 	  if (thread->inf != curr_inf)
 	    {
@@ -5104,7 +5104,7 @@ handle_no_resumed (struct execution_control_state *ecs)
 	}
 
       if (!ignore_event
-	  && (thread->executing || thread->has_pending_waitstatus ()))
+	  && (thread->executing () || thread->has_pending_waitstatus ()))
 	{
 	  /* Either there were no unwaited-for children left in the
 	     target at some point, but there are now, or some target
@@ -5722,7 +5722,7 @@ restart_threads (struct thread_info *event_thread)
 	{
 	  infrun_debug_printf ("restart threads: [%s] resumed",
 			      target_pid_to_str (tp->ptid).c_str ());
-	  gdb_assert (tp->executing || tp->has_pending_waitstatus ());
+	  gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
 	  continue;
 	}
 
@@ -5869,7 +5869,7 @@ finish_step_over (struct execution_control_state *ecs)
 	     do_target_wait.  */
 	  tp->set_resumed (true);
 
-	  gdb_assert (!tp->executing);
+	  gdb_assert (!tp->executing ());
 
 	  regcache = get_thread_regcache (tp);
 	  tp->set_stop_pc (regcache_read_pc (regcache));
@@ -7419,7 +7419,7 @@ restart_after_all_stop_detach (process_stratum_target *proc_target)
       /* If we have any thread that is already executing, then we
 	 don't need to resume the target -- it is already been
 	 resumed.  */
-      if (thr->executing)
+      if (thr->executing ())
 	return;
 
       /* If we have a pending event to process, skip resuming the
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index e9433b2206b..29ca62d2bc1 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1280,7 +1280,7 @@ get_detach_signal (struct lwp_info *lp)
     {
       struct thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
 
-      if (target_is_non_stop_p () && !tp->executing)
+      if (target_is_non_stop_p () && !tp->executing ())
 	{
 	  if (tp->has_pending_waitstatus ())
 	    signo = tp->pending_waitstatus ().value.sig;
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index eb2aa5ac409..245939b6400 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1641,7 +1641,7 @@ thread_db_target::update_thread_list ()
 	continue;
 
       thread_info *thread = any_live_thread_of_inferior (inf);
-      if (thread == NULL || thread->executing)
+      if (thread == NULL || thread->executing ())
 	continue;
 
       /* It's best to avoid td_ta_thr_iter if possible.  That walks
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 85e357e604a..e2b9866d68a 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1998,7 +1998,7 @@ get_thread_current_frame_id (struct thread_info *tp)
      For the former, EXECUTING is true and we're in wait, about to
      move the thread.  Since we need to recompute the stack, we temporarily
      set EXECUTING to false.  */
-  executing = tp->executing;
+  executing = tp->executing ();
   set_executing (proc_target, inferior_ptid, false);
 
   id = null_frame_id;
diff --git a/gdb/target.c b/gdb/target.c
index ae2d659583e..d1c1bf523ed 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3815,7 +3815,7 @@ target_pass_ctrlc (void)
 	{
 	  /* A thread can be THREAD_STOPPED and executing, while
 	     running an infcall.  */
-	  if (thr->state == THREAD_RUNNING || thr->executing)
+	  if (thr->state == THREAD_RUNNING || thr->executing ())
 	    {
 	      /* We can get here quite deep in target layers.  Avoid
 		 switching thread context or anything that would
diff --git a/gdb/thread.c b/gdb/thread.c
index a82fb49140a..c95a9186681 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -319,6 +319,16 @@ thread_info::deletable () const
 
 /* See gdbthread.h.  */
 
+void
+thread_info::set_executing (bool executing)
+{
+  m_executing = executing;
+  if (executing)
+    this->set_stop_pc (~(CORE_ADDR) 0);
+}
+
+/* See gdbthread.h.  */
+
 void
 thread_info::set_resumed (bool resumed)
 {
@@ -625,13 +635,13 @@ any_live_thread_of_inferior (inferior *inf)
       curr_tp = inferior_thread ();
       if (curr_tp->state == THREAD_EXITED)
 	curr_tp = NULL;
-      else if (!curr_tp->executing)
+      else if (!curr_tp->executing ())
 	return curr_tp;
     }
 
   for (thread_info *tp : inf->non_exited_threads ())
     {
-      if (!tp->executing)
+      if (!tp->executing ())
 	return tp;
 
       tp_executing = tp;
@@ -841,24 +851,11 @@ set_running (process_stratum_target *targ, ptid_t ptid, bool running)
     gdb::observers::target_resumed.notify (ptid);
 }
 
-
-/* Helper for set_executing.  Set's the thread's 'executing' field
-   from EXECUTING, and if EXECUTING is true also clears the thread's
-   stop_pc.  */
-
-static void
-set_executing_thread (thread_info *thr, bool executing)
-{
-  thr->executing = executing;
-  if (executing)
-    thr->set_stop_pc (~(CORE_ADDR) 0);
-}
-
 void
 set_executing (process_stratum_target *targ, ptid_t ptid, bool executing)
 {
   for (thread_info *tp : all_non_exited_threads (targ, ptid))
-    set_executing_thread (tp, executing);
+    tp->set_executing (executing);
 
   /* It only takes one running thread to spawn more threads.  */
   if (executing)
@@ -895,7 +892,7 @@ finish_thread_state (process_stratum_target *targ, ptid_t ptid)
   bool any_started = false;
 
   for (thread_info *tp : all_non_exited_threads (targ, ptid))
-    if (set_running_thread (tp, tp->executing))
+    if (set_running_thread (tp, tp->executing ()))
       any_started = true;
 
   if (any_started)
@@ -922,7 +919,7 @@ validate_registers_access (void)
      at the prompt) when a thread is not executing for some internal
      reason, but is marked running from the user's perspective.  E.g.,
      the thread is waiting for its turn in the step-over queue.  */
-  if (tp->executing)
+  if (tp->executing ())
     error (_("Selected thread is running."));
 }
 
@@ -940,7 +937,7 @@ can_access_registers_thread (thread_info *thread)
     return false;
 
   /* ... or from a spinning thread.  FIXME: see validate_registers_access.  */
-  if (thread->executing)
+  if (thread->executing ())
     return false;
 
   return true;
@@ -1997,7 +1994,7 @@ update_threads_executing (void)
 
       for (thread_info *tp : inf->non_exited_threads ())
 	{
-	  if (tp->executing)
+	  if (tp->executing ())
 	    {
 	      targ->threads_executing = true;
 	      return;