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

[Daniel Jacobowitz <drow at false dot org>]

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