Re: [PATCH v2 05/10] Add gdb/nat common functions for listing threads

Kamil Rytarowski
2020-09-07T20:00:48Z

[-- Attachment #1.1: Type: text/plain, Size: 2877 bytes --]

On 07.09.2020 20:59, Simon Marchi wrote:
> On 2020-09-03 8:29 p.m., Kamil Rytarowski wrote:
>> @@ -39,4 +45,121 @@ pid_to_exec_file (pid_t pid)
>> return buf;
>> }
>>
>> +/* Generic thread (LWP) lister within a specified process. The callback
>> + parameters is a C++ function that is called for each detected thread. */
>
> The behavior seems to be that if the callback returns true, the iteration is
> stopped. If so, it would be good to document it. As well as the return value
> of this function.
>

OK.

>> +
>> +static bool
>> +netbsd_thread_lister (const pid_t pid,
>> + gdb::function_view<bool (const struct kinfo_lwp *)>
>> + callback)
>> +{
>> + int mib[5] = {CTL_KERN, KERN_LWP, pid, sizeof (struct kinfo_lwp), 0};
>> + size_t size;
>> +
>> + if (sysctl (mib, ARRAY_SIZE (mib), NULL, &size, NULL, 0) == -1 || size == 0)
>> + perror_with_name (("sysctl"));
>> +
>> + mib[4] = size / sizeof (size_t);
>> +
>> + gdb::unique_xmalloc_ptr<struct kinfo_lwp[]> kl
>> + ((struct kinfo_lwp *) xcalloc (size, 1));
>> +
>> + if (sysctl (mib, ARRAY_SIZE (mib), kl.get (), &size, NULL, 0) == -1
>> + || size == 0)
>> + perror_with_name (("sysctl"));
>
> Is there a chance that the number of threads changes between the two sysctl
> calls? Or does that assume that the process is not executing? It would be
> good to spell out any such assumption in a comment.
>

This function could be used for alive processes, but currently it is
used only for the stopped ones, thus the number of threads does not change.

>> +
>> + for (size_t i = 0; i < size / sizeof (struct kinfo_lwp); i++)
>> + {
>> + struct kinfo_lwp *l = &kl[i];
>> +
>> + /* Return true if the specified thread is alive. */
>> + auto lwp_alive
>> + = [] (struct kinfo_lwp *lwp)
>> + {
>> + switch (lwp->l_stat)
>> + {
>> + case LSSLEEP:
>> + case LSRUN:
>> + case LSONPROC:
>> + case LSSTOP:
>> + case LSSUSPENDED:
>> + return true;
>> + default:
>> + return false;
>> + }
>> + };
>> +
>> + /* Ignore embryonic or demised threads. */
>> + if (!lwp_alive (l))
>> + continue;
>> +
>> + if (callback (l))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/* Return true if PTID is still active in the inferior. */
>> +
>> +bool
>> +thread_alive (ptid_t ptid)
>> +{
>> + pid_t pid = ptid.pid ();
>> + lwpid_t lwp = ptid.lwp ();
>> +
>> + auto fn
>> + = [&lwp] (const struct kinfo_lwp *kl)
>> + {
>> + return kl->l_lid == lwp;
>> + };
>
> Just a nit, but would it be better to capture variables known to be
> scalars as value instead of as reference?
>

It's not critical here, but I will change it to capture by value.

> Simon
>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]