Re: PRecord sets memory even when it says it did not

[Michael Snyder <msnyder at vmware dot com>]

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.