Re: [PATCH 06/11] gdb: split iterate_over_threads in for_each_thread and find_thread

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@efficios.com> writes:

The subject line:

  Subject: Re: [PATCH 06/11] gdb: split iterate_over_threads in for_each_thread and find_thread

should say 'into' not 'in'.  But otherwise:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> Same rationale as the previous patch, I think the code would be clearer
> and simpler with separate "for each" and "find" functions rather than
> one that does both jobs.
>
> No need for the callbacks of the "for each" function to return anything,
> as none of them needs to interrupt the iteration.
>
> Change-Id: I4b7bcc5e9a319369d75a22b11114e943951e546a
> ---
>  gdb/aix-thread.c |  2 +-
>  gdb/breakpoint.c |  3 +--
>  gdb/fbsd-tdep.c  |  2 +-
>  gdb/gdbthread.h  | 21 +++++++++++++++++----
>  gdb/infcmd.c     |  9 ++++-----
>  gdb/infrun.c     |  4 ++--
>  gdb/mi/mi-main.c | 15 ++++++---------
>  gdb/procfs.c     |  6 +++---
>  gdb/sol-thread.c |  4 ++--
>  gdb/thread.c     | 27 ++++++++++++---------------
>  10 files changed, 49 insertions(+), 44 deletions(-)
>
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index 5469ddca4f79..675d8d5070cd 100644
> --- a/gdb/aix-thread.c
> +++ b/gdb/aix-thread.c
> @@ -894,7 +894,7 @@ pd_update (pid_t pid)
>  
>    tid = get_signaled_thread (pid);
>    if (tid != 0)
> -    thread = iterate_over_threads ([&] (struct thread_info *thread)
> +    thread = find_thread ([&] (thread_info *thread)
>        {
>  	return thread->ptid.lwp () == tid;
>        });
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 99447b532306..101dc57ee6bb 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12694,10 +12694,9 @@ delete_breakpoint (struct breakpoint *bpt)
>       event-top.c won't do anything, and temporary breakpoints with
>       commands won't work.  */
>  
> -  iterate_over_threads ([&] (struct thread_info *th)
> +  for_each_thread ([&] (struct thread_info *th)
>      {
>        bpstat_remove_bp_location (th->control.stop_bpstat, bpt);
> -      return false;
>      });
>  
>    /* Now that breakpoint is removed from breakpoint list, update the
> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
> index 4bbe0c120e6a..419f935ea72f 100644
> --- a/gdb/fbsd-tdep.c
> +++ b/gdb/fbsd-tdep.c
> @@ -705,7 +705,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);
> +      signalled_thr = find_thread (find_signalled_thread);
>        if (signalled_thr == NULL)
>  	signalled_thr = curr_thr;
>      }
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index c56c4ce40366..729dcf5ab068 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -791,10 +791,23 @@ 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 gdb::function_view<bool (struct thread_info *)> thread_callback_func;
> -extern struct thread_info *iterate_over_threads (thread_callback_func);
> +/* 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 *)>;
> +
> +/* Return the first thread for which CALLBACK returns true, or nullptr if
> +   there is no such thread.  */
> +
> +extern struct thread_info *find_thread (find_thread_callback_ftype callback);
>  
>  /* Pull in the internals of the inferiors/threads ranges and
>     iterators.  Must be done after struct thread_info is defined.  */
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index c6519b71abcb..4a907ea8ce86 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -685,7 +685,7 @@ starti_command (const char *args, int from_tty)
>    run_command_1 (args, from_tty, RUN_STOP_AT_FIRST_INSN);
>  }
>  
> -static bool
> +static void
>  proceed_thread_callback (struct thread_info *thread)
>  {
>    /* We go through all threads individually instead of compressing
> @@ -698,15 +698,14 @@ proceed_thread_callback (struct thread_info *thread)
>       way to tell the target `hold this thread stopped until I say
>       otherwise', then we can optimize this.  */
>    if (thread->state () != THREAD_STOPPED)
> -    return false;
> +    return;
>  
>    if (!thread->inf->has_execution ())
> -    return false;
> +    return;
>  
>    switch_to_thread (thread);
>    clear_proceed_status (0);
>    proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> -  return false;
>  }
>  
>  static void
> @@ -763,7 +762,7 @@ continue_1 (int all_threads)
>        scoped_disable_commit_resumed disable_commit_resumed
>  	("continue all threads in non-stop");
>  
> -      iterate_over_threads (proceed_thread_callback);
> +      for_each_thread (proceed_thread_callback);
>  
>        if (current_ui->prompt_state == PROMPT_BLOCKED)
>  	{
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 19836c4d89e7..b295956f8ea5 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6695,7 +6695,7 @@ restart_threads (struct thread_info *event_thread, inferior *inf)
>      }
>  }
>  
> -/* Callback for iterate_over_threads.  Find a resumed thread that has
> +/* Callback for find_thread.  Find a resumed thread that has
>     a pending waitstatus.  */
>  
>  static bool
> @@ -6777,7 +6777,7 @@ finish_step_over (struct execution_control_state *ecs)
>        if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
>         return 0;
>  
> -      pending = iterate_over_threads (resumed_thread_with_pending_status);
> +      pending = find_thread (resumed_thread_with_pending_status);
>        if (pending != nullptr)
>  	{
>  	  struct thread_info *tp = ecs->event_thread;
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index bf08fe822b32..7e448947a2fe 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -278,10 +278,9 @@ exec_continue (const char *const *argv, int argc)
>  	      pid = inf->pid;
>  	    }
>  
> -	  iterate_over_threads ([&] (struct thread_info *thread)
> +	  for_each_thread ([&] (struct thread_info *thread)
>  	    {
>  	      proceed_thread (thread, pid);
> -	      return false;
>  	    });
>  	  disable_commit_resumed.reset_and_commit ();
>  	}
> @@ -364,16 +363,15 @@ 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");
>  
> -      iterate_over_threads ([&] (struct thread_info *thread)
> +      for_each_thread ([&] (struct thread_info *thread)
>  	{
>  	  if (thread->state () != THREAD_RUNNING)
> -	    return false;
> +	    return;
>  
>  	  if (thread->ptid.pid () != inf->pid)
> -	    return false;
> +	    return;
>  
>  	  target_stop (thread->ptid);
> -	  return false;
>  	});
>      }
>    else
> @@ -505,7 +503,7 @@ mi_cmd_target_detach (const char *command, const char *const *argv, int argc)
>  
>        /* Pick any thread in the desired process.  Current
>  	 target_detach detaches from the parent of inferior_ptid.  */
> -      tp = iterate_over_threads ([&] (struct thread_info *ti)
> +      tp = find_thread ([&] (struct thread_info *ti)
>  	{
>  	  return ti->ptid.pid () == pid && ti->state () != THREAD_EXITED;
>  	});
> @@ -604,7 +602,7 @@ print_one_inferior (struct inferior *inferior, bool recurse,
>  
>        if (inferior->pid != 0)
>  	{
> -	  iterate_over_threads ([&] (struct thread_info *ti)
> +	  for_each_thread ([&] (struct thread_info *ti)
>  	    {
>  	      if (ti->ptid.pid () == inferior->pid)
>  		{
> @@ -613,7 +611,6 @@ print_one_inferior (struct inferior *inferior, bool recurse,
>  		  if (core != -1)
>  		    cores.insert (core);
>  		}
> -	      return false;
>  	    });
>  	}
>  
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index cea9f823bbca..a208393da073 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -2395,8 +2395,8 @@ procfs_xfer_memory (gdb_byte *readbuf, const gdb_byte *writebuf,
>     descriptors for the parent process, but discard any file
>     descriptors we may have accumulated for the threads.
>  
> -   As this function is called by iterate_over_threads, it always
> -   returns zero (so that iterate_over_threads will keep
> +   As this function is called by proc_iterate_over_threads, it always
> +   returns zero (so that proc_iterate_over_threads will keep
>     iterating).  */
>  
>  static int
> @@ -3541,7 +3541,7 @@ find_signalled_thread (struct thread_info *info)
>  static enum gdb_signal
>  find_stop_signal (void)
>  {
> -  struct thread_info *info = iterate_over_threads (find_signalled_thread);
> +  struct thread_info *info = find_thread (find_signalled_thread);
>  
>    if (info)
>      return info->stop_signal ();
> diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
> index c765a4205a04..b5a68b9e46c1 100644
> --- a/gdb/sol-thread.c
> +++ b/gdb/sol-thread.c
> @@ -1119,14 +1119,14 @@ sol_thread_target::get_ada_task_ptid (long lwp, ULONGEST thread)
>         };
>  
>    struct thread_info *thread_info
> -    = iterate_over_threads (thread_db_find_thread_from_tid);
> +    = find_thread (thread_db_find_thread_from_tid);
>  
>    if (thread_info == NULL)
>      {
>        /* The list of threads is probably not up to date.  Find any
>  	 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_info = find_thread (thread_db_find_thread_from_tid);
>      }
>  
>    gdb_assert (thread_info != NULL);
> diff --git a/gdb/thread.c b/gdb/thread.c
> index a6ae2a751397..b47479b12e8e 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -597,28 +597,25 @@ 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.
> - */
> +/* 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 *
> -iterate_over_threads (gdb::function_view<bool (struct thread_info *)> callback)
> +find_thread (find_thread_callback_ftype callback)
>  {
>    for (thread_info &tp : all_threads_safe ())
>      if (callback (&tp))
>        return &tp;
>  
> -  return NULL;
> +  return nullptr;
>  }
>  
>  /* See gdbthread.h.  */
> -- 
> 2.53.0