[rfc] Correct semantics of target_read_partial, add target_read_whole

[rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

Originally, target_read_partial was supposed to read "however much it could
manage to" and then higher level functions were supposed to handle the rest.
But every existing implementation always reads enough data in its first
call; the one remote protocol implementation did so by issuing as many
packets as necessary, which defeated the point of the original design.

This patch adjusts the remote protocol layer not to do that.  It also
promotes a useful function from auxv.c to target.c:

+/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
+   be read using OPS.  The return value will be -1 if the transfer
+   fails or is not supported; 0 if the object is empty; and the length
+   of the object otherwise.  If a positive value is returned, a
+   sufficiently large buffer will be allocated using xmalloc and
+   returned in *BUF_P containing the contents of the object.
+
+   This method should be used for objects sufficiently small to store
+   in a single xmalloced buffer, when no fixed bound on the object's
+   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
+   through this function.  */
+
+extern LONGEST target_read_whole (struct target_ops *ops,
+                                 enum target_object object,
+                                 const char *annex, gdb_byte **buf_p);

When you use to_xfer_partial to get at memory, obviously you don't want "the
whole object".  But for other objects, such as auxv vectors or XML
description files, usually you do.  This provides a central interface
to handle short reads correctly, instead of letting that code circulate
(buggily in many cases) through other files.

It should have no net measurable change on GDB's behavior, and I've been
using an earlier version of this patch for several months now.  I verified
that the same number of qPart:auxv:read packets are used when talking to
gdbserver; the third patch in this sequence will cut it down from two to
one.

Unless someone sees a problem with this patch, I will commit it when the
qXfer changes are ready.

-- 
Daniel Jacobowitz
CodeSourcery

2006-06-21  Daniel Jacobowitz  <dan@codesourcery.com>

	* target.c (target_read): Stop if target_read_partial returns 0
	when some bytes have already been read.
	(target_write): Likewise for target_write_partial.
	(target_read_whole): New.
	* target.h (target_read): Improve description.
	(target_read_whole): New prototype.

	* auxv.c (target_auxv_read): Delete.
	(target_auxv_search, fprint_target_auxv): Use target_read_whole.
	* auxv.h (target_auxv_read): Delete prototype.
	* avr-tdep.c (avr_io_reg_read_command): Use target_read_whole.
	* ia64-tdep.c (getunwind_table, get_kernel_table): Likewise.
	* linux-nat.c (linux_nat_make_corefile_notes): Likewise.
	* procfs.c (procfs_make_note_section): Likewise.
	* remote.c (remote_xfer_partial): Don't loop here.
	* sparc-tdep.c (sparc_fetch_wcookie): Use target_read.

Index: src/gdb/auxv.c
===================================================================
--- src.orig/gdb/auxv.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/auxv.c	2006-06-21 17:06:27.000000000 -0400
@@ -76,43 +76,6 @@ procfs_xfer_auxv (struct target_ops *ops
   return n;
 }
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-LONGEST
-target_auxv_read (struct target_ops *ops, gdb_byte **data)
-{
-  size_t auxv_alloc = 512, auxv_pos = 0;
-  gdb_byte *auxv = xmalloc (auxv_alloc);
-  int n;
-
-  while (1)
-    {
-      n = target_read_partial (ops, TARGET_OBJECT_AUXV,
-			       NULL, &auxv[auxv_pos], 0,
-			       auxv_alloc - auxv_pos);
-      if (n <= 0)
-	break;
-      auxv_pos += n;
-      if (auxv_pos < auxv_alloc) /* Read all there was.  */
-	break;
-      gdb_assert (auxv_pos == auxv_alloc);
-      auxv_alloc *= 2;
-      auxv = xrealloc (auxv, auxv_alloc);
-    }
-
-  if (auxv_pos == 0)
-    {
-      xfree (auxv);
-      *data = NULL;
-      return n;
-    }
-
-  *data = auxv;
-  return auxv_pos;
-}
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
@@ -148,7 +111,7 @@ target_auxv_search (struct target_ops *o
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int n = target_auxv_read (ops, &data);
+  LONGEST n = target_read_whole (ops, TARGET_OBJECT_AUXV, NULL, &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
@@ -184,7 +147,7 @@ fprint_target_auxv (struct ui_file *file
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int len = target_auxv_read (ops, &data);
+  LONGEST len = target_read_whole (ops, TARGET_OBJECT_AUXV, NULL, &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
Index: src/gdb/auxv.h
===================================================================
--- src.orig/gdb/auxv.h	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/auxv.h	2006-06-21 17:06:27.000000000 -0400
@@ -31,12 +31,6 @@
 struct target_ops;		/* Forward declaration.  */
 
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-extern LONGEST target_auxv_read (struct target_ops *ops, gdb_byte **data);
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
Index: src/gdb/avr-tdep.c
===================================================================
--- src.orig/gdb/avr-tdep.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/avr-tdep.c	2006-06-21 17:06:27.000000000 -0400
@@ -1323,35 +1323,22 @@ static void
 avr_io_reg_read_command (char *args, int from_tty)
 {
   LONGEST bufsiz = 0;
-  char buf[400];
+  gdb_byte *buf;
   char query[400];
   char *p;
   unsigned int nreg = 0;
   unsigned int val;
   int i, j, k, step;
 
-  /* Just get the maximum buffer size. */
-  bufsiz = target_read_partial (&current_target, TARGET_OBJECT_AVR,
-				NULL, NULL, 0, 0);
-  if (bufsiz < 0)
-    {
-      fprintf_unfiltered (gdb_stderr,
-			  _("ERR: info io_registers NOT supported "
-			    "by current target\n"));
-      return;
-    }
-  if (bufsiz > sizeof (buf))
-    bufsiz = sizeof (buf);
-
   /* Find out how many io registers the target has. */
-  strcpy (query, "avr.io_reg");
-  target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf, 0,
-		       bufsiz);
+  bufsiz = target_read_whole (&current_target, TARGET_OBJECT_AVR,
+			      "avr.io_reg", &buf);
 
-  if (strncmp (buf, "", bufsiz) == 0)
+  if (bufsiz <= 0)
     {
       fprintf_unfiltered (gdb_stderr,
-			  _("info io_registers NOT supported by target\n"));
+			  _("ERR: info io_registers NOT supported "
+			    "by current target\n"));
       return;
     }
 
@@ -1359,9 +1346,12 @@ avr_io_reg_read_command (char *args, int
     {
       fprintf_unfiltered (gdb_stderr,
 			  _("Error fetching number of io registers\n"));
+      xfree (buf);
       return;
     }
 
+  xfree (buf);
+
   reinitialize_more_filter ();
 
   printf_unfiltered (_("Target has %u io registers:\n\n"), nreg);
@@ -1377,8 +1367,8 @@ avr_io_reg_read_command (char *args, int
         j = nreg - i;           /* last block is less than 8 registers */
 
       snprintf (query, sizeof (query) - 1, "avr.io_reg:%x,%x", i, j);
-      target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf,
-			   0, bufsiz);
+      bufsiz = target_read_whole (&current_target, TARGET_OBJECT_AVR, query,
+				  &buf);
 
       p = buf;
       for (k = i; k < (i + j); k++)
@@ -1393,6 +1383,8 @@ avr_io_reg_read_command (char *args, int
 		break;
 	    }
 	}
+
+      xfree (buf);
     }
 }
 
Index: src/gdb/ia64-tdep.c
===================================================================
--- src.orig/gdb/ia64-tdep.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/ia64-tdep.c	2006-06-21 17:06:27.000000000 -0400
@@ -2458,8 +2458,8 @@ ia64_access_mem (unw_addr_space_t as,
 }
 
 /* Call low-level function to access the kernel unwind table.  */
-static int
-getunwind_table (void *buf, size_t len)
+static LONGEST
+getunwind_table (gdb_byte **buf_p)
 {
   LONGEST x;
 
@@ -2470,10 +2470,11 @@ getunwind_table (void *buf, size_t len)
      we want to preserve fall back to the running kernel's table, then
      we should find a way to override the corefile layer's
      xfer_partial method.  */
-  x = target_read_partial (&current_target, TARGET_OBJECT_UNWIND_TABLE, NULL,
-			   buf, 0, len);
 
-  return (int)x;
+  x = target_read_whole (&current_target, TARGET_OBJECT_UNWIND_TABLE,
+			 NULL, buf_p);
+
+  return x;
 }
 
 /* Get the kernel unwind table.  */				 
@@ -2484,14 +2485,15 @@ get_kernel_table (unw_word_t ip, unw_dyn
 
   if (!ktab) 
     {
+      gdb_byte *ktab_buf;
       size_t size;
-      size = getunwind_table (NULL, 0);
-      if ((int)size < 0)
-        return -UNW_ENOINFO;
-      ktab_size = size;
-      ktab = xmalloc (ktab_size);
-      getunwind_table (ktab, ktab_size);
-		          
+
+      ktab_size = getunwind_table (&ktab_buf);
+      if (ktab_size <= 0)
+	return -UNW_ENOINFO;
+      else
+	ktab = (struct ia64_table_entry *) ktab_buf;
+
       for (etab = ktab; etab->start_offset; ++etab)
         etab->info_offset += KERNEL_START;
     }
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/linux-nat.c	2006-06-21 17:06:27.000000000 -0400
@@ -2697,7 +2697,8 @@ linux_nat_make_corefile_notes (bfd *obfd
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_whole (&current_target, TARGET_OBJECT_AUXV, NULL,
+				&auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/procfs.c
===================================================================
--- src.orig/gdb/procfs.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/procfs.c	2006-06-21 17:06:27.000000000 -0400
@@ -6130,7 +6130,8 @@ procfs_make_note_section (bfd *obfd, int
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_whole (&current_target, TARGET_OBJECT_AUXV, NULL,
+				&auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/sparc-tdep.c
===================================================================
--- src.orig/gdb/sparc-tdep.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/sparc-tdep.c	2006-06-21 17:06:27.000000000 -0400
@@ -158,7 +158,7 @@ sparc_fetch_wcookie (void)
   gdb_byte buf[8];
   int len;
 
-  len = target_read_partial (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
+  len = target_read (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
   if (len == -1)
     return 0;
 
Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/target.c	2006-06-21 17:06:27.000000000 -0400
@@ -1373,8 +1373,9 @@ target_read (struct target_ops *ops,
 					  (gdb_byte *) buf + xfered,
 					  offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1395,8 +1396,9 @@ target_write (struct target_ops *ops,
 					   (gdb_byte *) buf + xfered,
 					   offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1404,6 +1406,45 @@ target_write (struct target_ops *ops,
   return len;
 }
 
+/* Perform a full target read of unknown size.  */
+
+LONGEST
+target_read_whole (struct target_ops *ops,
+		   enum target_object object,
+		   const char *annex, gdb_byte **buf_p)
+{
+  size_t buf_alloc = 512, buf_pos = 0;
+  gdb_byte *buf = xmalloc (buf_alloc);
+  LONGEST n, total;
+
+  total = 0;
+  while (1)
+    {
+      n = target_read (ops, object, annex, &buf[buf_pos],
+		       buf_pos, buf_alloc - buf_pos);
+      if (n < 0)
+	{
+	  /* An error occurred.  */
+	  xfree (buf);
+	  return -1;
+	}
+
+      buf_pos += n;
+      if (buf_pos < buf_alloc)
+	{
+	  /* Read all there was.  */
+	  if (buf_pos == 0)
+	    xfree (buf);
+	  else
+	    *buf_p = buf;
+	  return buf_pos;
+	}
+
+      buf_alloc *= 2;
+      buf = xrealloc (buf, buf_alloc);
+    }
+}
+
 /* Memory transfer methods.  */
 
 void
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2006-06-21 17:04:01.000000000 -0400
+++ src/gdb/target.h	2006-06-21 17:06:27.000000000 -0400
@@ -246,7 +246,10 @@ extern LONGEST target_write_partial (str
 				     const char *annex, const gdb_byte *buf,
 				     ULONGEST offset, LONGEST len);
 
-/* Wrappers to perform the full transfer.  */
+/* Wrappers to perform a full transfer.  These functions take the
+   same arguments as the partial versions, above, but a return
+   value of a positive number less than LEN implies that no more
+   data can be read or written.  */
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
 			    const char *annex, gdb_byte *buf,
@@ -257,6 +260,22 @@ extern LONGEST target_write (struct targ
 			     const char *annex, const gdb_byte *buf,
 			     ULONGEST offset, LONGEST len);
 
+/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
+   be read using OPS.  The return value will be -1 if the transfer
+   fails or is not supported; 0 if the object is empty; and the length
+   of the object otherwise.  If a positive value is returned, a
+   sufficiently large buffer will be allocated using xmalloc and
+   returned in *BUF_P containing the contents of the object.
+
+   This method should be used for objects sufficiently small to store
+   in a single xmalloced buffer, when no fixed bound on the object's
+   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
+   through this function.  */
+
+extern LONGEST target_read_whole (struct target_ops *ops,
+				  enum target_object object,
+				  const char *annex, gdb_byte **buf_p);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2006-06-21 17:07:19.000000000 -0400
+++ src/gdb/remote.c	2006-06-21 17:08:38.000000000 -0400
@@ -5128,35 +5128,23 @@ remote_xfer_partial (struct target_ops *
     case TARGET_OBJECT_AUXV:
       if (remote_protocol_packets[PACKET_qPart_auxv].support != PACKET_DISABLE)
 	{
-	  unsigned int total = 0;
-	  while (len > 0)
-	    {
-	      LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
-	      snprintf (rs->buf, get_remote_packet_size (),
-			"qPart:auxv:read::%s,%s",
-			phex_nz (offset, sizeof offset),
-			phex_nz (n, sizeof n));
-	      i = putpkt (rs->buf);
-	      if (i < 0)
-		return total > 0 ? total : i;
-	      rs->buf[0] = '\0';
-	      getpkt (&rs->buf, &rs->buf_size, 0);
-	      if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
-		  != PACKET_OK)
-		return total > 0 ? total : -1;
-	      if (strcmp (rs->buf, "OK") == 0)
-		break;		/* Got EOF indicator.  */
-	      /* Got some data.  */
-	      i = hex2bin (rs->buf, readbuf, len);
-	      if (i > 0)
-		{
-		  readbuf = (void *) ((char *) readbuf + i);
-		  offset += i;
-		  len -= i;
-		  total += i;
-		}
-	    }
-	  return total;
+	  LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
+	  snprintf (rs->buf, get_remote_packet_size (),
+		    "qPart:auxv:read::%s,%s",
+		    phex_nz (offset, sizeof offset),
+		    phex_nz (n, sizeof n));
+	  i = putpkt (rs->buf);
+	  if (i < 0)
+	    return i;
+	  rs->buf[0] = '\0';
+	  getpkt (&rs->buf, &rs->buf_size, 0);
+	  if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
+	      != PACKET_OK)
+	    return -1;
+	  if (strcmp (rs->buf, "OK") == 0)
+	    return 0;		/* Got EOF indicator.  */
+	  /* Got some data.  */
+	  return hex2bin (rs->buf, readbuf, len);
 	}
       return -1;
 

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Mark Kettenis]

> Date: Wed, 21 Jun 2006 23:23:55 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Originally, target_read_partial was supposed to read "however much it could
> manage to" and then higher level functions were supposed to handle the rest.
> But every existing implementation always reads enough data in its first
> call; the one remote protocol implementation did so by issuing as many
> packets as necessary, which defeated the point of the original design.

Ah, it all makes sense to me now.  I'm wondering whether we should
"export" target_read_partial() (and target_write_partial()) at all.
It's never right to use them except for implementing higher-level
target read/write functions isn't it?

> This patch adjusts the remote protocol layer not to do that.  It also
> promotes a useful function from auxv.c to target.c:
> 
> +/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
> +   be read using OPS.  The return value will be -1 if the transfer
> +   fails or is not supported; 0 if the object is empty; and the length
> +   of the object otherwise.  If a positive value is returned, a
> +   sufficiently large buffer will be allocated using xmalloc and
> +   returned in *BUF_P containing the contents of the object.
> +
> +   This method should be used for objects sufficiently small to store
> +   in a single xmalloced buffer, when no fixed bound on the object's
> +   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
> +   through this function.  */
> +
> +extern LONGEST target_read_whole (struct target_ops *ops,
> +                                 enum target_object object,
> +                                 const char *annex, gdb_byte **buf_p);
> 
> When you use to_xfer_partial to get at memory, obviously you don't want "the
> whole object".  But for other objects, such as auxv vectors or XML
> description files, usually you do.  This provides a central interface
> to handle short reads correctly, instead of letting that code circulate
> (buggily in many cases) through other files.

Agreed.  I have a (small) concern that the introduction of
target_read_whole() will cause confusion with target_read().  Perhaps
a better name would be target_read_alloc?

You might consider commiting the sparc-tdep.c change seperately; it's
"obvious".

Mark

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

On Thu, Jun 22, 2006 at 08:24:54PM +0200, Mark Kettenis wrote:
> > Date: Wed, 21 Jun 2006 23:23:55 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > Originally, target_read_partial was supposed to read "however much it could
> > manage to" and then higher level functions were supposed to handle the rest.
> > But every existing implementation always reads enough data in its first
> > call; the one remote protocol implementation did so by issuing as many
> > packets as necessary, which defeated the point of the original design.
> 
> Ah, it all makes sense to me now.  I'm wondering whether we should
> "export" target_read_partial() (and target_write_partial()) at all.
> It's never right to use them except for implementing higher-level
> target read/write functions isn't it?

There's some messiness in kod.c.  But didn't we decide that was on the
chopping block to be removed?  Other than that, in my current working
tree there are absolutely no calls to these functions outside of
target.c.  So I think you're right: we can stop exporting them at all.

Vladimir made a related comment on gdb@ a week or two ago.  GDB has
developed too many ways to do the same thing.  We desperately need to
start bringing that number back down instead of up.  Since my patch
adds one (target_read_whole), I have an onus to remove at least one :-)
I will make the functions static in the next version of this patch.

And, thanks a lot for taking a look at these changes.  I can't say this
enough - I realize the work I'm doing is outside of the usual areas for
some of the other maintainers, but I rely on feedback to keep GDB the
best it can be!

> Agreed.  I have a (small) concern that the introduction of
> target_read_whole() will cause confusion with target_read().  Perhaps
> a better name would be target_read_alloc?

Sounds reasonable to me.  I couldn't think of a better name at the
time.

> You might consider commiting the sparc-tdep.c change seperately; it's
> "obvious".

Since I have to redo these patches anyway, and I've already put in a
couple of other conflicting bits, I might as well.  I'll do that later.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Vladimir Prus]

Daniel Jacobowitz wrote:

> Originally, target_read_partial was supposed to read "however much it
> could manage to" and then higher level functions were supposed to handle
> the rest. But every existing implementation always reads enough data in
> its first call; the one remote protocol implementation did so by issuing
> as many packets as necessary, which defeated the point of the original
> design.
> 
> This patch adjusts the remote protocol layer not to do that.  It also
> promotes a useful function from auxv.c to target.c:
> 
> +/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
> +   be read using OPS.  The return value will be -1 if the transfer
> +   fails or is not supported; 0 if the object is empty; and the length
> +   of the object otherwise.  If a positive value is returned, a
> +   sufficiently large buffer will be allocated using xmalloc and
> +   returned in *BUF_P containing the contents of the object.
> +
> +   This method should be used for objects sufficiently small to store
> +   in a single xmalloced buffer, when no fixed bound on the object's
> +   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
> +   through this function.  */
> +
> +extern LONGEST target_read_whole (struct target_ops *ops,
> +                                 enum target_object object,
> +                                 const char *annex, gdb_byte **buf_p);

Dan,

did you notice my post on gdb-devel about having two different methods of
reading method. The post subject was: "target memory read/write methods".
To quote:

  One way to read a memory is:

  - target_read_memory, which calls
    - xfer_using_stratum, which calls
      - target_xfer_partial (iterating over 'stratums'), which
        - goes to function pointer in target_ops

   This is the predominant method.

  Another way to read memory is:

  - get_target_memory{unsigned}, which calls:
    - target_read, which calls:
      - target_xfer_partial


Your patch add target_read_whole, that calls 'target_read', which until now
is used only by get_target_memory{unsigned}, which is used in 3 places in
entire gdb. In addition 'xfer_using_stratum' already has some code to
repeatedly call target_xfer_partial. I also might not understand what
stratums are but should not they be used in all cases? Otherwise, behaviour
of target_read_while for 'object = TARGET_OBJECT_MEMORY' will be different
from the behaviour of 'target_read_memory'.

I think that either:
1. There are way too many code paths for reading
2. There are way too few comments.

Is seems to me that the right solution would be to have target_read_whole
directly call xfer_using_stratum.

Also, how you target_read_whole work when   

  target_xfer_partial_p ()

return false? The 'target_read' function does not check it and
'target_xfer_partial' will just assert.

Should 'target_read_whole' check for target_xfer_partial_p and fail is its
false?

- Volodya

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

On Sat, Jun 24, 2006 at 02:06:07PM +0400, Vladimir Prus wrote:
> did you notice my post on gdb-devel about having two different methods of
> reading method. The post subject was: "target memory read/write methods".

As I told Vladimir on IRC, I'd been putting this off because it's such
a complicated question :-)

> To quote:
> 
>   One way to read a memory is:
> 
>   - target_read_memory, which calls
>     - xfer_using_stratum, which calls
>       - target_xfer_partial (iterating over 'stratums'), which
>         - goes to function pointer in target_ops
> 
>    This is the predominant method.

And the right one.

> 
>   Another way to read memory is:
> 
>   - get_target_memory{unsigned}, which calls:
>     - target_read, which calls:
>       - target_xfer_partial

I think this is just wrong, given the way GDB has changed since.

> Your patch add target_read_whole, that calls 'target_read', which until now
> is used only by get_target_memory{unsigned}, which is used in 3 places in
> entire gdb. In addition 'xfer_using_stratum' already has some code to
> repeatedly call target_xfer_partial. I also might not understand what
> stratums are but should not they be used in all cases? Otherwise, behaviour
> of target_read_while for 'object = TARGET_OBJECT_MEMORY' will be different
> from the behaviour of 'target_read_memory'.

You have to understand what the strata (plural of stratum :-) are to
understand what's going on here.  Roughly, each stratum is a view of
the target.  The actual details are murkier than this and in need of
a lot of changes, but it's low priority and fairly fragile, so it gets
left alone.  When debugging a threaded native program, we might have:

  thread_stratum -> process_stratum -> file_stratum

When debugging a core file we might have:

  core_stratum -> file_stratum

That core file case is the most interesting one for xfer_using_stratum. 
A core file often has some of memory, but not all of it.  For instance,
it might have all the data sections but none of the code sections; we
have to load those from the executable instead.  This means that for
memory, we may need to fulfill a single memory read from more than one
stratum.

For other objects, this is generally not true.  We want to fill the
request from the first stratum that knows the object we're requesting.
We don't support sharing objects across strata for anything besides
memory.

This fits together with another piece of the puzzle:

> Also, how you target_read_whole work when   
> 
>   target_xfer_partial_p ()
> 
> return false? The 'target_read' function does not check it and
> 'target_xfer_partial' will just assert.
> 
> Should 'target_read_whole' check for target_xfer_partial_p and fail is its
> false?

That's not actually true.

static int
target_xfer_partial_p (void)
{
  return (target_stack != NULL
          && target_stack->to_xfer_partial != default_xfer_partial);
}

versus

  gdb_assert (ops->to_xfer_partial != NULL);

A target which does not initialize to_xfer_partial will have it set to
the default in add_target.  The default pushes requests down the target
stack until it finds a non-default implementation and calls that.  Many
implementations seem to stop there; that's probably a bug waiting to
happen, I don't know if the implementations should pass unknown objects
further down or if default_xfer_partial should.  But since we're
usually interested in the process layer's to_xfer_partial it doesn't
cause a problem in practice.

> I think that either:
> 1. There are way too many code paths for reading
> 2. There are way too few comments.

Both true.  Feel free to propose fixes for either :-)

This is one of several areas in which GDB has too many interfaces for
doing the same thing.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Vladimir Prus]

Daniel Jacobowitz wrote:


> +/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
> +   be read using OPS.  The return value will be -1 if the transfer
> +   fails or is not supported; 0 if the object is empty; and the length
> +   of the object otherwise.  If a positive value is returned, a
> +   sufficiently large buffer will be allocated using xmalloc and
> +   returned in *BUF_P containing the contents of the object.
> +
> +   This method should be used for objects sufficiently small to store
> +   in a single xmalloced buffer, when no fixed bound on the object's
> +   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
> +   through this function.  */
> +
> +extern LONGEST target_read_whole (struct target_ops *ops,
> +                                 enum target_object object,
> +                                 const char *annex, gdb_byte **buf_p);

Is there any reason why 'target_read_whole' calls 'target_read', as opposed
to calling 'target_read_partial' directly? I mean, if target_read_whole can
do several reads itself, there's no point to use 'target_read'.

- Volodya

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

On Wed, Jun 28, 2006 at 12:18:02PM +0400, Vladimir Prus wrote:
> Is there any reason why 'target_read_whole' calls 'target_read', as opposed
> to calling 'target_read_partial' directly? I mean, if target_read_whole can
> do several reads itself, there's no point to use 'target_read'.

Thank you!  Indeed we shouldn't use target_read; it will issue
gratuitous extra packets (by breaking the transfer in the wrong place).

Obviously I need to proofread all these changes again when I get home.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Thu, Jun 22, 2006 at 08:24:54PM +0200, Mark Kettenis wrote:
>> > Date: Wed, 21 Jun 2006 23:23:55 -0400
>> > From: Daniel Jacobowitz <drow@false.org>
>> > 
>> > Originally, target_read_partial was supposed to read "however much it
>> > could manage to" and then higher level functions were supposed to
>> > handle the rest. But every existing implementation always reads enough
>> > data in its first call; the one remote protocol implementation did so
>> > by issuing as many packets as necessary, which defeated the point of
>> > the original design.
>> 
>> Ah, it all makes sense to me now.  I'm wondering whether we should
>> "export" target_read_partial() (and target_write_partial()) at all.
>> It's never right to use them except for implementing higher-level
>> target read/write functions isn't it?
> 
> There's some messiness in kod.c.  But didn't we decide that was on the
> chopping block to be removed?  Other than that, in my current working
> tree there are absolutely no calls to these functions outside of
> target.c.  So I think you're right: we can stop exporting them at all.

There are also target_write_memory_partial and target_read_memory_partial.

The write variant is used only for implementation for the 'load' command. My
work on implementing flash support will hopefully kill that use.

The read variant is used in MI implementation of read, where, guess what,
it's wrapped in a loop. I'll add replacing that loop with
target_read_memory to my todo.

- Volodya

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

Here is an improved version of my previous patch.  According to
suggestions I received, I've renamed target_read_whole to the hopefully
clearer target_read_object_alloc (since it reads an entire object, and
allocates memory for it).  I've eliminated the extra use of
target_read; the code to use target_read_partial directly is slightly
more complicated, but more efficient and overall more sensible.  And:
the _partial methods are no longer exported!  Roughly no one ever wants
to use them; use target_read or target_read_object_alloc instead.

Any further comments on this patch?

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-05  Daniel Jacobowitz  <dan@codesourcery.com>

	* target.c (target_read): Stop if target_read_partial returns 0
	when some bytes have already been read.
	(target_write): Likewise for target_write_partial.
	(target_read_partial, target_write_partial): Make static.
	(target_read_object_alloc): New.
	* target.h: Doc fixes.
	(target_read_partial, target_write_partial): Delete prototypes.
	(target_read_object_alloc): New prototype.

	* auxv.c (target_auxv_read): Delete.
	(target_auxv_search, fprint_target_auxv): Use target_read_object_alloc.
	* auxv.h (target_auxv_read): Delete prototype.
	* avr-tdep.c (avr_io_reg_read_command): Use target_read_object_alloc.
	* ia64-tdep.c (getunwind_table, get_kernel_table): Likewise.
	* linux-nat.c (linux_nat_make_corefile_notes): Likewise.
	* procfs.c (procfs_make_note_section): Likewise.
	* remote.c (remote_xfer_partial): Don't loop here.
	* sparc-tdep.c (sparc_fetch_wcookie): Use target_read.

Index: src/gdb/auxv.c
===================================================================
--- src.orig/gdb/auxv.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/auxv.c	2006-07-05 12:14:34.000000000 -0400
@@ -1,6 +1,6 @@
 /* Auxiliary vector support for GDB, the GNU debugger.
 
-   Copyright (C) 2004 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -76,43 +76,6 @@ procfs_xfer_auxv (struct target_ops *ops
   return n;
 }
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-LONGEST
-target_auxv_read (struct target_ops *ops, gdb_byte **data)
-{
-  size_t auxv_alloc = 512, auxv_pos = 0;
-  gdb_byte *auxv = xmalloc (auxv_alloc);
-  int n;
-
-  while (1)
-    {
-      n = target_read_partial (ops, TARGET_OBJECT_AUXV,
-			       NULL, &auxv[auxv_pos], 0,
-			       auxv_alloc - auxv_pos);
-      if (n <= 0)
-	break;
-      auxv_pos += n;
-      if (auxv_pos < auxv_alloc) /* Read all there was.  */
-	break;
-      gdb_assert (auxv_pos == auxv_alloc);
-      auxv_alloc *= 2;
-      auxv = xrealloc (auxv, auxv_alloc);
-    }
-
-  if (auxv_pos == 0)
-    {
-      xfree (auxv);
-      *data = NULL;
-      return n;
-    }
-
-  *data = auxv;
-  return auxv_pos;
-}
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
@@ -148,7 +111,7 @@ target_auxv_search (struct target_ops *o
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int n = target_auxv_read (ops, &data);
+  LONGEST n = target_read_object_alloc (ops, TARGET_OBJECT_AUXV, NULL, &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
@@ -184,7 +147,8 @@ fprint_target_auxv (struct ui_file *file
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int len = target_auxv_read (ops, &data);
+  LONGEST len = target_read_object_alloc (ops, TARGET_OBJECT_AUXV, NULL,
+					  &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
Index: src/gdb/auxv.h
===================================================================
--- src.orig/gdb/auxv.h	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/auxv.h	2006-07-05 12:14:17.000000000 -0400
@@ -1,6 +1,6 @@
 /* Auxiliary vector support for GDB, the GNU debugger.
 
-   Copyright (C) 2004 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -31,12 +31,6 @@
 struct target_ops;		/* Forward declaration.  */
 
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-extern LONGEST target_auxv_read (struct target_ops *ops, gdb_byte **data);
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
Index: src/gdb/avr-tdep.c
===================================================================
--- src.orig/gdb/avr-tdep.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/avr-tdep.c	2006-07-05 12:13:55.000000000 -0400
@@ -1,7 +1,7 @@
 /* Target-dependent code for Atmel AVR, for GDB.
 
    Copyright (C) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004,
-   2005 Free Software Foundation, Inc.
+   2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -1323,35 +1323,22 @@ static void
 avr_io_reg_read_command (char *args, int from_tty)
 {
   LONGEST bufsiz = 0;
-  char buf[400];
+  gdb_byte *buf;
   char query[400];
   char *p;
   unsigned int nreg = 0;
   unsigned int val;
   int i, j, k, step;
 
-  /* Just get the maximum buffer size. */
-  bufsiz = target_read_partial (&current_target, TARGET_OBJECT_AVR,
-				NULL, NULL, 0, 0);
-  if (bufsiz < 0)
-    {
-      fprintf_unfiltered (gdb_stderr,
-			  _("ERR: info io_registers NOT supported "
-			    "by current target\n"));
-      return;
-    }
-  if (bufsiz > sizeof (buf))
-    bufsiz = sizeof (buf);
-
   /* Find out how many io registers the target has. */
-  strcpy (query, "avr.io_reg");
-  target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf, 0,
-		       bufsiz);
+  bufsiz = target_read_object_alloc (&current_target, TARGET_OBJECT_AVR,
+				     "avr.io_reg", &buf);
 
-  if (strncmp (buf, "", bufsiz) == 0)
+  if (bufsiz <= 0)
     {
       fprintf_unfiltered (gdb_stderr,
-			  _("info io_registers NOT supported by target\n"));
+			  _("ERR: info io_registers NOT supported "
+			    "by current target\n"));
       return;
     }
 
@@ -1359,9 +1346,12 @@ avr_io_reg_read_command (char *args, int
     {
       fprintf_unfiltered (gdb_stderr,
 			  _("Error fetching number of io registers\n"));
+      xfree (buf);
       return;
     }
 
+  xfree (buf);
+
   reinitialize_more_filter ();
 
   printf_unfiltered (_("Target has %u io registers:\n\n"), nreg);
@@ -1377,8 +1367,8 @@ avr_io_reg_read_command (char *args, int
         j = nreg - i;           /* last block is less than 8 registers */
 
       snprintf (query, sizeof (query) - 1, "avr.io_reg:%x,%x", i, j);
-      target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf,
-			   0, bufsiz);
+      bufsiz = target_read_object_alloc (&current_target, TARGET_OBJECT_AVR,
+					 query, &buf);
 
       p = buf;
       for (k = i; k < (i + j); k++)
@@ -1393,6 +1383,8 @@ avr_io_reg_read_command (char *args, int
 		break;
 	    }
 	}
+
+      xfree (buf);
     }
 }
 
Index: src/gdb/ia64-tdep.c
===================================================================
--- src.orig/gdb/ia64-tdep.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/ia64-tdep.c	2006-07-05 12:13:52.000000000 -0400
@@ -1,7 +1,7 @@
 /* Target-dependent code for the IA-64 for GDB, the GNU debugger.
 
-   Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004, 2005 Free Software
-   Foundation, Inc.
+   Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -2458,8 +2458,8 @@ ia64_access_mem (unw_addr_space_t as,
 }
 
 /* Call low-level function to access the kernel unwind table.  */
-static int
-getunwind_table (void *buf, size_t len)
+static LONGEST
+getunwind_table (gdb_byte **buf_p)
 {
   LONGEST x;
 
@@ -2470,10 +2470,11 @@ getunwind_table (void *buf, size_t len)
      we want to preserve fall back to the running kernel's table, then
      we should find a way to override the corefile layer's
      xfer_partial method.  */
-  x = target_read_partial (&current_target, TARGET_OBJECT_UNWIND_TABLE, NULL,
-			   buf, 0, len);
 
-  return (int)x;
+  x = target_read_object_alloc (&current_target, TARGET_OBJECT_UNWIND_TABLE,
+				NULL, buf_p);
+
+  return x;
 }
 
 /* Get the kernel unwind table.  */				 
@@ -2484,14 +2485,15 @@ get_kernel_table (unw_word_t ip, unw_dyn
 
   if (!ktab) 
     {
+      gdb_byte *ktab_buf;
       size_t size;
-      size = getunwind_table (NULL, 0);
-      if ((int)size < 0)
-        return -UNW_ENOINFO;
-      ktab_size = size;
-      ktab = xmalloc (ktab_size);
-      getunwind_table (ktab, ktab_size);
-		          
+
+      ktab_size = getunwind_table (&ktab_buf);
+      if (ktab_size <= 0)
+	return -UNW_ENOINFO;
+      else
+	ktab = (struct ia64_table_entry *) ktab_buf;
+
       for (etab = ktab; etab->start_offset; ++etab)
         etab->info_offset += KERNEL_START;
     }
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/linux-nat.c	2006-07-05 12:12:58.000000000 -0400
@@ -2697,7 +2697,8 @@ linux_nat_make_corefile_notes (bfd *obfd
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_object_alloc (&current_target, TARGET_OBJECT_AUXV,
+				       NULL, &auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/procfs.c
===================================================================
--- src.orig/gdb/procfs.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/procfs.c	2006-07-05 12:12:50.000000000 -0400
@@ -6130,7 +6130,8 @@ procfs_make_note_section (bfd *obfd, int
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_object_alloc (&current_target, TARGET_OBJECT_AUXV,
+				       NULL, &auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: src/gdb/sparc-tdep.c
===================================================================
--- src.orig/gdb/sparc-tdep.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/sparc-tdep.c	2006-07-05 12:12:01.000000000 -0400
@@ -158,7 +158,7 @@ sparc_fetch_wcookie (void)
   gdb_byte buf[8];
   int len;
 
-  len = target_read_partial (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
+  len = target_read (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
   if (len == -1)
     return 0;
 
Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/target.c	2006-07-05 12:21:09.000000000 -0400
@@ -1341,7 +1341,7 @@ default_xfer_partial (struct target_ops 
    (inbuf, outbuf)", instead of separate read/write methods, make life
    easier.  */
 
-LONGEST
+static LONGEST
 target_read_partial (struct target_ops *ops,
 		     enum target_object object,
 		     const char *annex, gdb_byte *buf,
@@ -1350,7 +1350,7 @@ target_read_partial (struct target_ops *
   return target_xfer_partial (ops, object, annex, buf, NULL, offset, len);
 }
 
-LONGEST
+static LONGEST
 target_write_partial (struct target_ops *ops,
 		      enum target_object object,
 		      const char *annex, const gdb_byte *buf,
@@ -1373,8 +1373,9 @@ target_read (struct target_ops *ops,
 					  (gdb_byte *) buf + xfered,
 					  offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1395,8 +1396,9 @@ target_write (struct target_ops *ops,
 					   (gdb_byte *) buf + xfered,
 					   offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1404,6 +1406,62 @@ target_write (struct target_ops *ops,
   return len;
 }
 
+/* Perform a full target read of unknown size.  */
+
+LONGEST
+target_read_object_alloc (struct target_ops *ops,
+			  enum target_object object,
+			  const char *annex, gdb_byte **buf_p)
+{
+  size_t buf_alloc, buf_pos;
+  gdb_byte *buf;
+  LONGEST n;
+
+  /* This function does not have a length parameter; it reads the
+     entire OBJECT).  Also, it doesn't support objects fetched partly
+     from one target and partly from another (in a different stratum,
+     e.g. a core file and an executable).  Both reasons make it
+     unsuitable for reading memory.  */
+  gdb_assert (object != TARGET_OBJECT_MEMORY);
+
+  /* Start by reading up to 4K at a time.  The target will throttle
+     this number down if necessary.  */
+  buf_alloc = 4096;
+  buf = xmalloc (buf_alloc);
+  buf_pos = 0;
+  while (1)
+    {
+      n = target_read_partial (ops, object, annex, &buf[buf_pos],
+			       buf_pos, buf_alloc - buf_pos);
+      if (n < 0)
+	{
+	  /* An error occurred.  */
+	  xfree (buf);
+	  return -1;
+	}
+      else if (n == 0)
+	{
+	  /* Read all there was.  */
+	  if (buf_pos == 0)
+	    xfree (buf);
+	  else
+	    *buf_p = buf;
+	  return buf_pos;
+	}
+
+      buf_pos += n;
+
+      /* If the buffer is filling up, expand it.  */
+      if (buf_alloc < buf_pos * 2)
+	{
+	  buf_alloc *= 2;
+	  buf = xrealloc (buf, buf_alloc);
+	}
+
+      QUIT;
+    }
+}
+
 /* Memory transfer methods.  */
 
 void
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/target.h	2006-07-05 12:12:25.000000000 -0400
@@ -180,38 +180,8 @@ extern char *target_signal_to_name (enum
 /* Given a name (SIGHUP, etc.), return its signal.  */
 enum target_signal target_signal_from_name (char *);
 \f
-/* Request the transfer of up to LEN 8-bit bytes of the target's
-   OBJECT.  The OFFSET, for a seekable object, specifies the starting
-   point.  The ANNEX can be used to provide additional data-specific
-   information to the target.
-
-   Return the number of bytes actually transfered, zero when no
-   further transfer is possible, and -1 when the transfer is not
-   supported.
-
-   NOTE: cagney/2003-10-17: The current interface does not support a
-   "retry" mechanism.  Instead it assumes that at least one byte will
-   be transfered on each call.
-
-   NOTE: cagney/2003-10-17: The current interface can lead to
-   fragmented transfers.  Lower target levels should not implement
-   hacks, such as enlarging the transfer, in an attempt to compensate
-   for this.  Instead, the target stack should be extended so that it
-   implements supply/collect methods and a look-aside object cache.
-   With that available, the lowest target can safely and freely "push"
-   data up the stack.
-
-   NOTE: cagney/2003-10-17: Unlike the old query and the memory
-   transfer mechanisms, these methods are explicitly parameterized by
-   the target that it should be applied to.
-
-   NOTE: cagney/2003-10-17: Just like the old query and memory xfer
-   methods, these new methods perform partial transfers.  The only
-   difference is that these new methods thought to include "partial"
-   in the name.  The old code's failure to do this lead to much
-   confusion and duplication of effort as each target object attempted
-   to locally take responsibility for something it didn't have to
-   worry about.  */
+/* Target objects which can be transfered using target_read,
+   target_write, et cetera.  */
 
 enum target_object
 {
@@ -229,17 +199,17 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, TARGET_OBJECT_PROC, ... */
 };
 
-extern LONGEST target_read_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex, gdb_byte *buf,
-				    ULONGEST offset, LONGEST len);
-
-extern LONGEST target_write_partial (struct target_ops *ops,
-				     enum target_object object,
-				     const char *annex, const gdb_byte *buf,
-				     ULONGEST offset, LONGEST len);
+/* Request that OPS transfer up to LEN 8-bit bytes of the target's
+   OBJECT.  The OFFSET, for a seekable object, specifies the
+   starting point.  The ANNEX can be used to provide additional
+   data-specific information to the target.
+
+   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.
+   Unlike the raw to_xfer_partial interface, callers of these
+   functions do not need to retry partial transfers.  */
 
-/* Wrappers to perform the full transfer.  */
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
 			    const char *annex, gdb_byte *buf,
@@ -250,6 +220,22 @@ extern LONGEST target_write (struct targ
 			     const char *annex, const gdb_byte *buf,
 			     ULONGEST offset, LONGEST len);
 
+/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
+   be read using OPS.  The return value will be -1 if the transfer
+   fails or is not supported; 0 if the object is empty; or the length
+   of the object otherwise.  If a positive value is returned, a
+   sufficiently large buffer will be allocated using xmalloc and
+   returned in *BUF_P containing the contents of the object.
+
+   This method should be used for objects sufficiently small to store
+   in a single xmalloc'd buffer, when no fixed bound on the object's
+   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
+   through this function.  */
+
+extern LONGEST target_read_object_alloc (struct target_ops *ops,
+					 enum target_object object,
+					 const char *annex, gdb_byte **buf_p);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -409,9 +395,33 @@ struct target_ops
 					      CORE_ADDR load_module_addr,
 					      CORE_ADDR offset);
 
-    /* Perform partial transfers on OBJECT.  See target_read_partial
-       and target_write_partial for details of each variant.  One, and
-       only one, of readbuf or writebuf must be non-NULL.  */
+    /* Request that OPS transfer up to LEN 8-bit bytes of the target's
+       OBJECT.  The OFFSET, for a seekable object, specifies the
+       starting point.  The ANNEX can be used to provide additional
+       data-specific information to the target.
+
+       Return the number of bytes actually transfered, zero when no
+       further transfer is possible, and -1 when the transfer is not
+       supported.  Return of a positive value smaller than LEN does
+       not indicate the end of the object, only the end of the
+       transfer; higher level code should continue transferring if
+       desired.  This is handled in target.c.
+
+       The interface does not support a "retry" mechanism.  Instead it
+       assumes that at least one byte will be transfered on each
+       successful call.
+
+       NOTE: cagney/2003-10-17: The current interface can lead to
+       fragmented transfers.  Lower target levels should not implement
+       hacks, such as enlarging the transfer, in an attempt to
+       compensate for this.  Instead, the target stack should be
+       extended so that it implements supply/collect methods and a
+       look-aside object cache.  With that available, the lowest
+       target can safely and freely "push" data up the stack.
+
+       See target_read and target_write for more information.  One,
+       and only one, of readbuf or writebuf must be non-NULL.  */
+
     LONGEST (*to_xfer_partial) (struct target_ops *ops,
 				enum target_object object, const char *annex,
 				gdb_byte *readbuf, const gdb_byte *writebuf,
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2006-07-05 12:09:56.000000000 -0400
+++ src/gdb/remote.c	2006-07-05 12:12:01.000000000 -0400
@@ -5147,35 +5147,23 @@ remote_xfer_partial (struct target_ops *
     case TARGET_OBJECT_AUXV:
       if (remote_protocol_packets[PACKET_qPart_auxv].support != PACKET_DISABLE)
 	{
-	  unsigned int total = 0;
-	  while (len > 0)
-	    {
-	      LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
-	      snprintf (rs->buf, get_remote_packet_size (),
-			"qPart:auxv:read::%s,%s",
-			phex_nz (offset, sizeof offset),
-			phex_nz (n, sizeof n));
-	      i = putpkt (rs->buf);
-	      if (i < 0)
-		return total > 0 ? total : i;
-	      rs->buf[0] = '\0';
-	      getpkt (&rs->buf, &rs->buf_size, 0);
-	      if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
-		  != PACKET_OK)
-		return total > 0 ? total : -1;
-	      if (strcmp (rs->buf, "OK") == 0)
-		break;		/* Got EOF indicator.  */
-	      /* Got some data.  */
-	      i = hex2bin (rs->buf, readbuf, len);
-	      if (i > 0)
-		{
-		  readbuf = (void *) ((char *) readbuf + i);
-		  offset += i;
-		  len -= i;
-		  total += i;
-		}
-	    }
-	  return total;
+	  LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
+	  snprintf (rs->buf, get_remote_packet_size (),
+		    "qPart:auxv:read::%s,%s",
+		    phex_nz (offset, sizeof offset),
+		    phex_nz (n, sizeof n));
+	  i = putpkt (rs->buf);
+	  if (i < 0)
+	    return i;
+	  rs->buf[0] = '\0';
+	  getpkt (&rs->buf, &rs->buf_size, 0);
+	  if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
+	      != PACKET_OK)
+	    return -1;
+	  if (strcmp (rs->buf, "OK") == 0)
+	    return 0;		/* Got EOF indicator.  */
+	  /* Got some data.  */
+	  return hex2bin (rs->buf, readbuf, len);
 	}
       return -1;
 

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Mark Kettenis]

> Date: Wed, 5 Jul 2006 15:06:39 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Here is an improved version of my previous patch.  According to
> suggestions I received, I've renamed target_read_whole to the hopefully
> clearer target_read_object_alloc (since it reads an entire object, and
> allocates memory for it).

Hmm, target_read also reads objects (although not necessary complete
objects), but doesn't have "object" in its name.  Perhaps I have an
unhealthy sense for symmetry, but I'd rather have the new function
named target_read_alloc.  It's shorter too ;-).

Otherwise, this really lloks good!

> 2006-07-05  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* target.c (target_read): Stop if target_read_partial returns 0
> 	when some bytes have already been read.
> 	(target_write): Likewise for target_write_partial.
> 	(target_read_partial, target_write_partial): Make static.
> 	(target_read_object_alloc): New.
> 	* target.h: Doc fixes.
> 	(target_read_partial, target_write_partial): Delete prototypes.
> 	(target_read_object_alloc): New prototype.
> 
> 	* auxv.c (target_auxv_read): Delete.
> 	(target_auxv_search, fprint_target_auxv): Use target_read_object_alloc.
> 	* auxv.h (target_auxv_read): Delete prototype.
> 	* avr-tdep.c (avr_io_reg_read_command): Use target_read_object_alloc.
> 	* ia64-tdep.c (getunwind_table, get_kernel_table): Likewise.
> 	* linux-nat.c (linux_nat_make_corefile_notes): Likewise.
> 	* procfs.c (procfs_make_note_section): Likewise.
> 	* remote.c (remote_xfer_partial): Don't loop here.
> 	* sparc-tdep.c (sparc_fetch_wcookie): Use target_read.

Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

[Daniel Jacobowitz]

On Wed, Jul 05, 2006 at 09:34:17PM +0200, Mark Kettenis wrote:
> > Date: Wed, 5 Jul 2006 15:06:39 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > Here is an improved version of my previous patch.  According to
> > suggestions I received, I've renamed target_read_whole to the hopefully
> > clearer target_read_object_alloc (since it reads an entire object, and
> > allocates memory for it).
> 
> Hmm, target_read also reads objects (although not necessary complete
> objects), but doesn't have "object" in its name.  Perhaps I have an
> unhealthy sense for symmetry, but I'd rather have the new function
> named target_read_alloc.  It's shorter too ;-).
> 
> Otherwise, this really lloks good!

All right - checked in with that name then.

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* target.c (target_read): Stop if target_read_partial returns 0
	when some bytes have already been read.
	(target_write): Likewise for target_write_partial.
	(target_read_partial, target_write_partial): Make static.
	(target_read_alloc): New.
	* target.h: Doc fixes.
	(target_read_partial, target_write_partial): Delete prototypes.
	(target_read_alloc): New prototype.

	* auxv.c (target_auxv_read): Delete.
	(target_auxv_search, fprint_target_auxv): Use target_read_alloc.
	* auxv.h (target_auxv_read): Delete prototype.
	* avr-tdep.c (avr_io_reg_read_command): Use target_read_alloc.
	* ia64-tdep.c (getunwind_table, get_kernel_table): Likewise.
	* linux-nat.c (linux_nat_make_corefile_notes): Likewise.
	* procfs.c (procfs_make_note_section): Likewise.
	* remote.c (remote_xfer_partial): Don't loop here.
	* sparc-tdep.c (sparc_fetch_wcookie): Use target_read.

Index: auxv.c
===================================================================
RCS file: /cvs/src/src/gdb/auxv.c,v
retrieving revision 1.5
diff -u -p -r1.5 auxv.c
--- auxv.c	17 Dec 2005 22:33:59 -0000	1.5
+++ auxv.c	12 Jul 2006 18:12:31 -0000
@@ -1,6 +1,6 @@
 /* Auxiliary vector support for GDB, the GNU debugger.
 
-   Copyright (C) 2004 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -76,43 +76,6 @@ procfs_xfer_auxv (struct target_ops *ops
   return n;
 }
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-LONGEST
-target_auxv_read (struct target_ops *ops, gdb_byte **data)
-{
-  size_t auxv_alloc = 512, auxv_pos = 0;
-  gdb_byte *auxv = xmalloc (auxv_alloc);
-  int n;
-
-  while (1)
-    {
-      n = target_read_partial (ops, TARGET_OBJECT_AUXV,
-			       NULL, &auxv[auxv_pos], 0,
-			       auxv_alloc - auxv_pos);
-      if (n <= 0)
-	break;
-      auxv_pos += n;
-      if (auxv_pos < auxv_alloc) /* Read all there was.  */
-	break;
-      gdb_assert (auxv_pos == auxv_alloc);
-      auxv_alloc *= 2;
-      auxv = xrealloc (auxv, auxv_alloc);
-    }
-
-  if (auxv_pos == 0)
-    {
-      xfree (auxv);
-      *data = NULL;
-      return n;
-    }
-
-  *data = auxv;
-  return auxv_pos;
-}
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
@@ -148,7 +111,7 @@ target_auxv_search (struct target_ops *o
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int n = target_auxv_read (ops, &data);
+  LONGEST n = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL, &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
@@ -184,7 +147,8 @@ fprint_target_auxv (struct ui_file *file
 {
   CORE_ADDR type, val;
   gdb_byte *data;
-  int len = target_auxv_read (ops, &data);
+  LONGEST len = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL,
+				   &data);
   gdb_byte *ptr = data;
   int ents = 0;
 
Index: auxv.h
===================================================================
RCS file: /cvs/src/src/gdb/auxv.h,v
retrieving revision 1.3
diff -u -p -r1.3 auxv.h
--- auxv.h	17 Dec 2005 22:33:59 -0000	1.3
+++ auxv.h	12 Jul 2006 18:12:31 -0000
@@ -1,6 +1,6 @@
 /* Auxiliary vector support for GDB, the GNU debugger.
 
-   Copyright (C) 2004 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -31,12 +31,6 @@
 struct target_ops;		/* Forward declaration.  */
 
 
-/* Read all the auxv data into a contiguous xmalloc'd buffer,
-   stored in *DATA.  Return the size in bytes of this data.
-   If zero, there is no data and *DATA is null.
-   if < 0, there was an error and *DATA is null.  */
-extern LONGEST target_auxv_read (struct target_ops *ops, gdb_byte **data);
-
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
Index: avr-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/avr-tdep.c,v
retrieving revision 1.89
diff -u -p -r1.89 avr-tdep.c
--- avr-tdep.c	21 Jan 2006 22:25:07 -0000	1.89
+++ avr-tdep.c	12 Jul 2006 18:12:31 -0000
@@ -1,7 +1,7 @@
 /* Target-dependent code for Atmel AVR, for GDB.
 
    Copyright (C) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004,
-   2005 Free Software Foundation, Inc.
+   2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -1323,35 +1323,22 @@ static void
 avr_io_reg_read_command (char *args, int from_tty)
 {
   LONGEST bufsiz = 0;
-  char buf[400];
+  gdb_byte *buf;
   char query[400];
   char *p;
   unsigned int nreg = 0;
   unsigned int val;
   int i, j, k, step;
 
-  /* Just get the maximum buffer size. */
-  bufsiz = target_read_partial (&current_target, TARGET_OBJECT_AVR,
-				NULL, NULL, 0, 0);
-  if (bufsiz < 0)
-    {
-      fprintf_unfiltered (gdb_stderr,
-			  _("ERR: info io_registers NOT supported "
-			    "by current target\n"));
-      return;
-    }
-  if (bufsiz > sizeof (buf))
-    bufsiz = sizeof (buf);
-
   /* Find out how many io registers the target has. */
-  strcpy (query, "avr.io_reg");
-  target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf, 0,
-		       bufsiz);
+  bufsiz = target_read_alloc (&current_target, TARGET_OBJECT_AVR,
+			      "avr.io_reg", &buf);
 
-  if (strncmp (buf, "", bufsiz) == 0)
+  if (bufsiz <= 0)
     {
       fprintf_unfiltered (gdb_stderr,
-			  _("info io_registers NOT supported by target\n"));
+			  _("ERR: info io_registers NOT supported "
+			    "by current target\n"));
       return;
     }
 
@@ -1359,9 +1346,12 @@ avr_io_reg_read_command (char *args, int
     {
       fprintf_unfiltered (gdb_stderr,
 			  _("Error fetching number of io registers\n"));
+      xfree (buf);
       return;
     }
 
+  xfree (buf);
+
   reinitialize_more_filter ();
 
   printf_unfiltered (_("Target has %u io registers:\n\n"), nreg);
@@ -1377,8 +1367,8 @@ avr_io_reg_read_command (char *args, int
         j = nreg - i;           /* last block is less than 8 registers */
 
       snprintf (query, sizeof (query) - 1, "avr.io_reg:%x,%x", i, j);
-      target_read_partial (&current_target, TARGET_OBJECT_AVR, query, buf,
-			   0, bufsiz);
+      bufsiz = target_read_alloc (&current_target, TARGET_OBJECT_AVR,
+				  query, &buf);
 
       p = buf;
       for (k = i; k < (i + j); k++)
@@ -1393,6 +1383,8 @@ avr_io_reg_read_command (char *args, int
 		break;
 	    }
 	}
+
+      xfree (buf);
     }
 }
 
Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.139
diff -u -p -r1.139 ia64-tdep.c
--- ia64-tdep.c	18 Apr 2006 19:20:06 -0000	1.139
+++ ia64-tdep.c	12 Jul 2006 18:12:32 -0000
@@ -1,7 +1,7 @@
 /* Target-dependent code for the IA-64 for GDB, the GNU debugger.
 
-   Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004, 2005 Free Software
-   Foundation, Inc.
+   Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -2458,8 +2458,8 @@ ia64_access_mem (unw_addr_space_t as,
 }
 
 /* Call low-level function to access the kernel unwind table.  */
-static int
-getunwind_table (void *buf, size_t len)
+static LONGEST
+getunwind_table (gdb_byte **buf_p)
 {
   LONGEST x;
 
@@ -2470,10 +2470,11 @@ getunwind_table (void *buf, size_t len)
      we want to preserve fall back to the running kernel's table, then
      we should find a way to override the corefile layer's
      xfer_partial method.  */
-  x = target_read_partial (&current_target, TARGET_OBJECT_UNWIND_TABLE, NULL,
-			   buf, 0, len);
 
-  return (int)x;
+  x = target_read_alloc (&current_target, TARGET_OBJECT_UNWIND_TABLE,
+			 NULL, buf_p);
+
+  return x;
 }
 
 /* Get the kernel unwind table.  */				 
@@ -2484,14 +2485,15 @@ get_kernel_table (unw_word_t ip, unw_dyn
 
   if (!ktab) 
     {
+      gdb_byte *ktab_buf;
       size_t size;
-      size = getunwind_table (NULL, 0);
-      if ((int)size < 0)
-        return -UNW_ENOINFO;
-      ktab_size = size;
-      ktab = xmalloc (ktab_size);
-      getunwind_table (ktab, ktab_size);
-		          
+
+      ktab_size = getunwind_table (&ktab_buf);
+      if (ktab_size <= 0)
+	return -UNW_ENOINFO;
+      else
+	ktab = (struct ia64_table_entry *) ktab_buf;
+
       for (etab = ktab; etab->start_offset; ++etab)
         etab->info_offset += KERNEL_START;
     }
Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.47
diff -u -p -r1.47 linux-nat.c
--- linux-nat.c	6 May 2006 23:55:36 -0000	1.47
+++ linux-nat.c	12 Jul 2006 18:12:32 -0000
@@ -2697,7 +2697,8 @@ linux_nat_make_corefile_notes (bfd *obfd
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_alloc (&current_target, TARGET_OBJECT_AUXV,
+				NULL, &auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.69
diff -u -p -r1.69 procfs.c
--- procfs.c	18 Apr 2006 19:20:06 -0000	1.69
+++ procfs.c	12 Jul 2006 18:12:33 -0000
@@ -6130,7 +6130,8 @@ procfs_make_note_section (bfd *obfd, int
       note_data = thread_args.note_data;
     }
 
-  auxv_len = target_auxv_read (&current_target, &auxv);
+  auxv_len = target_read_alloc (&current_target, TARGET_OBJECT_AUXV,
+				NULL, &auxv);
   if (auxv_len > 0)
     {
       note_data = elfcore_write_note (obfd, note_data, note_size,
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.219
diff -u -p -r1.219 remote.c
--- remote.c	5 Jul 2006 19:03:47 -0000	1.219
+++ remote.c	12 Jul 2006 18:12:34 -0000
@@ -5147,35 +5147,23 @@ remote_xfer_partial (struct target_ops *
     case TARGET_OBJECT_AUXV:
       if (remote_protocol_packets[PACKET_qPart_auxv].support != PACKET_DISABLE)
 	{
-	  unsigned int total = 0;
-	  while (len > 0)
-	    {
-	      LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
-	      snprintf (rs->buf, get_remote_packet_size (),
-			"qPart:auxv:read::%s,%s",
-			phex_nz (offset, sizeof offset),
-			phex_nz (n, sizeof n));
-	      i = putpkt (rs->buf);
-	      if (i < 0)
-		return total > 0 ? total : i;
-	      rs->buf[0] = '\0';
-	      getpkt (&rs->buf, &rs->buf_size, 0);
-	      if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
-		  != PACKET_OK)
-		return total > 0 ? total : -1;
-	      if (strcmp (rs->buf, "OK") == 0)
-		break;		/* Got EOF indicator.  */
-	      /* Got some data.  */
-	      i = hex2bin (rs->buf, readbuf, len);
-	      if (i > 0)
-		{
-		  readbuf = (void *) ((char *) readbuf + i);
-		  offset += i;
-		  len -= i;
-		  total += i;
-		}
-	    }
-	  return total;
+	  LONGEST n = min ((get_remote_packet_size () - 2) / 2, len);
+	  snprintf (rs->buf, get_remote_packet_size (),
+		    "qPart:auxv:read::%s,%s",
+		    phex_nz (offset, sizeof offset),
+		    phex_nz (n, sizeof n));
+	  i = putpkt (rs->buf);
+	  if (i < 0)
+	    return i;
+	  rs->buf[0] = '\0';
+	  getpkt (&rs->buf, &rs->buf_size, 0);
+	  if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qPart_auxv])
+	      != PACKET_OK)
+	    return -1;
+	  if (strcmp (rs->buf, "OK") == 0)
+	    return 0;		/* Got EOF indicator.  */
+	  /* Got some data.  */
+	  return hex2bin (rs->buf, readbuf, len);
 	}
       return -1;
 
Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.172
diff -u -p -r1.172 sparc-tdep.c
--- sparc-tdep.c	18 Apr 2006 19:20:06 -0000	1.172
+++ sparc-tdep.c	12 Jul 2006 18:12:34 -0000
@@ -158,7 +158,7 @@ sparc_fetch_wcookie (void)
   gdb_byte buf[8];
   int len;
 
-  len = target_read_partial (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
+  len = target_read (ops, TARGET_OBJECT_WCOOKIE, NULL, buf, 0, 8);
   if (len == -1)
     return 0;
 
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.119
diff -u -p -r1.119 target.c
--- target.c	5 May 2006 20:08:45 -0000	1.119
+++ target.c	12 Jul 2006 18:12:34 -0000
@@ -1341,7 +1341,7 @@ default_xfer_partial (struct target_ops 
    (inbuf, outbuf)", instead of separate read/write methods, make life
    easier.  */
 
-LONGEST
+static LONGEST
 target_read_partial (struct target_ops *ops,
 		     enum target_object object,
 		     const char *annex, gdb_byte *buf,
@@ -1350,7 +1350,7 @@ target_read_partial (struct target_ops *
   return target_xfer_partial (ops, object, annex, buf, NULL, offset, len);
 }
 
-LONGEST
+static LONGEST
 target_write_partial (struct target_ops *ops,
 		      enum target_object object,
 		      const char *annex, const gdb_byte *buf,
@@ -1373,8 +1373,9 @@ target_read (struct target_ops *ops,
 					  (gdb_byte *) buf + xfered,
 					  offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1395,8 +1396,9 @@ target_write (struct target_ops *ops,
 					   (gdb_byte *) buf + xfered,
 					   offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
-      if (xfer <= 0)
-	/* Call memory_error?  */
+      if (xfer == 0)
+	return xfered;
+      if (xfer < 0)
 	return -1;
       xfered += xfer;
       QUIT;
@@ -1404,6 +1406,72 @@ target_write (struct target_ops *ops,
   return len;
 }
 
+/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
+   be read using OPS.  The return value will be -1 if the transfer
+   fails or is not supported; 0 if the object is empty; or the length
+   of the object otherwise.  If a positive value is returned, a
+   sufficiently large buffer will be allocated using xmalloc and
+   returned in *BUF_P containing the contents of the object.
+
+   This method should be used for objects sufficiently small to store
+   in a single xmalloc'd buffer, when no fixed bound on the object's
+   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
+   through this function.  */
+
+LONGEST
+target_read_alloc (struct target_ops *ops,
+		   enum target_object object,
+		   const char *annex, gdb_byte **buf_p)
+{
+  size_t buf_alloc, buf_pos;
+  gdb_byte *buf;
+  LONGEST n;
+
+  /* This function does not have a length parameter; it reads the
+     entire OBJECT).  Also, it doesn't support objects fetched partly
+     from one target and partly from another (in a different stratum,
+     e.g. a core file and an executable).  Both reasons make it
+     unsuitable for reading memory.  */
+  gdb_assert (object != TARGET_OBJECT_MEMORY);
+
+  /* Start by reading up to 4K at a time.  The target will throttle
+     this number down if necessary.  */
+  buf_alloc = 4096;
+  buf = xmalloc (buf_alloc);
+  buf_pos = 0;
+  while (1)
+    {
+      n = target_read_partial (ops, object, annex, &buf[buf_pos],
+			       buf_pos, buf_alloc - buf_pos);
+      if (n < 0)
+	{
+	  /* An error occurred.  */
+	  xfree (buf);
+	  return -1;
+	}
+      else if (n == 0)
+	{
+	  /* Read all there was.  */
+	  if (buf_pos == 0)
+	    xfree (buf);
+	  else
+	    *buf_p = buf;
+	  return buf_pos;
+	}
+
+      buf_pos += n;
+
+      /* If the buffer is filling up, expand it.  */
+      if (buf_alloc < buf_pos * 2)
+	{
+	  buf_alloc *= 2;
+	  buf = xrealloc (buf, buf_alloc);
+	}
+
+      QUIT;
+    }
+}
+
 /* Memory transfer methods.  */
 
 void
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.84
diff -u -p -r1.84 target.h
--- target.h	5 Jul 2006 19:03:47 -0000	1.84
+++ target.h	12 Jul 2006 18:12:34 -0000
@@ -180,38 +180,8 @@ extern char *target_signal_to_name (enum
 /* Given a name (SIGHUP, etc.), return its signal.  */
 enum target_signal target_signal_from_name (char *);
 \f
-/* Request the transfer of up to LEN 8-bit bytes of the target's
-   OBJECT.  The OFFSET, for a seekable object, specifies the starting
-   point.  The ANNEX can be used to provide additional data-specific
-   information to the target.
-
-   Return the number of bytes actually transfered, zero when no
-   further transfer is possible, and -1 when the transfer is not
-   supported.
-
-   NOTE: cagney/2003-10-17: The current interface does not support a
-   "retry" mechanism.  Instead it assumes that at least one byte will
-   be transfered on each call.
-
-   NOTE: cagney/2003-10-17: The current interface can lead to
-   fragmented transfers.  Lower target levels should not implement
-   hacks, such as enlarging the transfer, in an attempt to compensate
-   for this.  Instead, the target stack should be extended so that it
-   implements supply/collect methods and a look-aside object cache.
-   With that available, the lowest target can safely and freely "push"
-   data up the stack.
-
-   NOTE: cagney/2003-10-17: Unlike the old query and the memory
-   transfer mechanisms, these methods are explicitly parameterized by
-   the target that it should be applied to.
-
-   NOTE: cagney/2003-10-17: Just like the old query and memory xfer
-   methods, these new methods perform partial transfers.  The only
-   difference is that these new methods thought to include "partial"
-   in the name.  The old code's failure to do this lead to much
-   confusion and duplication of effort as each target object attempted
-   to locally take responsibility for something it didn't have to
-   worry about.  */
+/* Target objects which can be transfered using target_read,
+   target_write, et cetera.  */
 
 enum target_object
 {
@@ -229,17 +199,17 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, TARGET_OBJECT_PROC, ... */
 };
 
-extern LONGEST target_read_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex, gdb_byte *buf,
-				    ULONGEST offset, LONGEST len);
-
-extern LONGEST target_write_partial (struct target_ops *ops,
-				     enum target_object object,
-				     const char *annex, const gdb_byte *buf,
-				     ULONGEST offset, LONGEST len);
+/* Request that OPS transfer up to LEN 8-bit bytes of the target's
+   OBJECT.  The OFFSET, for a seekable object, specifies the
+   starting point.  The ANNEX can be used to provide additional
+   data-specific information to the target.
+
+   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.
+   Unlike the raw to_xfer_partial interface, callers of these
+   functions do not need to retry partial transfers.  */
 
-/* Wrappers to perform the full transfer.  */
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
 			    const char *annex, gdb_byte *buf,
@@ -250,6 +220,22 @@ extern LONGEST target_write (struct targ
 			     const char *annex, const gdb_byte *buf,
 			     ULONGEST offset, LONGEST len);
 
+/* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will
+   be read using OPS.  The return value will be -1 if the transfer
+   fails or is not supported; 0 if the object is empty; or the length
+   of the object otherwise.  If a positive value is returned, a
+   sufficiently large buffer will be allocated using xmalloc and
+   returned in *BUF_P containing the contents of the object.
+
+   This method should be used for objects sufficiently small to store
+   in a single xmalloc'd buffer, when no fixed bound on the object's
+   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
+   through this function.  */
+
+extern LONGEST target_read_alloc (struct target_ops *ops,
+				  enum target_object object,
+				  const char *annex, gdb_byte **buf_p);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -409,9 +395,33 @@ struct target_ops
 					      CORE_ADDR load_module_addr,
 					      CORE_ADDR offset);
 
-    /* Perform partial transfers on OBJECT.  See target_read_partial
-       and target_write_partial for details of each variant.  One, and
-       only one, of readbuf or writebuf must be non-NULL.  */
+    /* Request that OPS transfer up to LEN 8-bit bytes of the target's
+       OBJECT.  The OFFSET, for a seekable object, specifies the
+       starting point.  The ANNEX can be used to provide additional
+       data-specific information to the target.
+
+       Return the number of bytes actually transfered, zero when no
+       further transfer is possible, and -1 when the transfer is not
+       supported.  Return of a positive value smaller than LEN does
+       not indicate the end of the object, only the end of the
+       transfer; higher level code should continue transferring if
+       desired.  This is handled in target.c.
+
+       The interface does not support a "retry" mechanism.  Instead it
+       assumes that at least one byte will be transfered on each
+       successful call.
+
+       NOTE: cagney/2003-10-17: The current interface can lead to
+       fragmented transfers.  Lower target levels should not implement
+       hacks, such as enlarging the transfer, in an attempt to
+       compensate for this.  Instead, the target stack should be
+       extended so that it implements supply/collect methods and a
+       look-aside object cache.  With that available, the lowest
+       target can safely and freely "push" data up the stack.
+
+       See target_read and target_write for more information.  One,
+       and only one, of readbuf or writebuf must be non-NULL.  */
+
     LONGEST (*to_xfer_partial) (struct target_ops *ops,
 				enum target_object object, const char *annex,
 				gdb_byte *readbuf, const gdb_byte *writebuf,