From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id ECE733858CD1 for ; Sat, 15 Jul 2023 02:00:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ECE733858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1689386424; bh=7FpqOhwbkRfkVxjqpL4y2MA5rKX6fGKkD9f2C1GAdQ8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Emrk0fffCNRuCRar+AmjVhYHhLmWJKX9Ja/ZEIuH1kuxrXjgDCOn5NUoDnRItd0vR 48IsPhy1/JABL65XyI4IKr8or6lFN+tfQ0uPfg43L+OoRsBkKkJwx+5LpNHvQn53sv lSLn8TcJaky7XGhPq5hOr2/ysYjlXC43S+aLXnxc= Received: from [10.0.0.170] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 077361E0AC; Fri, 14 Jul 2023 22:00:23 -0400 (EDT) Message-ID: Date: Fri, 14 Jul 2023 22:00:23 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] gdb/amd-dbgapi-target: Use inf param in detach Content-Language: fr To: Pedro Alves , Lancelot SIX , gdb-patches@sourceware.org Cc: lsix@lancelotsix.com References: <20230616092528.69358-1-lancelot.six@amd.com> <2c7f2ef1-dea2-5dbe-8d3f-b9b885be3b72@palves.net> <05217693-1a01-bffb-e74a-503b3fe3a604@amd.com> <31b6963a-6b3a-f18e-9863-40acff23c546@palves.net> From: Simon Marchi In-Reply-To: <31b6963a-6b3a-f18e-9863-40acff23c546@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 7/14/23 15:18, Pedro Alves wrote: > On 2023-07-14 19:54, Lancelot SIX wrote: >> On 14/07/2023 19:35, Pedro Alves wrote: > >>> IMO this is a case of the target method's inferior * parameter having >>> been added too soon -- it would only make sense to have it if nothing in >>> the body of the target method implementations is relying on inf being >>> current_inferior on entry. But that is not the case, plenty of target_ops::detach >>> code has that assumption. The presence of an explicit inferior pointer should mean >>> that detach target method implementations that call code that depends >>> on the inferior being the current inferior, should be using a >>> scoped_restore_current_thread/inferior before calling such global-state-assuming >>> code. But, they don't do that, instead, we have this mixed situation. IMO, it would >>> be better to remove the parameter to avoid confusion and stick to the >>> (if explicit param, then switch global state to it if you need it) rule. I agree with this assessment. I was probably the one adding this parameter, before I understood that having a function receive the context by parameter _and_ depend on the global context is bad (worse than just depending on global context). >>> Anyhow, your patch doesn't make it worse, so it's fine with me. >>> >> >> I can also go the other way around and always use `current_inferior ()` instead of the `inf` parameter in this detach implementation. >> >> What bugged me here is the inconsistency from one line to the next. >> > > Please go ahead and merge your patch. I quickly looked at what it would like to remove > the parameter throughout, and I think we'd just end up doing: > > inferior *inf = current_inferior (); > > at the top of the function, so using "inf" in the remove_breakpoints_inf call anyhow, as > we have multiple references to the current inferior. Either fix is fine with me. Simon