[RFA] Change target_write_memory_blocks to use std::vector

Message ID 20180225173703.6675-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Change target_write_memory_blocks to use std::vector
Related show

Commit Message

Tom Tromey Feb. 25, 2018, 5:37 p.m.
This changes target_write_memory_blocks to use std::vector, rather
than VEC.  This allows the removal of some cleanups.

Regression tested by the buildbot, though I am not certain that this
code path is tested there, so extra care should be taken during
review.

ChangeLog
2018-02-25  Tom Tromey  <tom@tromey.com>

	* target.h (memory_write_request_s): Remove typedef.  Don't define
	VEC.
	(target_write_memory_blocks): Change argument to std::vector.
	* target-memory.c (compare_block_starting_address): Return bool.
	Change argument types.
	(claim_memory): Change arguments to use std::vector.
	(split_regular_and_flash_blocks, blocks_to_erase)
	(compute_garbled_blocks): Likewise.
	(cleanup_request_data, cleanup_write_requests_vector): Remove.
	(target_write_memory_blocks): Change argument to std::vector.
	* symfile.c (struct load_section_data): Add constructor and
	destructor.  Use std::vector for "requests".
	(load_section_callback): Update.
	(clear_memory_write_data): Remove.
	(generic_load): Update.
---
 gdb/ChangeLog       |  18 ++++
 gdb/symfile.c       |  66 ++++++---------
 gdb/target-memory.c | 236 ++++++++++++++++++++--------------------------------
 gdb/target.h        |   9 +-
 4 files changed, 140 insertions(+), 189 deletions(-)

-- 
2.13.6

Comments

Simon Marchi Feb. 25, 2018, 10:26 p.m. | #1
On 2018-02-25 12:37 PM, Tom Tromey wrote:
> -  CORE_ADDR load_offset;

> -  struct load_progress_data *progress_data;

> -  VEC(memory_write_request_s) *requests;

> +  CORE_ADDR load_offset = 0;

> +  struct load_progress_data *progress_data = nullptr;

> +  std::vector<struct memory_write_request> requests;

> +

> +  load_section_data ()

> +  {

> +  }


Is this empty constructor needed?

Actually, I think it would be nice to give constructors to the data
structures when possible, to make it less likely to have them in
invalid states.

Here's an example, you can integrate it in your patch if you like it.

Simon


From 59f9a9d763dc4c0a879f1d36696efa2c4d423c86 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Sun, 25 Feb 2018 17:10:18 -0500
Subject: [PATCH] Add constructors to structures

---
 gdb/symfile.c       | 99 +++++++++++++++++++++++++----------------------------
 gdb/target-memory.c | 17 ++-------
 gdb/target.h        | 25 ++++++++------
 3 files changed, 63 insertions(+), 78 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 104d149584..cb1e44d605 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1892,46 +1892,56 @@ add_section_size_callback (bfd *abfd, asection *asec, void *data)
   *sum += bfd_get_section_size (asec);
 }

-/* Opaque data for load_section_callback.  */
-struct load_section_data {
-  CORE_ADDR load_offset = 0;
-  struct load_progress_data *progress_data = nullptr;
-  std::vector<struct memory_write_request> requests;
-
-  load_section_data ()
-  {
-  }
-
-  ~load_section_data ()
-  {
-    for (auto &&request : requests)
-      {
-	xfree (request.data);
-	xfree (request.baton);
-      }
-  }
-};
-
 /* Opaque data for load_progress.  */
-struct load_progress_data {
+struct load_progress_data
+{
   /* Cumulative data.  */
-  unsigned long write_count;
-  unsigned long data_count;
-  bfd_size_type total_size;
+  unsigned long write_count = 0;
+  unsigned long data_count = 0;
+  bfd_size_type total_size = 0;
 };

 /* Opaque data for load_progress for a single section.  */
-struct load_progress_section_data {
+struct load_progress_section_data
+{
+  load_progress_section_data (load_progress_data *cumulative_,
+			      const char *section_name_, ULONGEST section_size_,
+			      CORE_ADDR lma_, gdb_byte *buffer_)
+  : cumulative (cumulative_), section_name (section_name_),
+    section_size (section_size_), lma (lma_), buffer (buffer_)
+  {}
+
   struct load_progress_data *cumulative;

   /* Per-section data.  */
   const char *section_name;
-  ULONGEST section_sent;
+  ULONGEST section_sent = 0;
   ULONGEST section_size;
   CORE_ADDR lma;
   gdb_byte *buffer;
 };

+/* Opaque data for load_section_callback.  */
+struct load_section_data
+{
+  load_section_data (load_progress_data *progress_data_)
+  : progress_data (progress_data_)
+  {}
+
+  ~load_section_data ()
+  {
+    for (auto &&request : requests)
+      {
+	xfree (request.data);
+	delete ((load_progress_section_data *) request.baton);
+      }
+  }
+
+  CORE_ADDR load_offset = 0;
+  struct load_progress_data *progress_data;
+  std::vector<struct memory_write_request> requests;
+};
+
 /* Target write callback routine for progress reporting.  */

 static void
@@ -2001,11 +2011,8 @@ load_progress (ULONGEST bytes, void *untyped_arg)
 static void
 load_section_callback (bfd *abfd, asection *asec, void *data)
 {
-  struct memory_write_request new_request;
   struct load_section_data *args = (struct load_section_data *) data;
-  struct load_progress_section_data *section_data;
   bfd_size_type size = bfd_get_section_size (asec);
-  gdb_byte *buffer;
   const char *sect_name = bfd_get_section_name (abfd, asec);

   if ((bfd_get_section_flags (abfd, asec) & SEC_LOAD) == 0)
@@ -2014,25 +2021,16 @@ load_section_callback (bfd *abfd, asection *asec, void *data)
   if (size == 0)
     return;

-  memset (&new_request, 0, sizeof (struct memory_write_request));
-  section_data = XCNEW (struct load_progress_section_data);
-  new_request.begin = bfd_section_lma (abfd, asec) + args->load_offset;
-  new_request.end = new_request.begin + size; /* FIXME Should size
-						 be in instead?  */
-  new_request.data = (gdb_byte *) xmalloc (size);
-  new_request.baton = section_data;
-
-  buffer = new_request.data;
-
-  section_data->cumulative = args->progress_data;
-  section_data->section_name = sect_name;
-  section_data->section_size = size;
-  section_data->lma = new_request.begin;
-  section_data->buffer = buffer;
+  ULONGEST begin = bfd_section_lma (abfd, asec) + args->load_offset;
+  ULONGEST end = begin + size;
+  gdb_byte *buffer = (gdb_byte *) xmalloc (size);
+  bfd_get_section_contents (abfd, asec, buffer, 0, size);

-  args->requests.push_back (new_request);
+  load_progress_section_data *section_data
+    = new load_progress_section_data (args->progress_data, sect_name, size,
+				      begin, buffer);

-  bfd_get_section_contents (abfd, asec, buffer, 0, size);
+  args->requests.emplace_back (begin, end, buffer, section_data);
 }

 static void print_transfer_performance (struct ui_file *stream,
@@ -2043,15 +2041,10 @@ static void print_transfer_performance (struct ui_file *stream,
 void
 generic_load (const char *args, int from_tty)
 {
-  struct load_section_data cbdata;
   struct load_progress_data total_progress;
+  struct load_section_data cbdata (&total_progress);
   struct ui_out *uiout = current_uiout;

-  CORE_ADDR entry;
-
-  memset (&total_progress, 0, sizeof (total_progress));
-  cbdata.progress_data = &total_progress;
-
   if (args == NULL)
     error_no_arg (_("file to load"));

@@ -2100,7 +2093,7 @@ generic_load (const char *args, int from_tty)

   steady_clock::time_point end_time = steady_clock::now ();

-  entry = bfd_get_start_address (loadfile_bfd.get ());
+  CORE_ADDR entry = bfd_get_start_address (loadfile_bfd.get ());
   entry = gdbarch_addr_bits_remove (target_gdbarch (), entry);
   uiout->text ("Start address ");
   uiout->field_fmt ("address", "%s", paddress (target_gdbarch (), entry));
diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index a945983723..395bf0bae9 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -160,15 +160,7 @@ blocks_to_erase (const std::vector<memory_write_request> &written)
       if (!result.empty () && result.back ().end >= begin)
 	result.back ().end = end;
       else
-	{
-	  memory_write_request n;
-
-	  memset (&n, 0, sizeof (struct memory_write_request));
-	  n.begin = begin;
-	  n.end = end;
-
-	  result.push_back (n);
-	}
+	result.emplace_back (begin, end);
     }

   return result;
@@ -239,12 +231,7 @@ compute_garbled_blocks (const std::vector<memory_write_request> &erased_blocks,
 	     again for the remainder.  */
 	  if (written->begin > erased.begin)
 	    {
-	      struct memory_write_request n;
-
-	      memset (&n, 0, sizeof (struct memory_write_request));
-	      n.begin = erased.begin;
-	      n.end = written->begin;
-	      result.push_back (n);
+	      result.emplace_back (erased.begin, written->begin);
 	      erased.begin = written->begin;
 	      continue;
 	    }
diff --git a/gdb/target.h b/gdb/target.h
index 7636685f30..452855ae50 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1463,16 +1463,21 @@ void target_flash_done (void);

 /* Describes a request for a memory write operation.  */
 struct memory_write_request
-  {
-    /* Begining address that must be written.  */
-    ULONGEST begin;
-    /* Past-the-end address.  */
-    ULONGEST end;
-    /* The data to write.  */
-    gdb_byte *data;
-    /* A callback baton for progress reporting for this request.  */
-    void *baton;
-  };
+{
+  memory_write_request (ULONGEST begin_, ULONGEST end_,
+			gdb_byte *data_ = nullptr, void *baton_ = nullptr)
+  : begin (begin_), end (end_), data (data_), baton (baton_)
+  {}
+
+  /* Begining address that must be written.  */
+  ULONGEST begin;
+  /* Past-the-end address.  */
+  ULONGEST end;
+  /* The data to write.  */
+  gdb_byte *data;
+  /* A callback baton for progress reporting for this request.  */
+  void *baton;
+};

 /* Enumeration specifying different flash preservation behaviour.  */
 enum flash_preserve_mode
-- 
2.16.1
Tom Tromey Feb. 26, 2018, 7:37 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> +  load_section_data ()

>> +  {

>> +  }


Simon> Is this empty constructor needed?

I only added it for clarity.  Is there some standard approach to this?
Or a gdb standard?

Simon> Actually, I think it would be nice to give constructors to the data
Simon> structures when possible, to make it less likely to have them in
Simon> invalid states.

Simon> Here's an example, you can integrate it in your patch if you like it.

Yeah, this seems better to me.
I will pull it in.

Tom
Simon Marchi Feb. 26, 2018, 7:45 p.m. | #3
On 2018-02-26 14:37, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

>>> +  load_section_data ()

>>> +  {

>>> +  }

> 

> Simon> Is this empty constructor needed?

> 

> I only added it for clarity.  Is there some standard approach to this?

> Or a gdb standard?


Of all the C++ patches you did, I can't remember one where you added an 
empty constructor :).  I think the de-facto standard in C++ is to let 
the compiler generate a default constructor instead.  If we wanted to be 
explicit, we should rather do

   load_section_data () = default;

but I think it's just fine to not declare anything.

> Simon> Actually, I think it would be nice to give constructors to the 

> data

> Simon> structures when possible, to make it less likely to have them in

> Simon> invalid states.

> 

> Simon> Here's an example, you can integrate it in your patch if you 

> like it.

> 

> Yeah, this seems better to me.

> I will pull it in.


Ok, well at least it invalidates the point above :)

Simon

Patch

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 699d9e6fe0..104d149584 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1894,9 +1894,22 @@  add_section_size_callback (bfd *abfd, asection *asec, void *data)
 
 /* Opaque data for load_section_callback.  */
 struct load_section_data {
-  CORE_ADDR load_offset;
-  struct load_progress_data *progress_data;
-  VEC(memory_write_request_s) *requests;
+  CORE_ADDR load_offset = 0;
+  struct load_progress_data *progress_data = nullptr;
+  std::vector<struct memory_write_request> requests;
+
+  load_section_data ()
+  {
+  }
+
+  ~load_section_data ()
+  {
+    for (auto &&request : requests)
+      {
+	xfree (request.data);
+	xfree (request.baton);
+      }
+  }
 };
 
 /* Opaque data for load_progress.  */
@@ -1988,7 +2001,7 @@  load_progress (ULONGEST bytes, void *untyped_arg)
 static void
 load_section_callback (bfd *abfd, asection *asec, void *data)
 {
-  struct memory_write_request *new_request;
+  struct memory_write_request new_request;
   struct load_section_data *args = (struct load_section_data *) data;
   struct load_progress_section_data *section_data;
   bfd_size_type size = bfd_get_section_size (asec);
@@ -2001,44 +2014,25 @@  load_section_callback (bfd *abfd, asection *asec, void *data)
   if (size == 0)
     return;
 
-  new_request = VEC_safe_push (memory_write_request_s,
-			       args->requests, NULL);
-  memset (new_request, 0, sizeof (struct memory_write_request));
+  memset (&new_request, 0, sizeof (struct memory_write_request));
   section_data = XCNEW (struct load_progress_section_data);
-  new_request->begin = bfd_section_lma (abfd, asec) + args->load_offset;
-  new_request->end = new_request->begin + size; /* FIXME Should size
-						   be in instead?  */
-  new_request->data = (gdb_byte *) xmalloc (size);
-  new_request->baton = section_data;
+  new_request.begin = bfd_section_lma (abfd, asec) + args->load_offset;
+  new_request.end = new_request.begin + size; /* FIXME Should size
+						 be in instead?  */
+  new_request.data = (gdb_byte *) xmalloc (size);
+  new_request.baton = section_data;
 
-  buffer = new_request->data;
+  buffer = new_request.data;
 
   section_data->cumulative = args->progress_data;
   section_data->section_name = sect_name;
   section_data->section_size = size;
-  section_data->lma = new_request->begin;
+  section_data->lma = new_request.begin;
   section_data->buffer = buffer;
 
-  bfd_get_section_contents (abfd, asec, buffer, 0, size);
-}
-
-/* Clean up an entire memory request vector, including load
-   data and progress records.  */
-
-static void
-clear_memory_write_data (void *arg)
-{
-  VEC(memory_write_request_s) **vec_p = (VEC(memory_write_request_s) **) arg;
-  VEC(memory_write_request_s) *vec = *vec_p;
-  int i;
-  struct memory_write_request *mr;
+  args->requests.push_back (new_request);
 
-  for (i = 0; VEC_iterate (memory_write_request_s, vec, i, mr); ++i)
-    {
-      xfree (mr->data);
-      xfree (mr->baton);
-    }
-  VEC_free (memory_write_request_s, vec);
+  bfd_get_section_contents (abfd, asec, buffer, 0, size);
 }
 
 static void print_transfer_performance (struct ui_file *stream,
@@ -2049,19 +2043,15 @@  static void print_transfer_performance (struct ui_file *stream,
 void
 generic_load (const char *args, int from_tty)
 {
-  struct cleanup *old_cleanups;
   struct load_section_data cbdata;
   struct load_progress_data total_progress;
   struct ui_out *uiout = current_uiout;
 
   CORE_ADDR entry;
 
-  memset (&cbdata, 0, sizeof (cbdata));
   memset (&total_progress, 0, sizeof (total_progress));
   cbdata.progress_data = &total_progress;
 
-  old_cleanups = make_cleanup (clear_memory_write_data, &cbdata.requests);
-
   if (args == NULL)
     error_no_arg (_("file to load"));
 
@@ -2132,8 +2122,6 @@  generic_load (const char *args, int from_tty)
   print_transfer_performance (gdb_stdout, total_progress.data_count,
 			      total_progress.write_count,
 			      end_time - start_time);
-
-  do_cleanups (old_cleanups);
 }
 
 /* Report on STREAM the performance of a memory transfer operation,
diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index 81faa22a60..a945983723 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -26,20 +26,11 @@ 
 #include "gdb_sys_time.h"
 #include <algorithm>
 
-static int
-compare_block_starting_address (const void *a, const void *b)
+static bool
+compare_block_starting_address (const memory_write_request &a_req,
+				const memory_write_request &b_req)
 {
-  const struct memory_write_request *a_req
-    = (const struct memory_write_request *) a;
-  const struct memory_write_request *b_req
-    = (const struct memory_write_request *) b;
-
-  if (a_req->begin < b_req->begin)
-    return -1;
-  else if (a_req->begin == b_req->begin)
-    return 0;
-  else
-    return 1;
+  return a_req.begin < b_req.begin;
 }
 
 /* Adds to RESULT all memory write requests from BLOCK that are
@@ -49,17 +40,15 @@  compare_block_starting_address (const void *a, const void *b)
    that part of the memory request will be added.  */
 
 static void
-claim_memory (VEC(memory_write_request_s) *blocks,
-	      VEC(memory_write_request_s) **result,
+claim_memory (const std::vector<memory_write_request> &blocks,
+	      std::vector<memory_write_request> *result,
 	      ULONGEST begin,
 	      ULONGEST end)
 {
-  int i;
   ULONGEST claimed_begin;
   ULONGEST claimed_end;
-  struct memory_write_request *r;
 
-  for (i = 0; VEC_iterate (memory_write_request_s, blocks, i, r); ++i)
+  for (const memory_write_request &r : blocks)
     {
       /* If the request doesn't overlap [BEGIN, END), skip it.  We
 	 must handle END == 0 meaning the top of memory; we don't yet
@@ -67,28 +56,28 @@  claim_memory (VEC(memory_write_request_s) *blocks,
 	 memory, but there's an assertion in
 	 target_write_memory_blocks which checks for that.  */
 
-      if (begin >= r->end)
+      if (begin >= r.end)
 	continue;
-      if (end != 0 && end <= r->begin)
+      if (end != 0 && end <= r.begin)
 	continue;
 
-      claimed_begin = std::max (begin, r->begin);
+      claimed_begin = std::max (begin, r.begin);
       if (end == 0)
-	claimed_end = r->end;
+	claimed_end = r.end;
       else
-	claimed_end = std::min (end, r->end);
+	claimed_end = std::min (end, r.end);
 
-      if (claimed_begin == r->begin && claimed_end == r->end)
-	VEC_safe_push (memory_write_request_s, *result, r);
+      if (claimed_begin == r.begin && claimed_end == r.end)
+	result->push_back (r);
       else
 	{
-	  struct memory_write_request *n =
-	    VEC_safe_push (memory_write_request_s, *result, NULL);
+	  struct memory_write_request n = r;
+
+	  n.begin = claimed_begin;
+	  n.end = claimed_end;
+	  n.data += claimed_begin - r.begin;
 
-	  *n = *r;
-	  n->begin = claimed_begin;
-	  n->end = claimed_end;
-	  n->data += claimed_begin - r->begin;
+	  result->push_back (n);
 	}
     }
 }
@@ -98,9 +87,9 @@  claim_memory (VEC(memory_write_request_s) *blocks,
    regular memory to REGULAR_BLOCKS.  */
 
 static void
-split_regular_and_flash_blocks (VEC(memory_write_request_s) *blocks,
-				VEC(memory_write_request_s) **regular_blocks,
-				VEC(memory_write_request_s) **flash_blocks)
+split_regular_and_flash_blocks (const std::vector<memory_write_request> &blocks,
+				std::vector<memory_write_request> *regular_blocks,
+				std::vector<memory_write_request> *flash_blocks)
 {
   struct mem_region *region;
   CORE_ADDR cur_address;
@@ -116,7 +105,7 @@  split_regular_and_flash_blocks (VEC(memory_write_request_s) *blocks,
   cur_address = 0;
   while (1)
     {
-      VEC(memory_write_request_s) **r;
+      std::vector<memory_write_request> *r;
 
       region = lookup_mem_region (cur_address);
       r = region->attrib.mode == MEM_FLASH ? flash_blocks : regular_blocks;
@@ -156,34 +145,29 @@  block_boundaries (CORE_ADDR address, CORE_ADDR *begin, CORE_ADDR *end)
    returns write requests covering each group of flash blocks which must
    be erased.  */
 
-static VEC(memory_write_request_s) *
-blocks_to_erase (VEC(memory_write_request_s) *written)
+static std::vector<memory_write_request>
+blocks_to_erase (const std::vector<memory_write_request> &written)
 {
-  unsigned i;
-  struct memory_write_request *ptr;
-
-  VEC(memory_write_request_s) *result = NULL;
+  std::vector<memory_write_request> result;
 
-  for (i = 0; VEC_iterate (memory_write_request_s, written, i, ptr); ++i)
+  for (const memory_write_request &request : written)
     {
       CORE_ADDR begin, end;
 
-      block_boundaries (ptr->begin, &begin, 0);
-      block_boundaries (ptr->end - 1, 0, &end);
+      block_boundaries (request.begin, &begin, 0);
+      block_boundaries (request.end - 1, 0, &end);
 
-      if (!VEC_empty (memory_write_request_s, result)
-	  && VEC_last (memory_write_request_s, result)->end >= begin)
-	{
-	  VEC_last (memory_write_request_s, result)->end = end;
-	}
+      if (!result.empty () && result.back ().end >= begin)
+	result.back ().end = end;
       else
 	{
-	  struct memory_write_request *n =
-	    VEC_safe_push (memory_write_request_s, result, NULL);
+	  memory_write_request n;
 
-	  memset (n, 0, sizeof (struct memory_write_request));
-	  n->begin = begin;
-	  n->end = end;
+	  memset (&n, 0, sizeof (struct memory_write_request));
+	  n.begin = begin;
+	  n.end = end;
+
+	  result.push_back (n);
 	}
     }
 
@@ -196,14 +180,14 @@  blocks_to_erase (VEC(memory_write_request_s) *written)
    that will be erased but not rewritten (e.g. padding within a block
    which is only partially filled by "load").  */
 
-static VEC(memory_write_request_s) *
-compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
-			VEC(memory_write_request_s) *written_blocks)
+static std::vector<memory_write_request>
+compute_garbled_blocks (const std::vector<memory_write_request> &erased_blocks,
+			const std::vector<memory_write_request> &written_blocks)
 {
-  VEC(memory_write_request_s) *result = NULL;
+  std::vector<memory_write_request> result;
 
-  unsigned i, j;
-  unsigned je = VEC_length (memory_write_request_s, written_blocks);
+  unsigned j;
+  unsigned je = written_blocks.size ();
   struct memory_write_request *erased_p;
 
   /* Look at each erased memory_write_request in turn, and
@@ -213,19 +197,15 @@  compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
      the lists are sorted at this point it could be rewritten more
      efficiently, but the complexity is not generally worthwhile.  */
 
-  for (i = 0;
-       VEC_iterate (memory_write_request_s, erased_blocks, i, erased_p);
-       ++i)
+  for (const memory_write_request &erased_iter : erased_blocks)
     {
       /* Make a deep copy -- it will be modified inside the loop, but
 	 we don't want to modify original vector.  */
-      struct memory_write_request erased = *erased_p;
+      struct memory_write_request erased = erased_iter;
 
       for (j = 0; j != je;)
 	{
-	  struct memory_write_request *written
-	    = VEC_index (memory_write_request_s,
-			 written_blocks, j);
+	  const memory_write_request *written = &written_blocks[j];
 
 	  /* Now try various cases.  */
 
@@ -242,7 +222,7 @@  compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
 	     blocks.  */
 	  if (written->begin >= erased.end)
 	    {
-	      VEC_safe_push (memory_write_request_s, result, &erased);
+	      result.push_back (erased);
 	      goto next_erased;
 	    }
 
@@ -259,12 +239,12 @@  compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
 	     again for the remainder.  */
 	  if (written->begin > erased.begin)
 	    {
-	      struct memory_write_request *n =
-		VEC_safe_push (memory_write_request_s, result, NULL);
+	      struct memory_write_request n;
 
-	      memset (n, 0, sizeof (struct memory_write_request));
-	      n->begin = erased.begin;
-	      n->end = written->begin;
+	      memset (&n, 0, sizeof (struct memory_write_request));
+	      n.begin = erased.begin;
+	      n.end = written->begin;
+	      result.push_back (n);
 	      erased.begin = written->begin;
 	      continue;
 	    }
@@ -283,7 +263,7 @@  compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
 
       /* If we ran out of write requests without doing anything about
 	 ERASED, then that means it's really erased.  */
-      VEC_safe_push (memory_write_request_s, result, &erased);
+      result.push_back (erased);
 
     next_erased:
       ;
@@ -292,59 +272,30 @@  compute_garbled_blocks (VEC(memory_write_request_s) *erased_blocks,
   return result;
 }
 
-static void
-cleanup_request_data (void *p)
-{
-  VEC(memory_write_request_s) **v = (VEC(memory_write_request_s) **) p;
-  struct memory_write_request *r;
-  int i;
-
-  for (i = 0; VEC_iterate (memory_write_request_s, *v, i, r); ++i)
-    xfree (r->data);
-}
-
-static void
-cleanup_write_requests_vector (void *p)
-{
-  VEC(memory_write_request_s) **v = (VEC(memory_write_request_s) **) p;
-
-  VEC_free (memory_write_request_s, *v);
-}
-
 int
-target_write_memory_blocks (VEC(memory_write_request_s) *requests,
+target_write_memory_blocks (const std::vector<memory_write_request> &requests,
 			    enum flash_preserve_mode preserve_flash_p,
 			    void (*progress_cb) (ULONGEST, void *))
 {
-  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
-  VEC(memory_write_request_s) *blocks = VEC_copy (memory_write_request_s,
-						  requests);
-  unsigned i;
-  int err = 0;
+  std::vector<memory_write_request> blocks = requests;
   struct memory_write_request *r;
-  VEC(memory_write_request_s) *regular = NULL;
-  VEC(memory_write_request_s) *flash = NULL;
-  VEC(memory_write_request_s) *erased, *garbled;
+  std::vector<memory_write_request> regular;
+  std::vector<memory_write_request> flash;
+  std::vector<memory_write_request> erased, garbled;
 
   /* END == 0 would represent wraparound: a write to the very last
      byte of the address space.  This file was not written with that
      possibility in mind.  This is fixable, but a lot of work for a
      rare problem; so for now, fail noisily here instead of obscurely
      later.  */
-  for (i = 0; VEC_iterate (memory_write_request_s, requests, i, r); ++i)
-    gdb_assert (r->end != 0);
-
-  make_cleanup (cleanup_write_requests_vector, &blocks);
+  for (const memory_write_request &iter : requests)
+    gdb_assert (iter.end != 0);
 
   /* Sort the blocks by their start address.  */
-  qsort (VEC_address (memory_write_request_s, blocks),
-	 VEC_length (memory_write_request_s, blocks),
-	 sizeof (struct memory_write_request), compare_block_starting_address);
+  std::sort (blocks.begin (), blocks.end (), compare_block_starting_address);
 
   /* Split blocks into list of regular memory blocks,
      and list of flash memory blocks.  */
-  make_cleanup (cleanup_write_requests_vector, &regular);
-  make_cleanup (cleanup_write_requests_vector, &flash);
   split_regular_and_flash_blocks (blocks, &regular, &flash);
 
   /* If a variable is added to forbid flash write, even during "load",
@@ -354,37 +305,35 @@  target_write_memory_blocks (VEC(memory_write_request_s) *requests,
 
   /* Find flash blocks to erase.  */
   erased = blocks_to_erase (flash);
-  make_cleanup (cleanup_write_requests_vector, &erased);
 
   /* Find what flash regions will be erased, and not overwritten; then
      either preserve or discard the old contents.  */
   garbled = compute_garbled_blocks (erased, flash);
-  make_cleanup (cleanup_request_data, &garbled);
-  make_cleanup (cleanup_write_requests_vector, &garbled);
 
-  if (!VEC_empty (memory_write_request_s, garbled))
+  std::vector<gdb::unique_xmalloc_ptr<gdb_byte>> mem_holders;
+  if (!garbled.empty ())
     {
       if (preserve_flash_p == flash_preserve)
 	{
-	  struct memory_write_request *r;
-
 	  /* Read in regions that must be preserved and add them to
 	     the list of blocks we read.  */
-	  for (i = 0; VEC_iterate (memory_write_request_s, garbled, i, r); ++i)
+	  for (memory_write_request &iter : garbled)
 	    {
-	      gdb_assert (r->data == NULL);
-	      r->data = (gdb_byte *) xmalloc (r->end - r->begin);
-	      err = target_read_memory (r->begin, r->data, r->end - r->begin);
+	      gdb_assert (iter.data == NULL);
+	      gdb::unique_xmalloc_ptr<gdb_byte> holder
+		((gdb_byte *) xmalloc (iter.end - iter.begin));
+	      iter.data = holder.get ();
+	      mem_holders.push_back (std::move (holder));
+	      int err = target_read_memory (iter.begin, iter.data,
+					    iter.end - iter.begin);
 	      if (err != 0)
-		goto out;
+		return err;
 
-	      VEC_safe_push (memory_write_request_s, flash, r);
+	      flash.push_back (iter);
 	    }
 
-	  qsort (VEC_address (memory_write_request_s, flash),
-		 VEC_length (memory_write_request_s, flash),
-		 sizeof (struct memory_write_request),
-		 compare_block_starting_address);
+	  std::sort (flash.begin (), flash.end (),
+		     compare_block_starting_address);
 	}
     }
 
@@ -398,47 +347,44 @@  target_write_memory_blocks (VEC(memory_write_request_s) *requests,
      have the opportunity to batch flash requests.  */
 
   /* Write regular blocks.  */
-  for (i = 0; VEC_iterate (memory_write_request_s, regular, i, r); ++i)
+  for (const memory_write_request &iter : regular)
     {
       LONGEST len;
 
       len = target_write_with_progress (current_target.beneath,
 					TARGET_OBJECT_MEMORY, NULL,
-					r->data, r->begin, r->end - r->begin,
-					progress_cb, r->baton);
-      if (len < (LONGEST) (r->end - r->begin))
+					iter.data, iter.begin,
+					iter.end - iter.begin,
+					progress_cb, iter.baton);
+      if (len < (LONGEST) (iter.end - iter.begin))
 	{
 	  /* Call error?  */
-	  err = -1;
-	  goto out;
+	  return -1;
 	}
     }
 
-  if (!VEC_empty (memory_write_request_s, erased))
+  if (!erased.empty ())
     {
       /* Erase all pages.  */
-      for (i = 0; VEC_iterate (memory_write_request_s, erased, i, r); ++i)
-	target_flash_erase (r->begin, r->end - r->begin);
+      for (const memory_write_request &iter : erased)
+	target_flash_erase (iter.begin, iter.end - iter.begin);
 
       /* Write flash data.  */
-      for (i = 0; VEC_iterate (memory_write_request_s, flash, i, r); ++i)
+      for (const memory_write_request &iter : flash)
 	{
 	  LONGEST len;
 
 	  len = target_write_with_progress (&current_target,
 					    TARGET_OBJECT_FLASH, NULL,
-					    r->data, r->begin,
-					    r->end - r->begin,
-					    progress_cb, r->baton);
-	  if (len < (LONGEST) (r->end - r->begin))
+					    iter.data, iter.begin,
+					    iter.end - iter.begin,
+					    progress_cb, iter.baton);
+	  if (len < (LONGEST) (iter.end - iter.begin))
 	    error (_("Error writing data to flash"));
 	}
 
       target_flash_done ();
     }
 
- out:
-  do_cleanups (back_to);
-
-  return err;
+  return 0;
 }
diff --git a/gdb/target.h b/gdb/target.h
index 83cf48575f..7636685f30 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1473,8 +1473,6 @@  struct memory_write_request
     /* A callback baton for progress reporting for this request.  */
     void *baton;
   };
-typedef struct memory_write_request memory_write_request_s;
-DEF_VEC_O(memory_write_request_s);
 
 /* Enumeration specifying different flash preservation behaviour.  */
 enum flash_preserve_mode
@@ -1500,9 +1498,10 @@  enum flash_preserve_mode
      with a NULL baton, when preserved flash sectors are being rewritten.
 
    The function returns 0 on success, and error otherwise.  */
-int target_write_memory_blocks (VEC(memory_write_request_s) *requests,
-				enum flash_preserve_mode preserve_flash_p,
-				void (*progress_cb) (ULONGEST, void *));
+int target_write_memory_blocks
+    (const std::vector<memory_write_request> &requests,
+     enum flash_preserve_mode preserve_flash_p,
+     void (*progress_cb) (ULONGEST, void *));
 
 /* Print a line about the current target.  */