[patch/rfc] to_read/write_partial -> to_xfer_partial

[patch/rfc] to_read/write_partial -> to_xfer_partial

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

Hello,

Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html
> Having taken the change to this point, I'm now wondering if the read/write partial methods should be merged into:
> to_xfer_partial (targ, object, annex,
> offset, len,
> readbuf, writebuf)
> as that would make migrating existing targets easier.

Having implemented bfd-target and remote-target versions 
to_read/write_partial, I think this switch is going to make life easier. 
  The attached converts the code.

Comments?

I'll also update the bfd-target I've posted.

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 12577 bytes --]

2003-10-27  Andrew Cagney  <cagney@redhat.com>

	* target.h (struct target_ops): Replace "to_read_partial" and
	"to_write_partial" with "to_xfer_partial".  Update comments.
	* target.c (debug_to_write_partial): Delete function.
	(debug_to_xfer_partial): Replace debug_to_read_partial.
	(add_target, update_current_target, setup_target_debug): Set
	"to_xfer_partial" instead of "to_read_partial" and
	"to_write_partial".
	(default_xfer_partial): Replace "default_write_partial".
	(default_read_partial): Delete.
	(target_read_partial, target_write_partial): Call
	"to_xfer_partial".
	* remote.c (init_remote_ops): Set "to_xfer_partial".
	(init_remote_async_ops): Ditto.
	(remote_xfer_partial): Replace "remote_read_partial".

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.120
diff -u -r1.120 remote.c
--- remote.c	24 Oct 2003 17:37:03 -0000	1.120
+++ remote.c	27 Oct 2003 20:16:28 -0000
@@ -5102,8 +5102,8 @@
 }
 
 static LONGEST
-remote_read_partial (struct target_ops *ops, enum target_object object,
-		     const char *annex, void *buf,
+remote_xfer_partial (struct target_ops *ops, enum target_object object,
+		     const char *annex, const void *writebuf, void *readbuf,
 		     ULONGEST offset, LONGEST len)
 {
   struct remote_state *rs = get_remote_state ();
@@ -5112,6 +5112,10 @@
   char *p2 = &buf2[0];
   char query_type;
 
+  /* Only handle reads.  */
+  if (writebuf != NULL || readbuf == NULL)
+    return -1;
+
   /* Map pre-existing objects onto letters.  DO NOT do this for new
      objects!!!  Instead specify new query packets.  */
   switch (object)
@@ -5126,9 +5130,9 @@
       return -1;
     }
 
-  /* Note: a zero BUF, OFFSET and LEN can be used to query the minimum
+  /* Note: a zero OFFSET and LEN can be used to query the minimum
      buffer size.  */
-  if (buf == NULL && offset == 0 && len == 0)
+  if (offset == 0 && len == 0)
     return (rs->remote_packet_size);
   /* Minimum outbuf size is (rs->remote_packet_size) - if bufsiz is
      not large enough let the caller.  */
@@ -5141,7 +5145,7 @@
     error ("remote query is only available after target open");
 
   gdb_assert (annex != NULL);
-  gdb_assert (buf != NULL);
+  gdb_assert (readbuf != NULL);
 
   *p2++ = 'q';
   *p2++ = query_type;
@@ -5165,9 +5169,9 @@
   if (i < 0)
     return i;
 
-  getpkt (buf, len, 0);
+  getpkt (readbuf, len, 0);
 
-  return strlen (buf);
+  return strlen (readbuf);
 }
 
 static void
@@ -5445,7 +5449,7 @@
   remote_ops.to_pid_to_str = remote_pid_to_str;
   remote_ops.to_extra_thread_info = remote_threads_extra_info;
   remote_ops.to_stop = remote_stop;
-  remote_ops.to_read_partial = remote_read_partial;
+  remote_ops.to_xfer_partial = remote_xfer_partial;
   remote_ops.to_rcmd = remote_rcmd;
   remote_ops.to_stratum = process_stratum;
   remote_ops.to_has_all_memory = 1;
@@ -5965,7 +5969,7 @@
   remote_async_ops.to_pid_to_str = remote_pid_to_str;
   remote_async_ops.to_extra_thread_info = remote_threads_extra_info;
   remote_async_ops.to_stop = remote_stop;
-  remote_async_ops.to_read_partial = remote_read_partial;
+  remote_async_ops.to_xfer_partial = remote_xfer_partial;
   remote_async_ops.to_rcmd = remote_rcmd;
   remote_async_ops.to_stratum = process_stratum;
   remote_async_ops.to_has_all_memory = 1;
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.63
diff -u -r1.63 target.c
--- target.c	24 Oct 2003 20:24:06 -0000	1.63
+++ target.c	27 Oct 2003 20:16:28 -0000
@@ -73,14 +73,11 @@
 
 static void normal_target_post_startup_inferior (ptid_t ptid);
 
-static LONGEST default_read_partial (struct target_ops *ops,
+static LONGEST default_xfer_partial (struct target_ops *ops,
 				     enum target_object object,
-				     const char *annex, void *buf,
-				     ULONGEST offset, LONGEST len);
-static LONGEST default_write_partial (struct target_ops *ops,
-				      enum target_object object,
-				      const char *annex, const void *buf,
-				      ULONGEST offset, LONGEST len);
+				     const char *annex, const void *writebuf,
+				     void *readbuf, ULONGEST offset,
+				     LONGEST len);
 
 /* Transfer LEN bytes between target address MEMADDR and GDB address
    MYADDR.  Returns 0 for success, errno code for failure (which
@@ -223,8 +220,7 @@
 add_target (struct target_ops *t)
 {
   /* Provide default values for all "must have" methods.  */
-  t->to_read_partial = default_read_partial;
-  t->to_write_partial = default_write_partial;
+  t->to_xfer_partial = default_xfer_partial;
 
   if (!target_structs)
     {
@@ -433,8 +429,7 @@
       INHERIT (to_pid_to_str, t);
       INHERIT (to_extra_thread_info, t);
       INHERIT (to_stop, t);
-      /* Do not inherit to_read_partial.  */
-      /* Do not inherit to_write_partial.  */
+      /* Do not inherit to_xfer_partial.  */
       INHERIT (to_rcmd, t);
       INHERIT (to_enable_exception_callback, t);
       INHERIT (to_get_current_exception_event, t);
@@ -616,8 +611,7 @@
   de_fault (to_stop, 
 	    (void (*) (void)) 
 	    target_ignore);
-  current_target.to_read_partial = default_read_partial;
-  current_target.to_write_partial = default_write_partial;
+  current_target.to_xfer_partial = default_xfer_partial;
   de_fault (to_rcmd, 
 	    (void (*) (char *, struct ui_file *)) 
 	    tcomplain);
@@ -1079,55 +1073,30 @@
 /* More generic transfers.  */
 
 static LONGEST
-default_read_partial (struct target_ops *ops,
+default_xfer_partial (struct target_ops *ops,
 		      enum target_object object,
-		      const char *annex, void *buf,
-		      ULONGEST offset, LONGEST len)
-{
-  if (object == TARGET_OBJECT_MEMORY
-      && ops->to_xfer_memory != NULL)
-    /* If available, fall back to the target's "to_xfer_memory"
-       method.  */
-    {
-      int xfered;
-      errno = 0;
-      xfered = ops->to_xfer_memory (offset, buf, len, 0/*read*/, NULL, ops);
-      if (xfered > 0)
-	return xfered;
-      else if (xfered == 0 && errno == 0)
-	/* "to_xfer_memory" uses 0, cross checked against ERRNO as one
-           indication of an error.  */
-	return 0;
-      else
-	return -1;
-    }
-  else if (ops->beneath != NULL)
-    return target_read_partial (ops->beneath, object, annex, buf, offset, len);
-  else
-    return -1;
-}
-
-static LONGEST
-default_write_partial (struct target_ops *ops,
-		       enum target_object object,
-		       const char *annex, const void *buf,
-		       ULONGEST offset, LONGEST len)
+		      const char *annex, const void *writebuf,
+		      void *readbuf, ULONGEST offset, LONGEST len)
 {
   if (object == TARGET_OBJECT_MEMORY
       && ops->to_xfer_memory != NULL)
     /* If available, fall back to the target's "to_xfer_memory"
        method.  */
     {
-      int xfered;
+      int xfered = -1;
       errno = 0;
-      {
-	void *buffer = xmalloc (len);
-	struct cleanup *cleanup = make_cleanup (xfree, buffer);
-	memcpy (buffer, buf, len);
-	xfered = ops->to_xfer_memory (offset, buffer, len, 1/*write*/, NULL,
+      if (writebuf != NULL)
+	{
+	  void *buffer = xmalloc (len);
+	  struct cleanup *cleanup = make_cleanup (xfree, buffer);
+	  memcpy (buffer, writebuf, len);
+	  xfered = ops->to_xfer_memory (offset, buffer, len, 1/*write*/, NULL,
+					ops);
+	  do_cleanups (cleanup);
+	}
+      if (readbuf != NULL)
+	xfered = ops->to_xfer_memory (offset, readbuf, len, 0/*read*/, NULL,
 				      ops);
-	do_cleanups (cleanup);
-      }
       if (xfered > 0)
 	return xfered;
       else if (xfered == 0 && errno == 0)
@@ -1138,8 +1107,8 @@
 	return -1;
     }
   else if (ops->beneath != NULL)
-    return target_write_partial (ops->beneath, object, annex, buf, offset,
-				 len);
+    return ops->beneath->to_xfer_partial (ops->beneath, object, annex,
+					  writebuf, readbuf, offset, len);
   else
     return -1;
 }
@@ -1156,8 +1125,8 @@
 		     const char *annex, void *buf,
 		     ULONGEST offset, LONGEST len)
 {
-  gdb_assert (ops->to_read_partial != NULL);
-  return ops->to_read_partial (ops, object, annex, buf, offset, len);
+  gdb_assert (ops->to_xfer_partial != NULL);
+  return ops->to_xfer_partial (ops, object, annex, NULL, buf, offset, len);
 }
 
 LONGEST
@@ -1166,8 +1135,8 @@
 		      const char *annex, const void *buf,
 		      ULONGEST offset, LONGEST len)
 {
-  gdb_assert (ops->to_write_partial != NULL);
-  return ops->to_write_partial (ops, object, annex, buf, offset, len);
+  gdb_assert (ops->to_xfer_partial != NULL);
+  return ops->to_xfer_partial (ops, object, annex, buf, NULL, offset, len);
 }
 
 /* Wrappers to perform the full transfer.  */
@@ -2318,40 +2287,20 @@
 }
 
 static LONGEST
-debug_to_read_partial (struct target_ops *ops,
+debug_to_xfer_partial (struct target_ops *ops,
 		       enum target_object object,
-		       const char *annex, void *buf,
-		       ULONGEST offset, LONGEST len)
-{
-  LONGEST retval;
-
-  retval = target_read_partial (&debug_target, object, annex, buf, offset,
-				len);
-
-  fprintf_unfiltered (gdb_stdlog,
-		      "target_read_partial (%d, %s, 0x%lx,  0x%s, %s) = %s\n",
-		      (int) object, (annex ? annex : "(null)"),
-		      (long) buf, paddr_nz (offset),
-		      paddr_d (len), paddr_d (retval));
-
-  return retval;
-}
-
-static LONGEST
-debug_to_write_partial (struct target_ops *ops,
-			enum target_object object,
-			const char *annex, const void *buf,
-			ULONGEST offset, LONGEST len)
+		       const char *annex, const void *writebuf,
+		       void *readbuf, ULONGEST offset, LONGEST len)
 {
   LONGEST retval;
 
-  retval = target_write_partial (&debug_target, object, annex, buf, offset,
-				len);
+  retval = debug_target.to_xfer_partial (&debug_target, object, annex,
+					 writebuf, readbuf, offset, len);
 
   fprintf_unfiltered (gdb_stdlog,
-		      "target_write_partial (%d, %s, 0x%lx,  0x%s, %s) = %s\n",
+		      "target_xfer_partial (%d, %s, 0x%lx,  0x%lx,  0x%s, %s) = %s\n",
 		      (int) object, (annex ? annex : "(null)"),
-		      (long) buf, paddr_nz (offset),
+		      (long) writebuf, (long) readbuf, paddr_nz (offset),
 		      paddr_d (len), paddr_d (retval));
 
   return retval;
@@ -2454,8 +2403,7 @@
   current_target.to_thread_alive = debug_to_thread_alive;
   current_target.to_find_new_threads = debug_to_find_new_threads;
   current_target.to_stop = debug_to_stop;
-  current_target.to_read_partial = debug_to_read_partial;
-  current_target.to_write_partial = debug_to_write_partial;
+  current_target.to_xfer_partial = debug_to_xfer_partial;
   current_target.to_rcmd = debug_to_rcmd;
   current_target.to_enable_exception_callback = debug_to_enable_exception_callback;
   current_target.to_get_current_exception_event = debug_to_get_current_exception_event;
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.49
diff -u -r1.49 target.h
--- target.h	24 Oct 2003 20:24:06 -0000	1.49
+++ target.h	27 Oct 2003 20:16:28 -0000
@@ -211,10 +211,10 @@
    to locally take responsibility for something it didn't have to
    worry about.
 
-   NOTE: cagney/2003-10-17: For backward compatibility with the
-   "target_query" method that this replaced, when BUF, OFFSET and LEN
-   are NULL/zero, return the "minimum" buffer size.  See "remote.c"
-   for further information.  */
+   NOTE: cagney/2003-10-17: With a TARGET_OBJECT_KOD object, for
+   backward compatibility with the "target_query" method that this
+   replaced, when OFFSET and LEN are both zero, return the "minimum"
+   buffer size.  See "remote.c" for further information.  */
 
 enum target_object
 {
@@ -404,15 +404,13 @@
 					      struct objfile *objfile,
 					      CORE_ADDR offset);
 
-    /* See above.  */
-    LONGEST (*to_read_partial) (struct target_ops *ops,
+    /* 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.  */
+    LONGEST (*to_xfer_partial) (struct target_ops *ops,
 				enum target_object object,
-				const char *annex, void *buf, 
-				ULONGEST offset, LONGEST len);
-    LONGEST (*to_write_partial) (struct target_ops *ops,
-				 enum target_object object,
-				 const char *annex, const void *buf,
-				 ULONGEST offset, LONGEST len);
+				const char *annex, const void *writebuf,
+				void *readbuf, ULONGEST offset, LONGEST len);
 
     int to_magic;
     /* Need sub-structure for target machine related rather than comm related?

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Kevin Buettner]

On Oct 27,  3:25pm, Andrew Cagney wrote:

> Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html
> > Having taken the change to this point, I'm now wondering if the read/write partial methods should be merged into:
> > to_xfer_partial (targ, object, annex,
> > offset, len,
> > readbuf, writebuf)
> > as that would make migrating existing targets easier.
> 
> Having implemented bfd-target and remote-target versions 
> to_read/write_partial, I think this switch is going to make life easier. 

Could you offer a few more details on why you think that merging the
read/write methods into a single xfer method will make it easier to
migrate existing targets?

Kevin

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Mark Kettenis]

   Date: Mon, 27 Oct 2003 13:46:31 -0700
   From: Kevin Buettner <kevinb@redhat.com>

   On Oct 27,  3:25pm, Andrew Cagney wrote:

   > Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html

   > > Having taken the change to this point, I'm now wondering if the
   > > read/write partial methods should be merged into:
   > > to_xfer_partial (targ, object, annex,
   > > offset, len,
   > > readbuf, writebuf)
   > > as that would make migrating existing targets easier.
   > 
   > Having implemented bfd-target and remote-target versions 
   > to_read/write_partial, I think this switch is going to make life easier. 

   Could you offer a few more details on why you think that merging the
   read/write methods into a single xfer method will make it easier to
   migrate existing targets?

Yes please.  Personally I find xfer much more confusing than seperate
read/write methods.

Mark

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Andrew Cagney]

> On Oct 27,  3:25pm, Andrew Cagney wrote:
> 
> 
>> Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html
> 
>> > Having taken the change to this point, I'm now wondering if the read/write partial methods should be merged into:
>> > to_xfer_partial (targ, object, annex,
>> > offset, len,
>> > readbuf, writebuf)
>> > as that would make migrating existing targets easier.
> 
>> 
>> Having implemented bfd-target and remote-target versions 
>> to_read/write_partial, I think this switch is going to make life easier. 
> 
> 
> Could you offer a few more details on why you think that merging the
> read/write methods into a single xfer method will make it easier to
> migrate existing targets?

There's a tradeoff.  You'll notice that I started out with separate 
asthetically pleasing read/write methods, but eventually decided the 
cost was too high.

- the existing targets implement a memory centric "xfer".  Its going to 
be easier [for me] to convert that code to this new xfer variant.

- both the read and write paths use identical buffer overflow logic, and 
its that logic which contains the nasty edge cases and consequent bugs. 
  Compare target_read and target_write:

   LONGEST xfered = 0;
   while (xfered < len)
     {
       LONGEST xfer = target_read_partial (ops, object, annex,
                                           (bfd_byte *) buf + xfered,
                                           offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
       if (xfer <= 0)
         /* Call memory_error?  */
         return -1;
       xfered += xfer;
       QUIT;
     }
   return len;

and

   LONGEST xfered = 0;
   while (xfered < len)
     {
       LONGEST xfer = target_write_partial (ops, object, annex,
                                            (bfd_byte *) buf + xfered,
                                            offset + xfered, len - xfered);
       /* Call an observer, notifying them of the xfer progress?  */
       if (xfer <= 0)
         /* Call memory_error?  */
         return -1;
       xfered += xfer;
       QUIT;
     }
   return len;

In testing this interface I had to fix similar but not identical bugs in 
both paths.

- If this were OO, I'd be doing it differently :-/

- unlike the memory xfer method, this one takes an explicit read and 
write buffer - the problem of casting away "const" is avoided.

- the core code doesn't need to be aware of this target internal detail, 
continuing to use more sane read/write methods.

enjoy,
Andrew

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Kevin Buettner]

On Oct 27, 11:26am, Andrew Cagney wrote:
> Subject: Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

> >> Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html
> > 
> >> > Having taken the change to this point, I'm now wondering if the read/write partial methods should be merged into:
> >> > to_xfer_partial (targ, object, annex,
> >> > offset, len,
> >> > readbuf, writebuf)
> >> > as that would make migrating existing targets easier.
> > 
> >> Having implemented bfd-target and remote-target versions 
> >> to_read/write_partial, I think this switch is going to make life easier. 
> > 
> > Could you offer a few more details on why you think that merging the
> > read/write methods into a single xfer method will make it easier to
> > migrate existing targets?
> 
> There's a tradeoff.  You'll notice that I started out with separate 
> asthetically pleasing read/write methods, but eventually decided the 
> cost was too high.
> 
> - the existing targets implement a memory centric "xfer".  Its going to 
> be easier [for me] to convert that code to this new xfer variant.
> 
> - both the read and write paths use identical buffer overflow logic, and 
> its that logic which contains the nasty edge cases and consequent bugs. 

Is there any reason you can't keep the methods separate, but use a
common underlying "xfer" implementation?  (Which, I think, is how
it's presently done.)  In the past, when trying to figure out how an
xfer implementation worked, I recall looking at how the read/write
stubs called the xfer function.

Kevin

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Andrew Cagney]

> There's a tradeoff.  You'll notice that I started out with separate 
>> asthetically pleasing read/write methods, but eventually decided the 
>> cost was too high.
>> 
>> - the existing targets implement a memory centric "xfer".  Its going to 
>> be easier [for me] to convert that code to this new xfer variant.
>> 
>> - both the read and write paths use identical buffer overflow logic, and 
>> its that logic which contains the nasty edge cases and consequent bugs. 
> 
> 
> Is there any reason you can't keep the methods separate, but use a
> common underlying "xfer" implementation?  (Which, I think, is how
> it's presently done.)  In the past, when trying to figure out how an
> xfer implementation worked, I recall looking at how the read/write
> stubs called the xfer function.

Sorry, I'm lost.

How is which presently done?  The patch retains the existing target 
read/write partial interfaces but uses an underlying to_xfer_partial 
vector method.  This is how the existing to_xfer_memory is implemented.

Andrew

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Kevin Buettner]

On Oct 27,  7:02pm, Andrew Cagney wrote:

> > There's a tradeoff.  You'll notice that I started out with separate 
> >> asthetically pleasing read/write methods, but eventually decided the 
> >> cost was too high.
> >> 
> >> - the existing targets implement a memory centric "xfer".  Its going to 
> >> be easier [for me] to convert that code to this new xfer variant.
> >> 
> >> - both the read and write paths use identical buffer overflow logic, and 
> >> its that logic which contains the nasty edge cases and consequent bugs. 
> > 
> > 
> > Is there any reason you can't keep the methods separate, but use a
> > common underlying "xfer" implementation?  (Which, I think, is how
> > it's presently done.)  In the past, when trying to figure out how an
> > xfer implementation worked, I recall looking at how the read/write
> > stubs called the xfer function.
> 
> Sorry, I'm lost.
> 
> How is which presently done?  The patch retains the existing target 
> read/write partial interfaces but uses an underlying to_xfer_partial 
> vector method.  This is how the existing to_xfer_memory is implemented.

My recollection of how the code was structured was faulty.  I withdraw
my objections.

Kevin

Re: [patch/rfc] to_read/write_partial -> to_xfer_partial

[Andrew Cagney]

> Hello,
> 
> Per: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00641.html
> Having taken the change to this point, I'm now wondering if the read/write partial methods should be merged into:
> to_xfer_partial (targ, object, annex,
> offset, len,
> readbuf, writebuf)
> as that would make migrating existing targets easier.
> 
> Having implemented bfd-target and remote-target versions to_read/write_partial, I think this switch is going to make life easier.  The attached converts the code.
> 
> Comments?
> 
> I'll also update the bfd-target I've posted.

I've checked this in.

Andrew


> 2003-10-27  Andrew Cagney  <cagney@redhat.com>
> 
> 	* target.h (struct target_ops): Replace "to_read_partial" and
> 	"to_write_partial" with "to_xfer_partial".  Update comments.
> 	* target.c (debug_to_write_partial): Delete function.
> 	(debug_to_xfer_partial): Replace debug_to_read_partial.
> 	(add_target, update_current_target, setup_target_debug): Set
> 	"to_xfer_partial" instead of "to_read_partial" and
> 	"to_write_partial".
> 	(default_xfer_partial): Replace "default_write_partial".
> 	(default_read_partial): Delete.
> 	(target_read_partial, target_write_partial): Call
> 	"to_xfer_partial".
> 	* remote.c (init_remote_ops): Set "to_xfer_partial".
> 	(init_remote_async_ops): Ditto.
> 	(remote_xfer_partial): Replace "remote_read_partial".
>