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

[Doug Evans <dje@google.com>]

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]