From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13038 invoked by alias); 2 Mar 2020 12:25: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 12962 invoked by uid 89); 2 Mar 2020 12:25:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=HX-Received:906 X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Mar 2020 12:25:18 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583151917; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Qr3+9lWILvC6r0YNNr6ZcTktzt1ttw5CKlxIykxqywI=; b=Ft7inKMmfhwSQfVAbonUeaRwLJRLJWVv67EOm79DYlnMS79g2f+8KHpaL4AoVjA5a0iRq6 BpHWWbV7/NqqtvyoHtEa7PnJOWzuCM6dBu6fnU5sI+kYxZ9AxnLH/F10fZPUPJKuxye/YJ /AHUAe3B6JD8ZwtTdJah+OIyQCGiUJU= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-419-FUMAghsTOpSdj8wXZnkp9w-1; Mon, 02 Mar 2020 07:25:15 -0500 Received: by mail-ed1-f69.google.com with SMTP id d12so5741778edq.16 for ; Mon, 02 Mar 2020 04:25:15 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f909:7b00:21e8:4cd6:8ec1:f387? ([2001:8a0:f909:7b00:21e8:4cd6:8ec1:f387]) by smtp.gmail.com with ESMTPSA id g20sm1095011edr.20.2020.03.02.04.25.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 Mar 2020 04:25:13 -0800 (PST) Subject: Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet To: Andrew Burgess , gdb-patches@sourceware.org References: From: Pedro Alves Message-ID: Date: Mon, 02 Mar 2020 12:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-03/txt/msg00020.txt 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. > > 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