From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by sourceware.org (Postfix) with ESMTPS id D683238582A4 for ; Fri, 14 Jul 2023 18:35:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D683238582A4 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-f50.google.com with SMTP id ffacd0b85a97d-3110ab7110aso2328031f8f.3 for ; Fri, 14 Jul 2023 11:35:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689359757; x=1691951757; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=kSbJpWVZlFE8zLXOm+bRcc4mmOi0MxQRoeAFbgfVN+I=; b=f2p/Zy0Yc7jlzDS/wVlqhnY5SXu5I8J+OmS1ZduxgmbQSJn4uBz111A9Mfjl5e2Pfa 0cDRT4u+Yw+ajkV5VYYkOsUngpb+aP+F6biY/jR2K41vGktL6efSbSSh2aoJMwfknDFe t7JUHwy0UdirWwTG4UEjgXNvfYgqNmdBznDkk4ZQte6GOQiHSmS80krgjlElA87n9Bgp FfEJ6A4KzIlYuZBMcKbtini0HruG8wyhDkP/udyPAjKv/eTGFrgyo6G10RftkDu2VQDr Y0GWnRduyPjHXlYlU1jgWZuG7eOCTTMO6h5CkN4wkoq3cZOssGfeRf/i9ZsU5bph1icK QBhw== X-Gm-Message-State: ABy/qLYIuoMvyodZEzPgjsbdAyWmVTaCo5Svb1DzBXekR89qXSMJtQ0X GQqM3Q9KCznDEmtPDCm37inrzVEix+Q= X-Google-Smtp-Source: APBJJlEEnnkSkyfM01FAde6MLIXpqwjV7tTigXM+03YBNKzTAdxAcTuODba/hM+0ZKUCco1UzurPxg== X-Received: by 2002:adf:ce88:0:b0:313:f957:fc0c with SMTP id r8-20020adfce88000000b00313f957fc0cmr4767438wrn.47.1689359756556; Fri, 14 Jul 2023 11:35:56 -0700 (PDT) Received: from ?IPV6:2001:8a0:f91d:bc00:98b3:dfb7:8070:8955? ([2001:8a0:f91d:bc00:98b3:dfb7:8070:8955]) by smtp.gmail.com with ESMTPSA id d17-20020adfe891000000b003143cdc5949sm11791495wrm.9.2023.07.14.11.35.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 14 Jul 2023 11:35:55 -0700 (PDT) Message-ID: <2c7f2ef1-dea2-5dbe-8d3f-b9b885be3b72@palves.net> Date: Fri, 14 Jul 2023 19:35:55 +0100 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: en-US To: Lancelot SIX , gdb-patches@sourceware.org Cc: lsix@lancelotsix.com References: <20230616092528.69358-1-lancelot.six@amd.com> From: Pedro Alves In-Reply-To: <20230616092528.69358-1-lancelot.six@amd.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 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,T_SCC_BODY_TEXT_LINE autolearn=no 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 2023-06-16 10:25, Lancelot SIX via Gdb-patches wrote: > Current implementation of amd_dbgapi_target::detach (inferior *, int) > does the following: > > remove_breakpoints_inf (current_inferior ()); > detach_amd_dbgapi (inf); > beneath ()->detach (inf, from_tty); > > I find that using a mix of `current_inferior ()` and `inf` disturbing. > At this point, we know that both are the same (target_detach does assert > that `inf == current_inferior ()` before calling target_ops::detach). > > To improve consistency, this patch replaces `current_inferior ()` with > `inf` in amd_dbgapi_target::detach. > > Change-Id: I01b7ba2e661c25839438354b509d7abbddb7c5ed 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. Anyhow, your patch doesn't make it worse, so it's fine with me.