Better MI memory commands

[Vladimir Prus <vladimir at codesourcery dot com>]

The attached patch makes a few changes in MI memory commands, with the
goal of making it easy for frontend to just display memory view, even
around the boundaries of accessible memory. The changes are as follows:

- The code that reads memory is modified read around inaccessible memory
regions as defined by memory map, as opposed to giving error if the first
address to read happens to be inaccessible.
- If we think the memory should be readable, but get an error, we now use
binary search to find subrange at the beginning that is still readable.
With this patch, we do the same for the end. So, if the start address is not
readable but end address is, the readable tail will be returned.
- The current output of -data-read-memory is totally insane. This patch
introduces -data-read-memory-raw that clearly reports what regions were
read, and shows the data as hex string. I've also added -data-write-memory-raw
which, contrary to -data-write-memory can actually write *memory*, as opposed to
a single byte.

I've tested this against a certain bare-metal board, first with memory map and 
second with manually cleared memory map in GDB. In both cases, reads before 
and after accessible memory worked reasonably. 

OK?

Thanks,

-- 
Vladimir Prus
CodeSourcery
vladimir@codesourcery.com
(650) 331-3385 x722

commit 411034a96240a40e3293f05c5e223944177ac7b3
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Thu Jun 17 20:23:17 2010 +0400

    Easier and more stubborn MI memory read commands.
    
    	gdb/
    	* mi/mi-cmds.c (mi_cmds): Register data-read-memory-raw
    	and data-write-memory-raw.
    	* mi/mi-cmds.h (mi_cmd_data_read_memory_raw)
    	(mi_cmd_data_write_memory_raw): New.
    	* mi/mi-main.c (mi_cmd_data_read_memory): Use regular target_read.
    	(mi_cmd_data_read_memory_raw, mi_cmd_data_write_memory_raw): New.
    	* target.c (target_read_until_error): Remove.
    	(read_whatever_is_readable, free_memory_read_result_vector)
    	(read_memory_robust): New.
    	* target.h (target_read_until_error): Remove.
    	(struct memory_read_result, free_memory_read_result_vector)
    	(read_memory_robust): New.
    
    	gdb/doc
    	* gdb.texinfo (GDB/MI Data Manipulation): Document
    	-data-read-memory-raw and -data-write-memory-raw.

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index cbd636f..e1da81d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -26895,6 +26895,8 @@ don't appear in the actual output):
 @subheading The @code{-data-read-memory} Command
 @findex -data-read-memory
 
+This command is deprecated, use @code{-data-read-memory-raw} instead.
+
 @subsubheading Synopsis
 
 @smallexample
@@ -27006,6 +27008,120 @@ next-page="0x000013c0",prev-page="0x00001380",memory=[
 (gdb)
 @end smallexample
 
+@subheading The @code{-data-read-memory-raw} Command
+@findex -data-read-memory-raw
+
+@subsubheading Synopsis
+
+@smallexample
+ -data-read-memory-raw [ -o @var{byte-offset} ]
+   @var{address} @var{count}
+@end smallexample
+
+@noindent
+where:
+
+@table @samp
+@item @var{address}
+An expression specifying the address of the first memory word to be
+read.  Complex expressions containing embedded white space should be
+quoted using the C convention.
+
+@item @var{count}
+The number of bytes to read. This should be an integer literal.
+
+@item @var{byte-offset}
+The offsets in bytes relative to @var{address} at which to start
+reading.  This should be an integer literal.  This option is provided
+so that a frontend is not required to first evaluate address and then
+perform address arithmetics itself.
+
+@end table
+
+The commands attempts to read memory at the specified address.  When a
+memory map is available, regions marked as unreadable in the memory
+map will be skipped. In addition, when @value{GDBN} encouters an error
+reading a memory range, it will attempt to find readable subrange at
+the beginning and range. Essentially, this command will make
+reasonable effort to return all readable memory content within the
+requested range.
+
+The output of the command has a result record named @samp{memory},
+whose content is a list of tuples.  Each tuple represent a
+successfully read memory block and has the following
+fields:
+
+@table @code
+@item begin
+The start address of the memory block, as hexadecimal literal.
+
+@item end
+The end address of the memory block, as hexadecimal literal.
+
+@item offset
+The offset of the memory block, as hexadecimal literal, relative to
+the start address passed to @code{-data-read-memory-raw}.
+
+@item contents
+The contents of the memory block, as hex-encoded string of bytes.
+
+@end table
+
+
+@subsubheading @value{GDBN} Command
+
+The corresponding @value{GDBN} command is @samp{x}.
+
+@subsubheading Example
+
+@smallexample
+(gdb)
+-data-read-memory-raw &a 10
+^done,memory=[@{begin="0xbffff154",offset="0x00000000",
+              end="0xbffff15e",
+              contents="01000000020000000300"@}]
+(gdb)
+@end smallexample
+
+
+@subheading The @code{-data-write-memory-raw} Command
+@findex -data-write-memory-raw
+
+@subsubheading Synopsis
+
+@smallexample
+ -data-write-memory-raw @var{address} @var{contents}
+@end smallexample
+
+@noindent
+where:
+
+@table @samp
+@item @var{address}
+An expression specifying the address of the first memory word to be
+read.  Complex expressions containing embedded white space should be
+quoted using the C convention.
+
+@item @var{contents}
+The hex-encoded bytes to write.  The size of this parameter determines
+how many bytes should be written.
+
+@end table
+
+@subsubheading @value{GDBN} Command
+
+There's no corresponding @value{GDBN} command.
+
+@subsubheading Example
+
+@smallexample
+(gdb)
+-data-write-memory-raw &a "aabbccdd"
+^done
+(gdb)
+@end smallexample
+
+
 @c %%%%%%%%%%%%%%%%%%%%%%%%%%%% SECTION %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 @node GDB/MI Tracepoint Commands
 @section @sc{gdb/mi} Tracepoint Commands
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 8441e17..65fe0ae 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -51,7 +51,9 @@ struct mi_cmd mi_cmds[] =
   { "data-list-register-names", { NULL, 0 }, mi_cmd_data_list_register_names},
   { "data-list-register-values", { NULL, 0 }, mi_cmd_data_list_register_values},
   { "data-read-memory", { NULL, 0 }, mi_cmd_data_read_memory},
+  { "data-read-memory-raw", { NULL, 0 }, mi_cmd_data_read_memory_raw},
   { "data-write-memory", { NULL, 0 }, mi_cmd_data_write_memory},
+  { "data-write-memory-raw", {NULL, 0}, mi_cmd_data_write_memory_raw},
   { "data-write-register-values", { NULL, 0 }, mi_cmd_data_write_register_values},
   { "enable-timings", { NULL, 0 }, mi_cmd_enable_timings},
   { "enable-pretty-printing", { NULL, 0 }, mi_cmd_enable_pretty_printing},
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 5954aef..90a59c7 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -47,7 +47,9 @@ extern mi_cmd_argv_ftype mi_cmd_data_list_register_names;
 extern mi_cmd_argv_ftype mi_cmd_data_list_register_values;
 extern mi_cmd_argv_ftype mi_cmd_data_list_changed_registers;
 extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
+extern mi_cmd_argv_ftype mi_cmd_data_read_memory_raw;
 extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
+extern mi_cmd_argv_ftype mi_cmd_data_write_memory_raw;
 extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
 extern mi_cmd_argv_ftype mi_cmd_enable_timings;
 extern mi_cmd_argv_ftype mi_cmd_env_cd;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 85a3f99..a99a91e 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1352,9 +1352,9 @@ mi_cmd_data_read_memory (char *command, char **argv, int argc)
 
   /* Dispatch memory reads to the topmost target, not the flattened
      current_target.  */
-  nr_bytes = target_read_until_error (current_target.beneath,
-				      TARGET_OBJECT_MEMORY, NULL, mbuf,
-				      addr, total_bytes);
+  nr_bytes = target_read (current_target.beneath,
+			  TARGET_OBJECT_MEMORY, NULL, mbuf,
+			  addr, total_bytes);
   if (nr_bytes <= 0)
     error ("Unable to read memory.");
 
@@ -1437,6 +1437,88 @@ mi_cmd_data_read_memory (char *command, char **argv, int argc)
   do_cleanups (cleanups);
 }
 
+void
+mi_cmd_data_read_memory_raw (char *command, char **argv, int argc)
+{
+  struct gdbarch *gdbarch = get_current_arch ();
+  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 optind = 0;
+  char *optarg;
+  enum opt
+    {
+      OFFSET_OPT
+    };
+  static struct mi_opt opts[] =
+  {
+    {"o", OFFSET_OPT, 1},
+    { 0, 0, 0 }
+  };
+
+  while (1)
+    {
+      int opt = mi_getopt ("mi_cmd_data_read_memory_raw", argc, argv, opts,
+			   &optind, &optarg);
+      if (opt < 0)
+	break;
+      switch ((enum opt) opt)
+	{
+	case OFFSET_OPT:
+	  offset = atol (optarg);
+	  break;
+	}
+    }
+  argv += optind;
+  argc -= optind;
+
+  if (argc != 2)
+    error ("Usage: ADDR LENGTH.");
+
+  addr = parse_and_eval_address (argv[0]) + offset;
+  length = atol (argv[1]);
+
+  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)
+    error ("Unable to read memory.");
+
+  make_cleanup_ui_out_list_begin_end (uiout, "memory");
+  for (ix = 0;
+       VEC_iterate (memory_read_result_s, result, ix, read_result);
+       ++ix)
+    {
+      struct cleanup *t = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+      char *data, *p;
+      int i;
+
+      ui_out_field_core_addr (uiout, "begin", gdbarch, read_result->begin);
+      ui_out_field_core_addr (uiout, "offset", gdbarch, read_result->begin
+			      - addr);
+      ui_out_field_core_addr (uiout, "end", gdbarch, read_result->end);
+
+      data = xmalloc ((read_result->end - read_result->begin) * 2 + 1);
+
+      for (i = 0, p = data;
+	   i < (read_result->end - read_result->begin);
+	   ++i, p += 2)
+	{
+	  sprintf (p, "%02x", read_result->data[i]);
+	}
+      ui_out_field_string (uiout, "contents", data);
+      xfree (data);
+      do_cleanups (t);
+    }
+  do_cleanups (cleanups);
+}
+
+
 /* DATA-MEMORY-WRITE:
 
    COLUMN_OFFSET: optional argument. Must be preceeded by '-o'. The
@@ -1523,6 +1605,44 @@ mi_cmd_data_write_memory (char *command, char **argv, int argc)
   do_cleanups (old_chain);
 }
 
+/* DATA-MEMORY-WRITE-RAW:
+
+   ADDR: start address
+   DATA: string of bytes to write at that address. */
+void
+mi_cmd_data_write_memory_raw (char *command, char **argv, int argc)
+{
+  CORE_ADDR addr;
+  char *cdata;
+  gdb_byte *data;
+  int len, r, i;
+  struct cleanup *back_to;
+
+  if (argc != 2)
+    error ("Usage: ADDR DATA.");
+
+  addr = parse_and_eval_address (argv[0]);
+  cdata = argv[1];
+  len = strlen (cdata)/2;
+
+  data = xmalloc (len);
+  back_to = make_cleanup (xfree, data);
+
+  for (i = 0; i < len; ++i)
+    {
+      int x;
+      sscanf (cdata + i * 2, "%02x", &x);
+      data[i] = (gdb_byte)x;
+    }
+
+  r = target_write_memory (addr, data, len);
+  if (r != 0)
+    error (_("Could not write memory"));
+
+  do_cleanups (back_to);
+}
+
+
 void
 mi_cmd_enable_timings (char *command, char **argv, int argc)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 7aa77e6..f48ecdc 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1751,73 +1751,203 @@ target_read (struct target_ops *ops,
   return len;
 }
 
-LONGEST
-target_read_until_error (struct target_ops *ops,
-			 enum target_object object,
-			 const char *annex, gdb_byte *buf,
-			 ULONGEST offset, LONGEST len)
+/** Assuming that the entire [begin, end) range of memory cannot be read,
+    try to read whatever subrange is possible to read.
+
+    The function results, in RESULT, either zero or one memory block.
+    If there's a readable subrange at the beginning, it is completely
+    read and returned. Any further readable subrange will not be read.
+    Otherwise, if there's a readable subrange at the end, it will be
+    completely and returned.  Any readable subranges before it (obviously,
+    not starting at the beginning), will be ignored. In other cases --
+    either no readable subrange, or readable subrange (s) that is neither
+    at the beginning, or end, nothing is returned.
+
+    The purpose of this function is to handle a read across a boundary of
+    accessible memory in a case when memory map is not available. The above
+    restrictions are fine for this case, but will give incorrect results if
+    the memory is 'patchy'. However, supporting 'patchy' memory would require
+    trying to read every single byte, and it seems unacceptable solution.
+    Explicit memory map is recommended for this case -- and
+    target_read_memory_robust will take care of reading multiple ranges then.  */
+
+static void
+read_whatever_is_readable (struct target_ops *ops, ULONGEST begin, ULONGEST end,
+			   VEC(memory_read_result_s) **result)
 {
-  LONGEST xfered = 0;
+  gdb_byte *buf = xmalloc (end-begin);
+  ULONGEST current_begin = begin;
+  ULONGEST current_end = end;
+  int forward;
+  memory_read_result_s r;
+
+  /* If we previously failed to read 1 byte, nothing can be done here.  */
+  if (end - begin <= 1)
+    return;
 
+  /* 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) == 1)
+    {
+      forward = 1;
+      ++current_begin;
+    }
+  else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
+				buf + (end-begin) - 1, end - 1, 1) == 1)
+    {
+      forward = 0;
+      --current_end;
+    }
+  else
+    {
+      return;
+    }
+
+  /* Loop invariant is that the [current_begin, current_end) was previously
+     found to be not readable as a whole.
+
+     Note loop condition -- if the range has 1 byte, we can't divide the range
+     so there's no point trying further.  */
+  while (current_end - current_begin > 1)
+    {
+      ULONGEST first_half_begin, first_half_end;
+      ULONGEST second_half_begin, second_half_end;
+      LONGEST xfer;
+
+      ULONGEST middle = current_begin + (current_end - current_begin)/2;
+      if (forward)
+	{
+	  first_half_begin = current_begin;
+	  first_half_end = middle;
+	  second_half_begin = middle;
+	  second_half_end = current_end;
+	}
+      else
+	{
+	  first_half_begin = middle;
+	  first_half_end = current_end;
+	  second_half_begin = current_begin;
+	  second_half_end = middle;
+	}
+
+      xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
+			  buf + (first_half_begin - begin),
+			  first_half_begin,
+			  first_half_end - first_half_begin);
+
+      if (xfer == first_half_end - first_half_begin)
+	{
+	  /* This half reads up fine. So, the error must be in the other half.  */
+	  current_begin = second_half_begin;
+	  current_end = second_half_end;
+	}
+      else
+	{
+	  current_begin = first_half_begin;
+	  current_end = first_half_end;
+	}
+    }
+
+  if (forward)
+    {
+      /* The [begin, current_begin) range has been read.  */
+      r.begin = begin;
+      r.end = current_begin;
+      r.data = buf;
+    }
+  else
+    {
+      /* The [current_end, end) range has been read.  */
+      LONGEST rlen = end - current_end;
+      r.data = xmalloc (rlen);
+      memcpy (r.data, buf + current_end - begin, rlen);
+      r.begin = current_end;
+      r.end = end;
+      xfree (buf);
+    }
+  VEC_safe_push(memory_read_result_s, (*result), &r);
+}
+
+void
+free_memory_read_result_vector (void *x)
+{
+  VEC(memory_read_result_s) *v = 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) *
+read_memory_robust (struct target_ops *ops, ULONGEST offset, LONGEST len)
+{
+  VEC(memory_read_result_s) *result = 0;
+
+  LONGEST xfered = 0;
   while (xfered < len)
     {
-      LONGEST xfer = target_read_partial (ops, object, annex,
-					  (gdb_byte *) buf + xfered,
-					  offset + xfered, len - xfered);
+      struct mem_region *region = lookup_mem_region (offset + xfered);
+      LONGEST rlen;
 
-      /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer == 0)
-	return xfered;
-      if (xfer < 0)
+      /* If there is no explicit region, a fake one should be created.  */
+      gdb_assert (region);
+
+      if (region->hi == 0)
+	rlen = len - xfered;
+      else
+	rlen = region->hi - offset;
+
+      if (region->attrib.mode == MEM_NONE || region->attrib.mode == MEM_WO)
 	{
-	  /* We've got an error.  Try to read in smaller blocks.  */
-	  ULONGEST start = offset + xfered;
-	  ULONGEST remaining = len - xfered;
-	  ULONGEST half;
-
-	  /* If an attempt was made to read a random memory address,
-	     it's likely that the very first byte is not accessible.
-	     Try reading the first byte, to avoid doing log N tries
-	     below.  */
-	  xfer = target_read_partial (ops, object, annex, 
-				      (gdb_byte *) buf + xfered, start, 1);
-	  if (xfer <= 0)
-	    return xfered;
-	  start += 1;
-	  remaining -= 1;
-	  half = remaining/2;
-	  
-	  while (half > 0)
+	  /* Cannot read this region. Note that we can end up here only
+	     if the region is explicitly marked inaccessible, or
+	     'inaccessible-by-default' is in effect.  */
+	  xfered += rlen;
+	}
+      else
+	{
+	  LONGEST to_read = min (len - xfered, rlen);
+	  gdb_byte *buffer = (gdb_byte *)xmalloc (to_read);
+
+	  LONGEST xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL,
+				      (gdb_byte *) buffer,
+				      offset + xfered, to_read);
+	  /* Call an observer, notifying them of the xfer progress?  */
+	  if (xfer == 0)
 	    {
-	      xfer = target_read_partial (ops, object, annex,
-					  (gdb_byte *) buf + xfered,
-					  start, half);
-	      if (xfer == 0)
-		return xfered;
-	      if (xfer < 0)
-		{
-		  remaining = half;		  
-		}
-	      else
-		{
-		  /* We have successfully read the first half.  So, the
-		     error must be in the second half.  Adjust start and
-		     remaining to point at the second half.  */
-		  xfered += xfer;
-		  start += xfer;
-		  remaining -= xfer;
-		}
-	      half = remaining/2;
+	      xfree (buffer);
+	      xfered += to_read;
 	    }
-
-	  return xfered;
+	  else if (xfer < 0)
+	    {
+	      /* Got an error reading full chunk. See if maybe we can read
+		 some subrange.  */
+	      xfree (buffer);
+	      read_whatever_is_readable (ops, offset + xfered, offset + xfered + to_read, &result);
+	      xfered += to_read;
+	    }
+	  else
+	    {
+	      struct memory_read_result r;
+	      r.data = buffer;
+	      r.begin = offset + xfered;
+	      r.end = r.begin + xfer;
+	      VEC_safe_push (memory_read_result_s, result, &r);
+	      xfered += xfer;
+	    }
+	  QUIT;
 	}
-      xfered += xfer;
-      QUIT;
     }
-  return len;
+  return result;
 }
 
+
 /* An alternative to target_write with progress callbacks.  */
 
 LONGEST
diff --git a/gdb/target.h b/gdb/target.h
index 870a1eb..66e1166 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -292,10 +292,22 @@ extern LONGEST target_read (struct target_ops *ops,
 			    const char *annex, gdb_byte *buf,
 			    ULONGEST offset, LONGEST len);
 
-extern LONGEST target_read_until_error (struct target_ops *ops,
-					enum target_object object,
-					const char *annex, gdb_byte *buf,
-					ULONGEST offset, LONGEST len);
+struct memory_read_result
+  {
+    /* 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 *);
+
+extern VEC(memory_read_result_s)*
+read_memory_robust (struct target_ops *ops, ULONGEST offset, LONGEST len);
   
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,