Re: [RFA] Change target_write_memory_blocks to use std::vector

[Simon Marchi <simark@simark.ca>]

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