Seems like a bug in target_read_stack / dcache_xfer_memory?

Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

OK, this is right at the hairy edge of my understanding, and I
admit up front that I did not carefully follow the email thread, but...

There was this email thread about reading beyond the stack, and
using dcache, which I did not follow carefully.  And now I think
I'm running into it.

Short version:  code in memory_xfer_partial looks like this:

1280            /* FIXME drow/2006-08-09: If we're going to preserve const
1281               correctness dcache_xfer_memory should take readbuf and
1282               writebuf.  */
1283            res = dcache_xfer_memory (ops, target_dcache, memaddr,
1284                                      (void *) writebuf,
1285                                      reg_len, 1);
1286          if (res <= 0)
1287            return -1;

I think that's wrong.  I think it needs to test for "res == 0".

Comment at dcache_xfer_memory says:

   The meaning of the result is the same as for target_write

(Gripe: someone please fix that comment.  Why should I have to
go find another function in another file to find out what this
function returns?)

So the comment at target_write says...  oh wait!  There is no
comment at target_write!  I'm afraid I'm going to have to start
getting grumpy now...

Well, that returns target_write_with_progress.  The comment
there makes no mention of the return value.  But it returns
target_write_partial.  STILL no comment about the return value.
But this returns target_xfer_partial.  GUESS WHAT?

Well, this returns memory_xfer_partial, which is right back
where I started.  And *its* comment says:

   The arguments and return
   value are just as for target_xfer_partial

Ummm,  come on guys.  Its Sunday and I've had a long day.
Joke's getting old.  Whoever took the comments, please put them back.

Anyway, I don't even remember now how I figured this out, but
I *THINK* what all these guys return is either 0 for success,
or an errno value less than zoro.

And if that's true, then line 1286 up there needs to bail out
on zero and let the other target stack methods have a chance
to read the memory.

Eh?

We now return you to the nice Michael.   ;-)

Daniel, you're not the target, you're only Cc:ed because your
name is in the one comment that I *did* find.

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[drow]

On Sun, Oct 18, 2009 at 03:31:53PM -0700, Michael Snyder wrote:
>   The arguments and return
>   value are just as for target_xfer_partial

The comment is on the logical home of this method, other places should
refer to the header: the definition of to_xfer_partial in struct
target_ops in target.h.

> Anyway, I don't even remember now how I figured this out, but
> I *THINK* what all these guys return is either 0 for success,
> or an errno value less than zoro.

No, they return:

       the number of bytes actually transfered, zero when no
       further transfer is possible, and -1 when the transfer is not
       supported.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Hui Zhu]

I just want talk to you about it.

+            if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len))
+              {
+                entry->u.mem.mem_entry_not_accessible = 1;
+                if (record_debug)
+                  warning (_("Process record: error reading memory at "
+                             "addr = %s len = %d."),
+                           paddress (gdbarch, entry->u.mem.addr),
+                           entry->u.mem.len);
+              }
+            else
+              {
+                if (target_write_memory (entry->u.mem.addr,
+					 record_get_loc (entry),
+					 entry->u.mem.len))

Maybe we need change it to:
 if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len) <= 0)
 if (target_write_memory (entry->u.mem.addr,
+					 record_get_loc (entry),
+					 entry->u.mem.len) <= 0)

Thanks,
Hui

On Mon, Oct 19, 2009 at 06:31, Michael Snyder <msnyder@vmware.com> wrote:
> OK, this is right at the hairy edge of my understanding, and I
> admit up front that I did not carefully follow the email thread, but...
>
> There was this email thread about reading beyond the stack, and
> using dcache, which I did not follow carefully.  And now I think
> I'm running into it.
>
> Short version:  code in memory_xfer_partial looks like this:
>
> 1280            /* FIXME drow/2006-08-09: If we're going to preserve const
> 1281               correctness dcache_xfer_memory should take readbuf and
> 1282               writebuf.  */
> 1283            res = dcache_xfer_memory (ops, target_dcache, memaddr,
> 1284                                      (void *) writebuf,
> 1285                                      reg_len, 1);
> 1286          if (res <= 0)
> 1287            return -1;
>
> I think that's wrong.  I think it needs to test for "res == 0".
>
> Comment at dcache_xfer_memory says:
>
>  The meaning of the result is the same as for target_write
>
> (Gripe: someone please fix that comment.  Why should I have to
> go find another function in another file to find out what this
> function returns?)
>
> So the comment at target_write says...  oh wait!  There is no
> comment at target_write!  I'm afraid I'm going to have to start
> getting grumpy now...
>
> Well, that returns target_write_with_progress.  The comment
> there makes no mention of the return value.  But it returns
> target_write_partial.  STILL no comment about the return value.
> But this returns target_xfer_partial.  GUESS WHAT?
>
> Well, this returns memory_xfer_partial, which is right back
> where I started.  And *its* comment says:
>
>  The arguments and return
>  value are just as for target_xfer_partial
>
> Ummm,  come on guys.  Its Sunday and I've had a long day.
> Joke's getting old.  Whoever took the comments, please put them back.
>
> Anyway, I don't even remember now how I figured this out, but
> I *THINK* what all these guys return is either 0 for success,
> or an errno value less than zoro.
>
> And if that's true, then line 1286 up there needs to bail out
> on zero and let the other target stack methods have a chance
> to read the memory.
>
> Eh?
>
> We now return you to the nice Michael.   ;-)
>
> Daniel, you're not the target, you're only Cc:ed because your
> name is in the one comment that I *did* find.
>
>
>

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

drow@false.org wrote:
> On Sun, Oct 18, 2009 at 03:31:53PM -0700, Michael Snyder wrote:
>>   The arguments and return
>>   value are just as for target_xfer_partial
> 
> The comment is on the logical home of this method, other places should
> refer to the header: the definition of to_xfer_partial in struct
> target_ops in target.h.

OK, shouldn't we say so?  I want to drop this conversation,
though, and focus on the code problem.  Sorry again for my
aggrieved tone yesterday -- I was tired.   ;-(


>> Anyway, I don't even remember now how I figured this out, but
>> I *THINK* what all these guys return is either 0 for success,
>> or an errno value less than zoro.
> 
> No, they return:
> 
>        the number of bytes actually transfered, zero when no
>        further transfer is possible, and -1 when the transfer is not
>        supported.

OK, so suppose dcache_xfer_memory returns zero in this context.
That means no transfer is possible.  Shouldn't we give the other
targets on the stack a shot?

In the case I'm looking at, the next target down is a core file,
and I know it has the memory location available.  If I force gdb
out of this error return, core_xfer_partial will succeed.

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Hui Zhu wrote:
> I just want talk to you about it.
> 
> +            if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len))
> +              {
> +                entry->u.mem.mem_entry_not_accessible = 1;
> +                if (record_debug)
> +                  warning (_("Process record: error reading memory at "
> +                             "addr = %s len = %d."),
> +                           paddress (gdbarch, entry->u.mem.addr),
> +                           entry->u.mem.len);
> +              }
> +            else
> +              {
> +                if (target_write_memory (entry->u.mem.addr,
> +					 record_get_loc (entry),
> +					 entry->u.mem.len))
> 
> Maybe we need change it to:
>  if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len) <= 0)
>  if (target_write_memory (entry->u.mem.addr,
> +					 record_get_loc (entry),
> +					 entry->u.mem.len) <= 0)

Yeah, I think you're right.
That warning used to be an error, so we wouldn't fall through,
but now we do.

However, this is not what causes my current problem.
I'll leave it up to you to find a fix for this one.    ;-)



> On Mon, Oct 19, 2009 at 06:31, Michael Snyder <msnyder@vmware.com> wrote:
>> OK, this is right at the hairy edge of my understanding, and I
>> admit up front that I did not carefully follow the email thread, but...
>>
>> There was this email thread about reading beyond the stack, and
>> using dcache, which I did not follow carefully.  And now I think
>> I'm running into it.
>>
>> Short version:  code in memory_xfer_partial looks like this:
>>
>> 1280            /* FIXME drow/2006-08-09: If we're going to preserve const
>> 1281               correctness dcache_xfer_memory should take readbuf and
>> 1282               writebuf.  */
>> 1283            res = dcache_xfer_memory (ops, target_dcache, memaddr,
>> 1284                                      (void *) writebuf,
>> 1285                                      reg_len, 1);
>> 1286          if (res <= 0)
>> 1287            return -1;
>>
>> I think that's wrong.  I think it needs to test for "res == 0".
>>
>> Comment at dcache_xfer_memory says:
>>
>>  The meaning of the result is the same as for target_write
>>
>> (Gripe: someone please fix that comment.  Why should I have to
>> go find another function in another file to find out what this
>> function returns?)
>>
>> So the comment at target_write says...  oh wait!  There is no
>> comment at target_write!  I'm afraid I'm going to have to start
>> getting grumpy now...
>>
>> Well, that returns target_write_with_progress.  The comment
>> there makes no mention of the return value.  But it returns
>> target_write_partial.  STILL no comment about the return value.
>> But this returns target_xfer_partial.  GUESS WHAT?
>>
>> Well, this returns memory_xfer_partial, which is right back
>> where I started.  And *its* comment says:
>>
>>  The arguments and return
>>  value are just as for target_xfer_partial
>>
>> Ummm,  come on guys.  Its Sunday and I've had a long day.
>> Joke's getting old.  Whoever took the comments, please put them back.
>>
>> Anyway, I don't even remember now how I figured this out, but
>> I *THINK* what all these guys return is either 0 for success,
>> or an errno value less than zoro.
>>
>> And if that's true, then line 1286 up there needs to bail out
>> on zero and let the other target stack methods have a chance
>> to read the memory.
>>
>> Eh?
>>
>> We now return you to the nice Michael.   ;-)
>>
>> Daniel, you're not the target, you're only Cc:ed because your
>> name is in the one comment that I *did* find.
>>
>>
>>

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Daniel Jacobowitz]

On Mon, Oct 19, 2009 at 10:43:24AM -0700, Michael Snyder wrote:
> drow@false.org wrote:
> >On Sun, Oct 18, 2009 at 03:31:53PM -0700, Michael Snyder wrote:
> >>  The arguments and return
> >>  value are just as for target_xfer_partial
> >
> >The comment is on the logical home of this method, other places should
> >refer to the header: the definition of to_xfer_partial in struct
> >target_ops in target.h.
> 
> OK, shouldn't we say so?  I want to drop this conversation,
> though, and focus on the code problem.  Sorry again for my
> aggrieved tone yesterday -- I was tired.   ;-(

Sure - I'm just as annoyed by the tangle as you are - I just remember
hunting for this comment myself :-)

> OK, so suppose dcache_xfer_memory returns zero in this context.
> That means no transfer is possible.  Shouldn't we give the other
> targets on the stack a shot?
> 
> In the case I'm looking at, the next target down is a core file,
> and I know it has the memory location available.  If I force gdb
> out of this error return, core_xfer_partial will succeed.

You haven't really described the situation, so I'm guessing.  But the
problem can't be in the code you cited.  It's got to be further down
the call stack.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Oct 19, 2009 at 10:43:24AM -0700, Michael Snyder wrote:
>> drow@false.org wrote:
>>> On Sun, Oct 18, 2009 at 03:31:53PM -0700, Michael Snyder wrote:
>>>>  The arguments and return
>>>>  value are just as for target_xfer_partial
>>> The comment is on the logical home of this method, other places should
>>> refer to the header: the definition of to_xfer_partial in struct
>>> target_ops in target.h.
>> OK, shouldn't we say so?  I want to drop this conversation,
>> though, and focus on the code problem.  Sorry again for my
>> aggrieved tone yesterday -- I was tired.   ;-(
> 
> Sure - I'm just as annoyed by the tangle as you are - I just remember
> hunting for this comment myself :-)
> 
>> OK, so suppose dcache_xfer_memory returns zero in this context.
>> That means no transfer is possible.  Shouldn't we give the other
>> targets on the stack a shot?
>>
>> In the case I'm looking at, the next target down is a core file,
>> and I know it has the memory location available.  If I force gdb
>> out of this error return, core_xfer_partial will succeed.
> 
> You haven't really described the situation, so I'm guessing.  But the
> problem can't be in the code you cited.  It's got to be further down
> the call stack.

Can you explain why it can't be in the code that I cited?
I don't understand why, for instance, this scenario couldn't
apply:

  * memory_xfer_partial is called for a stack-like location
    (say, 4 bytes beyond the topmost frame).
  * inf is non-null,
    region->attrib.cache is true or
    stach_cache_enabled_p and object == TARGET_OBJECT_STACK_MEMORY
  * therefore we call dcache_xfer_memory,
  * The requested location isn't cached, so we return zero.



----
However, of course I'll describe the situation.  You can't
quite replicate it yet, unles you've applied my recent patches.

1) load testsuite/gdb.reverse/solib-reverse
2) break main
3) record
4) until 41
5) record save foo
6) record restore foo (this kills the running process,
    loads the core file/log, and puts you back at main)
7) until 41

The "until" command tries to read beyond the top of stack,
which is fine for the running process and fine for the core
file, but for some reason in this instance wants to go into
dcache, where nothing currently should be cached.

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Daniel Jacobowitz]

On Mon, Oct 19, 2009 at 12:35:07PM -0700, Michael Snyder wrote:
>  * The requested location isn't cached, so we return zero.

Simple: that isn't the interface of dcache_xfer_memory.  It should
fill the cache.  Trace it down to dcache_read_line.

> The "until" command tries to read beyond the top of stack,
> which is fine for the running process and fine for the core
> file, but for some reason in this instance wants to go into
> dcache, where nothing currently should be cached.

Beyond the *top*?  And there's something mapped there?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Oct 19, 2009 at 12:35:07PM -0700, Michael Snyder wrote:
>>  * The requested location isn't cached, so we return zero.
> 
> Simple: that isn't the interface of dcache_xfer_memory.  It should
> fill the cache.  Trace it down to dcache_read_line.

Well that's what it does.

> 
>> The "until" command tries to read beyond the top of stack,
>> which is fine for the running process and fine for the core
>> file, but for some reason in this instance wants to go into
>> dcache, where nothing currently should be cached.
> 
> Beyond the *top*?  And there's something mapped there?

Arr, you know, "top" as in "TOS", as in the next location
that would be used if you pushed something.

Anyway, yes, that's what it does.  dcache returns zero,
and memory_xfer_partial bails out instead of trying the
next target down the target stack.

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Doug Evans]

On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com> wrote:
> Anyway, yes, that's what it does.  dcache returns zero,
> and memory_xfer_partial bails out instead of trying the
> next target down the target stack.

Hi.  If it will help I'll play with your testcase tomorrow.
I'll also volunteer to make a pass through the code and add some comments.
[I mention that just in case you or someone is already in the process
of doing that.]

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Doug Evans wrote:
> On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com> wrote:
>> Anyway, yes, that's what it does.  dcache returns zero,
>> and memory_xfer_partial bails out instead of trying the
>> next target down the target stack.
> 
> Hi.  If it will help I'll play with your testcase tomorrow.
> I'll also volunteer to make a pass through the code and add some comments.
> [I mention that just in case you or someone is already in the process
> of doing that.]

Sure it will help.  Thanks, Doug.

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Michael Snyder wrote:
> Doug Evans wrote:
>> On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com> wrote:
>>> Anyway, yes, that's what it does.  dcache returns zero,
>>> and memory_xfer_partial bails out instead of trying the
>>> next target down the target stack.
>> Hi.  If it will help I'll play with your testcase tomorrow.
>> I'll also volunteer to make a pass through the code and add some comments.
>> [I mention that just in case you or someone is already in the process
>> of doing that.]
> 
> Sure it will help.  Thanks, Doug.

And the test case is in the repo now -- solib-precsave.exp.
How to run tests: 
http://www.sourceware.org/gdb/wiki/ProcessRecord#head-2f56f7474cf60c6a5879ba6d8a4e4d034e6d0c8e

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Doug Evans]

 On Thu, Oct 22, 2009 at 12:54 PM, Michael Snyder <msnyder@vmware.com> wrote:
> Michael Snyder wrote:
>>
>> Doug Evans wrote:
>>>
>>> On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com>
>>> wrote:
>>>>
>>>> Anyway, yes, that's what it does.  dcache returns zero,
>>>> and memory_xfer_partial bails out instead of trying the
>>>> next target down the target stack.
>>>
>>> Hi.  If it will help I'll play with your testcase tomorrow.
>>> I'll also volunteer to make a pass through the code and add some
>>> comments.
>>> [I mention that just in case you or someone is already in the process
>>> of doing that.]
>>
>> Sure it will help.  Thanks, Doug.
>
> And the test case is in the repo now -- solib-precsave.exp.
> How to run tests:
> http://www.sourceware.org/gdb/wiki/ProcessRecord#head-2f56f7474cf60c6a5879ba6d8a4e4d034e6d0c8e

Thanks for the testcase.
You may be right about needing to test for "res == 0" but I'm less
sure now, so I'm going to leave it as is, at least for now.
dcache calls target_read (TARGET_OBJECT_RAW_MEMORY) which should DTRT.
And in fact it does with this patch. :-)
Checked in.

2009-10-23  Doug Evans  <dje@google.com>

        * record.c (record_core_xfer_partial): Pass correct offset to
        record_beneath_to_xfer_partial.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.33
diff -u -p -r1.33 record.c
--- record.c    23 Oct 2009 16:11:37 -0000      1.33
+++ record.c    23 Oct 2009 17:07:08 -0000
@@ -1629,6 +1629,7 @@ record_core_xfer_partial (struct target_
                if (offset >= p->addr)
                  {
                    struct record_core_buf_entry *entry;
+                  ULONGEST sec_offset;

                    if (offset >= p->endaddr)
                      continue;
@@ -1636,7 +1637,7 @@ record_core_xfer_partial (struct target_
                    if (offset + len > p->endaddr)
                      len = p->endaddr - offset;

-                   offset -= p->addr;
+                   sec_offset = offset - p->addr;

                    /* Read readbuf or write writebuf p, offset, len.  */
                    /* Check flags.  */
@@ -1673,7 +1674,8 @@ record_core_xfer_partial (struct target_
                            record_core_buf_list = entry;
                          }

-                        memcpy (entry->buf + offset, writebuf, (size_t) len);
+                        memcpy (entry->buf + sec_offset, writebuf,
+                               (size_t) len);
                      }
                    else
                      {
@@ -1683,7 +1685,8 @@ record_core_xfer_partial (struct target_
                                    object, annex, readbuf, writebuf,
                                    offset, len);

-                       memcpy (readbuf, entry->buf + offset, (size_t) len);
+                       memcpy (readbuf, entry->buf + sec_offset,
+                              (size_t) len);
                      }

                    return len;

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Michael Snyder]

Doug Evans wrote:
>  On Thu, Oct 22, 2009 at 12:54 PM, Michael Snyder <msnyder@vmware.com> wrote:
>> Michael Snyder wrote:
>>> Doug Evans wrote:
>>>> On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com>
>>>> wrote:
>>>>> Anyway, yes, that's what it does.  dcache returns zero,
>>>>> and memory_xfer_partial bails out instead of trying the
>>>>> next target down the target stack.
>>>> Hi.  If it will help I'll play with your testcase tomorrow.
>>>> I'll also volunteer to make a pass through the code and add some
>>>> comments.
>>>> [I mention that just in case you or someone is already in the process
>>>> of doing that.]
>>> Sure it will help.  Thanks, Doug.
>> And the test case is in the repo now -- solib-precsave.exp.
>> How to run tests:
>> http://www.sourceware.org/gdb/wiki/ProcessRecord#head-2f56f7474cf60c6a5879ba6d8a4e4d034e6d0c8e
> 
> Thanks for the testcase.
> You may be right about needing to test for "res == 0" but I'm less
> sure now, so I'm going to leave it as is, at least for now.
> dcache calls target_read (TARGET_OBJECT_RAW_MEMORY) which should DTRT.
> And in fact it does with this patch. :-)
> Checked in.

Thanks for the fix.  It handles the case I was concerned with.


> 2009-10-23  Doug Evans  <dje@google.com>
> 
>         * record.c (record_core_xfer_partial): Pass correct offset to
>         record_beneath_to_xfer_partial.
> 
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 record.c
> --- record.c    23 Oct 2009 16:11:37 -0000      1.33
> +++ record.c    23 Oct 2009 17:07:08 -0000
> @@ -1629,6 +1629,7 @@ record_core_xfer_partial (struct target_
>                 if (offset >= p->addr)
>                   {
>                     struct record_core_buf_entry *entry;
> +                  ULONGEST sec_offset;
> 
>                     if (offset >= p->endaddr)
>                       continue;
> @@ -1636,7 +1637,7 @@ record_core_xfer_partial (struct target_
>                     if (offset + len > p->endaddr)
>                       len = p->endaddr - offset;
> 
> -                   offset -= p->addr;
> +                   sec_offset = offset - p->addr;
> 
>                     /* Read readbuf or write writebuf p, offset, len.  */
>                     /* Check flags.  */
> @@ -1673,7 +1674,8 @@ record_core_xfer_partial (struct target_
>                             record_core_buf_list = entry;
>                           }
> 
> -                        memcpy (entry->buf + offset, writebuf, (size_t) len);
> +                        memcpy (entry->buf + sec_offset, writebuf,
> +                               (size_t) len);
>                       }
>                     else
>                       {
> @@ -1683,7 +1685,8 @@ record_core_xfer_partial (struct target_
>                                     object, annex, readbuf, writebuf,
>                                     offset, len);
> 
> -                       memcpy (readbuf, entry->buf + offset, (size_t) len);
> +                       memcpy (readbuf, entry->buf + sec_offset,
> +                              (size_t) len);
>                       }
> 
>                     return len;

Re: Seems like a bug in target_read_stack / dcache_xfer_memory?

[Hui Zhu]

Thanks Doug.

Hui

On Sat, Oct 24, 2009 at 01:18, Doug Evans <dje@google.com> wrote:
>  On Thu, Oct 22, 2009 at 12:54 PM, Michael Snyder <msnyder@vmware.com> wrote:
>> Michael Snyder wrote:
>>>
>>> Doug Evans wrote:
>>>>
>>>> On Mon, Oct 19, 2009 at 2:35 PM, Michael Snyder <msnyder@vmware.com>
>>>> wrote:
>>>>>
>>>>> Anyway, yes, that's what it does.  dcache returns zero,
>>>>> and memory_xfer_partial bails out instead of trying the
>>>>> next target down the target stack.
>>>>
>>>> Hi.  If it will help I'll play with your testcase tomorrow.
>>>> I'll also volunteer to make a pass through the code and add some
>>>> comments.
>>>> [I mention that just in case you or someone is already in the process
>>>> of doing that.]
>>>
>>> Sure it will help.  Thanks, Doug.
>>
>> And the test case is in the repo now -- solib-precsave.exp.
>> How to run tests:
>> http://www.sourceware.org/gdb/wiki/ProcessRecord#head-2f56f7474cf60c6a5879ba6d8a4e4d034e6d0c8e
>
> Thanks for the testcase.
> You may be right about needing to test for "res == 0" but I'm less
> sure now, so I'm going to leave it as is, at least for now.
> dcache calls target_read (TARGET_OBJECT_RAW_MEMORY) which should DTRT.
> And in fact it does with this patch. :-)
> Checked in.
>
> 2009-10-23  Doug Evans  <dje@google.com>
>
>        * record.c (record_core_xfer_partial): Pass correct offset to
>        record_beneath_to_xfer_partial.
>
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 record.c
> --- record.c    23 Oct 2009 16:11:37 -0000      1.33
> +++ record.c    23 Oct 2009 17:07:08 -0000
> @@ -1629,6 +1629,7 @@ record_core_xfer_partial (struct target_
>                if (offset >= p->addr)
>                  {
>                    struct record_core_buf_entry *entry;
> +                  ULONGEST sec_offset;
>
>                    if (offset >= p->endaddr)
>                      continue;
> @@ -1636,7 +1637,7 @@ record_core_xfer_partial (struct target_
>                    if (offset + len > p->endaddr)
>                      len = p->endaddr - offset;
>
> -                   offset -= p->addr;
> +                   sec_offset = offset - p->addr;
>
>                    /* Read readbuf or write writebuf p, offset, len.  */
>                    /* Check flags.  */
> @@ -1673,7 +1674,8 @@ record_core_xfer_partial (struct target_
>                            record_core_buf_list = entry;
>                          }
>
> -                        memcpy (entry->buf + offset, writebuf, (size_t) len);
> +                        memcpy (entry->buf + sec_offset, writebuf,
> +                               (size_t) len);
>                      }
>                    else
>                      {
> @@ -1683,7 +1685,8 @@ record_core_xfer_partial (struct target_
>                                    object, annex, readbuf, writebuf,
>                                    offset, len);
>
> -                       memcpy (readbuf, entry->buf + offset, (size_t) len);
> +                       memcpy (readbuf, entry->buf + sec_offset,
> +                              (size_t) len);
>                      }
>
>                    return len;
>