From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 58wJCBTb9l/yBAAAWB0awg (envelope-from ) for ; Thu, 07 Jan 2021 04:57:40 -0500 Received: by simark.ca (Postfix, from userid 112) id 143B41E965; Thu, 7 Jan 2021 04:57:40 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A9D9A1E590 for ; Thu, 7 Jan 2021 04:57:38 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0BB24395206E; Thu, 7 Jan 2021 09:57:38 +0000 (GMT) Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by sourceware.org (Postfix) with ESMTPS id 5CF36395206E for ; Thu, 7 Jan 2021 09:57:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5CF36395206E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42a.google.com with SMTP id y17so4983346wrr.10 for ; Thu, 07 Jan 2021 01:57:34 -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; bh=V3RpoL59b4KvkvhlL+dSFxGW+HphRnO2Y9m8Pxn9BNw=; b=hckJf6j+xc6V0iqS4iguLCGTLBuU5I9LHh02nKdFIXJUMzd2df9IJ/cutDCIUtm65/ 9u2MijIOKdceJEZn8WmWmB6447VidEF69wn1gfgx4PAKuUxpeCD0h5oz766sIXYo8VML oGIqJkdA6Zb0QvdQhy8Er1bimp3Qo/ntv4TN56sClF9abqNMAbZfhtgFeAoJpvvU1+NN hyEYLrEiPw90XTZ3iJ+ebsICe/IfeNadoxmLdHROX1dWO7wbmWcMTxAzhvGg14s5gXc3 umCVus5G7XjjExxe05qHQ6oJQoFnvra/3YNsLMZeVyCWM59BIT03RkxwhMU+Ss9JIwOF ZRYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=V3RpoL59b4KvkvhlL+dSFxGW+HphRnO2Y9m8Pxn9BNw=; b=f41wv1B9NzgE1Hw8pCau87QJOLeFY5RLXomiHlppXhmQRcrdB3soMMrrQpmRzAOuDV LrepPesEpvppjCtTeWZre7bxK19omkVPr2Df7XJYrtMuzZ5QLASnu0YuWP99RomQixDG qXcupWbr0X0R6+h2pknqLIucS6hAkR88gs+w6GXNmGTW2tGXM/oS1iERxSqjocXhusJ9 fR9W3V5HaoUCBLZOnHTNJYHslk0spnLwlzKEPcwqZTytOuY9FOxDPZKu3ZKXXnKSqDWW /dJaQpoaJn68SR/sCOgX8sU9UZddvagzxHF6Q7qOJvTKcOiNKmfm+8jiZPhHxp9GLMBu aiAw== X-Gm-Message-State: AOAM533IcwxLbdW19/blkYpws8Sv0KOYrSQFfedNBa/oN1kmqgDCbYdC BROrHSKFrqJ1dGdsRkiQpxkXxMx+82GMzw== X-Google-Smtp-Source: ABdhPJzI9wCVa1Qlv7rdWkYRnovOcs0OVhRcDI/4xmdFCuqTWoxCYRz7gaS6gJPzCwTqtTEAxHaQSg== X-Received: by 2002:a5d:678d:: with SMTP id v13mr8171604wru.71.1610013453352; Thu, 07 Jan 2021 01:57:33 -0800 (PST) Received: from localhost (host86-166-129-230.range86-166.btcentralplus.com. [86.166.129.230]) by smtp.gmail.com with ESMTPSA id b83sm6970095wmd.48.2021.01.07.01.57.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Jan 2021 01:57:32 -0800 (PST) Date: Thu, 7 Jan 2021 09:57:31 +0000 From: Andrew Burgess To: Simon Marchi Subject: Re: [PATCHv2] gdb: better handling of 'S' packets Message-ID: <20210107095731.GO2945@embecosm.com> References: <20201111153548.1364526-1-andrew.burgess@embecosm.com> <20201210162948.GC2945@embecosm.com> <20201223230949.GM2945@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 09:56:53 up 29 days, 14:41, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" * Simon Marchi [2021-01-06 16:19:54 -0500]: > On 2020-12-23 6:09 p.m., Andrew Burgess wrote: > > In V2 I've reduced the patch to focus only on the minimum changes > > required to fix the original bug (PR gdb/26819). > > > > I think that this new patch is much more obviously the right thing to > > do. > > > > Once this is merged I'll put together a new patch with the extra > > functionality from V1, but that can come later. > > > > All feedback welcome. > > > > Thanks, > > Andrew > > > > --- > > > > commit 00190f2b3958e2e822b3d6b078148175f995486c > > Author: Andrew Burgess > > Date: Tue Nov 10 17:54:11 2020 +0000 > > > > gdb: better handling of 'S' packets > > > > This commit builds on work started in the following two commits: > > > > commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493 > > Date: Thu Jan 30 14:35:40 2020 +0000 > > > > gdb/remote: Restore support for 'S' stop reply packet > > > > commit cada5fc921e39a1945c422eea055c8b326d8d353 > > Date: Wed Mar 11 12:30:13 2020 +0000 > > > > gdb: Handle W and X remote packets without giving a warning > > > > This is related to how GDB handles remote targets that send back 'S' > > packets. > > > > In the first of the above commits we fixed GDB's ability to handle a > > single process, single threaded target that sends back 'S' packets. > > Although the 'T' packet would always be preferred to 'S' these days, > > there's nothing really wrong with 'S' for this situation. > > > > The second commit above fixed an oversight in the first commit, a > > single-process, multi-threaded target can send back a process wide > > event, for example the process exited event 'W' without including a > > process-id, this also is fine as there is no ambiguity in this case. > > > > In PR gdb/26819 we run into yet another problem with the above > > commits. In this case we have a single process with two threads, GDB > > hits a breakpoint in thread 2 and then performs a stepi: > > > > (gdb) b main > > Breakpoint 1 at 0x1212340830: file infinite_loop.S, line 10. > > (gdb) c > > Continuing. > > > > Thread 2 hit Breakpoint 1, main () at infinite_loop.S:10 > > 10 in infinite_loop.S > > (gdb) set debug remote 1 > > (gdb) stepi > > Sending packet: $vCont;s:2#24...Packet received: S05 > > ../binutils-gdb/gdb/infrun.c:5807: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed. > > > > What happens in this case is that on the RISC-V target displaced > > stepping is not supported, so when the stepi is issued GDB steps just > > thread 2. As only a single thread was set running the target decides > > that is can get away with sending back an 'S' packet without a > > thread-id. GDB then associates the stop with thread 1 (the first > > non-exited thread), but as thread 1 was not previously set executing > > the assertion seen above triggers. > > > > As an aside I am surprised that the target sends pack 'S' in this > > situation. The target is happy to send back 'T' (including thread-id) > > when multiple threads are set running, so (to me) it would seem easier > > to just always use the 'T' packet when multiple threads are in use. > > However, the target only uses 'T' when multiple threads are actually > > executing, otherwise an 'S' packet it used. > > > > Still, when looking at the above situation we can see that GDB should > > be able to understand which thread the 'S' reply is referring too. > > > > The problem is that is that in commit 24ed6739b699 (above) when a stop > > reply comes in with no thread-id we look for the first non-exited > > thread and select that as the thread the stop applies too. > > > > What we should really do is check the threads executing flag too, and > > so find the first non-exited, executing thread, and associate the stop > > event with this thread. In the above example both thread 1 and 2 are > > non-exited, but only thread 2 is executing, so this is what we should > > use. > > > > Initially I planned to always look for the first non-exited, executing > > thread, however, this doesn't always work. > > > > When GDB initially connects to a target it queries the target for a > > list of threads. These threads are created within GDB in the > > non-executing state. GDB then asks the target for the last stop > > reason with the '?' packet. If the reply to '?' doesn't include a > > thread-id then GDB needs to look through all the threads and find a > > suitable candidate. At this point no threads will be marked > > executing, so all we can do is find the first non-exited thread (as we > > currently do). > > I'm thinking about how this could work with my patch that makes the remote > target track its own resume state: > > https://sourceware.org/pipermail/gdb-patches/2020-December/174274.html > > I'm trying to update that patch to make the remote target track the resume > state even in all-stop, as discussed in that thread. In all-stop, when we > receive a stop-reply indicating that a thread stopped, all the threads of > the target are assumed to be stopped, so I marked all of them as NOT_RESUMED. > > I'm thinking that the remote target could assume that all threads are initially > RESUMED. In all-stop, we process the stop reply we got in response to '?', and > that will mark all the threads as NOT_RESUMED. > > So I think your code could use that new state and implement what you initially > wanted: look for the first non-exited, resumed thread (resumed from the > point of view of the remote target). And that would simplify things a bit. OK, I'll wait for a while to see if you post a revised version of the above patch, then update this to match. Thanks, Andrew > > > ptid_t > > -remote_target::process_stop_reply (struct stop_reply *stop_reply, > > - struct target_waitstatus *status) > > +remote_target::select_thread_for_ambiguous_stop_reply > > + (const struct target_waitstatus *status) > > { > > - ptid_t ptid; > > + /* Some stop events apply to all threads in an inferior, while others > > + only apply to a single thread. */ > > + bool is_stop_for_all_threads > > + = (status->kind == TARGET_WAITKIND_EXITED > > + || status->kind == TARGET_WAITKIND_SIGNALLED); > > > > - *status = stop_reply->ws; > > - ptid = stop_reply->ptid; > > + struct remote_state *rs = get_remote_state (); > > > > - /* If no thread/process was reported by the stub then use the first > > - non-exited thread in the current target. */ > > - if (ptid == null_ptid) > > + /* Track the possible threads in this structure. */ > > + struct thread_choices > > + { > > + /* Constructor. */ > > + thread_choices (struct remote_state *rs, bool is_stop_for_all_threads) > > + : m_rs (rs), > > + m_is_stop_for_all_threads (is_stop_for_all_threads) > > + { /* Nothing. */ } > > + > > + /* Disable/delete these. */ > > + thread_choices () = delete; > > I don't think it's necessary to delete the default constructor if you have > defined a non-default constructor. > > > + DISABLE_COPY_AND_ASSIGN (thread_choices); > > + > > + /* Consider thread THR setting the internal thread tracking variables > > + as appropriate. */ > > + void consider_thread (thread_info *thr) > > + { > > + /* Record the first non-exited thread as a fall-back response. This > > + is only every used during the initial connection to the target. */ > > every -> ever > > Simon