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 BB1E03858C78 for ; Wed, 6 Sep 2023 13:21:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BB1E03858C78 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 386DLff9018777 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 6 Sep 2023 09:21:46 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 386DLff9018777 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1694006507; bh=bJrQemw9tK7tuAYMnimwdbPtUVderMcZqBgyqlfE/YM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=NDDQVM8kYB6icnt7a/uaJ6HzozL/zwEwxLVRA/vypaMVg5b+cChHVFVaG1TObNh0L Wmsh/zrSgJMfR6E02MMHh3/xYR8zOeWAmXabpqN9uoLbaAkY/w2nNotwBjKCArMbyu 0lq0XVoaX/yOoJ3BObhLBohOF5Zlz5UAgIEHowvY= 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 ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9F1211E092; Wed, 6 Sep 2023 09:21:41 -0400 (EDT) Message-ID: <79f9ba63-e2c8-4a32-a310-6666d75cfae1@polymtl.ca> Date: Wed, 6 Sep 2023 09:21:41 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] gdb: c++ify btrace_target_info Content-Language: fr To: Markus Metzger , gdb-patches@sourceware.org Cc: vries@gcc.gnu.org References: <20230906074727.426831-1-markus.t.metzger@intel.com> <20230906074727.426831-2-markus.t.metzger@intel.com> From: Simon Marchi In-Reply-To: <20230906074727.426831-2-markus.t.metzger@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 6 Sep 2023 13:21:41 +0000 X-Spam-Status: No, score=-3037.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: > @@ -460,9 +483,8 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf) > if (!cpu_supports_bts ()) > error (_("BTS support has been disabled for the target cpu.")); > > - gdb::unique_xmalloc_ptr tinfo > - (XCNEW (btrace_target_info)); > - tinfo->ptid = ptid; > + std::unique_ptr tinfo > + { new linux_btrace_target_info (ptid) }; Not mandatory, but we often define aliases for unique pointer types, to ease reading a bit: using linux_btrace_target_info_up = std::unique_ptr; > @@ -775,7 +799,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo) > } > > 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? > @@ -81,7 +82,7 @@ struct btrace_tinfo_pt > #endif /* HAVE_LINUX_PERF_EVENT_H */ > > /* Branch trace target information per thread. */ > -struct btrace_target_info > +struct linux_btrace_target_info : public btrace_target_info You can make the class (struct) final. > { > /* The ptid of this thread. */ > ptid_t ptid; Not shown here, but the linux_btrace_target_info has this union: /* The branch tracing format specific information. */ union { /* CONF.FORMAT == BTRACE_FORMAT_BTS. */ struct btrace_tinfo_bts bts; /* CONF.FORMAT == BTRACE_FORMAT_PT. */ struct btrace_tinfo_pt pt; } variant; This might be a good candidate to become separate sub-classes (not in this patch, but later)? > @@ -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. > + {} We usually put constructors and methods before fields. > }; > > /* See to_enable_btrace in target.h. */ > diff --git a/gdb/remote.c b/gdb/remote.c > index 55f2fc3b6b5..043781cea06 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -14423,15 +14423,45 @@ parse_xml_btrace_conf (struct btrace_config *conf, const char *xml) > #endif /* !defined (HAVE_LIBEXPAT) */ > } > > -struct btrace_target_info > +struct remote_btrace_target_info : public btrace_target_info final here too. > { > /* 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. > + > + remote_btrace_target_info (ptid_t _ptid, const struct btrace_config &_conf) In C++ we tend to drop the struct keyword when not needed. > + : ptid (_ptid), conf (_conf) > + {} Did you take this style of leading underscore from somewhere? I typically don't mind giving the same name to fields an constructor parameters, it works just fine. Otherwise, I've seen others use trailing underscores. > + > + remote_btrace_target_info (ptid_t _ptid) > + : ptid (_ptid), conf ({}) The latter (conf) is probably not need, it will automatically default-construct. > + {} > }; > > +/* Get the remote version of a btrace_target_info. */ > + > +static remote_btrace_target_info * > +get_remote_btrace_target_info (btrace_target_info *gtinfo) > +{ > + if (gtinfo == nullptr) > + return nullptr; I don't think the nullptr check is needed, checked_static_cast (which is either static_cast or dynamic_cast) should accept nullptr. > @@ -14576,7 +14605,7 @@ struct btrace_target_info * > remote_target::enable_btrace (thread_info *tp, > const struct btrace_config *conf) > { > - struct btrace_target_info *tinfo = NULL; > + struct remote_btrace_target_info *tinfo = NULL; Since you touch this line, I would suggest moving the declaration to where the variable is initialized. > struct packet_config *packet = NULL; > struct remote_state *rs = get_remote_state (); > char *buf = rs->buf.data (); > @@ -14620,8 +14649,7 @@ remote_target::enable_btrace (thread_info *tp, > target_pid_to_str (ptid).c_str ()); > } > > - tinfo = XCNEW (struct btrace_target_info); > - tinfo->ptid = ptid; > + tinfo = new remote_btrace_target_info { ptid }; If I read things correctly, I think that a further step (not in this patch) could be to make remote_target::enable_btrace return an std::unique_ptr (btrace_target_info_up). > > /* If we fail to read the configuration, we lose some information, but the > tracing itself is not impacted. */ > @@ -14641,7 +14669,7 @@ remote_target::enable_btrace (thread_info *tp, > /* Disable branch tracing. */ > > void > -remote_target::disable_btrace (struct btrace_target_info *tinfo) > +remote_target::disable_btrace (struct btrace_target_info *gtinfo) ... and this could probably accept a btrace_target_info_up. > { > struct remote_state *rs = get_remote_state (); > char *buf = rs->buf.data (); > @@ -14650,6 +14678,7 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo) > if (m_features.packet_support (PACKET_Qbtrace_off) != PACKET_ENABLE) > error (_("Target does not support branch tracing.")); > > + remote_btrace_target_info *tinfo = get_remote_btrace_target_info (gtinfo); > set_general_thread (tinfo->ptid); > > buf += xsnprintf (buf, endbuf - buf, "%s", > @@ -14667,7 +14696,7 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo) > target_pid_to_str (tinfo->ptid).c_str ()); > } > > - xfree (tinfo); > + delete gtinfo; > } > > /* Teardown branch tracing. */ > @@ -14676,7 +14705,7 @@ void > remote_target::teardown_btrace (struct btrace_target_info *tinfo) > { > /* We must not talk to the target during teardown. */ > - xfree (tinfo); > + delete tinfo; And it seems like this could take a btrace_target_info_up too? I just noticed that remote_target::teardown_btrace deletes the tinfo it receives, but x86_linux_nat_target::teardown_btrace doesn't. Is that a bug / leak on the x86-linux-nat side? > } > > /* Read the branch trace. */ > @@ -14723,8 +14752,10 @@ remote_target::read_btrace (struct btrace_data *btrace, > } > > const struct btrace_config * > -remote_target::btrace_conf (const struct btrace_target_info *tinfo) > +remote_target::btrace_conf (const struct btrace_target_info *gtinfo) > { > + const remote_btrace_target_info *tinfo > + = get_remote_btrace_target_info (gtinfo); > return &tinfo->conf; > } > > diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h > index e287c93a6c1..7ae865b3978 100644 > --- a/gdbsupport/btrace-common.h > +++ b/gdbsupport/btrace-common.h > @@ -214,7 +214,10 @@ struct btrace_data > }; > > /* Target specific branch trace information. */ > -struct btrace_target_info; > +struct btrace_target_info > +{ > + virtual ~btrace_target_info () {} > +}; I would typically make the destructor pure to make it impossible to instantiate an object of this type: virtual ~btrace_target_info () = 0; If I remember correctly, you then need to define the destructor in the .cc file then, to avoid an undefined reference, something like: btrace_target_info::~btrace_target_info () = default; Simon