[PATCH/python] notify memory changed.

[PATCH/python] notify memory changed.

[Yao Qi]

When I was looking at observer 'memory_changed' and the callers of
'write_memory', I think observer 'memory_changed' should be notified
after write_memory in infpy_write_memory.  Regression tested
on x86_64-linux native and gdbserver.  OK to apply?

Note that write_memory is called in many places without notifying
'memory_changed' observer, because most of these memory writes are
for either inferior call or displaced stepping.

gdb:

2012-06-18  Yao Qi  <yao@codesourcery.com>

	* python/py-inferior.c (infpy_write_memory): Invoke
	observer_notify_memory_changed.
---
 gdb/python/py-inferior.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index efbf14b..bdd39b2 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -494,6 +494,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
 	  break;
 	}
       write_memory (addr, buffer, length);
+      observer_notify_memory_changed (addr, length, buffer);
     }
   GDB_PY_HANDLE_EXCEPTION (except);
 
-- 
1.7.0.4

Re: [PATCH/python] notify memory changed.

[Joel Brobecker]

> 2012-06-18  Yao Qi  <yao@codesourcery.com>
> 
> 	* python/py-inferior.c (infpy_write_memory): Invoke
> 	observer_notify_memory_changed.

Just wondering if it shouldn't be write_memory itself that should
call observer_notify_memory_changed?

> ---
>  gdb/python/py-inferior.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index efbf14b..bdd39b2 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -494,6 +494,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  	  break;
>  	}
>        write_memory (addr, buffer, length);
> +      observer_notify_memory_changed (addr, length, buffer);
>      }
>    GDB_PY_HANDLE_EXCEPTION (except);
>  
> -- 
> 1.7.0.4

-- 
Joel

Re: [PATCH/python] notify memory changed.

[Yao Qi]

On Tuesday 19 June 2012 08:13:44 Joel Brobecker wrote:
> > 2012-06-18  Yao Qi  <yao@codesourcery.com>
> >
> > 
> >
> >       * python/py-inferior.c (infpy_write_memory): Invoke
> >       observer_notify_memory_changed.
> 
> Just wondering if it shouldn't be write_memory itself that should
> call observer_notify_memory_changed?

So far, observer_notify_memory_changed is called in two places, ada-
lang.c:ada_value_assign and valops.c:value_assign.  Looks like they are all 
about memory changes by user-requested operations.  Change in this patch 
belongs to this type (user-requested changes).

The rest of calls to write_memory are about gdb-requested changes to achieve 
some functions, such as inferior call or displaced stepping.  I didn't think 
of carefully the usefulness of notifying observers for operations during inf-
call or displaced stepping.

-- 
Yao (齐尧)

Re: [PATCH/python] notify memory changed.

[Joel Brobecker]

> The rest of calls to write_memory are about gdb-requested changes to achieve 
> some functions, such as inferior call or displaced stepping.  I didn't think 
> of carefully the usefulness of notifying observers for operations during inf-
> call or displaced stepping.

Hmmm, yes, it is true that we probably don't want to alert the observers
for these (kind of temporary) memory writes. I still worry that we are
going to keep making the same mistake as the one you are fixing here,
but for now I see your point.

Perhaps, for later, we might want to have a look at having two routines,
one "silent", and one that notifies the observers, a little bit like
the routine we have for adding new threads, for instance.

-- 
Joel

Re: [PATCH/python] notify memory changed.

[Yao Qi]

On Wednesday 20 June 2012 07:52:22 Joel Brobecker wrote:
> Perhaps, for later, we might want to have a look at having two routines,
> one "silent", and one that notifies the observers, a little bit like
> the routine we have for adding new threads, for instance.

Agreed.  Here is a new one.

-- 
Yao (齐尧)


2012-06-21  Yao Qi  <yao@codesourcery.com>

	* corefile.c (write_memory_with_notification): New.
	Include "observer.h".
	* gdbcore.h: Declare write_memory_with_notification.
	* ada-lang.c (ada_value_assign): Replace 'write_memory' and
	'observer_notify_memory_changed' with 'write_memory_with_notification'.
	* valops.c (value_assign): Likewise.
	* python/py-inferior.c (infpy_write_memory): Call
	'write_memory_with_notification'.

---
 gdb/ada-lang.c           |    3 +--
 gdb/corefile.c           |   11 +++++++++++
 gdb/gdbcore.h            |    6 ++++++
 gdb/python/py-inferior.c |    2 +-
 gdb/valops.c             |    4 +---
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6f65472..7afcef8 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2534,8 +2534,7 @@ ada_value_assign (struct value *toval, struct value 
*fromval)
       else
         move_bits (buffer, value_bitpos (toval),
 		   value_contents (fromval), 0, bits, 0);
-      write_memory (to_addr, buffer, len);
-      observer_notify_memory_changed (to_addr, len, buffer);
+      write_memory_with_notification (to_addr, buffer, len);
 
       val = value_copy (toval);
       memcpy (value_contents_raw (val), value_contents (fromval),
diff --git a/gdb/corefile.c b/gdb/corefile.c
index 611cd62..ac8eff5 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -34,6 +34,7 @@
 #include "gdb_stat.h"
 #include "completer.h"
 #include "exceptions.h"
+#include "observer.h"
 
 /* Local function declarations.  */
 
@@ -361,6 +362,16 @@ write_memory (CORE_ADDR memaddr,
     memory_error (status, memaddr);
 }
 
+/* Same as write_memory, but notify 'memory_changed' observers.  */
+
+void
+write_memory_with_notification (CORE_ADDR memaddr, const bfd_byte *myaddr,
+				ssize_t len)
+{
+  write_memory (memaddr, myaddr, len);
+  observer_notify_memory_changed (memaddr, len, myaddr);
+}
+
 /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned
    integer.  */
 void
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index 1081f3f..d6c9de2 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -86,6 +86,12 @@ CORE_ADDR read_memory_typed_address (CORE_ADDR addr, struct 
type *type);
 extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 			  ssize_t len);
 
+/* Same as write_memory, but notify 'memory_changed' observers.  */
+
+extern void write_memory_with_notification (CORE_ADDR memaddr,
+					    const bfd_byte *myaddr,
+					    ssize_t len);
+
 /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned integer.  */
 extern void write_memory_unsigned_integer (CORE_ADDR addr, int len,
                                            enum bfd_endian byte_order,
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 0ea4f55..aa41073 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -493,7 +493,7 @@ infpy_write_memory (PyObject *self, PyObject *args, 
PyObject *kw)
 	  error = 1;
 	  break;
 	}
-      write_memory (addr, buffer, length);
+      write_memory_with_notification (addr, buffer, length);
     }
   GDB_PY_HANDLE_EXCEPTION (except);
 
diff --git a/gdb/valops.c b/gdb/valops.c
index 5002272..97d889b 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1299,9 +1299,7 @@ value_assign (struct value *toval, struct value 
*fromval)
 	    dest_buffer = value_contents (fromval);
 	  }
 
-	write_memory (changed_addr, dest_buffer, changed_len);
-	observer_notify_memory_changed (changed_addr, changed_len,
-					dest_buffer);
+	write_memory_with_notification (changed_addr, dest_buffer, changed_len);
       }
       break;
 
-- 
1.7.7.6

RE: [PATCH/python] notify memory changed.

[Abid, Hafiz]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Qi, Yao
> Sent: Thursday, June 21, 2012 2:54 AM
> To: gdb-patches@sourceware.org
> Cc: Joel Brobecker
> Subject: Re: [PATCH/python] notify memory changed.
> 
> On Wednesday 20 June 2012 07:52:22 Joel Brobecker wrote:
> > Perhaps, for later, we might want to have a look at having two
> routines,
> > one "silent", and one that notifies the observers, a little bit like
> > the routine we have for adding new threads, for instance.
> 
> Agreed.  Here is a new one.
> 
> --
> Yao (齐尧)
> 
> 
> 2012-06-21  Yao Qi  <yao@codesourcery.com>
> 
> 	* corefile.c (write_memory_with_notification): New.
> 	Include "observer.h".
> 	* gdbcore.h: Declare write_memory_with_notification.
> 	* ada-lang.c (ada_value_assign): Replace 'write_memory' and
> 	'observer_notify_memory_changed' with
> 'write_memory_with_notification'.
> 	* valops.c (value_assign): Likewise.
> 	* python/py-inferior.c (infpy_write_memory): Call
> 	'write_memory_with_notification'.
> 
> ---
>  gdb/ada-lang.c           |    3 +--
>  gdb/corefile.c           |   11 +++++++++++
>  gdb/gdbcore.h            |    6 ++++++
>  gdb/python/py-inferior.c |    2 +-
>  gdb/valops.c             |    4 +---
>  5 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 6f65472..7afcef8 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2534,8 +2534,7 @@ ada_value_assign (struct value *toval, struct
> value
> *fromval)
>        else
>          move_bits (buffer, value_bitpos (toval),
>  		   value_contents (fromval), 0, bits, 0);
> -      write_memory (to_addr, buffer, len);
> -      observer_notify_memory_changed (to_addr, len, buffer);
> +      write_memory_with_notification (to_addr, buffer, len);
> 
>        val = value_copy (toval);
>        memcpy (value_contents_raw (val), value_contents (fromval),
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index 611cd62..ac8eff5 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -34,6 +34,7 @@
>  #include "gdb_stat.h"
>  #include "completer.h"
>  #include "exceptions.h"
> +#include "observer.h"
> 
>  /* Local function declarations.  */
> 
> @@ -361,6 +362,16 @@ write_memory (CORE_ADDR memaddr,
>      memory_error (status, memaddr);
>  }
> 
> +/* Same as write_memory, but notify 'memory_changed' observers.  */
> +
> +void
> +write_memory_with_notification (CORE_ADDR memaddr, const bfd_byte
> *myaddr,
> +				ssize_t len)
> +{
> +  write_memory (memaddr, myaddr, len);
> +  observer_notify_memory_changed (memaddr, len, myaddr);
> +}
> +
Do you think it will be useful to have parameters of observer_notify_memory_changed in same order as write_memory. When I looked at this code, I had to look at the function to make sure it was not a typo.

>  /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned
>     integer.  */
>  void
> diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
> index 1081f3f..d6c9de2 100644
> --- a/gdb/gdbcore.h
> +++ b/gdb/gdbcore.h
> @@ -86,6 +86,12 @@ CORE_ADDR read_memory_typed_address (CORE_ADDR addr,
> struct
> type *type);
>  extern void write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  			  ssize_t len);
> 
> +/* Same as write_memory, but notify 'memory_changed' observers.  */
> +
> +extern void write_memory_with_notification (CORE_ADDR memaddr,
> +					    const bfd_byte *myaddr,
> +					    ssize_t len);
> +
>  /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned integer.
> */
>  extern void write_memory_unsigned_integer (CORE_ADDR addr, int len,
>                                             enum bfd_endian byte_order,
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 0ea4f55..aa41073 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -493,7 +493,7 @@ infpy_write_memory (PyObject *self, PyObject *args,
> PyObject *kw)
>  	  error = 1;
>  	  break;
>  	}
> -      write_memory (addr, buffer, length);
> +      write_memory_with_notification (addr, buffer, length);
>      }
>    GDB_PY_HANDLE_EXCEPTION (except);
> 
> diff --git a/gdb/valops.c b/gdb/valops.c
> index 5002272..97d889b 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1299,9 +1299,7 @@ value_assign (struct value *toval, struct value
> *fromval)
>  	    dest_buffer = value_contents (fromval);
>  	  }
> 
> -	write_memory (changed_addr, dest_buffer, changed_len);
> -	observer_notify_memory_changed (changed_addr, changed_len,
> -					dest_buffer);
> +	write_memory_with_notification (changed_addr, dest_buffer,
> changed_len);
>        }
>        break;
> 
> --
> 1.7.7.6
> 

Re: [PATCH/python] notify memory changed.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-06-21  Yao Qi  <yao@codesourcery.com>
Yao> 	* corefile.c (write_memory_with_notification): New.
Yao> 	Include "observer.h".
Yao> 	* gdbcore.h: Declare write_memory_with_notification.
Yao> 	* ada-lang.c (ada_value_assign): Replace 'write_memory' and
Yao> 	'observer_notify_memory_changed' with 'write_memory_with_notification'.
Yao> 	* valops.c (value_assign): Likewise.
Yao> 	* python/py-inferior.c (infpy_write_memory): Call
Yao> 	'write_memory_with_notification'.

Ok.

Tom

Re: [PATCH/python] notify memory changed.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

>> Do you think it will be useful to have parameters of
>> observer_notify_memory_changed in same order as write_memory. When I looked
>> at this code, I had to look at the function to make sure it was not a typo.

Yao> I am not sure.  Both 'write_memory' and 'observer_notify_memory_changed' 
Yao> exists here for some years, and I guess people get use to them.

It is fine if you want to change it.
I agree it would be clearer.

Tom