[PATCH 1/2] gdb, btrace: move xml parsing into remote.c

[PATCH 1/2] gdb, btrace: move xml parsing into remote.c

[Markus Metzger]

The code is only used in remote.c and all functions can be declared static.
---
 gdb/btrace.c | 318 ---------------------------------------------------
 gdb/btrace.h |   6 -
 gdb/remote.c | 317 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 317 insertions(+), 324 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index dbdcea0a8ea..a7b6e810cdc 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -29,7 +29,6 @@
 #include "disasm.h"
 #include "source.h"
 #include "filenames.h"
-#include "xml-support.h"
 #include "regcache.h"
 #include "gdbsupport/rsp-low.h"
 #include "gdbcmd.h"
@@ -2017,323 +2016,6 @@ btrace_free_objfile (struct objfile *objfile)
     btrace_clear (tp);
 }
 
-#if defined (HAVE_LIBEXPAT)
-
-/* Check the btrace document version.  */
-
-static void
-check_xml_btrace_version (struct gdb_xml_parser *parser,
-			  const struct gdb_xml_element *element,
-			  void *user_data,
-			  std::vector<gdb_xml_value> &attributes)
-{
-  const char *version
-    = (const char *) xml_find_attribute (attributes, "version")->value.get ();
-
-  if (strcmp (version, "1.0") != 0)
-    gdb_xml_error (parser, _("Unsupported btrace version: \"%s\""), version);
-}
-
-/* Parse a btrace "block" xml record.  */
-
-static void
-parse_xml_btrace_block (struct gdb_xml_parser *parser,
-			const struct gdb_xml_element *element,
-			void *user_data,
-			std::vector<gdb_xml_value> &attributes)
-{
-  struct btrace_data *btrace;
-  ULONGEST *begin, *end;
-
-  btrace = (struct btrace_data *) user_data;
-
-  switch (btrace->format)
-    {
-    case BTRACE_FORMAT_BTS:
-      break;
-
-    case BTRACE_FORMAT_NONE:
-      btrace->format = BTRACE_FORMAT_BTS;
-      btrace->variant.bts.blocks = new std::vector<btrace_block>;
-      break;
-
-    default:
-      gdb_xml_error (parser, _("Btrace format error."));
-    }
-
-  begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get ();
-  end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get ();
-  btrace->variant.bts.blocks->emplace_back (*begin, *end);
-}
-
-/* Parse a "raw" xml record.  */
-
-static void
-parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
-	       gdb_byte **pdata, size_t *psize)
-{
-  gdb_byte *bin;
-  size_t len, size;
-
-  len = strlen (body_text);
-  if (len % 2 != 0)
-    gdb_xml_error (parser, _("Bad raw data size."));
-
-  size = len / 2;
-
-  gdb::unique_xmalloc_ptr<gdb_byte> data ((gdb_byte *) xmalloc (size));
-  bin = data.get ();
-
-  /* We use hex encoding - see gdbsupport/rsp-low.h.  */
-  while (len > 0)
-    {
-      char hi, lo;
-
-      hi = *body_text++;
-      lo = *body_text++;
-
-      if (hi == 0 || lo == 0)
-	gdb_xml_error (parser, _("Bad hex encoding."));
-
-      *bin++ = fromhex (hi) * 16 + fromhex (lo);
-      len -= 2;
-    }
-
-  *pdata = data.release ();
-  *psize = size;
-}
-
-/* Parse a btrace pt-config "cpu" xml record.  */
-
-static void
-parse_xml_btrace_pt_config_cpu (struct gdb_xml_parser *parser,
-				const struct gdb_xml_element *element,
-				void *user_data,
-				std::vector<gdb_xml_value> &attributes)
-{
-  struct btrace_data *btrace;
-  const char *vendor;
-  ULONGEST *family, *model, *stepping;
-
-  vendor =
-    (const char *) xml_find_attribute (attributes, "vendor")->value.get ();
-  family
-    = (ULONGEST *) xml_find_attribute (attributes, "family")->value.get ();
-  model
-    = (ULONGEST *) xml_find_attribute (attributes, "model")->value.get ();
-  stepping
-    = (ULONGEST *) xml_find_attribute (attributes, "stepping")->value.get ();
-
-  btrace = (struct btrace_data *) user_data;
-
-  if (strcmp (vendor, "GenuineIntel") == 0)
-    btrace->variant.pt.config.cpu.vendor = CV_INTEL;
-
-  btrace->variant.pt.config.cpu.family = *family;
-  btrace->variant.pt.config.cpu.model = *model;
-  btrace->variant.pt.config.cpu.stepping = *stepping;
-}
-
-/* Parse a btrace pt "raw" xml record.  */
-
-static void
-parse_xml_btrace_pt_raw (struct gdb_xml_parser *parser,
-			 const struct gdb_xml_element *element,
-			 void *user_data, const char *body_text)
-{
-  struct btrace_data *btrace;
-
-  btrace = (struct btrace_data *) user_data;
-  parse_xml_raw (parser, body_text, &btrace->variant.pt.data,
-		 &btrace->variant.pt.size);
-}
-
-/* Parse a btrace "pt" xml record.  */
-
-static void
-parse_xml_btrace_pt (struct gdb_xml_parser *parser,
-		     const struct gdb_xml_element *element,
-		     void *user_data,
-		     std::vector<gdb_xml_value> &attributes)
-{
-  struct btrace_data *btrace;
-
-  btrace = (struct btrace_data *) user_data;
-  btrace->format = BTRACE_FORMAT_PT;
-  btrace->variant.pt.config.cpu.vendor = CV_UNKNOWN;
-  btrace->variant.pt.data = NULL;
-  btrace->variant.pt.size = 0;
-}
-
-static const struct gdb_xml_attribute block_attributes[] = {
-  { "begin", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
-  { "end", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_attribute btrace_pt_config_cpu_attributes[] = {
-  { "vendor", GDB_XML_AF_NONE, NULL, NULL },
-  { "family", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
-  { "model", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
-  { "stepping", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_pt_config_children[] = {
-  { "cpu", btrace_pt_config_cpu_attributes, NULL, GDB_XML_EF_OPTIONAL,
-    parse_xml_btrace_pt_config_cpu, NULL },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_pt_children[] = {
-  { "pt-config", NULL, btrace_pt_config_children, GDB_XML_EF_OPTIONAL, NULL,
-    NULL },
-  { "raw", NULL, NULL, GDB_XML_EF_OPTIONAL, NULL, parse_xml_btrace_pt_raw },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_attribute btrace_attributes[] = {
-  { "version", GDB_XML_AF_NONE, NULL, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_children[] = {
-  { "block", block_attributes, NULL,
-    GDB_XML_EF_REPEATABLE | GDB_XML_EF_OPTIONAL, parse_xml_btrace_block, NULL },
-  { "pt", NULL, btrace_pt_children, GDB_XML_EF_OPTIONAL, parse_xml_btrace_pt,
-    NULL },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_elements[] = {
-  { "btrace", btrace_attributes, btrace_children, GDB_XML_EF_NONE,
-    check_xml_btrace_version, NULL },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-#endif /* defined (HAVE_LIBEXPAT) */
-
-/* See btrace.h.  */
-
-void
-parse_xml_btrace (struct btrace_data *btrace, const char *buffer)
-{
-#if defined (HAVE_LIBEXPAT)
-
-  int errcode;
-  btrace_data result;
-  result.format = BTRACE_FORMAT_NONE;
-
-  errcode = gdb_xml_parse_quick (_("btrace"), "btrace.dtd", btrace_elements,
-				 buffer, &result);
-  if (errcode != 0)
-    error (_("Error parsing branch trace."));
-
-  /* Keep parse results.  */
-  *btrace = std::move (result);
-
-#else  /* !defined (HAVE_LIBEXPAT) */
-
-  error (_("Cannot process branch trace.  XML support was disabled at "
-	   "compile time."));
-
-#endif  /* !defined (HAVE_LIBEXPAT) */
-}
-
-#if defined (HAVE_LIBEXPAT)
-
-/* Parse a btrace-conf "bts" xml record.  */
-
-static void
-parse_xml_btrace_conf_bts (struct gdb_xml_parser *parser,
-			  const struct gdb_xml_element *element,
-			  void *user_data,
-			  std::vector<gdb_xml_value> &attributes)
-{
-  struct btrace_config *conf;
-  struct gdb_xml_value *size;
-
-  conf = (struct btrace_config *) user_data;
-  conf->format = BTRACE_FORMAT_BTS;
-  conf->bts.size = 0;
-
-  size = xml_find_attribute (attributes, "size");
-  if (size != NULL)
-    conf->bts.size = (unsigned int) *(ULONGEST *) size->value.get ();
-}
-
-/* Parse a btrace-conf "pt" xml record.  */
-
-static void
-parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
-			  const struct gdb_xml_element *element,
-			  void *user_data,
-			  std::vector<gdb_xml_value> &attributes)
-{
-  struct btrace_config *conf;
-  struct gdb_xml_value *size;
-
-  conf = (struct btrace_config *) user_data;
-  conf->format = BTRACE_FORMAT_PT;
-  conf->pt.size = 0;
-
-  size = xml_find_attribute (attributes, "size");
-  if (size != NULL)
-    conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
-}
-
-static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
-  { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_attribute btrace_conf_bts_attributes[] = {
-  { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_conf_children[] = {
-  { "bts", btrace_conf_bts_attributes, NULL, GDB_XML_EF_OPTIONAL,
-    parse_xml_btrace_conf_bts, NULL },
-  { "pt", btrace_conf_pt_attributes, NULL, GDB_XML_EF_OPTIONAL,
-    parse_xml_btrace_conf_pt, NULL },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_attribute btrace_conf_attributes[] = {
-  { "version", GDB_XML_AF_NONE, NULL, NULL },
-  { NULL, GDB_XML_AF_NONE, NULL, NULL }
-};
-
-static const struct gdb_xml_element btrace_conf_elements[] = {
-  { "btrace-conf", btrace_conf_attributes, btrace_conf_children,
-    GDB_XML_EF_NONE, NULL, NULL },
-  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
-};
-
-#endif /* defined (HAVE_LIBEXPAT) */
-
-/* See btrace.h.  */
-
-void
-parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
-{
-#if defined (HAVE_LIBEXPAT)
-
-  int errcode;
-  errcode = gdb_xml_parse_quick (_("btrace-conf"), "btrace-conf.dtd",
-				 btrace_conf_elements, xml, conf);
-  if (errcode != 0)
-    error (_("Error parsing branch trace configuration."));
-
-#else  /* !defined (HAVE_LIBEXPAT) */
-
-  error (_("Cannot process the branch trace configuration.  XML support "
-	   "was disabled at compile time."));
-
-#endif  /* !defined (HAVE_LIBEXPAT) */
-}
-
 /* See btrace.h.  */
 
 const struct btrace_insn *
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 0ec84113217..0d53cb794e7 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -393,12 +393,6 @@ extern void btrace_clear (struct thread_info *);
 /* Clear the branch trace for all threads when an object file goes away.  */
 extern void btrace_free_objfile (struct objfile *);
 
-/* Parse a branch trace xml document XML into DATA.  */
-extern void parse_xml_btrace (struct btrace_data *data, const char *xml);
-
-/* Parse a branch trace configuration xml document XML into CONF.  */
-extern void parse_xml_btrace_conf (struct btrace_config *conf, const char *xml);
-
 /* Dereference a branch trace instruction iterator.  Return a pointer to the
    instruction the iterator points to.
    May return NULL if the iterator points to a gap in the trace.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index 6fb3a015967..55f2fc3b6b5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14106,6 +14106,323 @@ remote_target::can_use_agent ()
   return (m_features.packet_support (PACKET_QAgent) != PACKET_DISABLE);
 }
 
+#if defined (HAVE_LIBEXPAT)
+
+/* Check the btrace document version.  */
+
+static void
+check_xml_btrace_version (struct gdb_xml_parser *parser,
+			  const struct gdb_xml_element *element,
+			  void *user_data,
+			  std::vector<gdb_xml_value> &attributes)
+{
+  const char *version
+    = (const char *) xml_find_attribute (attributes, "version")->value.get ();
+
+  if (strcmp (version, "1.0") != 0)
+    gdb_xml_error (parser, _("Unsupported btrace version: \"%s\""), version);
+}
+
+/* Parse a btrace "block" xml record.  */
+
+static void
+parse_xml_btrace_block (struct gdb_xml_parser *parser,
+			const struct gdb_xml_element *element,
+			void *user_data,
+			std::vector<gdb_xml_value> &attributes)
+{
+  struct btrace_data *btrace;
+  ULONGEST *begin, *end;
+
+  btrace = (struct btrace_data *) user_data;
+
+  switch (btrace->format)
+    {
+    case BTRACE_FORMAT_BTS:
+      break;
+
+    case BTRACE_FORMAT_NONE:
+      btrace->format = BTRACE_FORMAT_BTS;
+      btrace->variant.bts.blocks = new std::vector<btrace_block>;
+      break;
+
+    default:
+      gdb_xml_error (parser, _("Btrace format error."));
+    }
+
+  begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get ();
+  end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get ();
+  btrace->variant.bts.blocks->emplace_back (*begin, *end);
+}
+
+/* Parse a "raw" xml record.  */
+
+static void
+parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
+	       gdb_byte **pdata, size_t *psize)
+{
+  gdb_byte *bin;
+  size_t len, size;
+
+  len = strlen (body_text);
+  if (len % 2 != 0)
+    gdb_xml_error (parser, _("Bad raw data size."));
+
+  size = len / 2;
+
+  gdb::unique_xmalloc_ptr<gdb_byte> data ((gdb_byte *) xmalloc (size));
+  bin = data.get ();
+
+  /* We use hex encoding - see gdbsupport/rsp-low.h.  */
+  while (len > 0)
+    {
+      char hi, lo;
+
+      hi = *body_text++;
+      lo = *body_text++;
+
+      if (hi == 0 || lo == 0)
+	gdb_xml_error (parser, _("Bad hex encoding."));
+
+      *bin++ = fromhex (hi) * 16 + fromhex (lo);
+      len -= 2;
+    }
+
+  *pdata = data.release ();
+  *psize = size;
+}
+
+/* Parse a btrace pt-config "cpu" xml record.  */
+
+static void
+parse_xml_btrace_pt_config_cpu (struct gdb_xml_parser *parser,
+				const struct gdb_xml_element *element,
+				void *user_data,
+				std::vector<gdb_xml_value> &attributes)
+{
+  struct btrace_data *btrace;
+  const char *vendor;
+  ULONGEST *family, *model, *stepping;
+
+  vendor
+    = (const char *) xml_find_attribute (attributes, "vendor")->value.get ();
+  family
+    = (ULONGEST *) xml_find_attribute (attributes, "family")->value.get ();
+  model
+    = (ULONGEST *) xml_find_attribute (attributes, "model")->value.get ();
+  stepping
+    = (ULONGEST *) xml_find_attribute (attributes, "stepping")->value.get ();
+
+  btrace = (struct btrace_data *) user_data;
+
+  if (strcmp (vendor, "GenuineIntel") == 0)
+    btrace->variant.pt.config.cpu.vendor = CV_INTEL;
+
+  btrace->variant.pt.config.cpu.family = *family;
+  btrace->variant.pt.config.cpu.model = *model;
+  btrace->variant.pt.config.cpu.stepping = *stepping;
+}
+
+/* Parse a btrace pt "raw" xml record.  */
+
+static void
+parse_xml_btrace_pt_raw (struct gdb_xml_parser *parser,
+			 const struct gdb_xml_element *element,
+			 void *user_data, const char *body_text)
+{
+  struct btrace_data *btrace;
+
+  btrace = (struct btrace_data *) user_data;
+  parse_xml_raw (parser, body_text, &btrace->variant.pt.data,
+		 &btrace->variant.pt.size);
+}
+
+/* Parse a btrace "pt" xml record.  */
+
+static void
+parse_xml_btrace_pt (struct gdb_xml_parser *parser,
+		     const struct gdb_xml_element *element,
+		     void *user_data,
+		     std::vector<gdb_xml_value> &attributes)
+{
+  struct btrace_data *btrace;
+
+  btrace = (struct btrace_data *) user_data;
+  btrace->format = BTRACE_FORMAT_PT;
+  btrace->variant.pt.config.cpu.vendor = CV_UNKNOWN;
+  btrace->variant.pt.data = NULL;
+  btrace->variant.pt.size = 0;
+}
+
+static const struct gdb_xml_attribute block_attributes[] = {
+  { "begin", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { "end", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_attribute btrace_pt_config_cpu_attributes[] = {
+  { "vendor", GDB_XML_AF_NONE, NULL, NULL },
+  { "family", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { "model", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { "stepping", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_pt_config_children[] = {
+  { "cpu", btrace_pt_config_cpu_attributes, NULL, GDB_XML_EF_OPTIONAL,
+    parse_xml_btrace_pt_config_cpu, NULL },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_pt_children[] = {
+  { "pt-config", NULL, btrace_pt_config_children, GDB_XML_EF_OPTIONAL, NULL,
+    NULL },
+  { "raw", NULL, NULL, GDB_XML_EF_OPTIONAL, NULL, parse_xml_btrace_pt_raw },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_attribute btrace_attributes[] = {
+  { "version", GDB_XML_AF_NONE, NULL, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_children[] = {
+  { "block", block_attributes, NULL,
+    GDB_XML_EF_REPEATABLE | GDB_XML_EF_OPTIONAL, parse_xml_btrace_block, NULL },
+  { "pt", NULL, btrace_pt_children, GDB_XML_EF_OPTIONAL, parse_xml_btrace_pt,
+    NULL },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_elements[] = {
+  { "btrace", btrace_attributes, btrace_children, GDB_XML_EF_NONE,
+    check_xml_btrace_version, NULL },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+#endif /* defined (HAVE_LIBEXPAT) */
+
+/* Parse a branch trace xml document XML into DATA.  */
+
+static void
+parse_xml_btrace (struct btrace_data *btrace, const char *buffer)
+{
+#if defined (HAVE_LIBEXPAT)
+
+  int errcode;
+  btrace_data result;
+  result.format = BTRACE_FORMAT_NONE;
+
+  errcode = gdb_xml_parse_quick (_("btrace"), "btrace.dtd", btrace_elements,
+				 buffer, &result);
+  if (errcode != 0)
+    error (_("Error parsing branch trace."));
+
+  /* Keep parse results.  */
+  *btrace = std::move (result);
+
+#else  /* !defined (HAVE_LIBEXPAT) */
+
+  error (_("Cannot process branch trace.  XML support was disabled at "
+	   "compile time."));
+
+#endif  /* !defined (HAVE_LIBEXPAT) */
+}
+
+#if defined (HAVE_LIBEXPAT)
+
+/* Parse a btrace-conf "bts" xml record.  */
+
+static void
+parse_xml_btrace_conf_bts (struct gdb_xml_parser *parser,
+			  const struct gdb_xml_element *element,
+			  void *user_data,
+			  std::vector<gdb_xml_value> &attributes)
+{
+  struct btrace_config *conf;
+  struct gdb_xml_value *size;
+
+  conf = (struct btrace_config *) user_data;
+  conf->format = BTRACE_FORMAT_BTS;
+  conf->bts.size = 0;
+
+  size = xml_find_attribute (attributes, "size");
+  if (size != NULL)
+    conf->bts.size = (unsigned int) *(ULONGEST *) size->value.get ();
+}
+
+/* Parse a btrace-conf "pt" xml record.  */
+
+static void
+parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
+			  const struct gdb_xml_element *element,
+			  void *user_data,
+			  std::vector<gdb_xml_value> &attributes)
+{
+  struct btrace_config *conf;
+  struct gdb_xml_value *size;
+
+  conf = (struct btrace_config *) user_data;
+  conf->format = BTRACE_FORMAT_PT;
+  conf->pt.size = 0;
+
+  size = xml_find_attribute (attributes, "size");
+  if (size != NULL)
+    conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
+}
+
+static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
+  { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_attribute btrace_conf_bts_attributes[] = {
+  { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_conf_children[] = {
+  { "bts", btrace_conf_bts_attributes, NULL, GDB_XML_EF_OPTIONAL,
+    parse_xml_btrace_conf_bts, NULL },
+  { "pt", btrace_conf_pt_attributes, NULL, GDB_XML_EF_OPTIONAL,
+    parse_xml_btrace_conf_pt, NULL },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_attribute btrace_conf_attributes[] = {
+  { "version", GDB_XML_AF_NONE, NULL, NULL },
+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
+};
+
+static const struct gdb_xml_element btrace_conf_elements[] = {
+  { "btrace-conf", btrace_conf_attributes, btrace_conf_children,
+    GDB_XML_EF_NONE, NULL, NULL },
+  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
+};
+
+#endif /* defined (HAVE_LIBEXPAT) */
+
+/* Parse a branch trace configuration xml document XML into CONF.  */
+
+static void
+parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
+{
+#if defined (HAVE_LIBEXPAT)
+
+  int errcode;
+  errcode = gdb_xml_parse_quick (_("btrace-conf"), "btrace-conf.dtd",
+				 btrace_conf_elements, xml, conf);
+  if (errcode != 0)
+    error (_("Error parsing branch trace configuration."));
+
+#else  /* !defined (HAVE_LIBEXPAT) */
+
+  error (_("Cannot process the branch trace configuration.  XML support "
+	   "was disabled at compile time."));
+
+#endif  /* !defined (HAVE_LIBEXPAT) */
+}
+
 struct btrace_target_info
 {
   /* The ptid of the traced thread.  */
-- 
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

[PATCH 2/2] 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     | 57 ++++++++++++++++++++++++++++----------
 gdb/nat/linux-btrace.h     | 12 +++++++-
 gdb/remote.c               | 55 ++++++++++++++++++++++++++++--------
 gdbsupport/btrace-common.h |  5 +++-
 4 files changed, 101 insertions(+), 28 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index c5b3f1c93cf..3960d462e67 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -277,8 +277,9 @@ 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,
-		     const uint8_t *end, const uint8_t *start, size_t size)
+perf_event_read_bts (struct 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>;
   struct perf_event_sample sample;
@@ -447,6 +448,28 @@ 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)
+{
+  if (gtinfo == nullptr)
+    return nullptr;
+
+  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)
+{
+  if (gtinfo == nullptr)
+    return nullptr;
+
+  return gdb::checked_static_cast<const linux_btrace_target_info *> (gtinfo);
+}
+
 /* Enable branch tracing in BTS format.  */
 
 static struct btrace_target_info *
@@ -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<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 +634,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 +776,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 +799,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo)
     }
 
   if (errcode == BTRACE_ERR_NONE)
-    xfree (tinfo);
+    delete gtinfo;
 
   return errcode;
 }
@@ -785,7 +809,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo)
 
 static enum btrace_error
 linux_read_bts (struct btrace_data_bts *btrace,
-		struct btrace_target_info *tinfo,
+		struct linux_btrace_target_info *tinfo,
 		enum btrace_read_type type)
 {
   struct perf_event_buffer *pevent;
@@ -888,7 +912,7 @@ linux_fill_btrace_pt_config (struct btrace_data_pt_config *conf)
 
 static enum btrace_error
 linux_read_pt (struct btrace_data_pt *btrace,
-	       struct btrace_target_info *tinfo,
+	       struct linux_btrace_target_info *tinfo,
 	       enum btrace_read_type type)
 {
   struct perf_event_buffer *pt;
@@ -921,9 +945,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 +978,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..1dff977cb44 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,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
 {
   /* The ptid of this thread.  */
   ptid_t ptid;
@@ -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 ({})
+    {}
 };
 
 /* 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
 {
   /* The ptid of the traced thread.  */
   ptid_t ptid;
 
   /* The obtained branch trace configuration.  */
   struct btrace_config conf;
+
+  remote_btrace_target_info (ptid_t _ptid, const struct btrace_config &_conf)
+    : ptid (_ptid), conf (_conf)
+    {}
+
+  remote_btrace_target_info (ptid_t _ptid)
+    : ptid (_ptid), conf ({})
+    {}
 };
 
+/* 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;
+
+  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)
+{
+  if (gtinfo == nullptr)
+    return nullptr;
+
+  return gdb::checked_static_cast<const remote_btrace_target_info *> (gtinfo);
+}
+
 /* Reset our idea of our target's btrace configuration.  */
 
 static void
@@ -14502,7 +14532,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 +14594,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 +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;
   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 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)
 {
   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;
 }
 
 /* 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 () {}
+};
 
 /* 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 1/2] gdb, btrace: move xml parsing into remote.c

[Simon Marchi]

On 9/6/23 03:47, Markus Metzger via Gdb-patches wrote:
> The code is only used in remote.c and all functions can be declared static.

I think this is fine.

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

Simon

Re: [PATCH 2/2] gdb: c++ify btrace_target_info

[Simon Marchi]

> @@ -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<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) };
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<linux_btrace_target_info>;

> @@ -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> (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

RE: [PATCH 2/2] gdb: c++ify btrace_target_info

[Metzger, Markus T]

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.

Now, that cannot happen today, since both linux_disable_pt and
linux_disable_bts return BTRACE_ERR_NONE.

>> @@ -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.


>>  {
>>    /* 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.


>> +    : 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.

I saw it in GDB.  I also saw (ptid_).  And, now that I searched for it,
I also see _foo_, __foo, __foo__.  Dropping the _.


>>  /* 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?

The linux target leaves that to linux_disable_btrace().


I'll send a v2 patch with the changes you requested.

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

Re: [PATCH 2/2] gdb: c++ify btrace_target_info

[Simon Marchi]

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

RE: [PATCH 2/2] gdb: c++ify btrace_target_info

[Metzger, Markus T]

Hello Simon,

>>>>    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.

In the remote case, when the disable request fails with an error, I wanted
to allow GDB to try to disable again.


>> 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).

This code is used in GDB and in gdbserver and, historically, the gdbserver
target ops used error codes everywhere, whereas the GDB target ops didn't.

IMHO it is better to build in error handling right from the beginning, even if
the first implementation wouldn't really have any error condition.  It is much
harder to add it afterwards.  Note that those were C days and you had to
propagate unhandled errors upwards.

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