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

[Kamil Rytarowski <kamil@netbsd.org>]

[-- 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 --]