Re: [PATCH 05/11] gdb, gdbserver: split iterate_over_lwps in for_each_lwp and find_lwp

[Andrew Burgess <aburgess@redhat.com>]

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

> From: Simon Marchi <simon.marchi@polymtl.ca>


> Subject: Re: [PATCH 05/11] gdb, gdbserver: split iterate_over_lwps in for_each_lwp and find_lwp

The subject line should say 'into' maybe?

>
> Even though it works, I have always been mildly annoyed by
> iterate_over_lwps being used for both iterating over all lwps and
> finding one lwp matching a criterion.  I think it would be clearer to
> have two functions for the two use cases.  Then it would be 100% clear
> at the call site what the intention is.  It would be clear that a
> callback returning bool is meant to be a predicate for the find
> function, while a callback returning void is meant to be a callback for
> the "for each" function.
>
> Therefore, split iterate_over_lwps in two:
>
>  - find_lwp to find the first lwp matching a boolean predicate (and the
>    given ptid filter)
>  - for_each_lwp to apply a function on all lwps (optionally filtering by
>    ptid or pid)
>
> The callbacks given to for_each_lwp can now return void.
>
> Introduce some overloads for for_each_lwp, for the various common use
> cases:
>
>  - filtering by ptid
>  - filtering by pid
>  - no ptid/pid filter
>
> find_lwp and two overloads of for_each_lwp are actually only used in
> gdb/linux-nat.c, so make them local to that file.  Only the pid variant
> of for_each_lwp is used in shared code.
>
> The pattern used in this patch serves as the basis for subsequent
> patches that split other "iterate over" functions the same way.
>
> Change-Id: I49d3af0916622300cc81e3c32d22e1aff13cf38f
> ---
>  gdb/arm-linux-nat.c                |  38 ++--
>  gdb/linux-nat.c                    | 276 +++++++++++++++--------------
>  gdb/nat/aarch64-linux-hw-point.c   |  17 +-
>  gdb/nat/linux-nat.h                |  16 +-
>  gdb/nat/loongarch-linux-hw-point.c |  18 +-
>  gdb/nat/x86-linux-dregs.c          |  15 +-
>  gdb/ppc-linux-nat.c                |  22 +--
>  gdb/s390-linux-nat.c               |  22 +--
>  gdbserver/linux-low.cc             |  20 +--
>  9 files changed, 208 insertions(+), 236 deletions(-)
>

> @@ -2940,7 +2953,7 @@ select_event_lwp (ptid_t filter, struct lwp_info **orig_lp, int *status)
>  
>  /* Return non-zero if LP has been resumed.  */

This comment needs updating now a bool is returned.

>  
> -static int
> +static bool
>  resumed_callback (struct lwp_info *lp)
>  {
>    return lp->resumed;

> diff --git a/gdb/nat/linux-nat.h b/gdb/nat/linux-nat.h
> index 42c0467191fe..4414749b93de 100644
> --- a/gdb/nat/linux-nat.h
> +++ b/gdb/nat/linux-nat.h
> @@ -46,17 +46,15 @@ extern tribool have_ptrace_getregset;
>  
>  extern ptid_t current_lwp_ptid (void);
>  
> -/* Function type for the CALLBACK argument of iterate_over_lwps.  */
> -using iterate_over_lwps_ftype = gdb::function_view<int (lwp_info *lwp)>;
> +/* Function type for the CALLBACK argument of for_each_lwp.  */
>  
> -/* Iterate over all LWPs.  Calls CALLBACK with its second argument set
> -   to DATA for every LWP in the list.  If CALLBACK returns nonzero for
> -   a particular LWP, return a pointer to the structure describing that
> -   LWP immediately.  Otherwise return NULL.  This function must be
> -   provided by the client.  */
> +using for_each_lwp_ftype = gdb::function_view<void (lwp_info *lwp)>;
>  
> -extern lwp_info *iterate_over_lwps (ptid_t filter,
> -				    iterate_over_lwps_ftype callback);
> +/* Iterate over all LWPs, calling CALLBACK for every LWP.
> +
> +   Only consider the LWPs with that pid.  */

Maybe: "Only consider the LWPs that match PID." would be better?

> +
> +extern void for_each_lwp (int pid, for_each_lwp_ftype callback);
>  
>  /* Return the ptid of LWP.  */
>  

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

Thanks,
Andrew