From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59853 invoked by alias); 23 May 2015 12:05:28 -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 59842 invoked by uid 89); 23 May 2015 12:05:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Sat, 23 May 2015 12:05:17 +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 (8.14.4/8.14.4) with ESMTP id t4NC5E1x031354 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Sat, 23 May 2015 08:05:14 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4NC5Bo2012242; Sat, 23 May 2015 08:05:12 -0400 Message-ID: <55606CF7.90404@redhat.com> Date: Sat, 23 May 2015 12:05:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Don Breazeal , gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Initialize last_resume_kind for remote fork child References: <1432320931-1550-1-git-send-email-donb@codesourcery.com> <1432320931-1550-3-git-send-email-donb@codesourcery.com> In-Reply-To: <1432320931-1550-3-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-05/txt/msg00620.txt.bz2 On 05/22/2015 07:55 PM, Don Breazeal wrote: > This patch fixes some intermittent test failures in > gdb.base/foll-vfork.exp where a vfork child could sometimes be > (incorrectly) resumed when handling the vfork event. In this > case the result was a subsequent event reported to the client > side as a SIGTRAP delivered to the as-yet-unknown child thread. > > The fix is to initialize the threadinfo.last_resume_kind field > for new fork children in gdbserver/linux-low.c:handle_extended_wait. > This prevents the event handler from incorrectly resuming the child > if the stop event that is generated when it starts is reported > before the vfork event that initiated it. I'm not seeing how what comes first makes a difference, actually. > The same thing is done > for the native case in linux-nat.c:linux_child_follow_fork. The code that resumes the child by mistake in question must be resume_stopped_resumed_lwps, and that is called similarly by both backends after pulling events out of the kernel, and right after linux_*_filter_event (which is what calls into handle_extended_wait): /* Now that we've pulled all events out of the kernel, resume LWPs that don't have an interesting event to report. */ iterate_over_lwps (minus_one_ptid, resume_stopped_resumed_lwps, &minus_one_ptid); ... /* Now that we've pulled all events out of the kernel, resume LWPs that don't have an interesting event to report. */ if (stopping_threads == NOT_STOPPING_THREADS) for_each_inferior (&all_threads, resume_stopped_resumed_lwps); That happens before linux_child_follow_fork is reached. The first difference is that gdb's version does not add the fork child to the lwp list in handle_extended_wait. But even if it did, the way linux-nat.c tags lwps as "not stopped-resumed" is different in gdb and gdbserver. gdb's has: static int resume_stopped_resumed_lwps (struct lwp_info *lp, void *data) { ptid_t *wait_ptid_p = data; if (lp->stopped && lp->resumed && !lwp_status_pending_p (lp)) { while gdbserver's: static void resume_stopped_resumed_lwps (struct inferior_list_entry *entry) { struct thread_info *thread = (struct thread_info *) entry; struct lwp_info *lp = get_thread_lwp (thread); if (lp->stopped && !lp->status_pending_p && thread->last_resume_kind != resume_stop && thread->last_status.kind == TARGET_WAITKIND_IGNORE) { So the main difference here is that linux-nat.c checks that "resumed" flag which doesn't exist in the gdbserver version. Checking lwp->resumed in gdb is equivalent to checking: (thread->last_resume_kind != resume_stop && thread->last_status.kind == TARGET_WAITKIND_IGNORE) in gdbserver/linux-low.c. gdbserver/linux-low.c's add_thread function creates threads with last_resume_kind == resume_continue by default. So the fix is to make sure to override that to resume_stop. gdb's linux_child_follow_fork version does the equivalent, but that's not what exactly prevents a spurious resume (it's the fact that add_lwp creates lwps with the 'resume' flag cleared). > > In addition, it seemed prudent to initialize lwp_info.status_pending_p > for the new fork child. That's fine. I think the child can well report a signal other than SIGSTOP first (or even be killed/disappear), in which case we should leave it pending (thus set status_pending_p), but that's a separate, preexisting issue. > I also rearranged the initialization code > so that all of the lwp_info initialization was together, rather than > intermixed with thread_info and process_info initialization. > > Tested native, native-gdbserver, native-extended-gdbserver on > x86_64 GNU/Linux. > > OK? So the fix is OK, though I think the commit log could be clarified. > > Thanks > --Don > > gdb/gdbserver/ > 2015-05-22 Don Breazeal > > * linux-low.c (handle_extended_wait): Seems incomplete. :-) > > --- > gdb/gdbserver/linux-low.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 9f3ea48..d763c66 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -457,6 +457,7 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat) > struct process_info *parent_proc; > struct process_info *child_proc; > struct lwp_info *child_lwp; > + struct thread_info *child_thr; > struct target_desc *tdesc; > > ptid = ptid_build (new_pid, new_pid, 0); > @@ -479,6 +480,10 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat) > child_lwp = add_lwp (ptid); > gdb_assert (child_lwp != NULL); > child_lwp->stopped = 1; > + child_lwp->must_set_ptrace_flags = 1; > + child_lwp->status_pending_p = 0; > + child_thr = get_lwp_thread (child_lwp); > + child_thr->last_resume_kind = resume_stop; > parent_proc = get_thread_process (event_thr); > child_proc->attached = parent_proc->attached; > clone_all_breakpoints (&child_proc->breakpoints, > @@ -488,7 +493,6 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat) > tdesc = xmalloc (sizeof (struct target_desc)); > copy_target_description (tdesc, parent_proc->tdesc); > child_proc->tdesc = tdesc; > - child_lwp->must_set_ptrace_flags = 1; > > /* Clone arch-specific process data. */ > if (the_low_target.new_fork != NULL) > Thanks, Pedro Alves