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

[Simon Marchi <simark@simark.ca>]

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.

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

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

Simon