PRecord sets memory even when it says it did not

[Michael Snyder]

Doug Evans wrote:
> On Mon, Sep 14, 2009 at 11:36 AM, Pedro Alves <pedro@codesourcery.com> wrote:
>> Hmmm.  On a less quicker look, how about if we get rid of the
>> dcache_xfer_memory and dcache_update calls in memory_xfer_partial,
>>
>> (excuse the pseudo-patch-written-in-email)
>>
>> target.c:memory_xfer_partial
>>
>> -  inf = find_inferior_pid (ptid_get_pid (inferior_ptid));
>> -
>> -  if (inf != NULL
>> -      && (region->attrib.cache
>> -         || (stack_cache_enabled_p && object == TARGET_OBJECT_STACK_MEMORY)))
>> -    {
>> -      if (readbuf != NULL)
>> -       res = dcache_xfer_memory (ops, target_dcache, memaddr, readbuf,
>> -                                 reg_len, 0);
>> -      else
>> -       /* FIXME drow/2006-08-09: If we're going to preserve const
>> -          correctness dcache_xfer_memory should take readbuf and
>> -          writebuf.  */
>> -       res = dcache_xfer_memory (ops, target_dcache, memaddr,
>> -                                 (void *) writebuf,
>> -                                 reg_len, 1);
>> -      if (res <= 0)
>> -       return -1;
>> -      else
>> -       {
>> -         if (readbuf && !show_memory_breakpoints)
>> -           breakpoint_restore_shadows (readbuf, memaddr, reg_len);
>> -         return res;
>> -       }
>> -    }
>> -
>> -  /* Make sure the cache gets updated no matter what - if we are writing
>> -     to the stack, even if this write is not tagged as such, we still need
>> -     to update the cache. */
>> -
>> -  if (inf != NULL
>> -      && readbuf == NULL
>> -      && !region->attrib.cache
>> -      && stack_cache_enabled_p
>> -      && object != TARGET_OBJECT_STACK_MEMORY)
>> -    {
>> -      dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
>> -    }
>>
>>
>> and replaced this call below, something like so:
>>
>>  do
>>    {
>> -      res = ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
>> -                                 readbuf, writebuf, memaddr, reg_len);
>> +      res = dcache_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
>> +                                 readbuf, writebuf, memaddr, reg_len);
>>
>>
>>      if (res > 0)
>>        break;
>>
>>      /* We want to continue past core files to executables, but not
>>         past a running target's memory.  */
>>      if (ops->to_has_all_memory (ops))
>>        break;
>>
>>      ops = ops->beneath;
>>    }
>>  while (ops != NULL);
>>
>> ... by a dcache_xfer_memory call, but tweak its interface to pass it
>> the object type?  Things would be tidier and dcache_xfer_memory
>> would then handle all this dcache updating/invalidating itself.
>>
>> On the plus side, when the dcache is in effect, with that change,
>> we'd again walk the whole target stack, which isn't true currently
>> (and looks like a possible design flaw).
> 
> OTOH, its nice having memory_xfer_partial do the region attribute processing.
> 
> It seems like what's needed is to move
> 
>   /* Make sure the cache gets updated no matter what - if we are
> writing
>      to the stack, even if this write is not tagged as such, we still
> need
>      to update the cache. */
> 
>   if (inf != NULL
>       && readbuf == NULL
>       && !region->attrib.cache
>       && stack_cache_enabled_p
>       && object != TARGET_OBJECT_STACK_MEMORY)
>     {
>       dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
>     }
> 
> to after the call
> 
>       res = ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
>                                   readbuf, writebuf, memaddr, reg_len);
> 
> predicated on res > 0.
> 
> [dcache.c does the write first, and then updates the cache if it
> succeeded, we just need to do that here too, methinks]


I like this better.

BTW, I've been checking other implementations of xxx_xfer_partial,
and I can't find any that call error(), but there are plenty that
return -1.  Seems like this failure would apply to all of those too.