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

Kamil Rytarowski
2020-04-01T21:40:37Z

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