From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 65A763858410 for ; Sat, 18 Dec 2021 10:21:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 65A763858410 Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-228-uK_ZWypvMsqzq98ztNWtQQ-1; Sat, 18 Dec 2021 05:21:11 -0500 X-MC-Unique: uK_ZWypvMsqzq98ztNWtQQ-1 Received: by mail-ed1-f70.google.com with SMTP id l14-20020aa7cace000000b003f7f8e1cbbdso3905838edt.20 for ; Sat, 18 Dec 2021 02:21:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=QDdCbbFdvlIjSqYBQ3P2tLdR4mlaMY1qmjvPhGsGVvY=; b=wViq/nS5WAJ/8LGPpcc/xJ4JE7feyEG1V8g/fFrlxnBDx97gyTqJ751j+ZRO1qIVbv /v0EZyrtmSbA+nUI3ZHW/jMznxtmaMjvCCUQN/dYDuEHdCGaclgLJ0vg97oJO8CSuGhR RTJqXi92UTQO+oubGJyDHyv6xE17RolmyzAYMytGbR+oXjtBtqirjvCd7x28FTB4oAC2 CZHevNOJ/+P6FT9bxETCcb4tdk8kAtQauXAw3m/UUv9RkoauQtX7yllJVjXuUwk9uthn GfYAbma1pI95XW3JFSFFqh3NR/U1w07ivBIEUcUGK9bQRwXryaVP4ETgNz4WSWIo4tHD YczA== X-Gm-Message-State: AOAM530Pm2CsXTQeAFoFHEy8mHAT7W4JBHeJVaSJyTgqU0ZUBHOhXtoB zTQ2LdPZHK4y9Fl/AoPQZSEucOi1797QwqYyagFCFAPDRg6VDafrVeyOMFGIZBmJ9RL21109i/Y 2QrM5FbkwvPWVpOKNvxLt0Q== X-Received: by 2002:aa7:dd13:: with SMTP id i19mr2487825edv.351.1639822869583; Sat, 18 Dec 2021 02:21:09 -0800 (PST) X-Google-Smtp-Source: ABdhPJz+ypFCyIOzUAr0phj7ja/vsaie8/V00ww+Q2oos0ZP8cVkfXD81kyyO5PmJhpjuzB9l8Y2yA== X-Received: by 2002:aa7:dd13:: with SMTP id i19mr2487801edv.351.1639822869216; Sat, 18 Dec 2021 02:21:09 -0800 (PST) Received: from localhost (92.40.179.220.threembb.co.uk. [92.40.179.220]) by smtp.gmail.com with ESMTPSA id y19sm4678912edc.17.2021.12.18.02.21.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 18 Dec 2021 02:21:08 -0800 (PST) Date: Sat, 18 Dec 2021 10:21:07 +0000 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off' Message-ID: <20211218102107.GA2634@redhat.com> References: <849047bbe03fae7f392d2b6f8f50ba299bf694f4.1638354426.git.aburgess@redhat.com> <209f1d51-b4fb-0086-9034-b2720219f2f6@palves.net> <20211217133507.GA140662@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 10:20:36 up 8 min, 1 X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_ABUSEAT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Sat, 18 Dec 2021 10:21:13 -0000 * Pedro Alves [2021-12-17 14:05:55 +0000]: > On 2021-12-17 13:35, Andrew Burgess wrote: > > * Pedro Alves [2021-12-16 21:15:31 +0000]: > > > >> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote: > >> > >>> This problem can clearly be seen I feel by looking at the > >>> remote_state::cached_wait_status flag. This flag tells GDB if there > >>> is a wait status cached in remote_state::buf. However, in > >>> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1 > >>> this flag is just set back to 0, doing this immediately discards any > >>> cached data. > >>> > >>> I don't know if this scheme ever made sense, maybe once upon a time, > >>> GDB would, when it found it had no cached stop, simply re-request the > >>> stop information from the target, however, this certainly isn't the > >>> case now, and resetting the cached_wait_status is (I claim) a bad > >>> thing. > >> > >> I don't think that was ever the case. Take a look at 2d717e4f8a54, > >> where the cached status was introduced to handle "attach". It was simply the case > >> back then that nothing would talk to the target between the initial attach > >> and consuming the event. It's not clear to me why putpkt/getpkt would > >> need to clear the flag back then. Looks more like a "just in case" safeguard. > > > > Thanks for this insight. I've updated the commit message to try and > > describe the history here a little more accurately, including adding > > the commit SHA you gave above, which should help if anyone needs to > > dig into this code in the future. > > > >> > >>> So, finally, in this commit, I propose to remove the > >>> remote_state::cached_wait_status flag and to stop using the ::buf to > >>> cache stop replies. Instead, stop replies will now always be stored > >>> in the ::stop_reply_queue. > >> > >> To be honest, I don't recall exactly why I didn't do that when introducing > >> the stop reply queue. > >> > >>> @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status, > >>> > >>> stop_reply = queued_stop_reply (ptid); > >>> if (stop_reply != NULL) > >>> - return process_stop_reply (stop_reply, status); > >>> - > >>> - if (rs->cached_wait_status) > >>> - /* Use the cached wait status, but only once. */ > >>> - rs->cached_wait_status = 0; > >>> + { > >>> + rs->waiting_for_stop_reply = 0; > >> > >> This is a difference described in the commit log, but looking at the resulting code, > >> I don't understand why clearing this flag is needed here, it looks like dead code to me. > >> I mean, if we have a cached status already, then we're not waiting for a stop reply > >> from the target. Did you run into a case where it was needed? > > > > No I never hit a case where it was definitely needed. Honestly, I > > just looked at the different code paths, and saw that it was pretty > > easy to merge the paths and carry out all the actions, so did that. > > > > However, you're right to call me out on that. It's exactly this sort > > of "just in case" code that was causing the cached packets to be > > discarded originally without warning. > > > > So, I've changed the unconditional "set flag to false" with an assert, > > and a comment. If it turns out we are wrong in our understanding of > > the situation, then hopefully, the problem will get reported, and we > > can figure out what's going on at that point! > > Yeah. I don't imagine how this will ever trigger, but definitely can't hurt > having an assert. > > - If we have an event queued it must mean that we have gotten the reply from the target > already. Recall this is all-stop RSP, vCont is synchronous. > > - And if we see a stop reply, and have it queued, we can't resume the target before > the queued event is processed, otherwise when we get to process the queued event, > the target is already running past what the stop the event was for! If we do ever > manage to sent a vCont with a pending queued event, then that's a bug. > > > > > Below is the updated patch. The only code change is the assert I > > mention above. All other changes are in the commit message. > > > > I'll give this a few days in case you want to follow up, then I'll > > push this. > > > > Thanks, this is OK. Just noticed a tiny typo in the new comment: > > > + { > > + /* Currently non of the paths that push a stop reply onto the queue > > > non -> none. I'd argue that "Currently" is redundant, and gives a false > impression that that might change without much consequence other than hitting > this assertion. I think you should remove that word. Thanks for catching that. I made the changes you suggested, and pushed this patch. Thanks, Andrew > > > + will have set the waiting_for_stop_reply flag. */ > > + gdb_assert (!rs->waiting_for_stop_reply); > > + event_ptid = process_stop_reply (stop_reply, status); > > + } >