From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id F2E383858D1E for ; Thu, 7 Sep 2023 13:54:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F2E383858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 387Ds6Xr017274 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 7 Sep 2023 09:54:11 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 387Ds6Xr017274 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1694094851; bh=cgD9ITWSSVZlUP0/Y0Z9WqzoHNjHs9GVM7JQIsSHf6A=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=jfzzH4YAB30TXNUWOF3xASxiXM+lc3z8o5qyRiXZMGZafk4W2DrFKngNMzBWfLWT5 nMjq55hHpSTLnLER07vb7SaN0nrO2S0CGsi0hS/1eGMoAd4tiX8kufrXtztvrr4RhN NemnlZKQhYpRcPl1WGmK1TkDChXIJXr7KGTgRk8o= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 148B81E092; Thu, 7 Sep 2023 09:54:05 -0400 (EDT) Message-ID: Date: Thu, 7 Sep 2023 09:54:05 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] gdb: c++ify btrace_target_info Content-Language: fr To: "Metzger, Markus T" Cc: "vries@gcc.gnu.org" , "gdb-patches@sourceware.org" References: <20230906074727.426831-1-markus.t.metzger@intel.com> <20230906074727.426831-2-markus.t.metzger@intel.com> <79f9ba63-e2c8-4a32-a310-6666d75cfae1@polymtl.ca> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 7 Sep 2023 13:54:06 +0000 X-Spam-Status: No, score=-3031.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=unavailable 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 9/7/23 06:42, Metzger, Markus T wrote: > Hello Simon, > > Thanks for your review. > >>> if (errcode == BTRACE_ERR_NONE) >>> - xfree (tinfo); >>> + delete gtinfo; >> >> A question about this, even if it's not introduced by this patch... if >> errcode is not BTRACE_ERR_NONE, who deletes gtinfo? > > It would be leaked. Since tracing couldn't be disabled, it would still > be in use, so leaking it seems better than deleting an in-use object > and risking either crash or data corruption. I'm not sure I follow. Even if something fails when trying to disable btrace (like a call to the OS fails for an unknown reason), GDB can still decide it doesn't want to do tracing / consume btrace data and get rid of the object. > Now, that cannot happen today, since both linux_disable_pt and > linux_disable_bts return BTRACE_ERR_NONE. Ok, well if these operations can't fail I think they should not return an error code (for the same reason I wrote below about hypothetical scenarios). >>> @@ -100,6 +101,15 @@ struct btrace_target_info >>> struct btrace_tinfo_pt pt; >>> } variant; >>> #endif /* HAVE_LINUX_PERF_EVENT_H */ >>> + >>> + >>> + linux_btrace_target_info (ptid_t _ptid, const struct btrace_config &_conf) >>> + : ptid (_ptid), conf (_conf), variant ({}) >>> + {} >>> + >>> + linux_btrace_target_info (ptid_t _ptid) >>> + : ptid (_ptid), conf ({}), variant ({}) >> >> It's probably not needed to explicitly initialize conf and variant. > > That's an interesting question. My interpretation is that not specifying a > ctor-initializer would leave things undefined*, whereas supplying an empty > aggregate would zero-initialize everything. > > *we'd default-construct conf, which has an implicitly defined default > constructor, which then default-constructs all the data members, and so on, > until we reach plain integers, on which no initialization is performed. Ok, replied to that in my response to v2. >>> { >>> /* The ptid of the traced thread. */ >>> ptid_t ptid; >>> >>> /* The obtained branch trace configuration. */ >>> struct btrace_config conf; >> >> So, these two fields are common to both btrace_target_info sub-classes. >> Do you intend to move them to the base class? That would allow getting >> rid of some (all?) casts in remote.c. > > No. I think it's better to leave the base class empty to give full freedom > to targets. Do you have an actual case of btrace target that wouldn't use these fields? My stance on it is: this is all internal GDB code, it's not an API we expose. I don't think we should complicate things for hypothetical scenarios, when we always have the freedom to re-work things later if need be. I think it makes more sense to keep things simple for what we have today. Simon