[PATCH v2] gdb: c++ify btrace_target_info

[PATCH v2] gdb: c++ify btrace_target_info

[Markus Metzger]

Following the example of private_thread_info and private_inferior, turn
struct btrace_target_info into a small class hierarchy.

Fixes PR gdb/30751.
---
 gdb/nat/linux-btrace.c      | 50 ++++++++++++++++++++++++++-----------
 gdb/nat/linux-btrace.h      | 11 +++++++-
 gdb/remote.c                | 48 ++++++++++++++++++++++++++---------
 gdbsupport/btrace-common.cc |  2 ++
 gdbsupport/btrace-common.h  |  5 +++-
 5 files changed, 87 insertions(+), 29 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index c5b3f1c93cf..eefcabf509f 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -277,7 +277,7 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
    part at the end and its upper part at the beginning of the buffer.  */
 
 static std::vector<btrace_block> *
-perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
+perf_event_read_bts (linux_btrace_target_info* tinfo, const uint8_t *begin,
 		     const uint8_t *end, const uint8_t *start, size_t size)
 {
   std::vector<btrace_block> *btrace = new std::vector<btrace_block>;
@@ -447,6 +447,22 @@ diagnose_perf_event_open_fail ()
   error (_("Failed to start recording: %s"), safe_strerror (errno));
 }
 
+/* Get the linux version of a btrace_target_info.  */
+
+static linux_btrace_target_info *
+get_linux_btrace_target_info (btrace_target_info *gtinfo)
+{
+  return gdb::checked_static_cast<linux_btrace_target_info *> (gtinfo);
+}
+
+/* Get the linux version of a btrace_target_info - const version.  */
+
+static const linux_btrace_target_info *
+get_linux_btrace_target_info (const btrace_target_info *gtinfo)
+{
+  return gdb::checked_static_cast<const linux_btrace_target_info *> (gtinfo);
+}
+
 /* Enable branch tracing in BTS format.  */
 
 static struct btrace_target_info *
@@ -460,9 +476,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<btrace_target_info> tinfo
-    (XCNEW (btrace_target_info));
-  tinfo->ptid = ptid;
+  std::unique_ptr<linux_btrace_target_info> tinfo
+    { new linux_btrace_target_info (ptid) };
 
   tinfo->conf.format = BTRACE_FORMAT_BTS;
   bts = &tinfo->variant.bts;
@@ -612,9 +627,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   if (pid == 0)
     pid = ptid.pid ();
 
-  gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
-    (XCNEW (btrace_target_info));
-  tinfo->ptid = ptid;
+  std::unique_ptr<linux_btrace_target_info> tinfo
+    { new linux_btrace_target_info (ptid) };
 
   tinfo->conf.format = BTRACE_FORMAT_PT;
   pt = &tinfo->variant.pt;
@@ -755,10 +769,13 @@ linux_disable_pt (struct btrace_tinfo_pt *tinfo)
 /* See linux-btrace.h.  */
 
 enum btrace_error
-linux_disable_btrace (struct btrace_target_info *tinfo)
+linux_disable_btrace (struct btrace_target_info *gtinfo)
 {
   enum btrace_error errcode;
 
+  linux_btrace_target_info *tinfo
+    = get_linux_btrace_target_info (gtinfo);
+
   errcode = BTRACE_ERR_NOT_SUPPORTED;
   switch (tinfo->conf.format)
     {
@@ -775,7 +792,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo)
     }
 
   if (errcode == BTRACE_ERR_NONE)
-    xfree (tinfo);
+    delete gtinfo;
 
   return errcode;
 }
@@ -784,8 +801,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo)
    BTRACE using the TYPE reading method.  */
 
 static enum btrace_error
-linux_read_bts (struct btrace_data_bts *btrace,
-		struct btrace_target_info *tinfo,
+linux_read_bts (btrace_data_bts *btrace, linux_btrace_target_info *tinfo,
 		enum btrace_read_type type)
 {
   struct perf_event_buffer *pevent;
@@ -887,8 +903,7 @@ linux_fill_btrace_pt_config (struct btrace_data_pt_config *conf)
    given by TINFO into BTRACE using the TYPE reading method.  */
 
 static enum btrace_error
-linux_read_pt (struct btrace_data_pt *btrace,
-	       struct btrace_target_info *tinfo,
+linux_read_pt (btrace_data_pt *btrace, linux_btrace_target_info *tinfo,
 	       enum btrace_read_type type)
 {
   struct perf_event_buffer *pt;
@@ -921,9 +936,12 @@ linux_read_pt (struct btrace_data_pt *btrace,
 
 enum btrace_error
 linux_read_btrace (struct btrace_data *btrace,
-		   struct btrace_target_info *tinfo,
+		   struct btrace_target_info *gtinfo,
 		   enum btrace_read_type type)
 {
+  linux_btrace_target_info *tinfo
+    = get_linux_btrace_target_info (gtinfo);
+
   switch (tinfo->conf.format)
     {
     case BTRACE_FORMAT_NONE:
@@ -951,8 +969,10 @@ linux_read_btrace (struct btrace_data *btrace,
 /* See linux-btrace.h.  */
 
 const struct btrace_config *
-linux_btrace_conf (const struct btrace_target_info *tinfo)
+linux_btrace_conf (const struct btrace_target_info *gtinfo)
 {
+  const linux_btrace_target_info *tinfo
+    = get_linux_btrace_target_info (gtinfo);
   return &tinfo->conf;
 }
 
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index ab69647c591..95bcae0ed90 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -23,6 +23,7 @@
 #define NAT_LINUX_BTRACE_H
 
 #include "gdbsupport/btrace-common.h"
+#include "gdbsupport/gdb-checked-static-cast.h"
 #if HAVE_LINUX_PERF_EVENT_H
 #  include <linux/perf_event.h>
 #endif
@@ -81,8 +82,16 @@ 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 final : public btrace_target_info
 {
+  linux_btrace_target_info (ptid_t ptid, const btrace_config &conf)
+    : ptid (ptid), conf (conf), variant ({})
+    {}
+
+  linux_btrace_target_info (ptid_t ptid)
+    : ptid (ptid), conf ({}), variant ({})
+    {}
+
   /* The ptid of this thread.  */
   ptid_t ptid;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 55f2fc3b6b5..b0ab3e8a8eb 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14423,8 +14423,16 @@ parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
 #endif  /* !defined (HAVE_LIBEXPAT) */
 }
 
-struct btrace_target_info
+struct remote_btrace_target_info final : public btrace_target_info
 {
+  remote_btrace_target_info (ptid_t ptid, const btrace_config &conf)
+    : ptid (ptid), conf (conf)
+    {}
+
+  remote_btrace_target_info (ptid_t ptid)
+    : ptid (ptid), conf ({})
+    {}
+
   /* The ptid of the traced thread.  */
   ptid_t ptid;
 
@@ -14432,6 +14440,22 @@ struct btrace_target_info
   struct btrace_config conf;
 };
 
+/* Get the remote version of a btrace_target_info.  */
+
+static remote_btrace_target_info *
+get_remote_btrace_target_info (btrace_target_info *gtinfo)
+{
+  return gdb::checked_static_cast<remote_btrace_target_info *> (gtinfo);
+}
+
+/* Get the remote version of a btrace_target_info - const version.  */
+
+static const remote_btrace_target_info *
+get_remote_btrace_target_info (const btrace_target_info *gtinfo)
+{
+  return gdb::checked_static_cast<const remote_btrace_target_info *> (gtinfo);
+}
+
 /* Reset our idea of our target's btrace configuration.  */
 
 static void
@@ -14502,7 +14526,7 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
 /* Read TP's btrace configuration from the target and store it into CONF.  */
 
 static void
-btrace_read_config (thread_info *tp, struct btrace_config *conf)
+btrace_read_config (thread_info *tp, btrace_config *conf)
 {
   /* target_read_stralloc relies on INFERIOR_PTID.  */
   scoped_restore_current_thread restore_thread;
@@ -14564,9 +14588,8 @@ remote_target::remote_btrace_maybe_reopen ()
 		      btrace_format_string (rs->btrace_config.format));
 	}
 
-      tp->btrace.target = XCNEW (struct btrace_target_info);
-      tp->btrace.target->ptid = tp->ptid;
-      tp->btrace.target->conf = rs->btrace_config;
+      tp->btrace.target
+	= new remote_btrace_target_info { tp->ptid, rs->btrace_config };
     }
 }
 
@@ -14576,7 +14599,6 @@ struct btrace_target_info *
 remote_target::enable_btrace (thread_info *tp,
 			      const struct btrace_config *conf)
 {
-  struct btrace_target_info *tinfo = NULL;
   struct packet_config *packet = NULL;
   struct remote_state *rs = get_remote_state ();
   char *buf = rs->buf.data ();
@@ -14620,8 +14642,7 @@ remote_target::enable_btrace (thread_info *tp,
 	       target_pid_to_str (ptid).c_str ());
     }
 
-  tinfo = XCNEW (struct btrace_target_info);
-  tinfo->ptid = ptid;
+  remote_btrace_target_info *tinfo = new remote_btrace_target_info { ptid };
 
   /* If we fail to read the configuration, we lose some information, but the
      tracing itself is not impacted.  */
@@ -14641,7 +14662,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)
 {
   struct remote_state *rs = get_remote_state ();
   char *buf = rs->buf.data ();
@@ -14650,6 +14671,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 +14689,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 +14698,7 @@ void
 remote_target::teardown_btrace (struct btrace_target_info *tinfo)
 {
   /* We must not talk to the target during teardown.  */
-  xfree (tinfo);
+  delete tinfo;
 }
 
 /* Read the branch trace.  */
@@ -14723,8 +14745,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.cc b/gdbsupport/btrace-common.cc
index 932a11888b0..3167eb0e860 100644
--- a/gdbsupport/btrace-common.cc
+++ b/gdbsupport/btrace-common.cc
@@ -21,6 +21,8 @@
 #include "btrace-common.h"
 
 
+btrace_target_info::~btrace_target_info () = default;
+
 /* See btrace-common.h.  */
 
 const char *
diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
index e287c93a6c1..c77142f4356 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 () = 0;
+};
 
 /* Enumeration of btrace read types.  */
 
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2] gdb: c++ify btrace_target_info

[Simon Marchi]

On 9/7/23 06:44, Markus Metzger wrote:
> Following the example of private_thread_info and private_inferior, turn
> struct btrace_target_info into a small class hierarchy.
> 
> Fixes PR gdb/30751.
> ---
>  gdb/nat/linux-btrace.c      | 50 ++++++++++++++++++++++++++-----------
>  gdb/nat/linux-btrace.h      | 11 +++++++-
>  gdb/remote.c                | 48 ++++++++++++++++++++++++++---------
>  gdbsupport/btrace-common.cc |  2 ++
>  gdbsupport/btrace-common.h  |  5 +++-
>  5 files changed, 87 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index c5b3f1c93cf..eefcabf509f 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -277,7 +277,7 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
>     part at the end and its upper part at the beginning of the buffer.  */
>  
>  static std::vector<btrace_block> *
> -perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
> +perf_event_read_bts (linux_btrace_target_info* tinfo, const uint8_t *begin,

Could you fix the misplaced * while at it?

> @@ -81,8 +82,16 @@ 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 final : public btrace_target_info
>  {
> +  linux_btrace_target_info (ptid_t ptid, const btrace_config &conf)
> +    : ptid (ptid), conf (conf), variant ({})
> +    {}

It seems like that this constructor is unused.

> +
> +  linux_btrace_target_info (ptid_t ptid)
> +    : ptid (ptid), conf ({}), variant ({})
> +    {}

I read your reply about zero-initializing the conf and variant fields.

I think that's fine, it's probably necessary since the users initialize
the relevant fields right after, but it's on the safer side since we are
replacing XCNEW.  But I would suggest putting the {} in the member
declaration directly (which we often do when C++ifying things):

  btrace_config conf {};

  union
  {
    /* CONF.FORMAT == BTRACE_FORMAT_BTS.  */
    struct btrace_tinfo_bts bts;

    /* CONF.FORMAT == BTRACE_FORMAT_PT.  */
    struct btrace_tinfo_pt pt;
  } variant {};


With those fixed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

RE: [PATCH v2] gdb: c++ify btrace_target_info

[Metzger, Markus T]

Hello Simon,

>With those fixed:
>
>Approved-By: Simon Marchi <simon.marchi@efficios.com>

I ended up with a lot of changes and a few more simplifications so there's a v3.

regards,
markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928