[RFA] Remove free_memory_read_result_vector

[Tom Tromey <tom at tromey dot com>]

This changes read_memory_robust to return a std::vector, allowing the
removal of free_memory_read_result_vector and associated cleanups.
This patch also changes the functions it touches to be a bit more
robust with regards to deallocation; it's perhaps possible that
read_memory_robust could have leaked in some situations.

This patch is based on my earlier series to remove some MI cleanups.
Regression tested by the buildbot.

ChangeLog
2017-09-23  Tom Tromey  <tom@tromey.com>

	* target.c (read_whatever_is_readable): Change type of "result".
	Update.
	(free_memory_read_result_vector): Remove.
	(read_memory_robust): Change return type.  Update.
	* mi/mi-main.c (mi_cmd_data_read_memory_bytes): Update.  Use
	bin2hex, std::string.
	* target.h (memory_read_result_s): Remove typedef.
	(free_memory_read_result_vector): Remove.
	(read_memory_robust): Return std::vector.
---
 gdb/ChangeLog    | 12 +++++++++
 gdb/mi/mi-main.c | 40 +++++++++---------------------
 gdb/target.c     | 75 ++++++++++++++++----------------------------------------
 gdb/target.h     | 40 ++++++++++++++++++++----------
 4 files changed, 71 insertions(+), 96 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 3362597..b1f2378 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1501,12 +1501,8 @@ mi_cmd_data_read_memory_bytes (const char *command, char **argv, int argc)
 {
   struct gdbarch *gdbarch = get_current_arch ();
   struct ui_out *uiout = current_uiout;
-  struct cleanup *cleanups;
   CORE_ADDR addr;
   LONGEST length;
-  memory_read_result_s *read_result;
-  int ix;
-  VEC(memory_read_result_s) *result;
   long offset = 0;
   int unit_size = gdbarch_addressable_memory_unit_size (gdbarch);
   int oind = 0;
@@ -1543,40 +1539,26 @@ mi_cmd_data_read_memory_bytes (const char *command, char **argv, int argc)
   addr = parse_and_eval_address (argv[0]) + offset;
   length = atol (argv[1]);
 
-  result = read_memory_robust (current_target.beneath, addr, length);
+  std::vector<memory_read_result> result
+    = read_memory_robust (current_target.beneath, addr, length);
 
-  cleanups = make_cleanup (free_memory_read_result_vector, &result);
-
-  if (VEC_length (memory_read_result_s, result) == 0)
+  if (result.size () == 0)
     error (_("Unable to read memory."));
 
   ui_out_emit_list list_emitter (uiout, "memory");
-  for (ix = 0;
-       VEC_iterate (memory_read_result_s, result, ix, read_result);
-       ++ix)
+  for (const memory_read_result &read_result : result)
     {
       ui_out_emit_tuple tuple_emitter (uiout, NULL);
-      char *data, *p;
-      int i;
-      int alloc_len;
-
-      uiout->field_core_addr ("begin", gdbarch, read_result->begin);
-      uiout->field_core_addr ("offset", gdbarch, read_result->begin - addr);
-      uiout->field_core_addr ("end", gdbarch, read_result->end);
 
-      alloc_len = (read_result->end - read_result->begin) * 2 * unit_size + 1;
-      data = (char *) xmalloc (alloc_len);
+      uiout->field_core_addr ("begin", gdbarch, read_result.begin);
+      uiout->field_core_addr ("offset", gdbarch, read_result.begin - addr);
+      uiout->field_core_addr ("end", gdbarch, read_result.end);
 
-      for (i = 0, p = data;
-	   i < ((read_result->end - read_result->begin) * unit_size);
-	   ++i, p += 2)
-	{
-	  sprintf (p, "%02x", read_result->data[i]);
-	}
-      uiout->field_string ("contents", data);
-      xfree (data);
+      std::string data = bin2hex (read_result.data.get (),
+				  (read_result.end - read_result.begin)
+				  * unit_size);
+      uiout->field_string ("contents", data.c_str ());
     }
-  do_cleanups (cleanups);
 }
 
 /* Implementation of the -data-write_memory command.
diff --git a/gdb/target.c b/gdb/target.c
index b1e6011..17fd0f1 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1630,43 +1630,37 @@ static void
 read_whatever_is_readable (struct target_ops *ops,
 			   const ULONGEST begin, const ULONGEST end,
 			   int unit_size,
-			   VEC(memory_read_result_s) **result)
+			   std::vector<memory_read_result> *result)
 {
-  gdb_byte *buf = (gdb_byte *) xmalloc (end - begin);
   ULONGEST current_begin = begin;
   ULONGEST current_end = end;
   int forward;
-  memory_read_result_s r;
   ULONGEST xfered_len;
 
   /* If we previously failed to read 1 byte, nothing can be done here.  */
   if (end - begin <= 1)
-    {
-      xfree (buf);
-      return;
-    }
+    return;
+
+  gdb::unique_xmalloc_ptr<gdb_byte> buf ((gdb_byte *) xmalloc (end - begin));
 
   /* Check that either first or the last byte is readable, and give up
      if not.  This heuristic is meant to permit reading accessible memory
      at the boundary of accessible region.  */
   if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
-			   buf, begin, 1, &xfered_len) == TARGET_XFER_OK)
+			   buf.get (), begin, 1, &xfered_len) == TARGET_XFER_OK)
     {
       forward = 1;
       ++current_begin;
     }
   else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
-				buf + (end - begin) - 1, end - 1, 1,
+				buf.get () + (end - begin) - 1, end - 1, 1,
 				&xfered_len) == TARGET_XFER_OK)
     {
       forward = 0;
       --current_end;
     }
   else
-    {
-      xfree (buf);
-      return;
-    }
+    return;
 
   /* Loop invariant is that the [current_begin, current_end) was previously
      found to be not readable as a whole.
@@ -1696,7 +1690,7 @@ read_whatever_is_readable (struct target_ops *ops,
 	}
 
       xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-			  buf + (first_half_begin - begin) * unit_size,
+			  buf.get () + (first_half_begin - begin) * unit_size,
 			  first_half_begin,
 			  first_half_end - first_half_begin);
 
@@ -1723,47 +1717,27 @@ read_whatever_is_readable (struct target_ops *ops,
   if (forward)
     {
       /* The [begin, current_begin) range has been read.  */
-      r.begin = begin;
-      r.end = current_begin;
-      r.data = buf;
+      result->emplace_back (begin, current_end, std::move (buf));
     }
   else
     {
       /* The [current_end, end) range has been read.  */
       LONGEST region_len = end - current_end;
 
-      r.data = (gdb_byte *) xmalloc (region_len * unit_size);
-      memcpy (r.data, buf + (current_end - begin) * unit_size,
+      gdb::unique_xmalloc_ptr<gdb_byte> data
+	((gdb_byte *) xmalloc (region_len * unit_size));
+      memcpy (data.get (), buf.get () + (current_end - begin) * unit_size,
 	      region_len * unit_size);
-      r.begin = current_end;
-      r.end = end;
-      xfree (buf);
+      result->emplace_back (current_end, end, std::move (data));
     }
-  VEC_safe_push(memory_read_result_s, (*result), &r);
 }
 
-void
-free_memory_read_result_vector (void *x)
-{
-  VEC(memory_read_result_s) **v = (VEC(memory_read_result_s) **) x;
-  memory_read_result_s *current;
-  int ix;
-
-  for (ix = 0; VEC_iterate (memory_read_result_s, *v, ix, current); ++ix)
-    {
-      xfree (current->data);
-    }
-  VEC_free (memory_read_result_s, *v);
-}
-
-VEC(memory_read_result_s) *
+std::vector<memory_read_result>
 read_memory_robust (struct target_ops *ops,
 		    const ULONGEST offset, const LONGEST len)
 {
-  VEC(memory_read_result_s) *result = 0;
+  std::vector<memory_read_result> result;
   int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ());
-  struct cleanup *cleanup = make_cleanup (free_memory_read_result_vector,
-					  &result);
 
   LONGEST xfered_total = 0;
   while (xfered_total < len)
@@ -1789,19 +1763,17 @@ read_memory_robust (struct target_ops *ops,
       else
 	{
 	  LONGEST to_read = std::min (len - xfered_total, region_len);
-	  gdb_byte *buffer = (gdb_byte *) xmalloc (to_read * unit_size);
-	  struct cleanup *inner_cleanup = make_cleanup (xfree, buffer);
+	  gdb::unique_xmalloc_ptr<gdb_byte> buffer
+	    ((gdb_byte *) xmalloc (to_read * unit_size));
 
 	  LONGEST xfered_partial =
-	      target_read (ops, TARGET_OBJECT_MEMORY, NULL,
-			   (gdb_byte *) buffer,
+	      target_read (ops, TARGET_OBJECT_MEMORY, NULL, buffer.get (),
 			   offset + xfered_total, to_read);
 	  /* Call an observer, notifying them of the xfer progress?  */
 	  if (xfered_partial <= 0)
 	    {
 	      /* Got an error reading full chunk.  See if maybe we can read
 		 some subrange.  */
-	      do_cleanups (inner_cleanup);
 	      read_whatever_is_readable (ops, offset + xfered_total,
 					 offset + xfered_total + to_read,
 					 unit_size, &result);
@@ -1809,20 +1781,15 @@ read_memory_robust (struct target_ops *ops,
 	    }
 	  else
 	    {
-	      struct memory_read_result r;
-
-	      discard_cleanups (inner_cleanup);
-	      r.data = buffer;
-	      r.begin = offset + xfered_total;
-	      r.end = r.begin + xfered_partial;
-	      VEC_safe_push (memory_read_result_s, result, &r);
+	      result.emplace_back (offset + xfered_total,
+				   offset + xfered_total + xfered_partial,
+				   std::move (buffer));
 	      xfered_total += xfered_partial;
 	    }
 	  QUIT;
 	}
     }
 
-  discard_cleanups (cleanup);
   return result;
 }
 
diff --git a/gdb/target.h b/gdb/target.h
index 87482dc..e089527 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -287,22 +287,36 @@ extern LONGEST target_read (struct target_ops *ops,
 			    ULONGEST offset, LONGEST len);
 
 struct memory_read_result
+{
+  memory_read_result (ULONGEST begin_, ULONGEST end_,
+		      gdb::unique_xmalloc_ptr<gdb_byte> &&data_)
+    : begin (begin_),
+      end (end_),
+      data (std::move (data_))
   {
-    /* First address that was read.  */
-    ULONGEST begin;
-    /* Past-the-end address.  */
-    ULONGEST end;
-    /* The data.  */
-    gdb_byte *data;
-};
-typedef struct memory_read_result memory_read_result_s;
-DEF_VEC_O(memory_read_result_s);
+  }
 
-extern void free_memory_read_result_vector (void *);
+  ~memory_read_result () = default;
+
+  memory_read_result (memory_read_result &&other)
+    : begin (other.begin),
+      end (other.end),
+      data (std::move (other.data))
+  {
+  }
+
+  DISABLE_COPY_AND_ASSIGN (memory_read_result);
+
+  /* First address that was read.  */
+  ULONGEST begin;
+  /* Past-the-end address.  */
+  ULONGEST end;
+  /* The data.  */
+  gdb::unique_xmalloc_ptr<gdb_byte> data;
+};
 
-extern VEC(memory_read_result_s)* read_memory_robust (struct target_ops *ops,
-						      const ULONGEST offset,
-						      const LONGEST len);
+extern std::vector<memory_read_result> read_memory_robust
+    (struct target_ops *ops, const ULONGEST offset, const LONGEST len);
 
 /* Request that OPS transfer up to LEN addressable units from BUF to the
    target's OBJECT.  When writing to a memory object, the addressable unit
-- 
2.9.5