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 B986C385841A for ; Fri, 3 Dec 2021 10:43:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B986C385841A Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-498-Rm1k_KL5N9G_HPvGw9c0Iw-1; Fri, 03 Dec 2021 05:43:38 -0500 X-MC-Unique: Rm1k_KL5N9G_HPvGw9c0Iw-1 Received: by mail-wm1-f71.google.com with SMTP id 69-20020a1c0148000000b0033214e5b021so1349020wmb.3 for ; Fri, 03 Dec 2021 02:43:38 -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=8H02ongZLEJh3LW/1LSJRu2FXi9W9KLFvf5Cz+cMVAo=; b=rYDLmsioIiL4W6cvnJWIaUqndTBQ5j5syPvauCb1GchEDQ/vVGxVhmMwVSZxXlMI1i idVrROZdQ3Qde2fh5OUrGZ7+lEX/kHOyMtmWCxt2lcFTulQGCzvugsrm/PUy195tuAAp BLsGYUmESXQgLA27Ef2PgolxU4lrQ45w2M7OzpPF3qK+kmAuiUh47ctTCbpyNIapppgs ADKZdKiq5xJUnwFRfbrTtMXl2YA1t82t+dg4bi8cJmpWz6sPfcNkiJKJwDPjBIyYzKqT iBTfM1Ekad17EOxEYauEU6Gxm889ieB5BPi+Yl4GCgQBRzx6gVnPmW4B4hTmdGlpPoBM X2bA== X-Gm-Message-State: AOAM531Q6tdkP1VRdx4ByPzGuyTkAHWdsvMFta9cX4SDYXnVt1HuIFJa Yn1v1suZpcfrGWugspoGOqq6ICstuuE7/PfBCJ0Mz+T5UTupLeu35Npg/Wd55+6eOos2nf756UU Pz26jrBlEtKAz8YhE49x3XA== X-Received: by 2002:adf:dd0d:: with SMTP id a13mr21479269wrm.259.1638528216871; Fri, 03 Dec 2021 02:43:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJygOG3Kyt9wJKqmpiouSnGBmTjPLqAmXK5H0ya9pRaEobpEOi4yHkyuGWw3EV5ee8Z8UhdqhQ== X-Received: by 2002:adf:dd0d:: with SMTP id a13mr21479243wrm.259.1638528216518; Fri, 03 Dec 2021 02:43:36 -0800 (PST) Received: from localhost (host86-134-238-138.range86-134.btcentralplus.com. [86.134.238.138]) by smtp.gmail.com with ESMTPSA id d9sm2240333wre.52.2021.12.03.02.43.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Dec 2021 02:43:36 -0800 (PST) Date: Fri, 3 Dec 2021 10:43:34 +0000 From: Andrew Burgess To: John Baldwin Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume. Message-ID: <20211203104334.GM2662946@redhat.com> References: <20210803185000.22171-1-jhb@FreeBSD.org> <20210803185000.22171-5-jhb@FreeBSD.org> <20211124132457.GD2662946@redhat.com> <20211201120201.GE2662946@redhat.com> <20211202111941.GJ2662946@redhat.com> <93bccc79-4d39-671c-eeb5-3591dec3cdb4@FreeBSD.org> MIME-Version: 1.0 In-Reply-To: <93bccc79-4d39-671c-eeb5-3591dec3cdb4@FreeBSD.org> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 10:43:12 up 13 days, 23:41, 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=-4.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, 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: Fri, 03 Dec 2021 10:43:41 -0000 * John Baldwin [2021-12-02 17:21:27 -0800]: > On 12/2/21 3:19 AM, Andrew Burgess wrote: > > * John Baldwin [2021-12-01 12:03:41 -0800]: > > > > > On 12/1/21 4:02 AM, Andrew Burgess wrote: > > > > * John Baldwin [2021-11-24 08:03:36 -0800]: > > > > > > > > > On 11/24/21 5:24 AM, Andrew Burgess wrote: > > > > > > * John Baldwin [2021-08-03 11:49:51 -0700]: > > > > > > > > > > > > > Commit 5b6d1e4fa4f ("Multi-target support") enabled async mode in the caller > > > > > > > (do_target_resume) after target::resume returns making this call > > > > > > > redundant. > > > > > > > > > > > > The only thing I spotted where this might result in a change of > > > > > > behaviour is in record-full.c in record_full_wait_1, there's this > > > > > > line: > > > > > > > > > > > > ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0); > > > > > > > > > > > > Doesn't this mean that if the record target is sitting on top of a > > > > > > Linux target, then me could potentially see a change of behaviour > > > > > > here? > > > > > > > > > > > > I know almost nothing about the record target, so I can't really offer > > > > > > any insights into whether the situation about could actually happen or > > > > > > not. > > > > > > > > > > > > Maybe somebody else will have some thoughts, but at a minimum, it is > > > > > > probably worth mentioning that there might be a change for that > > > > > > case.... Unless you know for sure that there isn't. > > > > > > > > > > Hmm, I was not aware of that case. This is a change Pedro had suggested > > > > > and it didn't show any regressions in the test suite on a Linux/x86-64 > > > > > VM, but it very well might be that the test suite doesn't cover > > > > > record? > > > > > > > > I suspect it's not well tested. > > > > > > > > > > > > > > Hmm, looking at record_full_target::resume, it enables async right after > > > > > calling the beneath method and it also does so before returning, so I > > > > > suspect it's explicit call is safe to elide for the same reason as this > > > > > patch. That is, in the code today, linux_nat_target::resume enables async > > > > > mode, then would return to remote_full_target::resume which would enable > > > > > async mode (without any other statements in between), and then return > > > > > to do_target_resume which would enable async mode a third time (again without > > > > > any other code in between aside from any RAII destructors when returning > > > > > from the target methods). Given, that I think this should be safe, and > > > > > I can add a patch to this series to remove the call from > > > > > remote_full_target::resume. > > > > > > > > That's all true, but I wasn't commenting on > > > > record_full_target::resume, rather record_full_wait_1, which is called > > > > from record_full_base_target::wait. > > > > > > > > I'd certainly be happy for your change to go in. Having looked again, > > > > I don't think there's going to be any problems, looking at other > > > > targets, it doesn't seem like having to (re-)enable async mode is > > > > something we'd normally expect to do on the ::wait path... > > > > > > > > I only pointed this out as it seems to be a case that doesn't align > > > > with the claims in your commit message... > > > > > > Oh, humm, I see (and indeed, I had misread your earlier message). It is > > > true that the commit log isn't accurate (and at the least that would > > > need to be amended). One thing I don't really understand are the cases > > > when async is disabled such that resume needs to re-enable it vs are > > > the cases in resume just trying to catch an edge case with attach or > > > create_inferior or the like where async mode isn't enabled by some other > > > hook. > > > > > > Ah, it seems that in wait_one if a target returns WAITKIND_NO_RESUMED, > > > we disable async on that target, and then the next time we want to > > > resume the target we re-enable async after target_resume returns. The > > > other places we disable async are either temporary places around > > > readline, etc. or when when an inferior exits (INF_EXEC_COMPLETE) > > > passed to inferior_event_handle). > > > > > > Given that, I do think the code in record_full_wait_1 is ok: > > > > > > 1) Any time ::wait() is invoked, it looks like target_async(1) should > > > have already been called by the preceding do_target_resume(). > > > > > > 2) The loop in record_full_wait_1() doesn't return to the wait_one() > > > loop where target_async(0) is invoked, so async for the underlying > > > target should stay enabled for the duration of the ::wait() call. > > > > > > 3) All the places that can invoke INF_EXEC_COMPLETE look to be in the > > > gdb core itself which is "above" a target ::wait() method on the > > > stack. > > > > Thanks for the detailed analysis. I agree that, to the best of my > > understanding, your change is fine. I think it would be worth adding > > this analysis to the commit message, then if there ever is a bug in > > the future, we can understand what we thought was going on :) > > I will include it, and I will merge the commit I had made to remove the > explicit target_async(1) calls in the record targets into this commit > so that the detailed commit log covers all of them. > > I noticed one additional call that could perhaps be elided which is the > call to target_async(1) near the end of remote::resume in the remote > target. Unlike the Linux native and record targets, this call is not > at the very end of the function, but I don't think it will break anything > to run the relevant code before enabling async mode rather than after: > > /* We are about to start executing the inferior, let's register it > with the event loop. NOTE: this is the one place where all the > execution commands end up. We could alternatively do this in each > of the execution commands in infcmd.c. */ > /* FIXME: ezannoni 1999-09-28: We may need to move this out of here > into infcmd.c in order to allow inferior function calls to work > NOT asynchronously. */ > if (target_can_async_p ()) > target_async (1); > > /* We've just told the target to resume. The remote server will > wait for the inferior to stop, and then send a stop reply. In > the mean time, we can't start another command/query ourselves > because the stub wouldn't be ready to process it. This applies > only to the base all-stop protocol, however. In non-stop (which > only supports vCont), the stub replies with an "OK", and is > immediate able to process further serial input. */ > if (!target_is_non_stop_p ()) > rs->waiting_for_stop_reply = 1; > > This would also in theory remove the FIXME comment, though I don't know > that this change is fully realizing the goal of that FIXME (it's also > over 20 years old though). If you think it makes sense to elide the > call in the remote target I can fold this into this commit as well. > I can also just leave the remote target as-is. I agree that this call can be safely removed. Thanks, Andrew