From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18120 invoked by alias); 24 Mar 2015 12:47: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 18102 invoked by uid 89); 24 Mar 2015 12:47:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 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; Tue, 24 Mar 2015 12:47:32 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2OClOXb026511 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 24 Mar 2015 08:47:24 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2OClLWd002656; Tue, 24 Mar 2015 08:47:22 -0400 Message-ID: <55115CD9.1010806@redhat.com> Date: Tue, 24 Mar 2015 12:47: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 v6 6/6] Remote fork catch References: <1424997977-13316-1-git-send-email-donb@codesourcery.com> <1426625788-4469-1-git-send-email-donb@codesourcery.com> <1426625788-4469-7-git-send-email-donb@codesourcery.com> In-Reply-To: <1426625788-4469-7-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00789.txt.bz2 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. > + > write_ptid (ptid_s, ptid); > > if (core != -1) > @@ -4144,3 +4153,12 @@ handle_target_event (int err, gdb_client_data client_data) > > return 0; > } > + > +/* Retrieve the last waitstatus reported to GDB. */ > + > +void > +get_last_target_status (ptid_t *ptid, struct target_waitstatus *last) > +{ > + *ptid = last_ptid; > + *last = last_status; > +} Looks like you forgot to delete the function. :-) > diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h > index 09a5624..8c6ec27 100644 > --- a/gdb/gdbserver/server.h > +++ b/gdb/gdbserver/server.h > @@ -113,6 +113,8 @@ typedef int gdb_fildes_t; > /* Functions from server.c. */ > extern int handle_serial_event (int err, gdb_client_data client_data); > extern int handle_target_event (int err, gdb_client_data client_data); > +extern void get_last_target_status (ptid_t *ptid, > + struct target_waitstatus *last); > > #include "remote-utils.h" > > diff --git a/gdb/remote.c b/gdb/remote.c > index d1ba62d..44ee89f 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8012,6 +8060,23 @@ extended_remote_kill (struct target_ops *ops) > int res; > int pid = ptid_get_pid (inferior_ptid); > struct remote_state *rs = get_remote_state (); > + struct thread_info *tp = find_thread_ptid (inferior_ptid); > + > + /* If we're stopped while forking and we haven't followed yet, > + kill the child task. We need to do this first because the > + parent will be sleeping if this is a vfork. */ > + > + if (tp != NULL && (tp->pending_follow.kind == TARGET_WAITKIND_FORKED > + || tp->pending_follow.kind == TARGET_WAITKIND_VFORKED)) Looks like this will miss killing the child if the user switches to some thread other than the one that forked, in case it was a multi-threaded program that forked. > + { > + ptid_t parent_ptid = inferior_ptid; > + > + inferior_ptid = tp->pending_follow.value.related_pid; > + set_general_thread (inferior_ptid); > + extended_remote_kill (ops); > + inferior_ptid = parent_ptid; > + set_general_thread (inferior_ptid); We never want to use the 'k' packet here, so this could just simply be: int child_pid = tp->pending_follow.value.related_pid; remote_vkill (child_pid); > + } > > res = remote_vkill (pid, rs); > if (res == -1 && !(rs->extended && remote_multi_process_p (rs))) > @@ -12036,6 +12101,14 @@ Specify the serial device it is connected to (e.g. /dev/ttya)."; > extended_remote_ops.to_supports_disable_randomization > = extended_remote_supports_disable_randomization; > extended_remote_ops.to_follow_fork = remote_follow_fork; > + extended_remote_ops.to_insert_fork_catchpoint > + = remote_insert_fork_catchpoint; > + extended_remote_ops.to_remove_fork_catchpoint > + = remote_remove_fork_catchpoint; > + extended_remote_ops.to_insert_vfork_catchpoint > + = remote_insert_vfork_catchpoint; > + extended_remote_ops.to_remove_vfork_catchpoint > + = remote_remove_vfork_catchpoint; > } > > static int > 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? > We know that the main thread is in pthread_join and the > +# forking thread is in fork, so we use this rather ungainly regexp > +# to capture an entry from 'info threads' that doesn't show one of > +# those routines, then extract the thread number. > + > +proc find_unforked_thread { } { > + gdb_test_multiple "info threads" "find unforked thread" { > + -re "(\[^\r]*Thread\[^\r]* in \[^fp]\[^ot]\[^rh]\[^kr]\[^e]\[^a]\[^d]\[^_]\[^j]\[^\r]*\r\n)" { > + regexp "(\[ ]*)(\[0-9]*)(\[ ]*Thread\[^\r]*\r\n)" $expect_out(0,string) ignore lead_spc threadnum rest > + } > + timeout { > + set threadnum -1 > + } > + } > + return $threadnum > +} > + > clean_restart ${binfile} > > if ![runto_main] then { > @@ -46,7 +66,8 @@ gdb_test "continue" "Catchpoint.*" "1, get to the fork event" > > gdb_test "info threads" " Thread .* Thread .* Thread .* Thread .*" "1, multiple threads found" > > -gdb_test "thread 1" ".*" "1, switched away from event thread" > +set threadnum [find_unforked_thread] > +gdb_test "thread $threadnum" ".*" "1, switched away from event thread to thread $threadnum" > > gdb_test "continue" "Not resuming.*" "1, refused to resume" > > Thanks, Pedro Alves