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

[Simon Marchi <simon.marchi@efficios.com>]

On 4/17/26 7:25 AM, Andrew Burgess wrote:
> 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?

Ok, fixed that in all other commits too.

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

Fixed.

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

Fixed.

>> +
>> +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,

Simon