From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10521 invoked by alias); 26 Feb 2020 19:28:42 -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 10513 invoked by uid 89); 26 Feb 2020 19:28:41 -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=Theres, There's X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Feb 2020 19:28:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582745318; 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=pnWkkgQ6BcOyEaexEtKFEyUZcC7EMX0JIzm/SPV1t5A=; b=F+lb0i/o5fJgY8DOqvaBc2YLbnwRQ0FOMrqnCRXpjs/jq0JAOQX8hltg0MAWJewSX4aXcZ ahrvHdfo6N72ysQk56cbsOt+0VT/SavBUr28/S13oep4BTcsLA39SPKrxvNM0Vt1LEZOrE ioyXuERq0lFoegXdlhUJw4Cc9cW4IJE= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-208-LnJSjzV9Pzyln_Kbad5CgQ-1; Wed, 26 Feb 2020 14:28:37 -0500 Received: by mail-wr1-f72.google.com with SMTP id t14so186828wrs.12 for ; Wed, 26 Feb 2020 11:28:37 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id b82sm3963134wmb.16.2020.02.26.11.28.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Feb 2020 11:28:34 -0800 (PST) Subject: Re: [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us To: Andrew Burgess , gdb-patches@sourceware.org References: <20200131000600.11699-1-andrew.burgess@embecosm.com> From: Pedro Alves Message-ID: <80314590-ba73-26f5-830d-698ff4ecb2d8@redhat.com> Date: Wed, 26 Feb 2020 19:28: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: <20200131000600.11699-1-andrew.burgess@embecosm.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2020-02/txt/msg00982.txt.bz2 On 1/31/20 12:06 AM, Andrew Burgess wrote: > With this commit: > > commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2 > Date: Fri Jan 10 20:06:08 2020 +0000 > > Multi-target support > > There was a regression in GDB's support for older aspects of the > remote protocol. Specifically, when a target sends the 'S' stop reply > packet, which doesn't include a thread-id, then GDB has to figure out > which thread actually stopped. > > Before the above commit GDB figured this out by using inferior_ptid in > process_stop_reply, which contained the ptid of the current > process/thread. With the above commit the inferior_ptid now has the > value null_ptid inside process_stop_reply, this can be seen in > do_target_wait, where we call switch_to_inferior_no_thread before > calling do_target_wait_1. > > The solution I propose in this commit is that, if we don't get a > thread id in the stop reply, then we should just ask the target for > the current thread. I'm really not sure it is worth it to add an extra remote roundtrip for every single-step. If the stub uses the S stop reply, then it must be it doesn't really support multiple threads. Otherwise, even before the multi-target patch, multi-threading support would be broken when the target stopped for a thread different from GDB's current thread (the inferior_ptid). The "Hc" ("set continue thread") + "s/c/S/C" (step/continue) mechanism was broken by design for multi-threading and led to the creation of vCont + T many years ago. So I think that making GDB use the first thread of the target is sufficient and basically restores GDB to its previous behavior. > > There's no test included with this commit, if anyone has any ideas for > how we could test this aspect of the remote protocol (short of writing > a new gdbserver) then I'd love to know. > > It is possible to trigger this bug by attaching GDB to a running GDB, > place a breakpoint on remote_parse_stop_reply, and manually change the > contents of buf - when we get a 'T' based stop packet, replace it with > an 'S' based packet, like this: > > (gdb) call memset (buf, "S05\0", 4) > > After this the GDB that is performing the remote debugging will crash > with this error: > > inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed. > > gdb/ChangeLog: > > * remote.c (remote_target::process_stop_reply): Don't use > inferior_ptid, instead ask the remote for the current thread. > > Change-Id: I06f6fe5cc9a5cfc6d7157772aa7582f44c413bd5 > --- > gdb/ChangeLog | 5 +++++ > gdb/remote.c | 18 +++++++++++++++--- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index be2987707ff..94ed57ebf33 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7668,10 +7668,22 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, > *status = stop_reply->ws; > ptid = stop_reply->ptid; > > - /* If no thread/process was reported by the stub, assume the current > - inferior. */ > + /* If no thread/process was reported by the stub then use the first > + thread in the current inferior. */ I suspect this comment was written before you made the code query the remote side? I was more expecting that this comment here would say something like /* If no thread/process was included in the stop reply, then query the remote current thread. */ > if (ptid == null_ptid) > - ptid = inferior_ptid; > + { > + ptid = remote_current_thread (null_ptid); > + if (ptid == null_ptid) > + { > + /* We didn't get a useful answer back from the remote target so > + we need to pick something - we can't just report null_ptid. > + Lets just pick the first thread in GDB's current inferior. */ > + struct thread_info *thread > + = first_thread_of_inferior (current_inferior ()); To be extra pedantic, it should be the first non-exited thread of the target instead of the first thread of the current inferior. Something around: for (thread_info *thr : all_non_exited_threads (this)) { ptid = thr->ptid; break; } gdb_assert (ptid != null_ptid); > + gdb_assert (thread != nullptr); > + ptid = thread->ptid; > + } > + } > Thanks, Pedro Alves