[RFA 0/2] remove cleanups from btrace code

[RFA 0/2] remove cleanups from btrace code

[Tom Tromey]

This short series removes the remaining cleanups from the btrace code.
It seemed simpler to review as two patches, which is why I've split it up.

Tested by the buildbot.

Tom

[RFA 2/2] Remove last cleanup from btrace code

[Tom Tromey]

This removes the last cleanup from btrace.c, replacing it with a use
of unique_xmalloc_ptr.

2018-06-07  Tom Tromey  <tom@tromey.com>

	* btrace.c (parse_xml_raw): Use gdb::unique_xmalloc_ptr.
---
 gdb/ChangeLog |  4 ++++
 gdb/btrace.c  | 11 ++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 690770572e8..b8894a24ba0 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -2056,8 +2056,7 @@ static void
 parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
 	       gdb_byte **pdata, size_t *psize)
 {
-  struct cleanup *cleanup;
-  gdb_byte *data, *bin;
+  gdb_byte *bin;
   size_t len, size;
 
   len = strlen (body_text);
@@ -2066,8 +2065,8 @@ parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
 
   size = len / 2;
 
-  bin = data = (gdb_byte *) xmalloc (size);
-  cleanup = make_cleanup (xfree, data);
+  gdb::unique_xmalloc_ptr<gdb_byte> data ((gdb_byte *) xmalloc (size));
+  bin = data.get ();
 
   /* We use hex encoding - see common/rsp-low.h.  */
   while (len > 0)
@@ -2084,9 +2083,7 @@ parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
       len -= 2;
     }
 
-  discard_cleanups (cleanup);
-
-  *pdata = data;
+  *pdata = data.release ();
   *psize = size;
 }
 
-- 
2.13.6

[RFA 1/2] Remove cleanups from btrace code

[Tom Tromey]

This removes some cleanups from the btrace code by minorly C++-ifying
struct btrace_data.

gdb/ChangeLog
2018-06-07  Tom Tromey  <tom@tromey.com>

	* common/btrace-common.h (struct btrace_data): Add constructor,
	destructor, move assignment operator.
	<empty, clear, fini>: New methods.
	<format>: Initialize.
	(btrace_data_init, btrace_data_fini, btrace_data_clear)
	(btrace_data_empty): Don't declare.
	* common/btrace-common.c (btrace_data_init): Remove.
	(btrace_data::fini): Rename from btrace_data_fini.
	(btrace_data::empty): Rename from btrace_data_empty.
	(btrace_data::clear): Rename from btrace_data_clear.  Return
	bool.
	* btrace.h (make_cleanup_btrace_data): Don't declare.
	* btrace.c (btrace_add_pc, btrace_stitch_trace, btrace_clear)
	(parse_xml_btrace): Update.
	(do_btrace_data_cleanup, make_cleanup_btrace_data): Remove.
	(maint_btrace_clear_packet_history_cmd): Update.

gdb/gdbserver/ChangeLog
2018-06-07  Tom Tromey  <tom@tromey.com>

	* linux-low.c (linux_low_read_btrace): Update.
---
 gdb/ChangeLog              | 19 +++++++++++++++++++
 gdb/btrace.c               | 47 +++++++++-------------------------------------
 gdb/btrace.h               |  3 ---
 gdb/common/btrace-common.c | 34 +++++++++++++--------------------
 gdb/common/btrace-common.h | 45 +++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/ChangeLog    |  4 ++++
 gdb/gdbserver/linux-low.c  | 13 +++----------
 7 files changed, 80 insertions(+), 85 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 269ee51254e..690770572e8 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1565,25 +1565,19 @@ btrace_add_pc (struct thread_info *tp)
   struct btrace_data btrace;
   struct btrace_block *block;
   struct regcache *regcache;
-  struct cleanup *cleanup;
   CORE_ADDR pc;
 
   regcache = get_thread_regcache (tp->ptid);
   pc = regcache_read_pc (regcache);
 
-  btrace_data_init (&btrace);
   btrace.format = BTRACE_FORMAT_BTS;
   btrace.variant.bts.blocks = NULL;
 
-  cleanup = make_cleanup_btrace_data (&btrace);
-
   block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL);
   block->begin = pc;
   block->end = pc;
 
   btrace_compute_ftrace (tp, &btrace, NULL);
-
-  do_cleanups (cleanup);
 }
 
 /* See btrace.h.  */
@@ -1777,7 +1771,7 @@ static int
 btrace_stitch_trace (struct btrace_data *btrace, struct thread_info *tp)
 {
   /* If we don't have trace, there's nothing to do.  */
-  if (btrace_data_empty (btrace))
+  if (btrace->empty ())
     return 0;
 
   switch (btrace->format)
@@ -1894,7 +1888,6 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
   struct btrace_thread_info *btinfo;
   struct btrace_target_info *tinfo;
   struct btrace_data btrace;
-  struct cleanup *cleanup;
   int errcode;
 
   DEBUG ("fetch thread %s (%s)", print_thread_id (tp),
@@ -1920,9 +1913,6 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
   /* We should not be called on running or exited threads.  */
   gdb_assert (can_access_registers_ptid (tp->ptid));
 
-  btrace_data_init (&btrace);
-  cleanup = make_cleanup_btrace_data (&btrace);
-
   /* Let's first try to extend the trace we already have.  */
   if (!btinfo->functions.empty ())
     {
@@ -1938,7 +1928,7 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
 	  errcode = target_read_btrace (&btrace, tinfo, BTRACE_READ_NEW);
 
 	  /* If we got any new trace, discard what we have.  */
-	  if (errcode == 0 && !btrace_data_empty (&btrace))
+	  if (errcode == 0 && !btrace.empty ())
 	    btrace_clear (tp);
 	}
 
@@ -1957,7 +1947,7 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
     error (_("Failed to read branch trace."));
 
   /* Compute the trace, provided we have any.  */
-  if (!btrace_data_empty (&btrace))
+  if (!btrace.empty ())
     {
       /* Store the raw trace data.  The stored data will be cleared in
 	 btrace_clear, so we always append the new trace.  */
@@ -1967,8 +1957,6 @@ btrace_fetch (struct thread_info *tp, const struct btrace_cpu *cpu)
       btrace_clear_history (btinfo);
       btrace_compute_ftrace (tp, &btrace, cpu);
     }
-
-  do_cleanups (cleanup);
 }
 
 /* See btrace.h.  */
@@ -1992,7 +1980,7 @@ btrace_clear (struct thread_info *tp)
 
   /* Must clear the maint data before - it depends on BTINFO->DATA.  */
   btrace_maint_clear (btinfo);
-  btrace_data_clear (&btinfo->data);
+  btinfo->data.clear ();
   btrace_clear_history (btinfo);
 }
 
@@ -2217,21 +2205,20 @@ static const struct gdb_xml_element btrace_elements[] = {
 void
 parse_xml_btrace (struct btrace_data *btrace, const char *buffer)
 {
-  struct cleanup *cleanup;
   int errcode;
 
 #if defined (HAVE_LIBEXPAT)
 
-  btrace->format = BTRACE_FORMAT_NONE;
+  btrace_data result;
+  result.format = BTRACE_FORMAT_NONE;
 
-  cleanup = make_cleanup_btrace_data (btrace);
   errcode = gdb_xml_parse_quick (_("btrace"), "btrace.dtd", btrace_elements,
-				 buffer, btrace);
+				 buffer, &result);
   if (errcode != 0)
     error (_("Error parsing branch trace."));
 
   /* Keep parse results.  */
-  discard_cleanups (cleanup);
+  *btrace = std::move (result);
 
 #else  /* !defined (HAVE_LIBEXPAT) */
 
@@ -2844,22 +2831,6 @@ btrace_is_empty (struct thread_info *tp)
   return btrace_insn_cmp (&begin, &end) == 0;
 }
 
-/* Forward the cleanup request.  */
-
-static void
-do_btrace_data_cleanup (void *arg)
-{
-  btrace_data_fini ((struct btrace_data *) arg);
-}
-
-/* See btrace.h.  */
-
-struct cleanup *
-make_cleanup_btrace_data (struct btrace_data *data)
-{
-  return make_cleanup (do_btrace_data_cleanup, data);
-}
-
 #if defined (HAVE_LIBIPT)
 
 /* Print a single packet.  */
@@ -3381,7 +3352,7 @@ maint_btrace_clear_packet_history_cmd (const char *args, int from_tty)
 
   /* Must clear the maint data before - it depends on BTINFO->DATA.  */
   btrace_maint_clear (btinfo);
-  btrace_data_clear (&btinfo->data);
+  btinfo->data.clear ();
 }
 
 /* The "maintenance btrace clear" command.  */
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 4d57edb41f7..bfb0b9f2787 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -507,7 +507,4 @@ extern int btrace_is_replaying (struct thread_info *tp);
 /* Return non-zero if the branch trace for TP is empty; zero otherwise.  */
 extern int btrace_is_empty (struct thread_info *tp);
 
-/* Create a cleanup for DATA.  */
-extern struct cleanup *make_cleanup_btrace_data (struct btrace_data *data);
-
 #endif /* BTRACE_H */
diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-common.c
index 6cbbdc192ea..ec0ba6af136 100644
--- a/gdb/common/btrace-common.c
+++ b/gdb/common/btrace-common.c
@@ -64,28 +64,20 @@ btrace_format_short_string (enum btrace_format format)
 /* See btrace-common.h.  */
 
 void
-btrace_data_init (struct btrace_data *data)
+btrace_data::fini ()
 {
-  data->format = BTRACE_FORMAT_NONE;
-}
-
-/* See btrace-common.h.  */
-
-void
-btrace_data_fini (struct btrace_data *data)
-{
-  switch (data->format)
+  switch (format)
     {
     case BTRACE_FORMAT_NONE:
       /* Nothing to do.  */
       return;
 
     case BTRACE_FORMAT_BTS:
-      VEC_free (btrace_block_s, data->variant.bts.blocks);
+      VEC_free (btrace_block_s, variant.bts.blocks);
       return;
 
     case BTRACE_FORMAT_PT:
-      xfree (data->variant.pt.data);
+      xfree (variant.pt.data);
       return;
     }
 
@@ -94,19 +86,19 @@ btrace_data_fini (struct btrace_data *data)
 
 /* See btrace-common.h.  */
 
-int
-btrace_data_empty (struct btrace_data *data)
+bool
+btrace_data::empty () const
 {
-  switch (data->format)
+  switch (format)
     {
     case BTRACE_FORMAT_NONE:
-      return 1;
+      return true;
 
     case BTRACE_FORMAT_BTS:
-      return VEC_empty (btrace_block_s, data->variant.bts.blocks);
+      return VEC_empty (btrace_block_s, variant.bts.blocks);
 
     case BTRACE_FORMAT_PT:
-      return (data->variant.pt.size == 0);
+      return (variant.pt.size == 0);
     }
 
   internal_error (__FILE__, __LINE__, _("Unkown branch trace format."));
@@ -115,10 +107,10 @@ btrace_data_empty (struct btrace_data *data)
 /* See btrace-common.h.  */
 
 void
-btrace_data_clear (struct btrace_data *data)
+btrace_data::clear ()
 {
-  btrace_data_fini (data);
-  btrace_data_init (data);
+  fini ();
+  format = BTRACE_FORMAT_NONE;
 }
 
 /* See btrace-common.h.  */
diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
index 80e9fc65ba9..debf833f05d 100644
--- a/gdb/common/btrace-common.h
+++ b/gdb/common/btrace-common.h
@@ -164,7 +164,32 @@ struct btrace_data_pt
 /* The branch trace data.  */
 struct btrace_data
 {
-  enum btrace_format format;
+  btrace_data () = default;
+
+  ~btrace_data ()
+  {
+    fini ();
+  }
+
+  btrace_data &operator= (btrace_data &&other)
+  {
+    if (this != &other)
+      {
+	fini ();
+	format = other.format;
+	variant = other.variant;
+	other.format = BTRACE_FORMAT_NONE;
+      }
+    return *this;
+  }
+
+  /* Return true if this is empty; false otherwise.  */
+  bool empty () const;
+
+  /* Clear this object.  */
+  void clear ();
+
+  enum btrace_format format = BTRACE_FORMAT_NONE;
 
   union
   {
@@ -174,6 +199,12 @@ struct btrace_data
     /* Format == BTRACE_FORMAT_PT.  */
     struct btrace_data_pt pt;
   } variant;
+
+private:
+
+  DISABLE_COPY_AND_ASSIGN (btrace_data);
+
+  void fini ();
 };
 
 /* Target specific branch trace information.  */
@@ -217,18 +248,6 @@ extern const char *btrace_format_string (enum btrace_format format);
 /* Return an abbreviation string representation of FORMAT.  */
 extern const char *btrace_format_short_string (enum btrace_format format);
 
-/* Initialize DATA.  */
-extern void btrace_data_init (struct btrace_data *data);
-
-/* Cleanup DATA.  */
-extern void btrace_data_fini (struct btrace_data *data);
-
-/* Clear DATA.  */
-extern void btrace_data_clear (struct btrace_data *data);
-
-/* Return non-zero if DATA is empty; zero otherwise.  */
-extern int btrace_data_empty (struct btrace_data *data);
-
 /* Append the branch trace data from SRC to the end of DST.
    Both SRC and DST must use the same format.
    Returns zero on success; a negative number otherwise.  */
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index f8507b79684..1211944d79a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7250,8 +7250,6 @@ linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
   enum btrace_error err;
   int i;
 
-  btrace_data_init (&btrace);
-
   err = linux_read_btrace (&btrace, tinfo, type);
   if (err != BTRACE_ERR_NONE)
     {
@@ -7260,14 +7258,14 @@ linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
       else
 	buffer_grow_str0 (buffer, "E.Generic Error.");
 
-      goto err;
+      return -1;
     }
 
   switch (btrace.format)
     {
     case BTRACE_FORMAT_NONE:
       buffer_grow_str0 (buffer, "E.No Trace.");
-      goto err;
+      return -1;
 
     case BTRACE_FORMAT_BTS:
       buffer_grow_str (buffer, "<!DOCTYPE btrace SYSTEM \"btrace.dtd\">\n");
@@ -7298,15 +7296,10 @@ linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
 
     default:
       buffer_grow_str0 (buffer, "E.Unsupported Trace Format.");
-      goto err;
+      return -1;
     }
 
-  btrace_data_fini (&btrace);
   return 0;
-
-err:
-  btrace_data_fini (&btrace);
-  return -1;
 }
 
 /* See to_btrace_conf target method.  */
-- 
2.13.6

RE: [RFA 0/2] remove cleanups from btrace code

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Tom Tromey
> Sent: 08 June 2018 00:44
> To: gdb-patches@sourceware.org
> Subject: [RFA 0/2] remove cleanups from btrace code
> 
> This short series removes the remaining cleanups from the btrace code.
> It seemed simpler to review as two patches, which is why I've split it up.
> 
> Tested by the buildbot.
> 

LGTM.

Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928