From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25167 invoked by alias); 9 Sep 2014 10:57:34 -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 25072 invoked by uid 89); 9 Sep 2014 10:57:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Tue, 09 Sep 2014 10:57:32 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s89AvT96024430 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 9 Sep 2014 06:57:29 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s89AvRk5028672; Tue, 9 Sep 2014 06:57:28 -0400 Message-ID: <540EDD17.30001@redhat.com> Date: Tue, 09 Sep 2014 10:57:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: "Breazeal, Don" , 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-SW-Source: 2014-09/txt/msg00218.txt.bz2 On 09/05/2014 07:56 PM, 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. Ah. > On entry, linux_child_follow_fork expects inferior_ptid to be > the followed inferior. I see. It does make sense. > 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? Unlike GDBserver, GDB actually still use libthread_db's create/exit event breakpoints even if PTRACE_O_TRACECLONE is supported. I think this is just historical at this point, and we could skip those event breakpoints, optimizing out a bunch of internal stops. The check_for_thread_db call has another effect -- as mentioned in the comment: /* Let the thread_db layer learn about this new process. */ check_for_thread_db (); if we don't call it, then linux-thread-db.c never adds the child process to its global thread_db_list list: /* List of known processes using thread_db, and the required bookkeeping. */ struct thread_db_info *thread_db_list; We don't really need to try all the available libthread_db's found in the path, we could just try try_thread_db_load with the same libthread_db we had loaded for the parent, or even skip that and share/refcount the bookkeeping in 'struct thread_db_info' (dlopen handle, functions pointers, etc.) between parent and child. This is about the same issue as mentioned just above: /* Let the shared library layer (solib-svr4) learn about this new process, relocate the cloned exec, pull in shared libraries, and install the solib event breakpoint. If a "cloned-VM" event was propagated better throughout the core, this wouldn't be required. */ > > 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? Yes. > > 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? The comment would be sufficient. Thanks, Pedro Alves