From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28751 invoked by alias); 5 Sep 2014 20:20:08 -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 28739 invoked by uid 89); 5 Sep 2014 20:20:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 05 Sep 2014 20:20:06 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1XPzzG-0000uk-Jb from donb@codesourcery.com for gdb-patches@sourceware.org; Fri, 05 Sep 2014 13:20:02 -0700 Received: from [127.0.0.1] ([172.30.9.231]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 5 Sep 2014 13:20:02 -0700 Message-ID: <540A1AF1.8010102@codesourcery.com> Date: Fri, 05 Sep 2014 20:20:00 -0000 From: "Breazeal, Don" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: Re: [PATCH 01/16 v2] Refactor native follow-fork References: <1407434395-19089-1-git-send-email-donb@codesourcery.com> <1408580964-27916-2-git-send-email-donb@codesourcery.com> <5409C69F.8030906@redhat.com> <540A0765.7080602@codesourcery.com> In-Reply-To: <540A0765.7080602@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00159.txt.bz2 One clarification... On 9/5/2014 11:56 AM, Breazeal, Don wrote: > Hi Pedro, > Thanks for reviewing this. > > On 9/5/2014 7:20 AM, Pedro Alves wrote: >> linux_child_follow_fork ends up with: >> >> static int >> linux_child_follow_fork (struct target_ops *ops, int follow_child, >> int detach_fork) >> { >> int has_vforked; >> int parent_pid, child_pid; >> >> has_vforked = (inferior_thread ()->pending_follow.kind >> == TARGET_WAITKIND_VFORKED); >> parent_pid = ptid_get_lwp (inferior_ptid); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> if (parent_pid == 0) >> parent_pid = ptid_get_pid (inferior_ptid); >> child_pid >> = ptid_get_pid (inferior_thread ()->pending_follow.value.related_pid); >> >> if (!follow_child) >> { >> ... >> } >> else >> { >> struct lwp_info *child_lp; >> >> child_lp = add_lwp (inferior_ptid); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> child_lp->stopped = 1; >> child_lp->last_resume_kind = resume_stop; >> >> /* Let the thread_db layer learn about this new process. */ >> check_for_thread_db (); >> } >> } >> >> Nothing appears to switch inferior_ptid to the child, so seems >> like we're adding the child_lp with the wrong lwp (and calling >> check_for_thread_db in the wrong context) ? Is this managing >> to work by chance because follow_fork_inferior leaves inferior_ptid >> pointing to the child? > > Yes, follow_fork_inferior always sets inferior_ptid to the followed > inferior. On entry, linux_child_follow_fork expects inferior_ptid to be > the followed inferior. So I think it is getting the correct inferior > from inferior_ptid in these cases. I can change that if you prefer; see > my question below about acceptable solutions. Er... I can change how inferior_ptid is passed to linux_child_follow_fork, not whether the correct ptid is used. :-P > > Regarding check_for_thread_db, there is something unrelated that I don't > understand here. If we have reached this function, then aren't we > guaranteed that PTRACE_O_TRACECLONE is supported, and that we are using > that instead of libthread_db for detecting thread events? If so, why do > we need to call check_for_thread_db at all? > > Then this at the top uses the wrong >> inferior_thread (): >> >> has_vforked = (inferior_thread ()->pending_follow.kind >> == TARGET_WAITKIND_VFORKED); >> >> >> and we're lucky that nothing end up using has_vforked in the >> follow child path? > > You are right, this is incorrect and unnecessary in the case where we > are following the child. > >> >> I'd much rather we don't have these assumptions in place. > > Would an acceptable solution be to move the definitions and assignments > of has_vforked, parent_pid, and child_pid into the follow-parent case, > as below? > > Would you also prefer that on entry to linux_child_follow_fork, > inferior_ptid is set to the parent like it was before, or would a > comment explaining that inferior_ptid is expected to be the followed > inferior be sufficient? > > static int > linux_child_follow_fork (struct target_ops *ops, int follow_child, > int detach_fork) > { > if (!follow_child) > { > struct lwp_info *child_lp = NULL; > int status = W_STOPCODE (0); > struct cleanup *old_chain; > int has_vforked; > int parent_pid, child_pid; > > has_vforked = (inferior_thread ()->pending_follow.kind > == TARGET_WAITKIND_VFORKED); > parent_pid = ptid_get_lwp (inferior_ptid); > if (parent_pid == 0) > parent_pid = ptid_get_pid (inferior_ptid); > child_pid > = ptid_get_pid (inferior_thread > ()->pending_follow.value.related_pid); > >> >> These files / targets also have to_follow_fork implementations: >> >> inf-ptrace.c: t->to_follow_fork = inf_ptrace_follow_fork; >> inf-ttrace.c: t->to_follow_fork = inf_ttrace_follow_fork; >> >> which will break if we don't adjust them as well. Did you >> check whether the refactored code (follow_fork_inferior) >> makes sense for those? > > I completely missed these; sorry about that. In theory I should be able > to make similar changes to these that maintains the existing > functionality. I don't currently have a way (that I know of) to test > either of them. Testing requires a non-Linux version of Unix and an > HP-UX system, correct? I'll start work on the changes in spite of that. > >> >> Thanks, >> Pedro Alves >> > > Thanks, > --Don > >