From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111790 invoked by alias); 14 Jan 2016 15:29:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 111770 invoked by uid 89); 14 Jan 2016 15:29:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Switch, *cleanup, ptid_get_pid, pid_t X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 14 Jan 2016 15:29:41 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 64C54C0A8466; Thu, 14 Jan 2016 15:29:40 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0EFTdhp031701; Thu, 14 Jan 2016 10:29:39 -0500 Message-ID: <5697BEE3.3020603@redhat.com> Date: Thu, 14 Jan 2016 15:29:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: John Baldwin , gdb-patches@sourceware.org Subject: Re: [PATCH v2 5/6] Add support for LWP-based threads on FreeBSD. References: <1452721551-657-1-git-send-email-jhb@FreeBSD.org> <1452721551-657-6-git-send-email-jhb@FreeBSD.org> In-Reply-To: <1452721551-657-6-git-send-email-jhb@FreeBSD.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-01/txt/msg00302.txt.bz2 [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