From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [IPv6:2610:1c1:1:606c::19:2]) by sourceware.org (Postfix) with ESMTPS id B67FE3858409 for ; Mon, 13 Dec 2021 17:20:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B67FE3858409 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 8694C968DB; Mon, 13 Dec 2021 17:20:11 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JCStz29vBz4dH7; Mon, 13 Dec 2021 17:20:11 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from [10.0.1.4] (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id DAC6B28C2C; Mon, 13 Dec 2021 17:20:10 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <72c3769f-4288-f048-5580-b76ee9ed7d44@FreeBSD.org> Date: Mon, 13 Dec 2021 09:20:09 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCHv3 0/2] Improve 'maint set target-async off' for remote targets Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1639416011; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KalrNTxG4K3WYcY64gzFv2sYUqK1hBtkqO5BtA9kA58=; b=YN6TNOYmkg7V056LIb0rjsBGGb0PTXUfbYbh4DaDiWIXqnCjWdo5Y/BYyV1CfmKQr8MzQX S+OFITm4UMMjmfTUDbOKZxWpuUdd9kg14a7GdtDa50zoh/ZQR+JCQnBvIcVOHIzNS9eBCF tt8IEUehN+U2gLZzq4e7gJnrvrYFG49NTsX+nuXiMVEdbw9YlKDgc9ii5S7ctVV9kUdAUW 4Fc9Bi1Qxha+NAUvPk4Exfb+XMcdRN1wYpvVfT46SQYhRk89Yl8QjjdHAlY9lurh43LJ6P QdjyVA3UpsuMo/9nicklwb1h/1fil3L52hFRqJeno9DP97U+vv0PxkBTIZKA6A== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1639416011; a=rsa-sha256; cv=none; b=pEJPwOMf1kKV0gq1Ko25/lQ+R5X9IZG2V9SO3SqlFoHps1JdayVD60Ah2jZDGfAcf9oRUo +78yTyC6BR+rzV2utRNT+E3e3qGAIgdSYNXPanFuhK5FONzg31v8ThpBZjAM9q0Gj28hPf Rj13fOwIzXSCyhGwniec4aBeHQ8n0ywo6KNqO803aaumKO2D8wJiaVZhHhEJBwKUDepqO9 +OcZxoDselTr8k+rWfmRttEyKkQQdTe36E2TGP7SftdzPO++JOR2o1Urcqxj57Vn1lo9Vx 9pwGqJ3vUFfYFb/3Vzsm8cjNM56hnewDTHPfAEV9aTyLIX5eEcWjmBDiB/EGxA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, 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: Mon, 13 Dec 2021 17:20:15 -0000 On 12/1/21 2:40 AM, Andrew Burgess via Gdb-patches wrote: > I pushed patches #1 to #5 from the original series, but it turned out > that the assert added in patch #5 would trigger in some tests. I > checked my cached results and I definitiely wasn't seeing that assert > during my testing, so my original assumption was that patch #6 fixed > the cause of the assert. > > However, upon retesting I found that this was not the case. After > rebuilding my development branch, the assert was present. But it > hadn't triggered during my testing. > > I honestly have no idea what I originally tested - it clearly showed > the expected improvement for native-extended-remote, so it contained > some version of my work, but obviously not patch #5. > > Which is why there's been a delay getting a new version of this patch > on the list, I've retested the remaining patches, then double checked. > > The old patch #5 is now patch #2 in this series, and the old patch #6 > is now patch #1. Patch #1 contains a fix that allows the assert in > patch #2, that previously caused problems, to now be just fine. > > Changes since v2: > > - Previous patches #1 to #4 are now committed, > > - Previous patch #5 is now patch #2, > > - Previous patch #6 is now patch #1, > > - Change in remote_target::push_stop_reply, the async event token is > now only marked if the target _is_ in async mode, rather than if > the target _can_ be in async mode. When async mode is later > enabled we were already marking the event token, so this should > not be a change in most cases. During remote_target::start_remote > however, we push events, then process them without ever enabling > async mode, this use case was what was previously triggering the > assert in patch #2. > > - Code is otherwise unchanged. > > Changes since v1: > > - The old patch #2 is removed, as Simon pointed out, this was just wrong. > > - The old patch #3, which was approved, is now patch #5, and is unchanged. > > - The old patch #4, is now patch #6, and still needs a review. > > - The old patch #1 has been split up into #1, #2, #3, and #4. Given > significant changes since V1 these should probably be reviewed > afresh. > > --- > > Andrew Burgess (2): > gdb/remote: some fixes for 'maint set target-async off' > gdb: add assert in remote_target::wait relating to async being off > > gdb/remote.c | 192 +++++++++++++++++++++++---------------------------- > 1 file changed, 87 insertions(+), 105 deletions(-) Patch 2 is straightforward and seems obviously correct to me. I think Patch 1 also looks ok, and the description sounds sensible. I'm just not super familiar enough with the remote target in general to give a detailed review. I do think storing the "parsed" stop reply state in an existing queue is probably cleaner overall (and my guess is the queue was added later and this cached reply just wasn't converted to use the queue at the time?) -- John Baldwin