[PATCH] Cleanup target memory reading

[PATCH] Cleanup target memory reading

[Vladimir Prus]

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


Hi,
at the moment, pretty much every read of target memory goes via 
target_read_memory, and then via xfer_using_stratum.

The only exception is the get_target_memory_unsigned function, which calls 
target_read function. It's the only use of 'target_read'.

The attached patch removes target_read, and makes get_target_memory_unsigned 
use target_read_memory.

OK?

- Volodya

2006-06-28  Vladimir Prus  <vladimir@codesourcery.com>

            * target.h (target_read): Remove
            (get_target_memory): Remove.
            * target.c (target_read): Remove
            (get_target_memory): Remove.
            (get_target_memory_unsigned): Use target_read_memory.

[-- Attachment #2: target_read.diff --]
[-- Type: text/x-diff, Size: 3065 bytes --]

=== target.c
==================================================================
--- target.c	(revision 16)
+++ target.c	(revision 18)
@@ -1359,30 +1359,7 @@
   return target_xfer_partial (ops, object, annex, NULL, buf, offset, len);
 }
 
-/* Wrappers to perform the full transfer.  */
 LONGEST
-target_read (struct target_ops *ops,
-	     enum target_object object,
-	     const char *annex, gdb_byte *buf,
-	     ULONGEST offset, LONGEST len)
-{
-  LONGEST xfered = 0;
-  while (xfered < len)
-    {
-      LONGEST xfer = target_read_partial (ops, object, annex,
-					  (gdb_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;
-}
-
-LONGEST
 target_write (struct target_ops *ops,
 	      enum target_object object,
 	      const char *annex, const gdb_byte *buf,
@@ -1404,25 +1381,13 @@
   return len;
 }
 
-/* Memory transfer methods.  */
-
-void
-get_target_memory (struct target_ops *ops, CORE_ADDR addr, gdb_byte *buf,
-		   LONGEST len)
-{
-  if (target_read (ops, TARGET_OBJECT_MEMORY, NULL, buf, addr, len)
-      != len)
-    memory_error (EIO, addr);
-}
-
 ULONGEST
-get_target_memory_unsigned (struct target_ops *ops,
-			    CORE_ADDR addr, int len)
+get_target_memory_unsigned (CORE_ADDR addr, int len)
 {
   gdb_byte buf[sizeof (ULONGEST)];
 
   gdb_assert (len <= sizeof (buf));
-  get_target_memory (ops, addr, buf, len);
+  target_read_memory (addr, buf, len);
   return extract_unsigned_integer (buf, len);
 }
 
=== target.h
==================================================================
--- target.h	(revision 16)
+++ target.h	(revision 18)
@@ -247,27 +247,19 @@
 				     ULONGEST offset, LONGEST len);
 
 /* Wrappers to perform the full transfer.  */
-extern LONGEST target_read (struct target_ops *ops,
-			    enum target_object object,
-			    const char *annex, gdb_byte *buf,
-			    ULONGEST offset, LONGEST len);
 
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,
 			     const char *annex, const gdb_byte *buf,
 			     ULONGEST offset, LONGEST len);
 
-/* Wrappers to target read/write that perform memory transfers.  They
-   throw an error if the memory transfer fails.
+/* Read an unsigned value of LEN bytes from ADDR.
+   Endianess differences between host and target are handled automatically.
 
-   NOTE: cagney/2003-10-23: The naming schema is lifted from
-   "frame.h".  The parameter order is lifted from get_frame_memory,
-   which in turn lifted it from read_memory.  */
+   Throws an error is unsuccessfull.
 
-extern void get_target_memory (struct target_ops *ops, CORE_ADDR addr,
-			       gdb_byte *buf, LONGEST len);
-extern ULONGEST get_target_memory_unsigned (struct target_ops *ops,
-					    CORE_ADDR addr, int len);
+*/
+extern ULONGEST get_target_memory_unsigned (CORE_ADDR addr, int len);
 \f
 
 /* If certain kinds of activity happen, target_wait should perform

Re: [PATCH] Cleanup target memory reading

[Mark Kettenis]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Wed, 28 Jun 2006 11:55:57 +0400
> 
> Hi,
> at the moment, pretty much every read of target memory goes via 
> target_read_memory, and then via xfer_using_stratum.
> 
> The only exception is the get_target_memory_unsigned function, which calls 
> target_read function. It's the only use of 'target_read'.
> 
> The attached patch removes target_read, and makes get_target_memory_unsigned 
> use target_read_memory.
> 
> OK?

Shouldn't we "fix" target_read() such that it uses
target_read_partial() instead?

Re: [PATCH] Cleanup target memory reading

[Daniel Jacobowitz]

On Wed, Jun 28, 2006 at 11:30:37PM +0200, Mark Kettenis wrote:
> > From: Vladimir Prus <ghost@cs.msu.su>
> > Date: Wed, 28 Jun 2006 11:55:57 +0400
> > 
> > Hi,
> > at the moment, pretty much every read of target memory goes via 
> > target_read_memory, and then via xfer_using_stratum.
> > 
> > The only exception is the get_target_memory_unsigned function, which calls 
> > target_read function. It's the only use of 'target_read'.
> > 
> > The attached patch removes target_read, and makes get_target_memory_unsigned 
> > use target_read_memory.
> > 
> > OK?
> 
> Shouldn't we "fix" target_read() such that it uses
> target_read_partial() instead?

/* Wrappers to perform the full transfer.  */
LONGEST
target_read (struct target_ops *ops,
             enum target_object object,
             const char *annex, gdb_byte *buf,
             ULONGEST offset, LONGEST len)
{
  LONGEST xfered = 0;
  while (xfered < len)
    {
      LONGEST xfer = target_read_partial (ops, object, annex,
                                          (gdb_byte *) buf + xfered,
                                          offset + xfered, len - xfered);

It already does.  It's just not an operation that is often useful.

We've got four, if you include my pending (pending rewrite, that is)
patch:

  - target_read_partial.  This is now an implementation detail.  It
reads some bytes.  Who knows how many.
  - target_read.  It reads a fixed, requested number of bytes.
  - xfer_using_stratum.  Similar, but allow the request to be split and
parts handled by different strata.  This is always the right choice for
memory, at least for consistency.
  - target_read_object_alloc, which is my current thinking of a name
for what was target_read_whole in my patch.  This is the basically
always appropriate choice to fetch an "object" other than memory, since
we don't have any way to find out what size it is otherwise, so we
can't request a particular size.

I don't want that last one to use the second one, because it's prone to
doing this:

  - target_read_object_alloc
    - target_read 512 bytes
      - target_read_partial gets 500
      - Second target_read_partial for 12 bytes
    - Another target_read

And we'd much rather group them in larger batches than twelve byte
reads.

I think Vlad's basically garbage collecting target_read, since #3 and
#4 from my list cover all the needs we've got right now.  Does this
seem reasonable?  I think we've got too many ways to do this, and could
do with fewer.

I'd even suggest a pass over the other memory reading functions,
pruning.  For instance, the ones that take a frame.  We know why they
were added, and it's in theory a good plan to associate memory reads
with frames, but it's not going to be completed any time in the
foreseeable future.  We've got lots of what the GCC folks called an
incomplete transition when they were ranting about internal
cleanliness.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Cleanup target memory reading

[Vladimir Prus]

Daniel Jacobowitz wrote:

>> Shouldn't we "fix" target_read() such that it uses
>> target_read_partial() instead?

> It already does.  It's just not an operation that is often useful.
> 
> We've got four, if you include my pending (pending rewrite, that is)
> patch:
> 
>   - target_read_partial.  This is now an implementation detail.  It
> reads some bytes.  Who knows how many.
>   - target_read.  It reads a fixed, requested number of bytes.
>   - xfer_using_stratum.  Similar, but allow the request to be split and
> parts handled by different strata.  This is always the right choice for
> memory, at least for consistency.
>   - target_read_object_alloc, which is my current thinking of a name
> for what was target_read_whole in my patch.  This is the basically
> always appropriate choice to fetch an "object" other than memory, since
> we don't have any way to find out what size it is otherwise, so we
> can't request a particular size.
> 
> I don't want that last one to use the second one, because it's prone to
> doing this:
> 
>   - target_read_object_alloc
>     - target_read 512 bytes
>       - target_read_partial gets 500
>       - Second target_read_partial for 12 bytes
>     - Another target_read
> 
> And we'd much rather group them in larger batches than twelve byte
> reads.
> 
> I think Vlad's basically garbage collecting target_read, since #3 and
> #4 from my list cover all the needs we've got right now.  Does this
> seem reasonable?  I think we've got too many ways to do this, and could
> do with fewer.

Yea, garbage collecting is precisely what I do here. The target_read is not
very useful to reading memory, since it's ignores strata, and it's not very
convenient for reading objects, since your upcoming
target_read_object_alloc does not require a caller to allocate buffer of
unknown size in advance.

If fact, I wanted to kill target_write as well, but won't do this until
either you and I implement qXfer write operation.

> I'd even suggest a pass over the other memory reading functions,
> pruning.  For instance, the ones that take a frame.  We know why they
> were added, and it's in theory a good plan to associate memory reads
> with frames, but it's not going to be completed any time in the
> foreseeable future.  We've got lots of what the GCC folks called an
> incomplete transition when they were ranting about internal
> cleanliness.

I can do a second look, it this seems reasonable.

- Volodya

Re: [PATCH] Cleanup target memory reading

[Daniel Jacobowitz]

On Wed, Jun 28, 2006 at 11:55:57AM +0400, Vladimir Prus wrote:
> 
> Hi,
> at the moment, pretty much every read of target memory goes via 
> target_read_memory, and then via xfer_using_stratum.
> 
> The only exception is the get_target_memory_unsigned function, which calls 
> target_read function. It's the only use of 'target_read'.
> 
> The attached patch removes target_read, and makes get_target_memory_unsigned 
> use target_read_memory.
> 
> OK?

Since this was posted I added a use of target_read in the SPARC target
:-(  Let's leave it for now.

> 2006-06-28  Vladimir Prus  <vladimir@codesourcery.com>
> 
>             * target.h (target_read): Remove
>             (get_target_memory): Remove.
>             * target.c (target_read): Remove
>             (get_target_memory): Remove.
>             (get_target_memory_unsigned): Use target_read_memory.

But if you want to commit the other two parts of this patch, that's OK.
Except:

> @@ -1404,25 +1381,13 @@
>    return len;
>  }
>  
> -/* Memory transfer methods.  */
> -
> -void
> -get_target_memory (struct target_ops *ops, CORE_ADDR addr, gdb_byte *buf,
> -		   LONGEST len)
> -{
> -  if (target_read (ops, TARGET_OBJECT_MEMORY, NULL, buf, addr, len)
> -      != len)
> -    memory_error (EIO, addr);
> -}
> -
>  ULONGEST
> -get_target_memory_unsigned (struct target_ops *ops,
> -			    CORE_ADDR addr, int len)
> +get_target_memory_unsigned (CORE_ADDR addr, int len)
>  {
>    gdb_byte buf[sizeof (ULONGEST)];
>  
>    gdb_assert (len <= sizeof (buf));
> -  get_target_memory (ops, addr, buf, len);
> +  target_read_memory (addr, buf, len);
>    return extract_unsigned_integer (buf, len);
>  }
>  

get_target_memory used to call memory_error if there was a problem.
target_read_memory doesn't; it returns an error code.  So you need
to check it yourself.

-- 
Daniel Jacobowitz
CodeSourcery