Re: [PATCH v2 5/6] Add support for LWP-based threads on FreeBSD.

Pedro Alves
2016-01-14T15:29:44Z

[Dropping binutils.]

Hi John,

This deserves a NEWS entry.

New commands ("set debug fbsd-lwp") need to documented in
the manual, even debug ones.

This generally looks good to me. A few minor comments inline below.

On 01/13/2016 09:45 PM, John Baldwin wrote:

> +
> +static int
> +fbsd_thread_alive (struct target_ops *ops, ptid_t ptid)
> +{
> + if (ptid_lwp_p (ptid))
> + {
> + struct ptrace_lwpinfo pl;
> +
> + if (ptrace (PT_LWPINFO, ptid_get_lwp (ptid), (caddr_t)&pl, sizeof pl)
> + == -1)

Space after cast.

> + return 0;
> +#ifdef PL_FLAG_EXITED
> + if (pl.pl_flags & PL_FLAG_EXITED)
> + return 0;
> +#endif
> + }
> +
> + return 1;
> +}
> +
> +/* Convert PTID to a string. Returns the string in a static
> + buffer. */
> +

> +#ifdef HAVE_STRUCT_PTRACE_LWPINFO_PL_TDNAME
> +/* Return the name assigned to a thread by an application. Returns
> + the string in a static buffer. */
> +
> +static const char *
> +fbsd_thread_name (struct target_ops *self, struct thread_info *thr)
> +{
> + struct ptrace_lwpinfo pl;
> + struct kinfo_proc kp;
> + int pid = ptid_get_pid (thr->ptid);
> + long lwp = ptid_get_lwp (thr->ptid);
> + static char buf[64];

Is this the kernel-side size limit? Worth it of a define/comment.
Mainly looking at the xsnprintf below and wondering whether
a we could see a longer name and thus cause gdb to internal error.

> +
> + /* Note that ptrace_lwpinfo returns the process command in pl_tdname
> + if a name has not been set explicitly. Return a NULL name in
> + that case. */
> + fbsd_fetch_kinfo_proc (pid, &kp);
> + if (ptrace (PT_LWPINFO, lwp, (caddr_t)&pl, sizeof pl) == -1)
> + perror_with_name (("ptrace"));

Space after cast.

> + if (strcmp (kp.ki_comm, pl.pl_tdname) == 0)
> + return NULL;
> + xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
> + return buf;
> +}
> +#endif
> +
> +#ifdef PT_LWP_EVENTS
> +/* Enable LWP events for a specific process.
> +
> + To catch LWP events, PT_LWP_EVENTS is set on every traced process.
> + This enables stops on the birth for new LWPs (excluding the "main" LWP)
> + and the death of LWPs (excluding the last LWP in a process). Note
> + that unlike fork events, the LWP that creates a new LWP does not
> + report an event. */
> +
> +static void
> +fbsd_enable_lwp_events (pid_t pid)
> +{
> + if (ptrace (PT_LWP_EVENTS, pid, (PTRACE_TYPE_ARG3)0, 1) == -1)
> + perror_with_name (("ptrace"));

Space after cast.

> +}
> +#endif
> +
> +/* Add threads for any new LWPs in a process.
> +
> + When LWP events are used, this function is only used to detect existing
> + threads when attaching to a process. On older systems, this function is
> + called to discover new threads each time the thread list is updated. */
> +
> +static void
> +fbsd_add_threads (pid_t pid)
> +{
> + struct cleanup *cleanup;
> + lwpid_t *lwps;
> + int i, nlwps;
> +
> + gdb_assert (!in_thread_list (pid_to_ptid (pid)));
> + nlwps = ptrace (PT_GETNUMLWPS, pid, NULL, 0);
> + if (nlwps == -1)
> + perror_with_name (("ptrace"));
> +
> + lwps = XCNEWVEC (lwpid_t, nlwps);
> + cleanup = make_cleanup (xfree, lwps);
> +
> + nlwps = ptrace (PT_GETLWPLIST, pid, (caddr_t)lwps, nlwps);
> + if (nlwps == -1)
> + perror_with_name (("ptrace"));
> +
> + for (i = 0; i < nlwps; i++)
> + {
> + ptid_t ptid = ptid_build (pid, lwps[i], 0);
> +
> + if (!in_thread_list (ptid))
> + {
> +#ifdef PT_LWP_EVENTS
> + struct ptrace_lwpinfo pl;
> +
> + /* Don't add exited threads. Note that this is only called
> + when attaching to a multi-threaded process. */
> + if (ptrace (PT_LWPINFO, lwps[i], (caddr_t)&pl, sizeof pl) == -1)

Space after cast.

> + perror_with_name (("ptrace"));
> + if (pl.pl_flags & PL_FLAG_EXITED)
> + continue;
> +#endif
> + if (debug_fbsd_lwp)
> + fprintf_unfiltered (gdb_stdlog,
> + "FLWP: adding thread for LWP %u\n",
> + lwps[i]);
> + add_thread (ptid);
> + }
> + }
> + do_cleanups (cleanup);
> +}
> +

> +static void (*super_resume) (struct target_ops *,
> + ptid_t,
> + int,
> + enum gdb_signal);
> +
> +static int
> +resume_one_thread_cb(struct thread_info *tp, void *data)

Space after _cb.

> +{
> + ptid_t *ptid = data;
> + int request;
> +
> + if (ptid_get_pid (tp->ptid) != ptid_get_pid (*ptid))
> + return 0;
> +
> + if (ptid_get_lwp (tp->ptid) == ptid_get_lwp (*ptid))
> + request = PT_RESUME;
> + else
> + request = PT_SUSPEND;
> +
> + if (ptrace (request, ptid_get_lwp (tp->ptid), (caddr_t)0, 0) == -1)

Space after cast.

> + perror_with_name (("ptrace"));
> + return 0;
> +}
> +
> +static int
> +resume_all_threads_cb(struct thread_info *tp, void *data)

Missing space.

> +{
> + ptid_t *filter = data;
> +
> + if (!ptid_match (tp->ptid, *filter))
> + return 0;
> +
> + if (ptrace (PT_RESUME, ptid_get_lwp (tp->ptid), (caddr_t)0, 0) == -1)

Missing space.

> + perror_with_name (("ptrace"));
> + return 0;
> +}
> +

> @@ -331,18 +581,80 @@ fbsd_wait (struct target_ops *ops,
> if (ptrace (PT_LWPINFO, pid, (caddr_t)&pl, sizeof pl) == -1)
> perror_with_name (("ptrace"));
>
> + wptid = ptid_build (pid, pl.pl_lwpid, 0);
> +
> +#ifdef PT_LWP_EVENTS
> + if (pl.pl_flags & PL_FLAG_EXITED)
> + {
> + /* If GDB attaches to a multi-threaded process, exiting
> + threads might be skipped during fbsd_post_attach that
> + have not yet reported their PL_FLAG_EXITED event.
> + Ignore EXITED events for an unknown LWP. */
> + if (in_thread_list (wptid))
> + {
> + if (debug_fbsd_lwp)
> + fprintf_unfiltered (gdb_stdlog,
> + "FLWP: deleting thread for LWP %u\n",
> + pl.pl_lwpid);
> + if (print_thread_events)
> + printf_unfiltered (_("[%s exited]\n"), target_pid_to_str
> + (wptid));
> + delete_thread (wptid);
> + }
> + if (ptrace (PT_CONTINUE, pid, (PTRACE_TYPE_ARG3)1, 0) == -1)

Missing space.

> + perror_with_name (("ptrace"));
> + continue;
> + }
> +#endif
> +
> + /* Switch to an LWP PTID on the first stop in a new process.
> + This is done after handling PL_FLAG_EXITED to avoid
> + switching to an exited LWP. It is done before checking
> + PL_FLAG_BORN in case the first stop reported after
> + attaching to an existing process is a PL_FLAG_BORN
> + event. */
> + if (in_thread_list (pid_to_ptid (pid)))
> + {
> + if (debug_fbsd_lwp)
> + fprintf_unfiltered (gdb_stdlog,
> + "FLWP: using LWP %u for first thread\n",
> + pl.pl_lwpid);
> + thread_change_ptid (pid_to_ptid (pid), wptid);
> + }
> +
> +#ifdef PT_LWP_EVENTS
> + if (pl.pl_flags & PL_FLAG_BORN)
> + {
> + /* If GDB attaches to a multi-threaded process, newborn
> + threads might be added by fbsd_add_threads that have
> + not yet reported their PL_FLAG_BORN event. Ignore
> + BORN events for an already-known LWP. */
> + if (!in_thread_list (wptid))
> + {
> + if (debug_fbsd_lwp)
> + fprintf_unfiltered (gdb_stdlog,
> + "FLWP: adding thread for LWP %u\n",
> + pl.pl_lwpid);
> + add_thread (wptid);
> + }
> + ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> + return wptid;
> + }
> +#endif
> +
> #ifdef TDP_RFPPWAIT
> if (pl.pl_flags & PL_FLAG_FORKED)
> {
> struct kinfo_proc kp;
> + ptid_t child_ptid;
> pid_t child;
>
> child = pl.pl_child_pid;
> ourstatus->kind = TARGET_WAITKIND_FORKED;
> - ourstatus->value.related_pid = pid_to_ptid (child);
>
> /* Make sure the other end of the fork is stopped too. */
> - if (!fbsd_is_child_pending (child))
> + child_ptid = fbsd_is_child_pending (child);
> + if (ptid_equal (child_ptid, null_ptid))
> {
> pid = waitpid (child, &status, 0);
> if (pid == -1)
> @@ -354,6 +666,7 @@ fbsd_wait (struct target_ops *ops,
> perror_with_name (("ptrace"));
>
> gdb_assert (pl.pl_flags & PL_FLAG_CHILD);
> + child_ptid = ptid_build(child, pl.pl_lwpid, 0);

Missing space.

> }
>
> /* For vfork, the child process will have the P_PPWAIT
> @@ -361,6 +674,7 @@ fbsd_wait (struct target_ops *ops,
> fbsd_fetch_kinfo_proc (child, &kp);
> if (kp.ki_flag & P_PPWAIT)
> ourstatus->kind = TARGET_WAITKIND_VFORKED;
> + ourstatus->value.related_pid = child_ptid;
>
> return wptid;
> }
> @@ -370,7 +684,7 @@ fbsd_wait (struct target_ops *ops,
> /* Remember that this child forked, but do not report it
> until the parent reports its corresponding fork
> event. */
> - fbsd_remember_child (ptid_get_pid (wptid));
> + fbsd_remember_child (wptid);
> continue;
> }
> #endif
> @@ -449,13 +763,19 @@ fbsd_enable_follow_fork (pid_t pid)
> if (ptrace (PT_FOLLOW_FORK, pid, (PTRACE_TYPE_ARG3)0, 1) == -1)
> perror_with_name (("ptrace"));
> }
> +#endif
>

> +/* Provide a prototype to silence -Wmissing-prototypes. */
> +extern initialize_file_ftype _initialize_fbsd_nat;
> +
> +void
> +_initialize_fbsd_nat (void)
> +{
> +#ifdef PT_LWPINFO
> + add_setshow_boolean_cmd ("fbsd-lwp", class_maintenance,
> + &debug_fbsd_lwp, _("\
> +Set debugging of FreeBSD lwp module."), _("\
> +Show debugging of FreeBSD lwp module."), _("\
> +Enables printf debugging output."),
> + NULL,
> + NULL,

Please implement a show callback, for i18n.

Thanks,
Pedro Alves