From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121612 invoked by alias); 28 Feb 2020 17:46:12 -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 120583 invoked by uid 89); 28 Feb 2020 17:46:04 -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=theory, HX-Received:6507 X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Feb 2020 17:46:02 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582911960; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UMOahYEk7PGKQ4kOVVPlPcbA1NV8SyfFy4J34qanZGg=; b=d0v+nVBw4b2gFdDARW/XMhcbwnhGhCyLbqohiyQUtzSUAMIUOLJev12yAQqS98EIhXLngl qmYxNLK4x3Db3PYH4wYLieVABF0QgvZSTbHlOrNdFe62X8Ie96FmO1WFKOHa8k6hBePqO1 zGKNU1Lc1EkcdWu4GWA6PjnsO1X51uw= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-145-RK2h-76EPtGp0cQydz2gDg-1; Fri, 28 Feb 2020 12:45:54 -0500 Received: by mail-wr1-f71.google.com with SMTP id p8so1646659wrw.5 for ; Fri, 28 Feb 2020 09:45:53 -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 w1sm3129471wmc.11.2020.02.28.09.45.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 28 Feb 2020 09:45:51 -0800 (PST) Subject: Re: [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet To: Andrew Burgess References: <80314590-ba73-26f5-830d-698ff4ecb2d8@redhat.com> <20200227161704.16480-1-andrew.burgess@embecosm.com> <14a8b8f2-f866-fcd8-bf23-c1f67d426421@redhat.com> <20200228140252.GO3317@embecosm.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <137d09cf-9f97-fa5e-19a0-71231a3f760a@redhat.com> Date: Fri, 28 Feb 2020 17:46: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: <20200228140252.GO3317@embecosm.com> 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-02/txt/msg01063.txt.bz2 On 2/28/20 2:02 PM, Andrew Burgess wrote: > * Pedro Alves [2020-02-27 19:46:12 +0000]: > >> On 2/27/20 4:17 PM, Andrew Burgess wrote: >> >>> The solution I propose in this commit is to use the first non exited >>> thread of the inferior. >> >> s/of the inferior/of the target/ >> >>> Additionally, GDB will give a warning if a >>> multi-threaded target stops without passing a thread-id. >>> >>> 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. >> >> I forgot to mention this earlier, but I think we could test this by using >> gdbserver's --disable-packet=Tthread option (or --disable-packet=threads >> to disable vCont as well.). This sets the disable_packet_Tthread global in >> gdbserver, which makes it skip including the thread in the T stop reply. >> >> $ gdbserver --disable-packet >> Disableable packets: >> vCont All vCont packets >> qC Querying the current thread >> qfThreadInfo Thread listing >> Tthread Passing the thread specifier in the T stop reply packet >> threads All of the above >> >> I added these options several years ago exactly to exercise support >> for old non-threaded protocol. ISTR having documented them, ... >> ... ah, here: >> https://sourceware.org/ml/gdb-patches/2008-06/msg00501.html > > Thanks, this looks super useful. I thought I'd write a test using > this feature but it turns out there's already > gdb.server/stop-reply-no-thread.exp which does > --disable-packet=Tthreads. > > So, I run the test (without my patch) and ... it passes. The reason: > > commit 3cada74087687907311b52781354ff551e10a0ed > Author: Pedro Alves > Date: Thu Jan 11 00:23:04 2018 +0000 > > Fix backwards compatibility with old GDBservers (PR remote/22597) Ah... I did have a feeling of deja vu... Thanks for digging it out. > > Specifically, this hunk: > > diff --git a/gdb/remote.c b/gdb/remote.c > index 81c772a5451..a1cd9ae1df3 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -6940,7 +6940,13 @@ Packet: '%s'\n"), > event->ptid = read_ptid (thr + strlen (";thread:"), > NULL); > else > - event->ptid = magic_null_ptid; > + { > + /* Either the current thread hasn't changed, > + or the inferior is not multi-threaded. > + The event must be for the thread we last > + set as (or learned as being) current. */ > + event->ptid = event->rs->general_thread; > + } > } > > if (rsa == NULL) > > This code sets the stop_reply::ptid to the general_thread if we have > no other thread specified in a 'T' packet. I haven't looked at the > gdbserver code yet, but from your description I don't think I can stop > gdbserver sending a 'T' and revert back to an 'S'. Right, you can't. > > It seems a little weird that we use general_thread here, I don't see > why that would be any more valid than the continue_thread, and you > might think that if we set a continue_thread just before we execute > the inferior then the stop event is possibly going to be for the > continue thread. > > Still, like you say, the design of the old mechanism is horribly > broken anyway, so I suspect there's little point worrying too much > about changing this stuff. > > So, my next thought was maybe I should be doing something similar, > instead of "fixing" a missing ptid in process_stop_event, fill in a > valid ptid earlier, so I did this: > > diff --git a/gdb/remote.c b/gdb/remote.c > index 4ac359ad5d9..67c5f298ee6 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7492,6 +7492,8 @@ Packet: '%s'\n"), > event->ws.value.sig = (enum gdb_signal) sig; > else > event->ws.value.sig = GDB_SIGNAL_UNKNOWN; > + if (event->ptid == null_ptid) > + event->ptid = event->rs->general_thread; > } > break; > case 'w': /* Thread exited. */ > > And this seems to do the trick. > > However, I'm still worried by the code in ::process_stop_event that > says: > > if (ptid == null_ptid) > ptid = inferior_ptid; > > After all, we know that as part of do_target_wait the inferior_ptid is > reset to null_ptid, so this code should be pointless now, right... Right. Relying on inferior_ptid when processing events is just wrong and should be removed. > > So I tried deleting it, and then both the stop-reply-no-thread.exp, > and my local testing with my minimal gdb stub failed because once > again we end up in ::process_stop_event with stop_reply->ptid being > null_ptid - which now is coming from general_thread. > > Out of interest I tried replacing both the uses of general_thread (one > in the 'T' packet and one I just added in the 'S' packet processing) > with continue_thread, and this didn't do any better, though now the > assertion that triggers is complaining that we end up using > minus_one_ptid, and this leads to my next thought... Right. > > I think that general_thread and continue_thread are more > representative of what GDB has asked of the inferior, rather than what > the inferior is telling GDB (maybe?). Let me explain my thinking: > GDB updates the general_thread to a valid (something other than > null_ptid, minus_one_ptid, etc) in only two places: > > (a) _AFTER_ processing a stop reply packet, to make the > general_thread match the thread that stopped, this is based on the > ptid that was parsed out of the stop reply, or > > (b) When we send a 'H' packet to the remote, setting either the > general 'g' or continue 'c' thread variable, which is done by GDB > just before an action. > > Now relying on (a) to set general_thread so we can read it on the > _next_ stop clearly makes no sense - what stopped this time has no > relevance to what might stop next time, and similarly, relying on the > value of general_thread set via the 'H' packet is clearly just wrong, > as GDB must send a 'Hc' (and sets the continue_thread) to pick which > thread should continue. The theory of the original fix was that if the stub isn't sending a thread id in the stop reply, then there's only one thread in the remote target, so the thread recorded in general_thread is going to be the right thread anyway. As you noticed, continue_thread is going to be minus_one_ptid when we tell the stub to resume all threads, so continue_thread was a better choice. I think. > > I think what I'm arguing is that using general_thread as a fall-back > when processing 'T' (and 'S') is wrong, we should instead in these > cases deliberately leave the stop event ptid as null_ptid (indicating > that the stop didn't name a thread), and then use my patch (with your > fixes) to pick a non-exited thread. That sounds good to me. Seems safer / better than relying on general_thread pointing to a thread, since in some cases it can point to a while process, or to no process/thread. > > if (rsa == NULL) > @@ -7668,10 +7664,35 @@ 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 > + non-exited thread in the current target. */ > if (ptid == null_ptid) > - ptid = inferior_ptid; > + { > + for (thread_info *thr : all_non_exited_threads (this)) > + { > + if (ptid != null_ptid) > + { > + static bool warned = false; > + > + if (!warned) > + { > + /* If you are seeing this warning then the remote target > + has multiple threads and either send an 'S' stop You mean "send" -> "sent", I think. Thanks, Pedro Alves