[PATCH 0/2] Remove for_each_thread

[PATCH 0/2] Remove for_each_thread

[Tom Tromey]

As mentioned elsewhere on the list, here is a short series to remove
for_each_thread.

My view is that in most cases using 'foreach' and an iterator is
preferable to using a callback-style iteration function.  An explicit
loop is clearer and often involves less code.

This series removes for_each_thread.  Note I left the NetBSD-specific
variant alone, as I can't build or test this.

Regression tested on x86-64 Fedora 43.

Signed-off-by: Tom Tromey <tromey@adacore.com>
---
Tom Tromey (2):
      Use bool in continue_1
      Remove for_each_thread

 gdb/breakpoint.c |  6 ++----
 gdb/gdbthread.h  |  9 ---------
 gdb/infcmd.c     | 59 +++++++++++++++++++++++++++-----------------------------
 gdb/inferior.h   |  2 +-
 gdb/mi/mi-main.c | 34 +++++++++++++++-----------------
 gdb/thread.c     |  9 ---------
 6 files changed, 47 insertions(+), 72 deletions(-)
---
base-commit: b301725d35c1d6d80649180dcad1d3bdc42de862
change-id: 20260513-remove-for-each-1d12c2c85b53

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/2] Use bool in continue_1

[Tom Tromey]

This changes continue_1 to use 'bool' and renames its parameter.  The
renaming is important to avoid shadowing the 'all_threads' function.
---
 gdb/infcmd.c     | 4 ++--
 gdb/inferior.h   | 2 +-
 gdb/mi/mi-main.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f3f15c9ae1d..d4cd2c7d5cd 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -746,12 +746,12 @@ ensure_not_running (void)
 }
 
 void
-continue_1 (int all_threads)
+continue_1 (bool all_threads_p)
 {
   ERROR_NO_INFERIOR;
   ensure_not_tfind_mode ();
 
-  if (non_stop && all_threads)
+  if (non_stop && all_threads_p)
     {
       /* Don't error out if the current thread is running, because
 	 there may be other stopped threads.  */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9c031035a23..d141aacba8a 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -225,7 +225,7 @@ extern void attach_command (const char *, int);
 
 extern void registers_info (const char *, bool);
 
-extern void continue_1 (int all_threads);
+extern void continue_1 (bool all_threads_p);
 
 extern void interrupt_target_1 (bool all_threads);
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 52e642e1464..53821e69042 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -286,7 +286,7 @@ exec_continue (const char *const *argv, int argc)
 	}
       else
 	{
-	  continue_1 (0);
+	  continue_1 (false);
 	}
     }
   else
@@ -296,7 +296,7 @@ exec_continue (const char *const *argv, int argc)
       if (current_context->all)
 	{
 	  sched_multi = 1;
-	  continue_1 (0);
+	  continue_1 (false);
 	}
       else
 	{
@@ -304,7 +304,7 @@ exec_continue (const char *const *argv, int argc)
 	     either all threads, or one thread, depending on the
 	     'scheduler-locking' variable.  Let's continue to do the
 	     same.  */
-	  continue_1 (1);
+	  continue_1 (true);
 	}
     }
 }

-- 
2.54.0

[PATCH 2/2] Remove for_each_thread

[Tom Tromey]

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(-)

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)
 {

-- 
2.54.0

Re: [PATCH 1/2] Use bool in continue_1

[Simon Marchi]

> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 52e642e1464..53821e69042 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -286,7 +286,7 @@ exec_continue (const char *const *argv, int argc)
>  	}
>        else
>  	{
> -	  continue_1 (0);
> +	  continue_1 (false);
>  	}

Could you please remove the braces here at the same time?

Otherwise LGTM.

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

Simon

Re: [PATCH 2/2] Remove for_each_thread

[Simon Marchi]

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

Re: [PATCH 2/2] Remove for_each_thread

[Tom Tromey]

>>>>> "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

Re: [PATCH 0/2] Remove for_each_thread

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> As mentioned elsewhere on the list, here is a short series to remove
Tom> for_each_thread.

I'm checking these in.  I addressed the review comments from Simon --
mostly removing some extraneous braces.  He'd also asked for a comment
about the semantic change, but that is now moot because this has been
rebased on top of commit f1fdd3164b8 ("gdb: use all_threads() in
for_each_thread()")

Tom