From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36235 invoked by alias); 26 Oct 2016 15:39:20 -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 36220 invoked by uid 89); 26 Oct 2016 15:39:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Break, Leave, *entry, 3016 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 ESMTP; Wed, 26 Oct 2016 15:39:09 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F1107C05A282; Wed, 26 Oct 2016 15:39:07 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9QFd6fu009962; Wed, 26 Oct 2016 11:39:07 -0400 Subject: Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were To: Luis Machado , gdb-patches@sourceware.org References: <1455677091-13683-1-git-send-email-palves@redhat.com> <1455677091-13683-5-git-send-email-palves@redhat.com> <56C45D7D.2040904@codesourcery.com> <56C4685D.5040204@redhat.com> From: Pedro Alves Message-ID: <6f0a267e-a2b1-b78b-a62d-6af4394b4686@redhat.com> Date: Wed, 26 Oct 2016 15:39:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <56C4685D.5040204@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00721.txt.bz2 On 02/17/2016 12:32 PM, Pedro Alves wrote: > On 02/17/2016 11:46 AM, Luis Machado wrote: >> The rest of the series looks good to me. > > Great, thanks! > This is the version that I pushed. The fork parent/child handling needed an extra tweak, to consider the case of the fork event not having migrated out of linux-low.c yet. This was exposed by sporadic failures in the gdb.threads/forking-threads-plus-breakpoint.exp test. >From 5a04c4cf5df6d13596e79e7b84520cbe245a5a4d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 26 Oct 2016 16:17:25 +0100 Subject: [PATCH] gdbserver: Leave already-vCont-resumed threads as they were Currently GDB never sends more than one action per vCont packet, when connected in non-stop mode. A follow up patch will change that, and it exposed a gdbserver problem with the vCont handling. For example, this in non-stop mode: => vCont;s:p1.1;c <= OK Should be equivalent to: => vCont;s:p1.1 <= OK => vCont;c <= OK But gdbserver currently doesn't handle this. In the latter case, "vCont;c" makes gdbserver clobber the previous step request. This patch fixes that. Note the server side must ignore resume actions for the thread that has a pending %Stopped notification (and any other threads with events pending), until GDB acks the notification with vStopped. Otherwise, e.g., the following case is mishandled: #1 => g (or any other packet) #2 <= [registers] #3 <= %Stopped T05 thread:p1.2 #4 => vCont s:p1.1;c #5 <= OK Above, the server must not resume thread p1.2 when it processes the vCont. GDB can't know that p1.2 stopped until it acks the %Stopped notification. (Otherwise it wouldn't send a default "c" action.) (The vCont documentation already specifies this.) Finally, special care must also be given to handling fork/vfork events. A (v)fork event actually tells us that two processes stopped -- the parent and the child. Until we follow the fork, we must not resume the child. Therefore, if we have a pending fork follow, we must not send a global wildcard resume action (vCont;c). We can still send process-wide wildcards though. (The comments above will be added as code comments to gdb in a follow up patch.) gdb/gdbserver/ChangeLog: 2016-10-26 Pedro Alves * linux-low.c (handle_extended_wait): Link parent/child fork threads. (linux_wait_1): Unlink them. (linux_set_resume_request): Ignore resume requests for already-resumed and unhandled fork child threads. * linux-low.h (struct lwp_info) : New field. * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): New functions. (handle_v_requests) : Don't call require_running. * server.h (in_queued_stop_replies): New declaration. --- gdb/gdbserver/ChangeLog | 13 +++++++++++ gdb/gdbserver/linux-low.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++ gdb/gdbserver/linux-low.h | 6 +++++ gdb/gdbserver/server.c | 33 +++++++++++++++++++++++++- gdb/gdbserver/server.h | 4 ++++ 5 files changed, 114 insertions(+), 1 deletion(-) diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog index 437bb4c..1a9c4e5 100644 --- a/gdb/gdbserver/ChangeLog +++ b/gdb/gdbserver/ChangeLog @@ -1,3 +1,16 @@ +2016-10-26 Pedro Alves + + * linux-low.c (handle_extended_wait): Link parent/child fork + threads. + (linux_wait_1): Unlink them. + (linux_set_resume_request): Ignore resume requests for + already-resumed and unhandled fork child threads. + * linux-low.h (struct lwp_info) : New field. + * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): + New functions. + (handle_v_requests) : Don't call require_running. + * server.h (in_queued_stop_replies): New declaration. + 2016-10-24 Yao Qi PR server/20733 diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 94c5bbe..f43ce7e 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -589,6 +589,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) event_lwp->status_pending_p = 1; event_lwp->status_pending = wstat; + /* Link the threads until the parent event is passed on to + higher layers. */ + event_lwp->fork_relative = child_lwp; + child_lwp->fork_relative = event_lwp; + /* If the parent thread is doing step-over with single-step breakpoints, the list of single-step breakpoints are cloned from the parent's. Remove them from the child process. @@ -3853,6 +3858,15 @@ linux_wait_1 (ptid_t ptid, { /* If the reported event is an exit, fork, vfork or exec, let GDB know. */ + + /* Break the unreported fork relationship chain. */ + if (event_child->waitstatus.kind == TARGET_WAITKIND_FORKED + || event_child->waitstatus.kind == TARGET_WAITKIND_VFORKED) + { + event_child->fork_relative->fork_relative = NULL; + event_child->fork_relative = NULL; + } + *ourstatus = event_child->waitstatus; /* Clear the event lwp's waitstatus since we handled it already. */ event_child->waitstatus.kind = TARGET_WAITKIND_IGNORE; @@ -4654,6 +4668,51 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) continue; } + /* Ignore (wildcard) resume requests for already-resumed + threads. */ + if (r->resume[ndx].kind != resume_stop + && thread->last_resume_kind != resume_stop) + { + if (debug_threads) + debug_printf ("already %s LWP %ld at GDB's request\n", + (thread->last_resume_kind + == resume_step) + ? "stepping" + : "continuing", + lwpid_of (thread)); + continue; + } + + /* Don't let wildcard resumes resume fork children that GDB + does not yet know are new fork children. */ + if (lwp->fork_relative != NULL) + { + struct inferior_list_entry *inf, *tmp; + struct lwp_info *rel = lwp->fork_relative; + + if (rel->status_pending_p + && (rel->waitstatus.kind == TARGET_WAITKIND_FORKED + || rel->waitstatus.kind == TARGET_WAITKIND_VFORKED)) + { + if (debug_threads) + debug_printf ("not resuming LWP %ld: has queued stop reply\n", + lwpid_of (thread)); + continue; + } + } + + /* If the thread has a pending event that has already been + reported to GDBserver core, but GDB has not pulled the + event out of the vStopped queue yet, likewise, ignore the + (wildcard) resume request. */ + if (in_queued_stop_replies (entry->id)) + { + if (debug_threads) + debug_printf ("not resuming LWP %ld: has queued stop reply\n", + lwpid_of (thread)); + continue; + } + lwp->resume = &r->resume[ndx]; thread->last_resume_kind = lwp->resume->kind; diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 5057e66..476816d 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -301,6 +301,12 @@ struct lwp_info information or exit status until it can be reported to GDB. */ struct target_waitstatus waitstatus; + /* A pointer to the fork child/parent relative. Valid only while + the parent fork event is not reported to higher layers. Used to + avoid wildcard vCont actions resuming a fork child before GDB is + notified about the parent's fork event. */ + struct lwp_info *fork_relative; + /* When stopped is set, this is where the lwp last stopped, with decr_pc_after_break already accounted for. If the LWP is running, this is the address at which the lwp was resumed. */ diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 2996e19..3f9ff2b 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -193,6 +193,38 @@ vstop_notif_reply (struct notif_event *event, char *own_buf) prepare_resume_reply (own_buf, vstop->ptid, &vstop->status); } +/* QUEUE_iterate callback helper for in_queued_stop_replies. */ + +static int +in_queued_stop_replies_ptid (QUEUE (notif_event_p) *q, + QUEUE_ITER (notif_event_p) *iter, + struct notif_event *event, + void *data) +{ + ptid_t filter_ptid = *(ptid_t *) data; + struct vstop_notif *vstop_event = (struct vstop_notif *) event; + + if (ptid_match (vstop_event->ptid, filter_ptid)) + return 0; + + /* Don't resume fork children that GDB does not know about yet. */ + if ((vstop_event->status.kind == TARGET_WAITKIND_FORKED + || vstop_event->status.kind == TARGET_WAITKIND_VFORKED) + && ptid_match (vstop_event->status.value.related_pid, filter_ptid)) + return 0; + + return 1; +} + +/* See server.h. */ + +int +in_queued_stop_replies (ptid_t ptid) +{ + return !QUEUE_iterate (notif_event_p, notif_stop.queue, + in_queued_stop_replies_ptid, &ptid); +} + struct notif_server notif_stop = { "vStopped", "Stop", NULL, vstop_notif_reply, @@ -2947,7 +2979,6 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) if (startswith (own_buf, "vCont;")) { - require_running (own_buf); handle_v_cont (own_buf); return; } diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h index 51b2191..f56c0f5 100644 --- a/gdb/gdbserver/server.h +++ b/gdb/gdbserver/server.h @@ -123,6 +123,10 @@ extern int handle_target_event (int err, gdb_client_data client_data); /* Get rid of the currently pending stop replies that match PTID. */ extern void discard_queued_stop_replies (ptid_t ptid); +/* Returns true if there's a pending stop reply that matches PTID in + the vStopped notifications queue. */ +extern int in_queued_stop_replies (ptid_t ptid); + #include "remote-utils.h" #include "utils.h" -- 2.5.5