[2/2] Remove for_each_thread

Message ID 20260513-remove-for-each-v1-2-8793bd79f781@adacore.com
State New
Headers
Series Remove for_each_thread |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom Tromey May 13, 2026, 5:32 p.m. UTC
  This patch removes the for_each_thread function, changing the callers
to use 'foreach' loops instead.  In general I think loops with
iterators should be preferred over callback-based approaches -- they
are easier to read and often result in less source code as well.  For
example, in this patch a helper function is inlined into its sole
caller.
---
 gdb/breakpoint.c |  6 ++----
 gdb/gdbthread.h  |  9 ---------
 gdb/infcmd.c     | 55 ++++++++++++++++++++++++++-----------------------------
 gdb/mi/mi-main.c | 28 +++++++++++++---------------
 gdb/thread.c     |  9 ---------
 5 files changed, 41 insertions(+), 66 deletions(-)
  

Comments

Simon Marchi May 14, 2026, 2:13 p.m. UTC | #1
On 2026-05-13 13:32, Tom Tromey wrote:
> This patch removes the for_each_thread function, changing the callers
> to use 'foreach' loops instead.  In general I think loops with
> iterators should be preferred over callback-based approaches -- they
> are easier to read and often result in less source code as well.  For
> example, in this patch a helper function is inlined into its sole
> caller.

Perhaps mention that there is a functional change, since you switch from
using all_threads_safe to all_threads everywhere.  This happens to fix
the bug that Markus was fixing here:

https://inbox.sourceware.org/gdb-patches/20260504071636.1571615-2-markus.t.metzger@intel.com/

I would have liked to see a test along with that fix (I supplied one
down-thread), but whatever.

The other callers don't seem to need all_threads_safe, at first glance.

> @@ -607,16 +605,16 @@ print_one_inferior (struct inferior *inferior, bool recurse,
>  
>        if (inferior->pid != 0)
>  	{
> -	  for_each_thread ([&] (struct thread_info *ti)
> +	  for (auto &ti : all_threads ())
>  	    {
> -	      if (ti->ptid.pid () == inferior->pid)
> +	      if (ti.ptid.pid () == inferior->pid)
>  		{
> -		  int core = target_core_of_thread (ti->ptid);
> +		  int core = target_core_of_thread (ti.ptid);
>  
>  		  if (core != -1)
>  		    cores.insert (core);
>  		}
> -	    });
> +	    }

You could remove the braces of the "for" here.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
  
Tom Tromey May 14, 2026, 5:48 p.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Perhaps mention that there is a functional change, since you switch from
Simon> using all_threads_safe to all_threads everywhere.  This happens to fix
Simon> the bug that Markus was fixing here:

Simon> https://inbox.sourceware.org/gdb-patches/20260504071636.1571615-2-markus.t.metzger@intel.com/

I was actually assuming I'd end up rebasing my patch on top of his when
it lands.

Tom
  

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 101dc57ee6b..bf35d14b1d2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12694,10 +12694,8 @@  delete_breakpoint (struct breakpoint *bpt)
      event-top.c won't do anything, and temporary breakpoints with
      commands won't work.  */
 
-  for_each_thread ([&] (struct thread_info *th)
-    {
-      bpstat_remove_bp_location (th->control.stop_bpstat, bpt);
-    });
+  for (auto &th : all_threads ())
+    bpstat_remove_bp_location (th.control.stop_bpstat, bpt);
 
   /* Now that breakpoint is removed from breakpoint list, update the
      global location list.  This will remove locations that used to
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index b3052b28d1a..224f1cc8621 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -791,15 +791,6 @@  extern struct thread_info *any_live_thread_of_inferior (inferior *inf);
 void thread_change_ptid (process_stratum_target *targ,
 			 ptid_t old_ptid, ptid_t new_ptid);
 
-/* Callback function type for function for_each_thread.  */
-
-using for_each_thread_callback_ftype
-  = gdb::function_view<void (thread_info *)>;
-
-/* Call CALLBACK once for each known thread.  */
-
-extern void for_each_thread (for_each_thread_callback_ftype callback);
-
 /* Callback function type for function find_thread.  */
 
 using find_thread_callback_ftype = gdb::function_view<bool (thread_info *)>;
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index d4cd2c7d5cd..dc067879b9d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -685,29 +685,6 @@  starti_command (const char *args, int from_tty)
   run_command_1 (args, from_tty, RUN_STOP_AT_FIRST_INSN);
 }
 
-static void
-proceed_thread_callback (struct thread_info *thread)
-{
-  /* We go through all threads individually instead of compressing
-     into a single target `resume_all' request, because some threads
-     may be stopped in internal breakpoints/events, or stopped waiting
-     for its turn in the displaced stepping queue (that is, they are
-     running from the user's perspective but internally stopped).  The
-     target side has no idea about why the thread is stopped, so a
-     `resume_all' command would resume too much.  If/when GDB gains a
-     way to tell the target `hold this thread stopped until I say
-     otherwise', then we can optimize this.  */
-  if (thread->state () != THREAD_STOPPED)
-    return;
-
-  if (!thread->inf->has_execution ())
-    return;
-
-  switch_to_thread (thread);
-  clear_proceed_status (0);
-  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
-}
-
 static void
 ensure_valid_thread (void)
 {
@@ -762,15 +739,35 @@  continue_1 (bool all_threads_p)
       scoped_disable_commit_resumed disable_commit_resumed
 	("continue all threads in non-stop");
 
-      for_each_thread (proceed_thread_callback);
+      for (auto &thread : all_threads ())
+	{
+	  /* We go through all threads individually instead of compressing
+	     into a single target `resume_all' request, because some threads
+	     may be stopped in internal breakpoints/events, or stopped waiting
+	     for its turn in the displaced stepping queue (that is, they are
+	     running from the user's perspective but internally stopped).  The
+	     target side has no idea about why the thread is stopped, so a
+	     `resume_all' command would resume too much.  If/when GDB gains a
+	     way to tell the target `hold this thread stopped until I say
+	     otherwise', then we can optimize this.  */
+	  if (thread.state () != THREAD_STOPPED)
+	    continue;
+
+	  if (!thread.inf->has_execution ())
+	    continue;
+
+	  switch_to_thread (&thread);
+	  clear_proceed_status (0);
+	  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
+	}
 
       if (current_ui->prompt_state == PROMPT_BLOCKED)
 	{
-	  /* If all threads in the target were already running,
-	     proceed_thread_callback ends up never calling proceed,
-	     and so nothing calls this to put the inferior's terminal
-	     settings in effect and remove stdin from the event loop,
-	     which we must when running a foreground command.  E.g.:
+	  /* If all threads in the target were already running, the
+	     above ends up never calling proceed, and so nothing calls
+	     this to put the inferior's terminal settings in effect
+	     and remove stdin from the event loop, which we must when
+	     running a foreground command.  E.g.:
 
 	      (gdb) c -a&
 	      Continuing.
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 53821e69042..8aee563bc03 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -278,10 +278,8 @@  exec_continue (const char *const *argv, int argc)
 	      pid = inf->pid;
 	    }
 
-	  for_each_thread ([&] (struct thread_info *thread)
-	    {
-	      proceed_thread (thread, pid);
-	    });
+	  for (auto &thread : all_threads ())
+	    proceed_thread (&thread, pid);
 	  disable_commit_resumed.reset_and_commit ();
 	}
       else
@@ -363,16 +361,16 @@  mi_cmd_exec_interrupt (const char *command, const char *const *argv, int argc)
       scoped_disable_commit_resumed disable_commit_resumed
 	("interrupting all threads of thread group");
 
-      for_each_thread ([&] (struct thread_info *thread)
+      for (auto &thread : all_threads ())
 	{
-	  if (thread->state () != THREAD_RUNNING)
-	    return;
+	  if (thread.state () != THREAD_RUNNING)
+	    continue;
 
-	  if (thread->ptid.pid () != inf->pid)
-	    return;
+	  if (thread.ptid.pid () != inf->pid)
+	    continue;
 
-	  target_stop (thread->ptid);
-	});
+	  target_stop (thread.ptid);
+	}
     }
   else
     {
@@ -607,16 +605,16 @@  print_one_inferior (struct inferior *inferior, bool recurse,
 
       if (inferior->pid != 0)
 	{
-	  for_each_thread ([&] (struct thread_info *ti)
+	  for (auto &ti : all_threads ())
 	    {
-	      if (ti->ptid.pid () == inferior->pid)
+	      if (ti.ptid.pid () == inferior->pid)
 		{
-		  int core = target_core_of_thread (ti->ptid);
+		  int core = target_core_of_thread (ti.ptid);
 
 		  if (core != -1)
 		    cores.insert (core);
 		}
-	    });
+	    }
 	}
 
       if (!cores.empty ())
diff --git a/gdb/thread.c b/gdb/thread.c
index 4f62ca280cd..197facf3214 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -602,15 +602,6 @@  find_thread_by_handle (gdb::array_view<const gdb_byte> handle,
 
 /* See gdbthread.h.  */
 
-void
-for_each_thread (for_each_thread_callback_ftype callback)
-{
-  for (thread_info &tp : all_threads_safe ())
-    callback (&tp);
-}
-
-/* See gdbthread.h.  */
-
 struct thread_info *
 find_thread (find_thread_callback_ftype callback)
 {