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

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Mon, Sep 14, 2009 at 09:54, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
> Hi Hui,
>
> I'm seeing PRecord changing memory even though I answer
> the query as to not change it.  Please see below.
>
> Thanks
>
> Marc
>
> GNU gdb (GDB) 6.8.50.20090913-cvs
> Copyright (C) 2009 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-linux-gnu".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> Reading symbols from /home/marc/testing/a.out...done.
> (gdb) l
> 1       int main() {
> 2           int a = 1;
> 3           int b = 10;
> 4
> 5           a++;
> 6           b++;
> 7
> 8           return a;
> 9       }
> 10
> (gdb) start
> Temporary breakpoint 1 at 0x80483f5: file b.cc, line 2.
> Starting program: /home/marc/testing/a.out
> re
> Temporary breakpoint 1, main () at b.cc:2
> 2           int a = 1;
> (gdb) record
> (gdb) n
> 3           int b = 10;
> (gdb) n
> 5           a++;
> (gdb) n
> 6           b++;
> (gdb) rn
> 5           a++;
> (gdb) p a
> $1 = 1
> (gdb) set var a = 8
> Because GDB is in replay mode, writing to memory will make the execution log unusable from this point onward.  Write memory at address 0xbffff6a0?(y or [n]) n
> Process record canceled the operation.
> (gdb) p a
> $2 = 8
>



Hi Marc,

Thanks for your help.

I just tried change it with "p a=99".  I think it must have something
different with "set var a = 8".

This issue is because some value cache about the memory.  So I add a
"free_all_values ();" before error.
It looks OK now.  Please help me try it.

Thanks,
Hui

2009-09-14  Hui Zhu  <teawater@gmail.com>

	* record.c (record_xfer_partial): Call free_all_values when
	cancel the operation.

---
 record.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/record.c
+++ b/record.c
@@ -1023,7 +1023,10 @@ record_xfer_partial (struct target_ops *
 		         "will make the execution log unusable from this "
 		         "point onward.  Write memory at address %s?"),
 		       paddress (target_gdbarch, offset)))
-	    error (_("Process record canceled the operation."));
+	    {
+	      free_all_values ();
+	      error (_("Process record canceled the operation."));
+	    }

 	  /* Destroy the record from here forward.  */
 	  record_list_release_next ();

[-- Attachment #2: prec-fix-memory-cancel-bug.txt --]
[-- Type: text/plain, Size: 585 bytes --]

---
 record.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/record.c
+++ b/record.c
@@ -1023,7 +1023,10 @@ record_xfer_partial (struct target_ops *
 		         "will make the execution log unusable from this "
 		         "point onward.  Write memory at address %s?"),
 		       paddress (target_gdbarch, offset)))
-	    error (_("Process record canceled the operation."));
+	    {
+	      free_all_values ();
+	      error (_("Process record canceled the operation."));
+	    }
 
 	  /* Destroy the record from here forward.  */
 	  record_list_release_next ();

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

[Marc Khouzam]

 

> -----Original Message-----
> From: Hui Zhu [mailto:teawater@gmail.com] 
> Sent: Monday, September 14, 2009 12:40 AM
> To: Marc Khouzam
> Cc: gdb@sourceware.org; Michael Snyder; gdb-patches ml
> Subject: Re: PRecord sets memory even when it says it did not
> 
...
> 
> Hi Marc,
> 
> Thanks for your help.
> 
> I just tried change it with "p a=99".  I think it must have something
> different with "set var a = 8".

I also tried it with p a=8 and in my case, the same things happens:
the memory is changed.

> This issue is because some value cache about the memory.  So I add a
> "free_all_values ();" before error.
> It looks OK now.  Please help me try it.

I tried with the patch and it did not fix the problem.
Let me know if I can do anything to help debug.
I'm seing this on both SLED and Ubuntu.


Thanks,
Marc


> 
> 2009-09-14  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_xfer_partial): Call free_all_values when
> 	cancel the operation.
> 
> ---
>  record.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> --- a/record.c
> +++ b/record.c
> @@ -1023,7 +1023,10 @@ record_xfer_partial (struct target_ops *
>  		         "will make the execution log unusable 
> from this "
>  		         "point onward.  Write memory at address %s?"),
>  		       paddress (target_gdbarch, offset)))
> -	    error (_("Process record canceled the operation."));
> +	    {
> +	      free_all_values ();
> +	      error (_("Process record canceled the operation."));
> +	    }
> 
>  	  /* Destroy the record from here forward.  */
>  	  record_list_release_next ();
> 

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

[Daniel Jacobowitz]

On Mon, Sep 14, 2009 at 12:39:35PM +0800, Hui Zhu wrote:
> > (gdb) set var a = 8
> > Because GDB is in replay mode, writing to memory will make the execution log unusable from this point onward.  Write memory at address 0xbffff6a0?(y or [n]) n
> > Process record canceled the operation.
> > (gdb) p a
> > $2 = 8

This should refetch the value from the target.  What value is cached?

> 2009-09-14  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_xfer_partial): Call free_all_values when
> 	cancel the operation.

I don't think this is a good idea; this is a memory management
function.  It's not supposed to change the apparent values.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Hui Zhu]

On Mon, Sep 14, 2009 at 23:53, Daniel Jacobowitz <drow@false.org> wrote:
> On Mon, Sep 14, 2009 at 12:39:35PM +0800, Hui Zhu wrote:
>> > (gdb) set var a = 8
>> > Because GDB is in replay mode, writing to memory will make the execution log unusable from this point onward.  Write memory at address 0xbffff6a0?(y or [n]) n
>> > Process record canceled the operation.
>> > (gdb) p a
>> > $2 = 8
>
> This should refetch the value from the target.  What value is cached?

This value didn't really changed.  It just be changed in cache, but
not memory.  If you s or rs and check "a" again, you will found that
"a"'s value is right.

(gdb) set var a = 8
Because GDB is in replay mode, writing to memory will make the
execution log unusable from this point onward.  Write memory at
address 0xbffff500?(y or [n])
Process record canceled the operation.
(gdb) p a
$2 = 1
(gdb) set var a = 8
Because GDB is in replay mode, writing to memory will make the
execution log unusable from this point onward.  Write memory at
address 0xbffff500?(y or [n]) n
Process record canceled the operation.
(gdb) p a
$3 = 8
(gdb) rn
3	     int b = 10;
(gdb) p a
$4 = 1
(gdb) n
5	    a++;
(gdb) p a
$5 = 1
(gdb) n

No more reverse-execution history.
main () at 1.c:6
6	    b++;
(gdb) p a
$6 = 2
(gdb)


BTW, this is really a strange thing.  If answer is enter, everything
is OK.  If answer is no, it was changed.

>
>> 2009-09-14  Hui Zhu  <teawater@gmail.com>
>>
>>       * record.c (record_xfer_partial): Call free_all_values when
>>       cancel the operation.
>
> I don't think this is a good idea; this is a memory management
> function.  It's not supposed to change the apparent values.
>

Agree.  Looks we have a strange thing need to be handled.


Thanks,
Hui

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

[Marc Khouzam]

 

> -----Original Message-----
> From: Hui Zhu [mailto:teawater@gmail.com] 
> Sent: Monday, September 14, 2009 12:15 PM
> To: Daniel Jacobowitz; Marc Khouzam; gdb@sourceware.org; 
> Michael Snyder; gdb-patches ml
> Subject: Re: PRecord sets memory even when it says it did not
> 
> On Mon, Sep 14, 2009 at 23:53, Daniel Jacobowitz 
> <drow@false.org> wrote:
> > On Mon, Sep 14, 2009 at 12:39:35PM +0800, Hui Zhu wrote:
> >> > (gdb) set var a = 8
> >> > Because GDB is in replay mode, writing to memory will 
> make the execution log unusable from this point onward.  
> Write memory at address 0xbffff6a0?(y or [n]) n
> >> > Process record canceled the operation.
> >> > (gdb) p a
> >> > $2 = 8
> >
> > This should refetch the value from the target.  What value 
> is cached?
> 
> This value didn't really changed.  It just be changed in cache, but
> not memory.  If you s or rs and check "a" again, you will found that
> "a"'s value is right.

I think Hui is right.  I ran another test with printf and stopping precord
and the memory seems ok, please see session below.

> BTW, this is really a strange thing.  If answer is enter, everything
> is OK.  If answer is no, it was changed.

For me the problem happens both times, when pressing 'n' or <enter>.
(you can see that in the session below too)

(gdb) l
1       #include <stdio.h>
2       int main()
3       {
4         int a = 0;
5         printf("a is %d\n",a);
6         return 0;
7       }
(gdb) start
Temporary breakpoint 1 at 0x8048485: file a.cc, line 4.
Starting program: /local/lmckhou/testing/a.out 
re
Temporary breakpoint 1, main () at a.cc:4
4         int a = 0;
(gdb) rec
(gdb) n
5         printf("a is %d\n",a);
(gdb) n
a is 0
6         return 0;
(gdb) rn
5         printf("a is %d\n",a);
(gdb) p a
$1 = 0
(gdb) set var a=8
Because GDB is in replay mode, writing to memory will make the execution log unusable from this point onward.  Write memory at address 0xbfffadf0?(y or [n]) 
Process record canceled the operation.
(gdb) p a
$2 = 8
(gdb) rec stop
Delete recorded log and stop recording?(y or n) y
(gdb) p a
$3 = 8
(gdb) n
a is 0        <------------------------------------------ a is actually set correctly
6         return 0;
(gdb) p a
$4 = 0

> 
> 
> >
> >> 2009-09-14  Hui Zhu  <teawater@gmail.com>
> >>
> >>       * record.c (record_xfer_partial): Call free_all_values when
> >>       cancel the operation.
> >
> > I don't think this is a good idea; this is a memory management
> > function.  It's not supposed to change the apparent values.
> >
> 
> Agree.  Looks we have a strange thing need to be handled.
> 
> 
> Thanks,
> Hui
> 

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

[Greg Law]

Hui Zhu wrote:
> On Mon, Sep 14, 2009 at 09:54, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
>> Hi Hui,
>>
>> I'm seeing PRecord changing memory even though I answer
>> the query as to not change it.  Please see below.
>>
>> Thanks
>>
>> Marc
>>
>> GNU gdb (GDB) 6.8.50.20090913-cvs
>> Copyright (C) 2009 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "i686-pc-linux-gnu".
>> For bug reporting instructions, please see:
>> <http://www.gnu.org/software/gdb/bugs/>...
>> Reading symbols from /home/marc/testing/a.out...done.
>> (gdb) l
>> 1       int main() {
>> 2           int a = 1;
>> 3           int b = 10;
>> 4
>> 5           a++;
>> 6           b++;
>> 7
>> 8           return a;
>> 9       }
>> 10
>> (gdb) start
>> Temporary breakpoint 1 at 0x80483f5: file b.cc, line 2.
>> Starting program: /home/marc/testing/a.out
>> re
>> Temporary breakpoint 1, main () at b.cc:2
>> 2           int a = 1;
>> (gdb) record
>> (gdb) n
>> 3           int b = 10;
>> (gdb) n
>> 5           a++;
>> (gdb) n
>> 6           b++;
>> (gdb) rn
>> 5           a++;
>> (gdb) p a
>> $1 = 1
>> (gdb) set var a = 8
>> Because GDB is in replay mode, writing to memory will make the execution log unusable from this point onward.  Write memory at address 0xbffff6a0?(y or [n]) n
>> Process record canceled the operation.
>> (gdb) p a
>> $2 = 8
>>
> 
> 
> 
> Hi Marc,
> 
> Thanks for your help.
> 
> I just tried change it with "p a=99".  I think it must have something
> different with "set var a = 8".

I've noticed something similar with UndoDB.  Until very recently, if we
failed a 'poke' operation (which we do when in replay mode) the data
would not be changed.  But with a recent gdb built from cvs (as of about 
two weeks ago), despite UndoDB failing the poke, the value still appears
to the user to have been written.

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

Cheers,

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

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

[Michael Snyder]

Marc Khouzam wrote:
>  
> 
>> -----Original Message-----
>> From: Hui Zhu [mailto:teawater@gmail.com] 
>> Sent: Monday, September 14, 2009 12:40 AM
>> To: Marc Khouzam
>> Cc: gdb@sourceware.org; Michael Snyder; gdb-patches ml
>> Subject: Re: PRecord sets memory even when it says it did not
>>
> ...
>> Hi Marc,
>>
>> Thanks for your help.
>>
>> I just tried change it with "p a=99".  I think it must have something
>> different with "set var a = 8".
> 
> I also tried it with p a=8 and in my case, the same things happens:
> the memory is changed.
> 
>> This issue is because some value cache about the memory.  So I add a
>> "free_all_values ();" before error.
>> It looks OK now.  Please help me try it.
> 
> I tried with the patch and it did not fix the problem.
> Let me know if I can do anything to help debug.
> I'm seing this on both SLED and Ubuntu.

Mark --

By any chance, if you change the memory, and then do
(say) a "stepi", does the memory revert to its previous
value?  Just wondering...

Michael

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

[Marc Khouzam]

> -----Original Message-----
> From: Greg Law [mailto:glaw@undo-software.com] 
> Sent: Monday, September 14, 2009 1:11 PM
> To: Hui Zhu
> Cc: Marc Khouzam; gdb@sourceware.org; Michael Snyder; gdb-patches ml
> Subject: Re: PRecord sets memory even when it says it did not
> 
...
> 
> I've noticed something similar with UndoDB.  Until very 
> recently, if we
> failed a 'poke' operation (which we do when in replay mode) the data
> would not be changed.  But with a recent gdb built from cvs 
> (as of about 
> two weeks ago), despite UndoDB failing the poke, the value 
> still appears
> to the user to have been written.
> 
> 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).

This does seem to be a recent problem because Hui had fixed it
on July 20th and now it is broken again:
http://sourceware.org/ml/gdb-patches/2009-07/msg00504.html

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

[Marc Khouzam]

 

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Michael Snyder
> Sent: Monday, September 14, 2009 1:14 PM
> To: Marc Khouzam
> Cc: 'Hui Zhu'; 'gdb-patches ml'
> Subject: Re: PRecord sets memory even when it says it did not
> 
...

> Mark --
> 
> By any chance, if you change the memory, and then do
> (say) a "stepi", does the memory revert to its previous
> value?  Just wondering...

I just ried and you are right, after stepi, 
p a
shows the previous value.

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

[Michael Snyder]

Marc Khouzam wrote:
>  
> 
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org 
>> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Michael Snyder
>> Sent: Monday, September 14, 2009 1:14 PM
>> To: Marc Khouzam
>> Cc: 'Hui Zhu'; 'gdb-patches ml'
>> Subject: Re: PRecord sets memory even when it says it did not
>>
> ...
> 
>> Mark --
>>
>> By any chance, if you change the memory, and then do
>> (say) a "stepi", does the memory revert to its previous
>> value?  Just wondering...
> 
> I just ried and you are right, after stepi, 
> p a
> shows the previous value.

Right.  So then, the actual memory in the child process
was not changed.  Some kind of local cache was changed,
and when we did "stepi", we flushed the cache.

As Greg Law mentioned, I seem to recall that there have
been some recent changes in memory caching.

Sounds to me as if the cache accepts the change
before the target rejects it.

Michael

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

[Pedro Alves]

On Monday 14 September 2009 18:10:40, Greg Law wrote:
> Hui Zhu wrote:
> > On Mon, Sep 14, 2009 at 09:54, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
> > I just tried change it with "p a=99".  I think it must have something
> > different with "set var a = 8".
> 
> I've noticed something similar with UndoDB.  Until very recently, if we
> failed a 'poke' operation (which we do when in replay mode) the data
> would not be changed.  But with a recent gdb built from cvs (as of about 
> two weeks ago), despite UndoDB failing the poke, the value still appears
> to the user to have been written.
> 
> 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".

-- 
Pedro Alves

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

[Marc Khouzam]

 

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

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

[Michael Snyder]

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.

That's the change, and here's the problem I think:
target.c::memory_xfer_partial calls dcache_xfer_memory
and dcache_update before calling ops->to_xfer_partial.

In this case, record_xfer_partial errors out, (because of
a query, but for this or other targets it could error out
for other reasons).  But there is no cleanup in place to
back out the change that has already been made in the
dcache.

Michael

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

[Pedro Alves]

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.

-- 
Pedro Alves

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

[Michael Snyder]

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?

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

[Pedro Alves]

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.

-- 
Pedro Alves

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

[Michael Snyder]

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.

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

[Pedro Alves]

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

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

[Doug Evans]

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]

Re: 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.

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

[Pedro Alves]

On Monday 14 September 2009 20:04:12, Doug Evans wrote:

> 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]

That's fine with me as well.


There's one wrinkly corner case not being handled.  If the to_xfer_partial
routine throws an error, say a ptrace error of some kind (usually
perror_with_name), the ops target may still have succeeded in writing a
part of that partial transfer, but, the matching caches lines aren't
being invalidated/committed.  This is fine if we assume that errors
are only thrown before attempting any transfer at all, but that is not
reality.  E.g., when xfering [1000, 2000) in a single go, that may
fail half way, say, at 1500 with a hard thrown error.  Only thing 100%
safe to in these cases is to just flush the whole cache area that was
attempted to be written, or perhaps simpler, the whole cache.  Not
something you'd see hapenning every day, but still something
to consider at some point.

-- 
Pedro Alves

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

[Doug Evans]

On Mon, Sep 14, 2009 at 1:35 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> There's one wrinkly corner case not being handled.  If the to_xfer_partial
> routine throws an error, say a ptrace error of some kind (usually
> perror_with_name), the ops target may still have succeeded in writing a
> part of that partial transfer, but, the matching caches lines aren't
> being invalidated/committed.  This is fine if we assume that errors
> are only thrown before attempting any transfer at all, but that is not
> reality.  E.g., when xfering [1000, 2000) in a single go, that may
> fail half way, say, at 1500 with a hard thrown error.  Only thing 100%
> safe to in these cases is to just flush the whole cache area that was
> attempted to be written, or perhaps simpler, the whole cache.  Not
> something you'd see hapenning every day, but still something
> to consider at some point.

I would have thought that this case would be handled by returning the
amount actually written instead of throwing an error (and that not
doing so is a bug, regardless of the needs of dcache).
A single ptrace call won't fail leaving behind a partial write, but I
can imagine a sequence of ptrace writes ultimately failing.