From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) by sourceware.org (Postfix) with ESMTPS id 548103858410 for ; Fri, 17 Dec 2021 14:05:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 548103858410 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f45.google.com with SMTP id t18so4224596wrg.11 for ; Fri, 17 Dec 2021 06:05:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=4VrwxtuF9lz61HPzdQ/nO66nByalisSDNBFRkxf0/c8=; b=MdNmmqcS3MIKnqjTlb7RO4h/rnFzzNCwlyEp/FUJLKnlqYmWnqRkE5359n9kE38WZ2 2rlwKP1S6drc5LD8p8UJUut3hbBkpgk/Me+1jMvp9Bgr7NvwilYkiqUNFlhwsK54L7HT paQma3ZDNGdTdaOwBBZksmEV9kM/ioKcGeaIrgNmGKgYvbh6l1B7zgalib1Ry0zIOtCb 23coAJi7wKr7atuyOhD/kOjuygXJKDbXRWV219geG7rCrgqWMPaADxzNbbPELkCw5ij0 8KIuvqAuZS9n3Z5XJQipxQML93LktZLgIOo31/zF6CXV5uSYqVl6CPBID/+IEPVb/l4h JRsA== X-Gm-Message-State: AOAM532emZHvjHVmHuVjFVerSdQj+Cc+DI4UhasCOtPJOcgoPgKp1V70 0rjeijOHwuSFjKolxso1AHE= X-Google-Smtp-Source: ABdhPJykVA5Qz7bFh5u/tBJpgpURfBRoNVAmlNeahiy/gujqGSFzpwXZ8xX4hhK2+xEjodlVLmYkmg== X-Received: by 2002:a05:6000:545:: with SMTP id b5mr2750190wrf.498.1639749957192; Fri, 17 Dec 2021 06:05:57 -0800 (PST) Received: from ?IPV6:2001:8a0:f912:1a00:fb57:3faf:e98:b979? ([2001:8a0:f912:1a00:fb57:3faf:e98:b979]) by smtp.gmail.com with ESMTPSA id z6sm9446740wmp.9.2021.12.17.06.05.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Dec 2021 06:05:56 -0800 (PST) Message-ID: Date: Fri, 17 Dec 2021 14:05:55 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off' Content-Language: en-US To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <849047bbe03fae7f392d2b6f8f50ba299bf694f4.1638354426.git.aburgess@redhat.com> <209f1d51-b4fb-0086-9034-b2720219f2f6@palves.net> <20211217133507.GA140662@redhat.com> From: Pedro Alves In-Reply-To: <20211217133507.GA140662@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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: Fri, 17 Dec 2021 14:06:00 -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. > + will have set the waiting_for_stop_reply flag. */ > + gdb_assert (!rs->waiting_for_stop_reply); > + event_ptid = process_stop_reply (stop_reply, status); > + }