From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106580 invoked by alias); 15 Apr 2015 15:39:35 -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 106571 invoked by uid 89); 15 Apr 2015 15:39:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD 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; Wed, 15 Apr 2015 15:39:33 +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 t3FFdUKP016872 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 15 Apr 2015 11:39:30 -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 t3FFdSZg020834; Wed, 15 Apr 2015 11:39:29 -0400 Message-ID: <552E8630.7060103@redhat.com> Date: Wed, 15 Apr 2015 15:39: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 v7 6/7] Remote fork catch References: <1428685786-18094-1-git-send-email-donb@codesourcery.com> <1428685786-18094-7-git-send-email-donb@codesourcery.com> In-Reply-To: <1428685786-18094-7-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg00575.txt.bz2 On 04/10/2015 06:09 PM, Don Breazeal wrote: > Hi Pedro, > This version of the patch incorporates changes based on your comments on > the previous version, as outlined below. > > On 3/24/2015 5:47 AM, Pedro Alves wrote: >> On 03/17/2015 08:56 PM, Don Breazeal wrote: >> >>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c >>> index 8fa6f8a..346f2c4 100644 >>> --- a/gdb/gdbserver/server.c >>> +++ b/gdb/gdbserver/server.c >>> @@ -1356,6 +1356,15 @@ handle_qxfer_threads_worker (struct inferior_list_entry *inf, void *arg) >>> int core = target_core_of_thread (ptid); >>> char core_s[21]; >>> >>> + /* Skip new threads created as the result of a fork if we are not done >>> + handling that fork event. We won't know whether to tell GDB about >>> + the new thread until we are done following the fork. */ >>> + if ((last_status.kind == TARGET_WAITKIND_FORKED >>> + || last_status.kind == TARGET_WAITKIND_VFORKED) >>> + && (ptid_get_pid (last_status.value.related_pid) >>> + == ptid_get_pid (ptid))) >>> + return; >> >> This use of last_status here is really just as bad as >> get_last_target_status, for the same reasons. What if a thread >> forks at the same time another thread hits a breakpoint, and >> we end up reporting the breakpoint first, leaving the fork >> pending? Sounds like we'll end up listing the child fork >> thread then. > > I moved this operation (removing the new, unreported thread from the list > reported by the target) to the host side in remote.c:remove_new_fork_child, > called from remote.c:remote_update_thread_list. Agreed, I think that's the right thing to do. This gives us the most flexibility to change the follow-fork model in core gdb. Also, if you disconnect while stopped at an unfollowed fork, and then reconnect, it's not clear to me whether the child should be hidden from the user until the next continue then. Again, leaving it up to the client gives us the flexibility either way. >>> diff --git a/gdb/testsuite/gdb.threads/fork-thread-pending.exp b/gdb/testsuite/gdb.threads/fork-thread-pending.exp >>> index d229232..594f376 100644 >>> --- a/gdb/testsuite/gdb.threads/fork-thread-pending.exp >>> +++ b/gdb/testsuite/gdb.threads/fork-thread-pending.exp >>> @@ -31,6 +31,26 @@ if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executab >>> return -1 >>> } >>> >>> +# Find a thread that did not fork and is not the main thread and >>> +# return its thread number. We can't just hard-code the thread >>> +# number since we have no guarantee as to the ordering of the threads >>> +# in gdb. >> >> I don't understand this -- the test runs to main first, so the main >> thread should always be thread 1, no? >> > > I can no longer reproduce the thread ordering problem that I was seeing > when I implemented this. Not sure why...my notes imply it might have > something to do with 'target remote', but I'm unsure at this point. Maybe it was necessary before: https://sourceware.org/ml/gdb-patches/2014-09/msg00734.html > At any rate this test doesn't need to be changed for this patch, so > I've removed it from the patch. > > BTW, I still intend to submit a patch that removes the need to use > get_last_target_status in linux-nat.c:linux_nat_kill, along with a test > for that scenario. That'd be great! > +/* Determine if THREAD is a pending fork parent thread. ARG contains > + the pid of the process who's threads we want to check, or -1 if > + we want to check all threads. */ > + > +static int > +pending_fork_parent_callback (struct thread_info *thread, void *arg) > +{ > + int pid = *(int *) arg; > + > + if (thread->pending_follow.kind == TARGET_WAITKIND_FORKED > + || thread->pending_follow.kind == TARGET_WAITKIND_VFORKED) > + { > + if ((pid == -1) || (pid == ptid_get_pid (thread->ptid))) Unnecessary parens: if (pid == -1 || pid == ptid_get_pid (thread->ptid)) > + return 1; > + } > + > + return 0; > +} > + > +/* If CONTEXT contains any fork child threads that have not been > + reported yet, remove them from the CONTEXT list. If such a > + thread exists it is because we are stopped at a fork catchpoint > + and have not yet called follow_fork, which will set up the > + host-side data structures for the new process. */ > + > +static void > +remove_new_fork_child (struct threads_listing_context *context) > +{ > + struct thread_info * thread; > + int pid = -1; > + > + /* Check to see if there is an in-progress fork parent. */ > + thread = iterate_over_threads (pending_fork_parent_callback, &pid); > + if (thread != NULL) In non-stop mode, if you're debugging multiple process, multiple processes can fork at the same, and then we end up with multiple threads with an in-progress fork parent. So this needs to walk the whole thread list, not just stop at the first. Either use ALL_NON_EXITED_THREADS, or move the loop below to pending_fork_parent_callback (or to a helper function called by that). > + { > + ptid_t child_ptid = thread->pending_follow.value.related_pid; > + struct thread_item *item; > + int i; > + > + for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i) > + { > + if (ptid_equal (item->ptid, child_ptid)) > + { > + VEC_ordered_remove (thread_item_t, context->items, i); > + break; > + } > + } > + } > +} > + > /* Implement the to_update_thread_list function for the remote > targets. */ > > @@ -2874,6 +2964,10 @@ remote_update_thread_list (struct target_ops *ops) > } > } > > + /* Remove any unreported fork child from CONTEXT so that > + we don't interfere with follow fork. */ > + remove_new_fork_child (&context); I think there's a race here, in non-stop mode. Consider: #1 - process forks just before gdb starts fetching the remote thread list. #2 - gdbserver adds the fork child its thread list. #3 - gdbserver queues the fork event, sends vStopped notification #4 - gdb/remote_update_thread_list pulls the thread list #5 - we're now in remove_new_fork_child, but we don't know about the fork event yet. It's still pending in the vStopped queue. So I think that we need to make remote_update_thread_list do, in this order: #1 - fetch the remote thread list #2 - fetch the pending vStopped notifications (remote_notif_get_pending_events) #3 - call remove_new_fork_child #4 - add threads we don't know about yet to our list. and make remove_new_fork_child also peek at the pending vStopped events queue (and in the future at any other layers of pending events in the core side.) > + child_pid = ptid_get_pid (thread->pending_follow.value.related_pid); > + res = remote_vkill (child_pid, rs); > + if (res != 0) > + error (_("Can't kill fork child process")); It'll probably be good to include the PID in the error message. > + } Thanks, Pedro Alves