Re: [PATCH] Implement basic threading support in the NetBSD target

[Kamil Rytarowski <n54@gmx.com>]

On 01.04.2020 22:43, Tom Tromey wrote:
>>>>>> "Kamil" == Kamil Rytarowski <n54@gmx.com> writes:
>
> Kamil> Use sysctl(3) as the portable interface to prompt NetBSD threads on
> Kamil> all supported NetBSD versions. In future newer versions could switch
> Kamil> to PT_LWPSTATUS ptrace(2) API that will be supported on NetBSD 10.0
> Kamil> and newer.
>
> Thank you for the patch.
>
> Kamil> +static void
> Kamil> +nbsd_thread_lister (const pid_t pid,
> Kamil> +		    std::function<bool (const struct kinfo_lwp *)> callback)
>
> Perhaps a const reference here would be better.
> Or maybe gdb::function_view, I'm not sure.
>

I've switched to gdb::function_view.

> Kamil> +      /* Return true if the specified thread is alive.  */
> Kamil> +      auto lwp_alive =
> Kamil> +	[&l]()
> Kamil> +	{
> Kamil> +	  switch (kl->l_stat)
> Kamil> +	    {
> Kamil> +	    case LSSLEEP:
> Kamil> +	    case LSRUN:
> Kamil> +	    case LSONPROC:
> Kamil> +	    case LSSTOP:
> Kamil> +	    case LSSUSPENDED:
> Kamil> +	      return true;
> Kamil> +	    default:
> Kamil> +	      return false;
> Kamil> +	    }
> Kamil> +	};
> Kamil> +
> Kamil> +      /* Ignore embryonic or demised threads.  */
> Kamil> +      if (!lwp_alive (l))
> Kamil> +	continue;
>
> I don't think a lambda is needed here.
>

I like lambda here as it matches the internals of the kernel behavior
here. Normally, I would add a local lwp_alive() helper function, but as
it is not needed anywhere else, I added it as a local inline lambda
function.

Using lambda a little bit simplifies the code as I can avoid "break"
that is nested inside for() and switch(). I could rewrite the code, but
in the current form it nicer reflects the NetBSD kernel.

PT_LWPSTATUS/PT_LWPNEXT would have implied lwp_alive().

> Kamil> +bool
> Kamil> +nbsd_nat_target::thread_alive (ptid_t ptid)
> Kamil> +{
> Kamil> +  pid_t pid = ptid.pid ();
> Kamil> +  int lwp = ptid.lwp ();
> Kamil> +  bool found = false;
> Kamil> +
> Kamil> +  auto fn =
> Kamil> +    [&lwp, &found] (const struct kinfo_lwp *kl)
> Kamil> +    {
> Kamil> +      if (kl->l_lid == lwp)
> Kamil> +	{
> Kamil> +	  found = true;
> Kamil> +	  return true;
> Kamil> +	}
> Kamil> +      return false;
> Kamil> +    };
> Kamil> +
> Kamil> +  nbsd_thread_lister (pid, fn);
> Kamil> +
> Kamil> +  return found;
>
> A classic pattern in gdb is to have one of these iteration functions
> (here, nbsd_thread_lister) return bool and have it return true if any
> callback returns true, and false otherwise.
>
> That would eliminate this "found" variable.
>

Done.

> Kamil> +const char *
> Kamil> +nbsd_nat_target::thread_name (struct thread_info *thr)
> Kamil> +{
> Kamil> +  static char buf[KI_LNAMELEN] = {};
> ...
> Kamil> +  return buf;
>
> This should return NULL if no name was found.
> The above bool return would make this easy as well.
>

Done.

> Kamil> +      fprintf_unfiltered (gdb_stdlog, "NLWP: pid=%d lwp=%d\n", pid, kl->l_lid);
>
> This line should be removed.
>

It was fixed in v2.

> Tom
>

Please review:

[PATCH v4] Implement basic threading support in the NetBSD target