[patch] only update dcache after write succeeds

[patch] only update dcache after write succeeds

[Doug Evans]

Hi.

Marc, Greg: Can you see if this patch fixes things for you?

2009-09-14  Doug Evans  <dje@google.com>

	* target.c (memory_xfer_partial): Only update dcache after we know
	the write succeeded.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.223
diff -u -p -r1.223 target.c
--- target.c	8 Sep 2009 23:52:45 -0000	1.223
+++ target.c	14 Sep 2009 19:11:38 -0000
@@ -1289,19 +1289,6 @@ memory_xfer_partial (struct target_ops *
 	}
     }
 
-  /* 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);
-    }
-
   /* If none of those methods found the memory we wanted, fall back
      to a target partial transfer.  Normally a single call to
      to_xfer_partial is enough; if it doesn't recognize an object
@@ -1331,6 +1318,20 @@ memory_xfer_partial (struct target_ops *
   if (readbuf && !show_memory_breakpoints)
     breakpoint_restore_shadows (readbuf, memaddr, reg_len);
 
+  /* 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 (res > 0
+      && inf != NULL
+      && writebuf != NULL
+      && !region->attrib.cache
+      && stack_cache_enabled_p
+      && object != TARGET_OBJECT_STACK_MEMORY)
+    {
+      dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
+    }
+
   /* If we still haven't got anything, return the last error.  We
      give up.  */
   return res;

Re: [patch] only update dcache after write succeeds

[Greg Law]

Doug Evans wrote:
> Hi.
> 
> Marc, Greg: Can you see if this patch fixes things for you?

Yep - that fixes it for us.

Thanks, Doug.

Greg
-- 
Greg Law, Undo Software                       http://undo-software.com/

RE: [patch] only update dcache after write succeeds

[Marc Khouzam]

> -----Original Message-----
> From: Greg Law [mailto:glaw@undo-software.com] 
> Sent: Monday, September 14, 2009 3:27 PM
> To: Doug Evans
> Cc: gdb-patches@sourceware.org; Marc Khouzam; Pedro Alves; 
> Michael Snyder; Hui Zhu
> Subject: Re: [patch] only update dcache after write succeeds
> 
> Doug Evans wrote:
> > Hi.
> > 
> > Marc, Greg: Can you see if this patch fixes things for you?
> 
> Yep - that fixes it for us.

And for me too.

Thanks everyone!
Marc

> 
> Thanks, Doug.
> 
> Greg
> -- 
> Greg Law, Undo Software                       
> http://undo-software.com/
> 

Re: [patch] only update dcache after write succeeds

[Michael Snyder]

Doug Evans wrote:
> Hi.
> 
> Marc, Greg: Can you see if this patch fixes things for you?

Hey Doug,

I tested this change, and it does indeed seem to fix the problem
with target record -- but.   ;-)

Part of the reason that it works is that record_xfer_partial
calls error() instead of returning -1.  If I change it so that
it returns -1, things get more complicated.

The do-while loop that used to follow and now preceeds this code
calls target-beneath, which in our case results in several different
target methods being called, one of which eventually returns > 0.

That just means that in the present case, calling error is correct.
But I worry about some of the other cases where the target method
returns -1, and whether badness might occur in some other cases.

Michael


> 2009-09-14  Doug Evans  <dje@google.com>
> 
> 	* target.c (memory_xfer_partial): Only update dcache after we know
> 	the write succeeded.
> 
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.223
> diff -u -p -r1.223 target.c
> --- target.c	8 Sep 2009 23:52:45 -0000	1.223
> +++ target.c	14 Sep 2009 19:11:38 -0000
> @@ -1289,19 +1289,6 @@ memory_xfer_partial (struct target_ops *
>  	}
>      }
>  
> -  /* 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);
> -    }
> -
>    /* If none of those methods found the memory we wanted, fall back
>       to a target partial transfer.  Normally a single call to
>       to_xfer_partial is enough; if it doesn't recognize an object
> @@ -1331,6 +1318,20 @@ memory_xfer_partial (struct target_ops *
>    if (readbuf && !show_memory_breakpoints)
>      breakpoint_restore_shadows (readbuf, memaddr, reg_len);
>  
> +  /* 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 (res > 0
> +      && inf != NULL
> +      && writebuf != NULL
> +      && !region->attrib.cache
> +      && stack_cache_enabled_p
> +      && object != TARGET_OBJECT_STACK_MEMORY)
> +    {
> +      dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
> +    }
> +
>    /* If we still haven't got anything, return the last error.  We
>       give up.  */
>    return res;

RE: [patch] only update dcache after write succeeds

[Marc Khouzam]

 

> -----Original Message-----
> From: Michael Snyder [mailto:msnyder@vmware.com] 
> Sent: Monday, September 14, 2009 3:27 PM
> To: Doug Evans
> Cc: gdb-patches@sourceware.org; Marc Khouzam; Greg Law; Pedro 
> Alves; Hui Zhu
> Subject: Re: [patch] only update dcache after write succeeds
> 
> Doug Evans wrote:
> > Hi.
> > 
> > Marc, Greg: Can you see if this patch fixes things for you?
> 
> Hey Doug,
> 
> I tested this change, and it does indeed seem to fix the problem
> with target record -- but.   ;-)
> 
> Part of the reason that it works is that record_xfer_partial
> calls error() instead of returning -1.  If I change it so that
> it returns -1, things get more complicated.

Just a note that PRecord used to return -1 which caused memory
to be changed even when the query was answered with 'n'.
That is why Hui changed it to error()

> 
> The do-while loop that used to follow and now preceeds this code
> calls target-beneath, which in our case results in several different
> target methods being called, one of which eventually returns > 0.
> 
> That just means that in the present case, calling error is correct.
> But I worry about some of the other cases where the target method
> returns -1, and whether badness might occur in some other cases.
> 
> Michael
> 
> 
> > 2009-09-14  Doug Evans  <dje@google.com>
> > 
> > 	* target.c (memory_xfer_partial): Only update dcache 
> after we know
> > 	the write succeeded.
> > 
> > Index: target.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/target.c,v
> > retrieving revision 1.223
> > diff -u -p -r1.223 target.c
> > --- target.c	8 Sep 2009 23:52:45 -0000	1.223
> > +++ target.c	14 Sep 2009 19:11:38 -0000
> > @@ -1289,19 +1289,6 @@ memory_xfer_partial (struct target_ops *
> >  	}
> >      }
> >  
> > -  /* 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);
> > -    }
> > -
> >    /* If none of those methods found the memory we wanted, fall back
> >       to a target partial transfer.  Normally a single call to
> >       to_xfer_partial is enough; if it doesn't recognize an object
> > @@ -1331,6 +1318,20 @@ memory_xfer_partial (struct target_ops *
> >    if (readbuf && !show_memory_breakpoints)
> >      breakpoint_restore_shadows (readbuf, memaddr, reg_len);
> >  
> > +  /* 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 (res > 0
> > +      && inf != NULL
> > +      && writebuf != NULL
> > +      && !region->attrib.cache
> > +      && stack_cache_enabled_p
> > +      && object != TARGET_OBJECT_STACK_MEMORY)
> > +    {
> > +      dcache_update (target_dcache, memaddr, (void *) 
> writebuf, reg_len);
> > +    }
> > +
> >    /* If we still haven't got anything, return the last error.  We
> >       give up.  */
> >    return res;
> 
> 

Re: [patch] only update dcache after write succeeds

[Michael Snyder]

Marc Khouzam wrote:
>  
> 
>> -----Original Message-----
>> From: Michael Snyder [mailto:msnyder@vmware.com] 
>> Sent: Monday, September 14, 2009 3:27 PM
>> To: Doug Evans
>> Cc: gdb-patches@sourceware.org; Marc Khouzam; Greg Law; Pedro 
>> Alves; Hui Zhu
>> Subject: Re: [patch] only update dcache after write succeeds
>>
>> Doug Evans wrote:
>>> Hi.
>>>
>>> Marc, Greg: Can you see if this patch fixes things for you?
>> Hey Doug,
>>
>> I tested this change, and it does indeed seem to fix the problem
>> with target record -- but.   ;-)
>>
>> Part of the reason that it works is that record_xfer_partial
>> calls error() instead of returning -1.  If I change it so that
>> it returns -1, things get more complicated.
> 
> Just a note that PRecord used to return -1 which caused memory
> to be changed even when the query was answered with 'n'.
> That is why Hui changed it to error()

OK -- and now I see why it used to change the memory anyway.
The caller (memory_xfer_partial) would call the target beneath.

Makes me suspect that Doug's change is OK in general.

Re: [patch] only update dcache after write succeeds

[Doug Evans]

On Mon, Sep 14, 2009 at 12:26 PM, Michael Snyder <msnyder@vmware.com> wrote:
> Doug Evans wrote:
>>
>> Hi.
>>
>> Marc, Greg: Can you see if this patch fixes things for you?
>
> Hey Doug,

Hey Michael, :-)

> I tested this change, and it does indeed seem to fix the problem
> with target record -- but.   ;-)
>
> Part of the reason that it works is that record_xfer_partial
> calls error() instead of returning -1.  If I change it so that
> it returns -1, things get more complicated.
>
> The do-while loop that used to follow and now preceeds this code
> calls target-beneath, which in our case results in several different
> target methods being called, one of which eventually returns > 0.
>
> That just means that in the present case, calling error is correct.
> But I worry about some of the other cases where the target method
> returns -1, and whether badness might occur in some other cases.

How does one reconcile  "eventually returns > 0" with "badness"?

IOW, if some target method does return > 0, then the write succeeded, right?
Are there different kinds of "success" in effect here?

Re: [patch] only update dcache after write succeeds

[Michael Snyder]

Doug Evans wrote:
> On Mon, Sep 14, 2009 at 12:26 PM, Michael Snyder <msnyder@vmware.com> wrote:
>> Doug Evans wrote:
>>> Hi.
>>>
>>> Marc, Greg: Can you see if this patch fixes things for you?
>> Hey Doug,
> 
> Hey Michael, :-)
> 
>> I tested this change, and it does indeed seem to fix the problem
>> with target record -- but.   ;-)
>>
>> Part of the reason that it works is that record_xfer_partial
>> calls error() instead of returning -1.  If I change it so that
>> it returns -1, things get more complicated.
>>
>> The do-while loop that used to follow and now preceeds this code
>> calls target-beneath, which in our case results in several different
>> target methods being called, one of which eventually returns > 0.
>>
>> That just means that in the present case, calling error is correct.
>> But I worry about some of the other cases where the target method
>> returns -1, and whether badness might occur in some other cases.
> 
> How does one reconcile  "eventually returns > 0" with "badness"?
> 
> IOW, if some target method does return > 0, then the write succeeded, right?
> Are there different kinds of "success" in effect here?

Well, maybe only in our case.  ;-)

If nobody else has any worries about it, I'm OK with it.

----
* In our case (process record), it's a bad thing for the target
beneath to be called after the user has said "no".

Re: [patch] only update dcache after write succeeds

[Pedro Alves]

On Monday 14 September 2009 21:20:38, Michael Snyder wrote:

> > IOW, if some target method does return > 0, then the write succeeded, right?
> > Are there different kinds of "success" in effect here?
> 
> Well, maybe only in our case.  ;-)
> 
> If nobody else has any worries about it, I'm OK with it.
> 
> ----
> * In our case (process record), it's a bad thing for the target
> beneath to be called after the user has said "no".

The user is saying "no" to the whole high level operation, not to
a single partial transfer.  In that case, you shouldn't even
attempt to partial xfer in the target beneath, I would say.  A thrown
error for those cases looks like the way to go.  Perhaps even better
would be to be able to foretell if the transfer is going to be
problematic and warn/query upfront, but it's hard in some cases.

-- 
Pedro Alves

Re: [patch] only update dcache after write succeeds

[Michael Snyder]

Pedro Alves wrote:
> On Monday 14 September 2009 21:20:38, Michael Snyder wrote:
> 
>>> IOW, if some target method does return > 0, then the write succeeded, right?
>>> Are there different kinds of "success" in effect here?
>> Well, maybe only in our case.  ;-)
>>
>> If nobody else has any worries about it, I'm OK with it.
>>
>> ----
>> * In our case (process record), it's a bad thing for the target
>> beneath to be called after the user has said "no".
> 
> The user is saying "no" to the whole high level operation, not to
> a single partial transfer.  In that case, you shouldn't even
> attempt to partial xfer in the target beneath, I would say.  A thrown
> error for those cases looks like the way to go.  Perhaps even better
> would be to be able to foretell if the transfer is going to be
> problematic and warn/query upfront, but it's hard in some cases.

Agree, "error()" is the way to go here.

Re: [patch] only update dcache after write succeeds

[Doug Evans]

On Mon, Sep 14, 2009 at 1:20 PM, Michael Snyder<msnyder@vmware.com> wrote:
>> IOW, if some target method does return > 0, then the write succeeded,
>> right?
>> Are there different kinds of "success" in effect here?
>
> Well, maybe only in our case.  ;-)
>
> If nobody else has any worries about it, I'm OK with it.
>
> ----
> * In our case (process record), it's a bad thing for the target
> beneath to be called after the user has said "no".

Righto.
But that seems like a separate issue (albeit one that collides with
dcache here).
We need a way for a target to say "I'm not handling this, and neither
can you." :-)
AIUI, right now it's done by punting with error (which doesn't seem
all that bad for the particular case at hand).
The alternative is to extend the error return values to mean different
things, but I'm guessing we're not in a rush to do that.

If you like, I don't mind a workaround where we invalidate lines just
written to instead of updating them.

Re: [patch] only update dcache after write succeeds

[Michael Snyder]

Doug Evans wrote:
> On Mon, Sep 14, 2009 at 1:20 PM, Michael Snyder<msnyder@vmware.com> wrote:
>>> IOW, if some target method does return > 0, then the write succeeded,
>>> right?
>>> Are there different kinds of "success" in effect here?
>> Well, maybe only in our case.  ;-)
>>
>> If nobody else has any worries about it, I'm OK with it.
>>
>> ----
>> * In our case (process record), it's a bad thing for the target
>> beneath to be called after the user has said "no".
> 
> Righto.
> But that seems like a separate issue (albeit one that collides with
> dcache here).
> We need a way for a target to say "I'm not handling this, and neither
> can you." :-)
> AIUI, right now it's done by punting with error (which doesn't seem
> all that bad for the particular case at hand).
> The alternative is to extend the error return values to mean different
> things, but I'm guessing we're not in a rush to do that.
> 
> If you like, I don't mind a workaround where we invalidate lines just
> written to instead of updating them.

I'm good with what you've got.  ;-)

Re: [patch] only update dcache after write succeeds

[Hui Zhu]

On Tue, Sep 15, 2009 at 03:16, Doug Evans <dje@google.com> wrote:
> Hi.
>
> Marc, Greg: Can you see if this patch fixes things for you?
>
> 2009-09-14  Doug Evans  <dje@google.com>
>
>        * target.c (memory_xfer_partial): Only update dcache after we know
>        the write succeeded.
>

Great! It works OK.

Thanks,
Hui

Re: [patch] only update dcache after write succeeds

[Doug Evans]

On Mon, Sep 14, 2009 at 12:16 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> Marc, Greg: Can you see if this patch fixes things for you?
>
> 2009-09-14  Doug Evans  <dje@google.com>
>
>        * target.c (memory_xfer_partial): Only update dcache after we know
>        the write succeeded.
>
> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.223
> diff -u -p -r1.223 target.c
> --- target.c    8 Sep 2009 23:52:45 -0000       1.223
> +++ target.c    14 Sep 2009 19:11:38 -0000
> @@ -1289,19 +1289,6 @@ memory_xfer_partial (struct target_ops *
>        }
>     }
>
> -  /* 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);
> -    }
> -
>   /* If none of those methods found the memory we wanted, fall back
>      to a target partial transfer.  Normally a single call to
>      to_xfer_partial is enough; if it doesn't recognize an object
> @@ -1331,6 +1318,20 @@ memory_xfer_partial (struct target_ops *
>   if (readbuf && !show_memory_breakpoints)
>     breakpoint_restore_shadows (readbuf, memaddr, reg_len);
>
> +  /* 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 (res > 0
> +      && inf != NULL
> +      && writebuf != NULL
> +      && !region->attrib.cache
> +      && stack_cache_enabled_p
> +      && object != TARGET_OBJECT_STACK_MEMORY)
> +    {
> +      dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
> +    }
> +
>   /* If we still haven't got anything, return the last error.  We
>      give up.  */
>   return res;
>

Of course I did forget to pass the correct length.

009-09-14  Doug Evans  <dje@google.com>

        * target.c (memory_xfer_partial): Pass correct length to dcache_update.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.225
diff -u -p -r1.225 target.c
--- target.c    15 Sep 2009 03:30:06 -0000      1.225
+++ target.c    15 Sep 2009 06:54:16 -0000
@@ -1333,7 +1333,7 @@ memory_xfer_partial (struct target_ops *
       && stack_cache_enabled_p
       && object != TARGET_OBJECT_STACK_MEMORY)
     {
-      dcache_update (target_dcache, memaddr, (void *) writebuf, reg_len);
+      dcache_update (target_dcache, memaddr, (void *) writebuf, res);
     }

   /* If we still haven't got anything, return the last error.  We