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

[Pedro Alves <pedro at codesourcery dot com>]

A Monday 14 September 2009 19:21:57, Michael Snyder escreveu:
> Pedro Alves wrote:
> > A Monday 14 September 2009 18:59:46, Michael Snyder escreveu:
> >> Pedro Alves wrote:
> >>> On Monday 14 September 2009 18:53:51, Marc Khouzam wrote:
> >>>>> -----Original Message-----
> >>>>> From: Pedro Alves [mailto:pedro@codesourcery.com] 
> >>>>> Sent: Monday, September 14, 2009 1:50 PM
> >>>>> To: gdb@sourceware.org
> >>>>> Cc: Greg Law; Hui Zhu; Marc Khouzam; Michael Snyder; gdb-patches ml
> >>>>> Subject: Re: PRecord sets memory even when it says it did not
> >>>>>
> >>>>>> Could this be related to the caching changes that have happened
> >>>>>> recently?  i.e. does the cache get updated even though the 
> >>>>> underlying
> >>>>>> poke operation failed?  If so, this issue would seem to be 
> >>>>> wider than
> >>>>>> just prec (and wider than reverse, too).
> >>>>> If so, then there's an easy way to find out: try again with
> >>>>> "set stack-cache off".
> >>>>>
> >>>> Yes, that seems to fix everything.
> >>> Then I'd suspect either a bug dcache_xfer_memory, or a missing
> >>> target_dcache_invalidate/dcache_invalidate call somewhere.
> >> I'm not too familiar with this dcache.
> >> Is it up to the target to_xfer_memory method
> >> to invalidate it before erroring out?
> > 
> > No.  dcache_xfer_memory tries to handle it itself already, with
> > dcache_invalidate_line, but there's probably a bug over there, which
> > should be easy to catch stepping through dcache_xfer_memory in the
> > offending case.  Note that it is dcache_xfer_memory
> > that calls the target to_xfer_memory routine, not
> > memory_xfer_partial.
> > 
> 
> Umm, that last sentence does not seem to be true.
> 1) I can see the call to ops->to_xfer_partial in memory_xfer_partial src
> 2) I've got a backtrace right here showing record_xfer_partial
> called by memory_xfer_partial.

Ah, oops.  I missed the dcache_update call.  Sorry, wasn't paying
that much attention.

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).

???  Doug?

-- 
Pedro Alves