From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111093 invoked by alias); 2 Mar 2020 15:19:21 -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 111077 invoked by uid 89); 2 Mar 2020 15:19:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HTo:U*palves X-HELO: mail-wm1-f67.google.com Received: from mail-wm1-f67.google.com (HELO mail-wm1-f67.google.com) (209.85.128.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Mar 2020 15:19:19 +0000 Received: by mail-wm1-f67.google.com with SMTP id u9so5152197wml.3 for ; Mon, 02 Mar 2020 07:19:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=eec0FQnWKMzmgt+VrwpET7iEV7SziaxhXqxJyJGY96g=; b=dhAzXXoqK8xCRlj5eQVUoLUN/7L+x+YwT9dgOC44+TfyHX6horLIZulWpSKphEQGf2 aeVbB7tZML9uIUttoHm/R4yOZZRDjJsZmRm/HNvJGtq6LXyi2pXdjfSbnyTspHO7Jr+i Gk9IGP7CmFHehQ/lQXc4n0D8gVRVVpU8NMmVlzcdZkvhEAP4nXQWMlQ6TzLgs5wwvBk6 aCoPRGn7Az7Uh5la4xASzX4QYu2oCCPLmLWfxIp2GZ/LuOaRyil+VX8S5uxZJAi7vFP+ Wfz2Uza58wTdOA3Ym7CWEHsWSSDrhTS9a3uFm3YHx3wlR+TKOvf8aOfZs9jY3ikL7bUb qLYQ== Return-Path: Received: from localhost ([212.69.42.53]) by smtp.gmail.com with ESMTPSA id j20sm17106143wmj.46.2020.03.02.07.19.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 02 Mar 2020 07:19:16 -0800 (PST) Date: Mon, 02 Mar 2020 15:19:00 -0000 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet Message-ID: <20200302151915.GQ3317@embecosm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: The Microsoft Motto: "We're the leaders, wait for us!" X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2020-03/txt/msg00025.txt Thanks for all the feedback. I pushed the patches with the fixes you suggested. * Pedro Alves [2020-03-02 12:25:12 +0000]: > On 3/2/20 11:54 AM, Andrew Burgess wrote: > > > My proposal for a fix then is: > > > > 1. Move the call to switch_to_inferior_no_thread into > > do_target_wait_1, this means that in call cases where we are waiting > > "in all cases" > > > for an inferior the inferior_ptid will be set to null_ptid. This is > > good as no wait code should rely on inferior_ptid. > > > > 2. Remove the use of general_thread from the 'T' packet processing. > > The general_thread read here was only ever correct by chance, and we > > shouldn't be using it this way. > > > > 3. Remove use of inferior_ptid from ::process_stop_event as this is > > wrong, and will always be null_ptid now anyway. > > > > 4. When a stop_even has null_ptid due to a lack of thread-id (either > > "stop_even" -> "stop_event" > > > from a T packet or an S packet) then pick the first non exited thread > > in the inferior and use that. This will be fine for single threaded > > "in the inferior" -> "in the target". > > > inferiors. A multi-threaded inferior really should be using T > > Instead of > "A multi-threaded inferior", > we should say > "A multi-thread or multi-inferior aware remote server/stub" > or something around that. > > > packets with a thread-id, so we give a warning if the inferior is > > multi-threaded, and we are still missing a thread-id. > > "inferior" -> "target". > > The "inferior" -> "target" distinction I'm making in these > small remarks above matters, because say the remote server > is debugging two single-threaded inferiors. We still want to > (and do) warn. I hadn't really considered this case, however, this raises a follow on question: Before entering the target wait code we call switch_to_inferior_no_thread, partly, as I understand it because having inferior_ptid pointing at a thread leads to invalid code that relies on this thread being _the_ event thread, when really we need to extract the inferior and thread-id from the stop event. So, why do we allow the current_inferior to remain valid while we perform the wait? Instead, why don't we clear both current_inferior and inferior_ptid, and then enter the wait code? Thanks, Andrew > > > > > 5. Extend the existing test that covered the T packet with missing > > thread-id to also cover the S packet. > > Excellent. > > > > > gdb/ChangeLog: > > > > * remote.c (remote_target::remote_parse_stop_reply): Don't use the > > general_thread if the stop reply is missing a thread-id. > > (remote_target::process_stop_reply): Use the first non-exited > > thread if the target didn't pass a thread-id. > > * infrun.c (do_target_wait): Move call to > > switch_to_inferior_no_thread to .... > > (do_target_wait_1): ... here. > > > > gdb/testsuite/ChangeLog: > > > > * gdb.server/stop-reply-no-thread.exp: Add test where T packet is > > disabled. > > --- > > gdb/ChangeLog | 10 +++ > > gdb/infrun.c | 8 ++- > > gdb/remote.c | 43 ++++++++---- > > gdb/testsuite/ChangeLog | 5 ++ > > gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++--------- > > 5 files changed, 101 insertions(+), 45 deletions(-) > > > > diff --git a/gdb/infrun.c b/gdb/infrun.c > > index d9a6f733519..43199b17b05 100644 > > --- a/gdb/infrun.c > > +++ b/gdb/infrun.c > > @@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid, > > ptid_t event_ptid; > > struct thread_info *tp; > > > > + /* We know that we are looking for an event in inferior INF, but we don't > > "in the inferior INF" -> "in the target of inferior INF". > > The distinction is important -- target_wait may well return an event > for an inferior different from INF. > > > + know which thread the event might come from. As such we want to make > > + sure that INFERIOR_PTID is reset so that non of the wait code relies > > "that non of" -> "that none of" ? > > > + on it - doing so is always a mistake. */ > > + switch_to_inferior_no_thread (inf); > > + > > OK with the nits above fixed. Thanks much for doing this! > > Pedro Alves >