[PATCH] gdb: make iterate_over_threads a template function

[PATCH] gdb: make iterate_over_threads a template function

[Andrew Burgess]

This started off as changing the return type of the callback function
that iterate_over_threads takes from int to bool.

But then I thought, why not make the whole thing templated, and remove
the need to pass void* arguments around.

So now we can do:

  bool
  callback_1 (struct thread_info *tp)
  { ... }

  bool
  callback_2 (struct thread_info *tp, const struct some_type &obj)
  { ... }

  void
  some_other_part_of_gdb ()
  {
    iterate_over_threads (callback_1);

    iterate_over_threads (callback_2, object_of_some_type);
  }

Which seems much nicer to me.

While updating all the callback functions I took this opportunity to
remove a code construct I really hate:

  if (condition)
    return true;
  return false;

This has now become:

  return (condition);

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* aix-thread.c (giter_count): Update argument types and return
	type, update implementation to match.
	(giter_accum): Likewise.
	(iter_tid): Likewise.
	(pd_update): Update call to iterate_over_threads.
	* breakpoint.c (bpstat_remove_breakpoint_callback): Update
	argument types and return type, update implementation to match.
	* fbsd-tdep.c (find_signalled_thread): Likewise.
	(fbsd_make_corefile_notes): Update call to iterate_over_threads.
	* gdbthread.h (thread_callback_func): Delete typedef.
	(iterate_over_threads): Delete declaration, rewrite as templated
	function.
	* infcmd.c (proceed_thread_callback): Update argument types and
	return type, update implementation to match.
	(continue_1): Update call to iterate_over_threads.
	* infrun.c (proceed_after_vfork_done): Update argument types and
	return type, update implementation to match.
	(handle_vfork_child_exec_or_exit): Update call to
	iterate_over_threads.
	(resumed_thread_with_pending_status): Update argument types and
	return type, update implementation to match.
	(finish_step_over): Update call to iterate_over_threads.
	* mi/mi-main.c (proceed_thread_callback): Update argument types
	and return type, update implementation to match.
	(exec_continue): Update call to iterate_over_threads.
	(interrupt_thread_callback): Update argument types and return
	type, update implementation to match.
	(mi_cmd_exec_interrupt): Update call to iterate_over_threads.
	(find_thread_of_process): Update argument types and return type,
	update implementation to match.
	(mi_cmd_target_detach): Update call to iterate_over_threads.
	(collect_cores): Update argument types and return type, update
	implementation to match.
	* procfs.c (find_signalled_thread): Update argument types and
	return type, update implementation to match.
	(find_stop_signal): Update call to iterate_over_threads.
	* sol-thread.c (thread_db_find_thread_from_tid): Update argument
	types and return type, update implementation to match.
	(sol_thread_target::get_ada_task_ptid): Update call to
	iterate_over_threads.
	* thread.c (iterate_over_threads): Delete implementation, this has
	now moved into gdbthread.h.
---
 gdb/ChangeLog    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/aix-thread.c | 25 ++++++++++++-------------
 gdb/breakpoint.c |  9 ++++-----
 gdb/fbsd-tdep.c  | 13 +++++--------
 gdb/gdbthread.h  | 28 +++++++++++++++++++++++-----
 gdb/infcmd.c     | 12 ++++++------
 gdb/infrun.c     | 22 ++++++++--------------
 gdb/mi/mi-main.c | 45 +++++++++++++++++----------------------------
 gdb/procfs.c     | 13 +++++--------
 gdb/sol-thread.c | 15 +++++----------
 gdb/thread.c     | 25 -------------------------
 11 files changed, 130 insertions(+), 122 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 889cb65fdcd..abab6dc5da9 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -639,12 +639,12 @@ pcmp (const void *p1v, const void *p2v)
    does not include that main thread either, and thus allows us
    to compare the two lists.  */
 
-static int
-giter_count (struct thread_info *thread, void *countp)
+static bool
+giter_count (struct thread_info *thread, int *countp)
 {
   if (PD_TID (thread->ptid))
-    (*(int *) countp)++;
-  return 0;
+    (*countp)++;
+  return false;
 }
 
 /* iterate_over_threads() callback for accumulating GDB thread pids.
@@ -654,15 +654,15 @@ giter_count (struct thread_info *thread, void *countp)
    does not include that main thread either, and thus allows us
    to compare the two lists.  */
 
-static int
-giter_accum (struct thread_info *thread, void *bufp)
+static bool
+giter_accum (struct thread_info *thread, struct thread_info ***bufp)
 {
   if (PD_TID (thread->ptid))
     {
-      **(struct thread_info ***) bufp = thread;
-      (*(struct thread_info ***) bufp)++;
+      **bufp = thread;
+      (*bufp)++;
     }
-  return 0;
+  return false;
 }
 
 /* ptid comparison function */
@@ -861,10 +861,9 @@ sync_threadlists (void)
 /* Iterate_over_threads() callback for locating a thread, using
    the TID of its associated kernel thread.  */
 
-static int
-iter_tid (struct thread_info *thread, void *tidp)
+static bool
+iter_tid (struct thread_info *thread, const pthdb_tid_t &tid)
 {
-  const pthdb_tid_t tid = *(pthdb_tid_t *)tidp;
   aix_thread_info *priv = get_aix_thread_info (thread);
 
   return priv->tid == tid;
@@ -895,7 +894,7 @@ pd_update (int set_infpid)
 
   tid = get_signaled_thread ();
   if (tid != 0)
-    thread = iterate_over_threads (iter_tid, &tid);
+    thread = iterate_over_threads (iter_tid, tid);
   if (!thread)
     ptid = inferior_ptid;
   else
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d479f008948..b1df9c52369 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12304,13 +12304,12 @@ bpstat_remove_bp_location (bpstat bps, struct breakpoint *bpt)
 }
 
 /* Callback for iterate_over_threads.  */
-static int
-bpstat_remove_breakpoint_callback (struct thread_info *th, void *data)
+static bool
+bpstat_remove_breakpoint_callback (struct thread_info *th,
+				   struct breakpoint *bpt)
 {
-  struct breakpoint *bpt = (struct breakpoint *) data;
-
   bpstat_remove_bp_location (th->control.stop_bpstat, bpt);
-  return 0;
+  return false;
 }
 
 /* Helper for breakpoint and tracepoint breakpoint_ops->mention
diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index 6cab31dde82..8f1a008566a 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -574,14 +574,11 @@ fbsd_core_xfer_siginfo (struct gdbarch *gdbarch, gdb_byte *readbuf,
   return len;
 }
 
-static int
-find_signalled_thread (struct thread_info *info, void *data)
+static bool
+find_signalled_thread (struct thread_info *info)
 {
-  if (info->suspend.stop_signal != GDB_SIGNAL_0
-      && info->ptid.pid () == inferior_ptid.pid ())
-    return 1;
-
-  return 0;
+  return (info->suspend.stop_signal != GDB_SIGNAL_0
+	  && info->ptid.pid () == inferior_ptid.pid ());
 }
 
 /* Return a byte_vector containing the contents of a core dump note
@@ -655,7 +652,7 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
     signalled_thr = curr_thr;
   else
     {
-      signalled_thr = iterate_over_threads (find_signalled_thread, NULL);
+      signalled_thr = iterate_over_threads (find_signalled_thread);
       if (signalled_thr == NULL)
 	signalled_thr = curr_thr;
     }
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index eef37f79e6a..bdd6310c3cd 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -505,11 +505,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);
 
-/* Iterator function to call a user-provided callback function
-   once for each known thread.  */
-typedef int (*thread_callback_func) (struct thread_info *, void *);
-extern struct thread_info *iterate_over_threads (thread_callback_func, void *);
-
 /* Pull in the internals of the inferiors/threads ranges and
    iterators.  Must be done after struct thread_info is defined.  */
 #include "thread-iter.h"
@@ -570,6 +565,29 @@ all_threads_safe ()
   return {};
 }
 
+/* Iterator function to call a user-provided CALLBACK function once for
+   each known thread.  The function CALLBACK must take the thread_info as
+   its first argument, but can then take zero or more additional arguments
+   of any type.
+
+   Call CALLBACK once for each thread, so long as the callback function
+   returns false.  If CALLBACK returns true, the iteration will end and the
+   current thread will be returned.  This can be useful for implementing a
+   search for a thread with arbitrary attributes, or for applying some
+   operation to every thread.  */
+
+template<typename... Types>
+struct thread_info *
+iterate_over_threads (bool (*callback) (struct thread_info *, Types...),
+		      Types... args)
+{
+  for (thread_info *tp : all_threads_safe ())
+    if (callback (tp, args...))
+      return tp;
+
+  return nullptr;
+}
+
 extern int thread_count (process_stratum_target *proc_target);
 
 /* Return true if we have any thread in any inferior.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 4351409af50..cf3a89c6d00 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -577,8 +577,8 @@ starti_command (const char *args, int from_tty)
   run_command_1 (args, from_tty, RUN_STOP_AT_FIRST_INSN);
 } 
 
-static int
-proceed_thread_callback (struct thread_info *thread, void *arg)
+static bool
+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
@@ -590,15 +590,15 @@ proceed_thread_callback (struct thread_info *thread, void *arg)
      thread stopped until I say otherwise', then we can optimize
      this.  */
   if (thread->state != THREAD_STOPPED)
-    return 0;
+    return false;
 
   if (!thread->inf->has_execution ())
-    return 0;
+    return false;
 
   switch_to_thread (thread);
   clear_proceed_status (0);
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
-  return 0;
+  return false;
 }
 
 static void
@@ -653,7 +653,7 @@ continue_1 (int all_threads)
 	 exit.  */
       scoped_restore_current_thread restore_thread;
 
-      iterate_over_threads (proceed_thread_callback, NULL);
+      iterate_over_threads (proceed_thread_callback);
 
       if (current_ui->prompt_state == PROMPT_BLOCKED)
 	{
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 7fc56dc51f0..a4f9b5102e1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -884,12 +884,9 @@ follow_inferior_reset_breakpoints (void)
 /* The child has exited or execed: resume threads of the parent the
    user wanted to be executing.  */
 
-static int
-proceed_after_vfork_done (struct thread_info *thread,
-			  void *arg)
+static bool
+proceed_after_vfork_done (struct thread_info *thread, int pid)
 {
-  int pid = * (int *) arg;
-
   if (thread->ptid.pid () == pid
       && thread->state == THREAD_RUNNING
       && !thread->executing
@@ -904,7 +901,7 @@ proceed_after_vfork_done (struct thread_info *thread,
       proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
 
-  return 0;
+  return false;
 }
 
 /* Called whenever we notice an exec or exit event, to handle
@@ -1035,7 +1032,7 @@ handle_vfork_child_exec_or_exit (int exec)
 	  infrun_debug_printf ("resuming vfork parent process %d",
 			       resume_parent);
 
-	  iterate_over_threads (proceed_after_vfork_done, &resume_parent);
+	  iterate_over_threads (proceed_after_vfork_done, resume_parent);
 	}
     }
 }
@@ -5845,12 +5842,10 @@ restart_threads (struct thread_info *event_thread)
 /* Callback for iterate_over_threads.  Find a resumed thread that has
    a pending waitstatus.  */
 
-static int
-resumed_thread_with_pending_status (struct thread_info *tp,
-				    void *arg)
+static bool
+resumed_thread_with_pending_status (struct thread_info *tp)
 {
-  return (tp->resumed
-	  && tp->suspend.waitstatus_pending_p);
+  return (tp->resumed && tp->suspend.waitstatus_pending_p);
 }
 
 /* Called when we get an event that may finish an in-line or
@@ -5918,8 +5913,7 @@ finish_step_over (struct execution_control_state *ecs)
       if (ecs->event_thread->stepping_over_watchpoint)
 	return 0;
 
-      pending = iterate_over_threads (resumed_thread_with_pending_status,
-				      NULL);
+      pending = iterate_over_threads (resumed_thread_with_pending_status);
       if (pending != NULL)
 	{
 	  struct thread_info *tp = ecs->event_thread;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 9d205f0208b..0f6919045a8 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -252,13 +252,11 @@ proceed_thread (struct thread_info *thread, int pid)
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
 
-static int
-proceed_thread_callback (struct thread_info *thread, void *arg)
+static bool
+proceed_thread_callback (struct thread_info *thread, int pid)
 {
-  int pid = *(int *)arg;
-
   proceed_thread (thread, pid);
-  return 0;
+  return false;
 }
 
 static void
@@ -288,7 +286,7 @@ exec_continue (char **argv, int argc)
 
 	      pid = inf->pid;
 	    }
-	  iterate_over_threads (proceed_thread_callback, &pid);
+	  iterate_over_threads (proceed_thread_callback, pid);
 	}
       else
 	{
@@ -342,19 +340,17 @@ mi_cmd_exec_continue (const char *command, char **argv, int argc)
     exec_continue (argv, argc);
 }
 
-static int
-interrupt_thread_callback (struct thread_info *thread, void *arg)
+static bool
+interrupt_thread_callback (struct thread_info *thread, int pid)
 {
-  int pid = *(int *)arg;
-
   if (thread->state != THREAD_RUNNING)
-    return 0;
+    return false;
 
   if (thread->ptid.pid () != pid)
-    return 0;
+    return false;
 
   target_stop (thread->ptid);
-  return 0;
+  return false;
 }
 
 /* Interrupt the execution of the target.  Note how we must play
@@ -383,7 +379,7 @@ mi_cmd_exec_interrupt (const char *command, char **argv, int argc)
     {
       struct inferior *inf = find_inferior_id (current_context->thread_group);
 
-      iterate_over_threads (interrupt_thread_callback, &inf->pid);
+      iterate_over_threads (interrupt_thread_callback, inf->pid);
     }
   else
     {
@@ -477,15 +473,10 @@ mi_cmd_exec_run (const char *command, char **argv, int argc)
 }
 
 
-static int
-find_thread_of_process (struct thread_info *ti, void *p)
+static bool
+find_thread_of_process (struct thread_info *ti, int pid)
 {
-  int pid = *(int *)p;
-
-  if (ti->ptid.pid () == pid && ti->state != THREAD_EXITED)
-    return 1;
-
-  return 0;
+  return (ti->ptid.pid () == pid && ti->state != THREAD_EXITED);
 }
 
 void
@@ -526,7 +517,7 @@ mi_cmd_target_detach (const char *command, char **argv, int argc)
 
       /* Pick any thread in the desired process.  Current
 	 target_detach detaches from the parent of inferior_ptid.  */
-      tp = iterate_over_threads (find_thread_of_process, &pid);
+      tp = iterate_over_threads (find_thread_of_process, pid);
       if (!tp)
 	error (_("Thread group is empty"));
 
@@ -612,11 +603,9 @@ struct collect_cores_data
   std::set<int> cores;
 };
 
-static int
-collect_cores (struct thread_info *ti, void *xdata)
+static bool
+collect_cores (struct thread_info *ti, struct collect_cores_data *data)
 {
-  struct collect_cores_data *data = (struct collect_cores_data *) xdata;
-
   if (ti->ptid.pid () == data->pid)
     {
       int core = target_core_of_thread (ti->ptid);
@@ -625,7 +614,7 @@ collect_cores (struct thread_info *ti, void *xdata)
 	data->cores.insert (core);
     }
 
-  return 0;
+  return false;
 }
 
 struct print_one_inferior_data
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 23c0aa22a7a..705d77f6c42 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -3574,21 +3574,18 @@ procfs_corefile_thread_callback (procinfo *pi, procinfo *thread, void *data)
   return 0;
 }
 
-static int
-find_signalled_thread (struct thread_info *info, void *data)
+static bool
+find_signalled_thread (struct thread_info *info)
 {
-  if (info->suspend.stop_signal != GDB_SIGNAL_0
-      && info->ptid.pid () == inferior_ptid.pid ())
-    return 1;
-
-  return 0;
+  return (info->suspend.stop_signal != GDB_SIGNAL_0
+	  && info->ptid.pid () == inferior_ptid.pid ());
 }
 
 static enum gdb_signal
 find_stop_signal (void)
 {
   struct thread_info *info =
-    iterate_over_threads (find_signalled_thread, NULL);
+    iterate_over_threads (find_signalled_thread);
 
   if (info)
     return info->suspend.stop_signal;
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index 18eacf1062a..14568729437 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -1117,22 +1117,17 @@ info_solthreads (const char *args, int from_tty)
 /* Callback routine used to find a thread based on the TID part of
    its PTID.  */
 
-static int
-thread_db_find_thread_from_tid (struct thread_info *thread, void *data)
+static bool
+thread_db_find_thread_from_tid (struct thread_info *thread, long tid)
 {
-  long *tid = (long *) data;
-
-  if (thread->ptid.tid () == *tid)
-    return 1;
-
-  return 0;
+  return (thread->ptid.tid () == tid);
 }
 
 ptid_t
 sol_thread_target::get_ada_task_ptid (long lwp, long thread)
 {
   struct thread_info *thread_info =
-    iterate_over_threads (thread_db_find_thread_from_tid, &thread);
+    iterate_over_threads (thread_db_find_thread_from_tid, thread);
 
   if (thread_info == NULL)
     {
@@ -1140,7 +1135,7 @@ sol_thread_target::get_ada_task_ptid (long lwp, long thread)
 	 thread that is missing from the list, and try again.  */
       update_thread_list ();
       thread_info = iterate_over_threads (thread_db_find_thread_from_tid,
-					  &thread);
+					  thread);
     }
 
   gdb_assert (thread_info != NULL);
diff --git a/gdb/thread.c b/gdb/thread.c
index 40051013c0a..0205bd3b18c 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -557,31 +557,6 @@ find_thread_by_handle (gdb::array_view<const gdb_byte> handle,
 					      inf);
 }
 
-/*
- * Thread iterator function.
- *
- * Calls a callback function once for each thread, so long as
- * the callback function returns false.  If the callback function
- * returns true, the iteration will end and the current thread
- * will be returned.  This can be useful for implementing a
- * search for a thread with arbitrary attributes, or for applying
- * some operation to every thread.
- *
- * FIXME: some of the existing functionality, such as
- * "Thread apply all", might be rewritten using this functionality.
- */
-
-struct thread_info *
-iterate_over_threads (int (*callback) (struct thread_info *, void *),
-		      void *data)
-{
-  for (thread_info *tp : all_threads_safe ())
-    if ((*callback) (tp, data))
-      return tp;
-
-  return NULL;
-}
-
 /* See gdbthread.h.  */
 
 bool
-- 
2.25.4

Re: [PATCH] gdb: make iterate_over_threads a template function

[Simon Marchi]

On 2021-05-22 12:51 p.m., Andrew Burgess wrote:> This started off as changing the return type of the callback function
> that iterate_over_threads takes from int to bool.
> 
> But then I thought, why not make the whole thing templated, and remove
> the need to pass void* arguments around.
> 
> So now we can do:
> 
>   bool
>   callback_1 (struct thread_info *tp)
>   { ... }
> 
>   bool
>   callback_2 (struct thread_info *tp, const struct some_type &obj)
>   { ... }
> 
>   void
>   some_other_part_of_gdb ()
>   {
>     iterate_over_threads (callback_1);
> 
>     iterate_over_threads (callback_2, object_of_some_type);
>   }
> 
> Which seems much nicer to me.
> 
> While updating all the callback functions I took this opportunity to
> remove a code construct I really hate:
> 
>   if (condition)
>     return true;
>   return false;
> 
> This has now become:
> 
>   return (condition);
> 
> There should be no user visible changes after this commit.

I kind of recall that when I introduced the find_thread /
for_each_thread functions in GDBserver, I had done the way you did, but
Pedro suggested to just have a single Func parameter (like it is now),
and that we could use lambdas if needed.  And that the syntax

    for_each_thread (my_callback, some, args);

... doesn't make it super clear what's happening.  Versus:

    for_each_thread ([captures] (thread_info *thread)
      {
         my_callback (thread, some, args);
      });

... which makes it look kind of like a regular for loop.

But speaking of for loops, the most readable and straightforward of all
would probably be to just use range-for loop directly.  Instead of
writing:

  iterate_over_threads (proceed_thread_callback, pid);

write:

  for (thread_info *thread : all_threads_safe ())
    proceed_thread (thread, pid);

And if it's easy to determine that the for loop body won't delete the
thread, use all_threads instead of all_threads_safe (I wouldn't be
suprised if the cost of using all_threads_safe over all_threads was
insignificant, so perhaps we could have only all_threads, and make it
safe by default).

For the cases where iterate_over_threads is used to find a thread (i.e.
the callback doesn't always return false), then it could also be
transformed in a ranged-for plus an "if" plus a "break".  But that just
makes the calling code more complex, I don't suggest that.  I would
instead suggest to keep using iterate_over_threads, but rename it
find_thread.  find_thread speaks more about what the intent is, whereas
iterate_over_threads speaks about the mean.  So something like:

  t = find_thread ([captures] (thread_info *thread)
    {
      return my_predicate (thread);
    };

Having find_thread in GDB would align it with how it is in GDBserver.
And I think we could get rid of for_each_thread in GDBserver in favor of
range-based for loops.

Simon

Re: [PATCH] gdb: make iterate_over_threads a template function

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This started off as changing the return type of the callback function
Andrew> that iterate_over_threads takes from int to bool.

Andrew> But then I thought, why not make the whole thing templated, and remove
Andrew> the need to pass void* arguments around.

The usual way to do this in gdb is to have the iteration function accept
a gdb::function_view.  This lets it work with lambdas, etc.

That said, I tend to agree with Simon's response here.  In particular,
so-called "external iteration" is, IMO, usually simpler to read and
understand, and is also more accommodating to special cases.  That may
be a bigger change than you want to make though.

thanks,
Tom