Re: [PATCH] Cleanup target memory reading

[Daniel Jacobowitz <drow at false dot org>]

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