Re: Better MI memory commands

[Vladimir Prus <vladimir at codesourcery dot com>]

On Friday 09 July 2010 22:59:06 Daniel Jacobowitz wrote:

> On Fri, Jul 09, 2010 at 10:54:07PM +0400, Vladimir Prus wrote:
> > On Wednesday 07 July 2010 20:29:55 Daniel Jacobowitz wrote:
> > 
> > > > -           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;
> > > >           }
> > > 
> > > Why do we skip to_read bytes if we succeed at reading zero bytes?
> > > For that matter, what does a return value of zero mean?  It seems like
> > > this would mean the same as -1.
> > 
> > I am not really sure. Per documentation of target_read:
> > 
> >    Return the number of bytes actually transfered, or -1 if the
> >    transfer is not supported or otherwise fails.  Return of a positive
> >    value less than LEN indicates that no further transfer is possible.
> > 
> > So, value of 0 seems to mean 'there are no more bytes that that, honest', 
> > and that we probably don't need to try further. Documentation for
> > to_xfer_partial seem to give such meaning to return of 0.
> 
> I'd suggest treating 0 and -1 the same, for memory.
> 
> > On the
> > other hand, it's not clear what return value of <LEN might mean,
> > and whether we should try to read remaining chunk. What would you suggest?
> 
> A return of less than LEN from xfer_partial doesn't mean anything; you
> just retry.  A return of less than LEN from target_read, though, is
> supposed to mean that there is no point in retrying; the next byte is
> inaccessible or does not exist.
> 
> It doesn't look like memory reads (unlike other partial transfers)
> implement that; usually they just fail.  But we can treat it that way
> anyway.  So <LEN means we got some number of bytes, and then we should
> see what happens after those successful bytes.

Here's a revised version.


Thanks,

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

commit 97fb083bbb395f6bb9e79f5f7dba6d1caeb9e8b2
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-bytes
    	and data-write-memory-bytes.
    	* mi/mi-cmds.h (mi_cmd_data_read_memory_bytes)
    	(mi_cmd_data_write_memory_bytes): New.
    	* mi/mi-main.c (mi_cmd_data_read_memory): Use regular target_read.
    	(mi_cmd_data_read_memory_bytes, mi_cmd_data_write_memory_bytes): 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..2765a71 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-bytes} instead.
+
 @subsubheading Synopsis
 
 @smallexample
@@ -27006,6 +27008,130 @@ next-page="0x000013c0",prev-page="0x00001380",memory=[
 (gdb)
 @end smallexample
 
+@subheading The @code{-data-read-memory-bytes} Command
+@findex -data-read-memory-bytes
+
+@subsubheading Synopsis
+
+@smallexample
+ -data-read-memory-bytes [ -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
+
+This command attempts to read all accessible memory regions in the
+specified range.  First, all regions marked as unreadable in the memory
+map (if one is defined) will be skipped.  Second, @value{GDBN} will
+attempt to read the remaining regions.  For each one, if reading full
+region results in an errors, @value{GDBN} will try to read a subset
+of the region.
+
+In general, every single byte in the region may be readable or not,
+and the only way to read every readable byte is to try a read at
+every address, which is not practical.   Therefore, @value{GDBN} will
+attempt to read all accessible bytes at either beginning or the end
+of the region, using a binary division scheme.  This heuristic works
+well for reading accross a memory map boundary.  Note that if a region
+has a readable range that is neither at the beginning or the end,
+@value{GDBN} will not read it.
+
+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-bytes}.
+
+@item contents
+The contents of the memory block, as hex-encoded string of bytes.
+
+@end table
+
+At present, if multiple
+
+
+@subsubheading @value{GDBN} Command
+
+The corresponding @value{GDBN} command is @samp{x}.
+
+@subsubheading Example
+
+@smallexample
+(gdb)
+-data-read-memory-bytes &a 10
+^done,memory=[@{begin="0xbffff154",offset="0x00000000",
+              end="0xbffff15e",
+              contents="01000000020000000300"@}]
+(gdb)
+@end smallexample
+
+
+@subheading The @code{-data-write-memory-bytes} Command
+@findex -data-write-memory-bytes
+
+@subsubheading Synopsis
+
+@smallexample
+ -data-write-memory-bytes @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-bytes &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..a6a884f 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-bytes", { NULL, 0 }, mi_cmd_data_read_memory_bytes},
   { "data-write-memory", { NULL, 0 }, mi_cmd_data_write_memory},
+  { "data-write-memory-bytes", {NULL, 0}, mi_cmd_data_write_memory_bytes},
   { "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..b357de6 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_bytes;
 extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
+extern mi_cmd_argv_ftype mi_cmd_data_write_memory_bytes;
 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..f7f28fa 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_bytes (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_bytes", 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: [ -o OFFSET ] 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_bytes (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..cf01b6c 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1751,73 +1751,204 @@ 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 read 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
+	{
+	  /* This half is not readable. Because we've tried one byte, we
+	     know some part of this half if actually redable. Go to the next
+	     iteration to divide again and try to read.
+
+	     We don't handle the other half, because this function only tries
+	     to read a single readable subrange.  */
+	  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);
+	  /* 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)
-	    return xfered;
-	  start += 1;
-	  remaining -= 1;
-	  half = remaining/2;
-	  
-	  while (half > 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;
+	      /* 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;
 	    }
-
-	  return xfered;
+	  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..e0a63ca 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -292,10 +292,23 @@ 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,