From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8443 invoked by alias); 5 Sep 2014 18:56:45 -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 8431 invoked by uid 89); 5 Sep 2014 18:56:43 -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 18:56:42 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1XPygY-0001sk-CO from donb@codesourcery.com ; Fri, 05 Sep 2014 11:56:38 -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 11:56:38 -0700 Message-ID: <540A0765.7080602@codesourcery.com> Date: Fri, 05 Sep 2014 18:56: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: Pedro Alves , 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> In-Reply-To: <5409C69F.8030906@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00151.txt.bz2 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. 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