From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40756 invoked by alias); 17 Feb 2016 12:32:33 -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 40746 invoked by uid 89); 17 Feb 2016 12:32:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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, 17 Feb 2016 12:32:31 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id C7AE980502; Wed, 17 Feb 2016 12:32:30 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1HCWToG020858; Wed, 17 Feb 2016 07:32:29 -0500 Message-ID: <56C4685D.5040204@redhat.com> Date: Wed, 17 Feb 2016 12:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Luis Machado , gdb-patches@sourceware.org Subject: Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were References: <1455677091-13683-1-git-send-email-palves@redhat.com> <1455677091-13683-5-git-send-email-palves@redhat.com> <56C45D7D.2040904@codesourcery.com> In-Reply-To: <56C45D7D.2040904@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-02/txt/msg00507.txt.bz2 On 02/17/2016 11:46 AM, Luis Machado wrote: > On 02/17/2016 12:44 AM, Pedro Alves wrote: >> 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-02-16 Pedro Alves >> >> * linux-low.c (linux_set_resume_request): Ignore resume requests >> for already-resumed threads. >> * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): >> New functions. >> * server.h (in_queued_stop_replies): New declaration. >> --- >> gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++++++++ >> gdb/gdbserver/server.c | 33 ++++++++++++++++++++++++++++++++- >> gdb/gdbserver/server.h | 4 ++++ >> 3 files changed, 63 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index 8b025bd..2cac4c0 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) >> continue; >> } >> >> + /* Ignore (wildcard) resume requests for already-resumed >> + requests. */ > > For already-resumed requests or threads? Looked a little confusing. Whoops, I meant "already-resumed threads". Fixed locally. > > If you really meant "requests", then we may need to adjust the wording a > bit, like "for requests that have already been acknowledged.". > > The rest of the series looks good to me. Great, thanks! -- Pedro Alves