[RFA] Change target_write_memory_blocks to use std::vector

[RFA] Change target_write_memory_blocks to use std::vector

[Tom Tromey]

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

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

[PATCH] Add test for load command

[Simon Marchi]

[[Hi Tom,

  What do you think of adding this test to check this code path?]]

There doesn't seem to by any test for the load command.  I suggest to
add this test, so that we can have a minimum of confidence we don't
break it completely while refactoring the code that implements it.

gdb/testsuite/ChangeLog:

	* gdb.base/load-command.c: New file.
	* gdb.base/load-command.exp: New file.
	* lib/gdb.exp (gdb_is_target_remote_prompt): Rename to...
	(gdb_is_target_1): ...this, and generalize for other targets
	than just remote.
	(gdb_is_target_remote): Use gdb_is_target_1.
	(gdb_is_target_native): use gdb_is_target_1.
---
 gdb/testsuite/gdb.base/load-command.c   | 22 +++++++++++++++
 gdb/testsuite/gdb.base/load-command.exp | 49 +++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp               | 25 +++++++++++------
 3 files changed, 88 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/load-command.c
 create mode 100644 gdb/testsuite/gdb.base/load-command.exp

diff --git a/gdb/testsuite/gdb.base/load-command.c b/gdb/testsuite/gdb.base/load-command.c
new file mode 100644
index 0000000000..ee73e2be17
--- /dev/null
+++ b/gdb/testsuite/gdb.base/load-command.c
@@ -0,0 +1,22 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int the_variable = 0x1234;
+
+int main ()
+{
+}
diff --git a/gdb/testsuite/gdb.base/load-command.exp b/gdb/testsuite/gdb.base/load-command.exp
new file mode 100644
index 0000000000..61ca272f17
--- /dev/null
+++ b/gdb/testsuite/gdb.base/load-command.exp
@@ -0,0 +1,49 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the "load" command.
+
+standard_testfile
+
+# Disable generation of position independent executable (PIE).  Otherwise, we
+# would have to manually specify an offset to load.
+
+set opts {debug ldflags=-no-pie}
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
+    return -1
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return -1
+}
+
+# The native target does not support the load command.
+if [gdb_is_target_native] {
+    unsupported "the native target does not support the load command"
+    return
+}
+
+# Manually change the value of the_variable.
+gdb_test "print/x the_variable" " = 0x1234" "check initial value of the_variable"
+gdb_test_no_output "set the_variable = 0x5555" "manually change the_variable"
+gdb_test "print/x the_variable" " = 0x5555" "check manually changed value of the_variable"
+
+# Re-load the binary using the load command.
+gdb_test "load ${binfile}" ".*Loading section .data.*Transfer rate:.*" "re-load binary"
+
+# Re-loading the binary should have reset the variable value.
+gdb_test "print/x the_variable" " = 0x1234" "check initial value of the_variable"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e90c461403..3cd10dcafb 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3154,14 +3154,19 @@ proc skip_compile_feature_tests {} {
     return $result
 }
 
-# Helper for gdb_is_target_remote.  PROMPT_REGEXP is the expected
-# prompt.
+# Helper for gdb_is_target_* procs.  TARGET_NAME is the name of the target
+# we're looking for (used to build the test name).  TARGET_STACK_REGEXP
+# is a regexp that will match the output of "maint print target-stack" if
+# the target in question is currently pushed.
 
-proc gdb_is_target_remote_prompt { prompt_regexp } {
+proc gdb_is_target_1 { target_name target_stack_regexp } {
+    global gdb_prompt
+
+    set prompt_regexp "$gdb_prompt $"
 
-    set test "probe for target remote"
+    set test "probe for target ${target_name}"
     gdb_test_multiple "maint print target-stack" $test {
-	-re ".*emote serial target in gdb-specific protocol.*$prompt_regexp" {
+	-re "${target_stack_regexp}${prompt_regexp}" {
 	    pass $test
 	    return 1
 	}
@@ -3175,10 +3180,14 @@ proc gdb_is_target_remote_prompt { prompt_regexp } {
 # Check whether we're testing with the remote or extended-remote
 # targets.
 
-proc gdb_is_target_remote {} {
-    global gdb_prompt
+proc gdb_is_target_remote { } {
+    return [gdb_is_target_1 "remote" ".*emote serial target in gdb-specific protocol.*"]
+}
+
+# Check whether we're testing with the native target.
 
-    return [gdb_is_target_remote_prompt "$gdb_prompt $"]
+proc gdb_is_target_native { } {
+    return [gdb_is_target_1 "native" ".*native \\(Native process\\).*"]
 }
 
 # Return the effective value of use_gdb_stub.
-- 
2.16.1

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

[Simon Marchi]

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

Re: [PATCH] Add test for load command

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> [[Hi Tom,
Simon>   What do you think of adding this test to check this code path?]]

Simon> There doesn't seem to by any test for the load command.  I suggest to
Simon> add this test, so that we can have a minimum of confidence we don't
Simon> break it completely while refactoring the code that implements it.

This seems like a good idea to me.  I read the test and it seems
reasonable.

Thank you very much for doing this.

Tom

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

[Tom Tromey]

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

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

[Simon Marchi]

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

Re: [PATCH] Add test for load command

[Simon Marchi]

On 2018-02-26 14:15, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> [[Hi Tom,
> Simon>   What do you think of adding this test to check this code 
> path?]]
> 
> Simon> There doesn't seem to by any test for the load command.  I 
> suggest to
> Simon> add this test, so that we can have a minimum of confidence we 
> don't
> Simon> break it completely while refactoring the code that implements 
> it.
> 
> This seems like a good idea to me.  I read the test and it seems
> reasonable.
> 
> Thank you very much for doing this.
> 
> Tom

Thanks.  I did a buildbot run to make sure I did not break other tests 
with the gdb.exp changes.  It looked good so I pushed it.  The new test 
doesn't cover everything (e.g. writing to flash), but it's better than 
nothing.

Simon