[RFC] MI notification on register changes

[RFC] MI notification on register changes

[Yao Qi]

Hi,
User can type command in console 'set $reg = foo' to modify register.
The modification may not really happen in registers, because register
is already saved in frame, and modification really goes to memory (frame).
The new MI notification this patch adds is about 'register change' from
user's point of view, instead of from machine's point of view.

Usually, MI frontend has a 'register view' to show the contents of
registers.  When users modify registers on a certain frame, this
notification is useful to tell MI frontend to refresh its 'register
view'.

This patch also extends the usage of field 'regnum' of 'struct value'.
Originally, this field is used to store the register number if content
is from register.  With this patch, this field also store the register
number if it "presents" a register.  For example, on non-innermost
frame, the value "reg" may be from memory instead of register.

   set $reg = foo

In order to emit 'register changed' notification, we have to know
whether a value presents a register or not.  I am not very sure about
the change here, so comments or advices are appreciated.

gdb/doc:

2012-10-23  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (GDB/MI Async Records): Add '=register-changed'.
	* observer.texi (GDB Observers): Add observer
	'register_changed'.
gdb:

2012-10-23  Yao Qi  <yao@codesourcery.com>

	* mi/mi-cmd-var.c (mi_cmd_var_assign): Set
	mi_suppress_notification.register_changed to 1 and restore it
	on exit.
	* mi/mi-interp.c: Include "regcache.h".
	Declare mi_register_changed.
	(mi_interpreter_init): Install mi_register_changed to observer
	'register_changed'.
	(mi_register_changed): New.
	* mi/mi-main.h (struct mi_suppress_notification)
	<register_changed>: New.

	* value.c (struct value) <regnum>: Update comments.
	* eval.c (evaluate_subexp_standard):

	* valops.c (value_assign): Get frame info of 'toval' earlier.
	(value_assign): Call observer_notify_register_changed if
	'toval' is related to a register.

	* NEWS: Mention it.

gdb/testsuite/

2012-10-23  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-reg-changed.c: New.
	* gdb.mi/mi-reg-changed.exp: New.
	* gdb.mi/mi-cli.exp: Update tests for new MI notification.
---
 gdb/NEWS                                |    2 +
 gdb/doc/gdb.texinfo                     |    5 +
 gdb/doc/observer.texi                   |    5 +
 gdb/eval.c                              |   12 +++-
 gdb/mi/mi-cmd-var.c                     |   12 ++-
 gdb/mi/mi-interp.c                      |   49 ++++++++++++
 gdb/mi/mi-main.h                        |    2 +
 gdb/testsuite/gdb.mi/mi-cli.exp         |    6 +-
 gdb/testsuite/gdb.mi/mi-reg-changed.c   |   30 +++++++
 gdb/testsuite/gdb.mi/mi-reg-changed.exp |  129 +++++++++++++++++++++++++++++++
 gdb/valops.c                            |   12 ++-
 gdb/value.c                             |    4 +-
 12 files changed, 254 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 485c21b..4660f99 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -68,6 +68,8 @@ py [command]
      "=tracepoint-downloaded".
   ** Passcount of tracepoint changes are now notified using new async record
      "=tracepoint-modified".
+  ** Register changes are now notified using new async record
+     "=register-changed".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 64cda47..b4b5c5f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27714,6 +27714,11 @@ written in an inferior.  The @var{id} is the identifier of the
 thread group corresponding to the affected inferior.  The optional
 @code{type="code"} part is reported if the memory written to holds
 executable code.
+
+@item =register-changed,name=@var{name},value=@var{value},group-id=@var{id},frame=@var{frame}
+Reports that register @var{name} on frame @var{frame} was changed to
+@var{value} in an inferior.  The @var{id} is the identifier of the
+thread group corresponding to the affected inferior.
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index e2033a6..bb16307 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -98,6 +98,11 @@ inferior has stopped.
 The target's register contents have changed.
 @end deftypefun
 
+@deftypefun void register_changed (int @var{reg_num}, struct frame_info *@var{frame}, ptid_t @var{inferior_ptid}, const gdb_byte *@var{buf})
+The content of register @var{reg_num} in frame @var{frame} inferior
+@var{inferior_ptid} has changed to the new content pointed by @var{buf}.
+@end deftypefun
+
 @deftypefun void executable_changed (void)
 The executable being debugged by GDB has changed: The user decided
 to debug a different program, or the program he was debugging has
diff --git a/gdb/eval.c b/gdb/eval.c
index 26e0cc8..b099499 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -939,7 +939,17 @@ evaluate_subexp_standard (struct type *expect_type,
 	if (val == NULL)
 	  error (_("Value of register %s not available."), name);
 	else
-	  return val;
+	  {
+	    /* Store the register number if it is not a user register.
+	       We don't have an easy way to get the number of cooked/frame
+	       register for a given user register, because on some arch,
+	       a 64-bit user register is composed by two 32-bit frame
+	       registers.  */
+	    if (regno < (gdbarch_num_regs (exp->gdbarch)
+			 + gdbarch_num_pseudo_regs (exp->gdbarch)))
+	      VALUE_REGNUM (val) = regno;
+	    return val;
+	  }
       }
     case OP_BOOL:
       (*pos) += 2;
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index dc47bc1..41cd889 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -618,6 +618,7 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
   struct varobj *var;
   char *expression, *val;
   struct cleanup *cleanup;
+  int *p;
 
   if (argc != 2)
     error (_("-var-assign: Usage: NAME EXPRESSION."));
@@ -630,11 +631,14 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
 
   expression = xstrdup (argv[1]);
 
-  /* MI command '-var-assign' may write memory, so suppress memory
-     changed notification if it does.  */
-  cleanup
-    = make_cleanup_restore_integer (&mi_suppress_notification.memory);
+  /* MI command '-var-assign' may write memory or register, so suppress
+     memory or register changed notification if it does.  */
+  p = &mi_suppress_notification.memory;
+  make_cleanup_restore_integer (p);
   mi_suppress_notification.memory = 1;
+  p = &mi_suppress_notification.register_changed;
+  cleanup = make_cleanup_restore_integer (p);
+  mi_suppress_notification.register_changed = 1;
 
   if (!varobj_set_value (var, expression))
     error (_("-var-assign: Could not assign "
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index ec14f03..86b0e5b 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -36,6 +36,7 @@
 #include "solist.h"
 #include "gdb.h"
 #include "objfiles.h"
+#include "regcache.h"
 
 /* These are the interpreter setup, etc. functions for the MI
    interpreter.  */
@@ -81,6 +82,8 @@ static void mi_tracepoint_downloaded (struct bp_location *loc);
 static void mi_command_param_changed (const char *param, const char *value);
 static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
 			       ssize_t len, const bfd_byte *myaddr);
+static void mi_register_changed (int regnum, struct frame_info *frame,
+				 ptid_t ptid, const gdb_byte *buf);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -146,6 +149,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_tracepoint_downloaded (mi_tracepoint_downloaded);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
+      observer_attach_register_changed (mi_register_changed);
 
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
@@ -924,6 +928,51 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about the register change.  */
+
+static void
+mi_register_changed (int regnum, struct frame_info *frame, ptid_t ptid,
+		     const gdb_byte *buf)
+{
+  if (!mi_suppress_notification.register_changed)
+    {
+      struct mi_interp *mi = top_level_interpreter_data ();
+      struct ui_out *mi_uiout
+	= interp_ui_out (top_level_interpreter ());
+      struct gdbarch *gdbarch = get_frame_arch (frame);
+      int regsize = register_size (gdbarch, regnum);
+      ULONGEST val = extract_unsigned_integer (buf, regsize,
+					       gdbarch_byte_order (gdbarch));
+      struct thread_info *ti = find_thread_ptid (ptid);
+      struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+      struct ui_out *saved_uiout;
+
+      gdb_assert (ti);
+      gdb_assert (inf);
+
+      target_terminal_ours ();
+
+      fprintf_unfiltered (mi->event_channel, "register-changed");
+
+      ui_out_redirect (mi_uiout, mi->event_channel);
+
+      ui_out_field_string (mi_uiout, "name",
+			   gdbarch_register_name (gdbarch, regnum));
+      ui_out_field_fmt (mi_uiout, "value", "0x%s", phex_nz (val, regsize));
+      ui_out_field_int (mi_uiout, "group-id", inf->num);
+      ui_out_field_int (mi_uiout, "thread-id", ti->num);
+
+      saved_uiout = current_uiout;
+      current_uiout = mi_uiout;
+      print_stack_frame (frame, 0, SRC_AND_LOC);
+      current_uiout = saved_uiout;
+
+      ui_out_redirect (mi_uiout, NULL);
+
+      gdb_flush (mi->event_channel);
+    }
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index a815fbe..0f4b064 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -43,6 +43,8 @@ struct mi_suppress_notification
   int traceframe;
   /* Memory changed notification suppressed?  */
   int memory;
+  /* Register changed notification suppressed?  */
+  int register_changed;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp
index bab2373..ad0cdb8 100644
--- a/gdb/testsuite/gdb.mi/mi-cli.exp
+++ b/gdb/testsuite/gdb.mi/mi-cli.exp
@@ -188,12 +188,8 @@ mi_gdb_test "-interpreter-exec console \"help set args\"" \
   {\~"Set argument list to give program being debugged when it is started\.\\nFollow this command with any number of args, to be passed to the program\.".*\^done} \
   "-interpreter-exec console \"help set args\""
 
-# NOTE: cagney/2003-02-03: Not yet.
-# mi_gdb_test "-interpreter-exec console \"set \$pc=0x0\"" \
-#   {.*=target-changed.*\^done} \
-#   "-interpreter-exec console \"set \$pc=0x0\""
 mi_gdb_test "888-interpreter-exec console \"set \$pc=0x0\"" \
-  {888\^done} \
+  {.*=register-changed,name=\".*\",value=\"0x0\",group-id=\".*\",frame=.*\^done} \
   "-interpreter-exec console \"set \$pc=0x0\""
 
 #mi_gdb_test "-interpreter-exec console \"\"" \
diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.c b/gdb/testsuite/gdb.mi/mi-reg-changed.c
new file mode 100644
index 0000000..3d6950b
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-reg-changed.c
@@ -0,0 +1,30 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static int
+middle (int x)
+{
+  x = 2 * x;
+  x++;
+  return x;
+}
+
+int
+main (int argc, char **argv)
+{
+  return middle (argc);
+}
diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.exp b/gdb/testsuite/gdb.mi/mi-reg-changed.exp
new file mode 100644
index 0000000..bd4037a
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-reg-changed.exp
@@ -0,0 +1,129 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile .c
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested mi-reg-changed.exp
+     return -1
+}
+
+proc test_register_change { } { with_test_prefix "simple" {
+    if [mi_gdb_start] {
+	return
+    }
+    mi_run_to_main
+
+    global hex
+    global decimal
+    global expect_out
+
+    set reg ""
+    # Extract the name of a register.
+    mi_gdb_test "-data-list-register-names" \
+	"\\\^done,register-names=\\\[\"(\[a-zA-Z0-9\]+)\".*\\\].*" \
+	"-data-list-register-names"
+    set reg $expect_out(3,string)
+
+    if { $reg == "" } {
+	fail "get the register name"
+	return
+    }
+
+    set reg_val ""
+    if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	set reg_val $expect_out(3,string)
+    }
+    if { $reg_val == "" } {
+	fail "get the content of register ${reg}"
+	return
+    }
+
+    mi_gdb_test "set \$${reg} = ${reg_val}" \
+	".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}.*\\^done" \
+	"change reg ${reg}"
+
+    mi_gdb_test "-var-create fp_var * \$${reg}" \
+	"\\^done,name=\"fp_var\",numchild=\"0\",value=\".*\",type=\".*\",has_more=\"0\"" \
+	"create objvar for reg ${reg}"
+    # GDB should not emit MI notification for register change requested
+    # from MI frontend.
+    mi_gdb_test "-var-assign fp_var ${reg_val}" \
+	"\\^done,value=\".*\"" "change reg ${reg} thru. varobj"
+
+    mi_gdb_exit
+}}
+
+
+proc test_register_on_frame { } { with_test_prefix "frame" {
+    if [mi_gdb_start] {
+	return
+    }
+    mi_run_to_main
+
+    global decimal
+    global hex
+    global expect_out
+
+    set reg ""
+
+    mi_gdb_test "-data-list-register-names" \
+	"\\\^done,register-names=\\\[\"(\[a-zA-Z0-9\]+)\".*\\\].*" \
+	"-data-list-register-names"
+    set reg $expect_out(3,string)
+
+    if { $reg == "" } {
+	fail "get the register name"
+	return
+    }
+
+    mi_gdb_test "-break-insert -t middle" \
+	"\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"del\".*" \
+	"-break-insert -t middle"
+    mi_execute_to "exec-continue" "breakpoint-hit" "middle" ".*" ".*" \
+	".*" { "" "disp=\"del\"" } "-exec-continue to middle"
+
+    set reg_val ""
+
+    if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	    set reg_val $expect_out(3,string)
+	}
+
+    mi_gdb_test "set \$${reg} = ${reg_val}" \
+	".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"middle\",.*}.*\\^done" \
+	"change reg ${reg}"
+
+    mi_gdb_test "up" ".*" "up"
+
+    set reg_val ""
+    if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	set reg_val $expect_out(3,string)
+    }
+
+    mi_gdb_test "set \$${reg} = ${reg_val}" \
+	".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}.*\\^done" \
+	    "change reg ${reg} in upper frame"
+
+    mi_gdb_exit
+}}
+
+test_register_change
+
+test_register_on_frame
+
+return 0
diff --git a/gdb/valops.c b/gdb/valops.c
index 502fb0d..256d29d 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval)
   struct type *type;
   struct value *val;
   struct frame_id old_frame;
+  struct frame_info *frame;
 
   if (!deprecated_value_modifiable (toval))
     error (_("Left operand of assignment is not a modifiable lvalue."));
@@ -1241,6 +1242,9 @@ value_assign (struct value *toval, struct value *fromval)
      and then restore the new frame afterwards.  */
   old_frame = get_frame_id (deprecated_safe_get_selected_frame ());
 
+  /* Figure out which frame this is in currently.  */
+  frame = frame_find_by_id (VALUE_FRAME_ID (toval));
+
   switch (VALUE_LVAL (toval))
     {
     case lval_internalvar:
@@ -1305,12 +1309,9 @@ value_assign (struct value *toval, struct value *fromval)
 
     case lval_register:
       {
-	struct frame_info *frame;
 	struct gdbarch *gdbarch;
 	int value_reg;
 
-	/* Figure out which frame this is in currently.  */
-	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
 	value_reg = VALUE_REGNUM (toval);
 
 	if (!frame)
@@ -1462,6 +1463,11 @@ value_assign (struct value *toval, struct value *fromval)
       set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
     }
 
+  if (VALUE_REGNUM (toval) >= 0)
+    observer_notify_register_changed (VALUE_REGNUM (toval), frame,
+				      inferior_ptid,
+				      value_contents (val));
+
   return val;
 }
 
diff --git a/gdb/value.c b/gdb/value.c
index 3feb1ca..7720cd1 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -318,7 +318,9 @@ struct value
      taken off this list.  */
   struct value *next;
 
-  /* Register number if the value is from a register.  */
+  /* Register number if the value presents a register.  The contents may
+     be from a register or memory, which is determined by field
+     'LVAL'.  */
   short regnum;
 
   /* Actual contents of the value.  Target byte-order.  NULL or not
-- 
1.7.7.6

ping: [RFC] MI notification on register changes

[Yao Qi]

On 10/23/2012 03:23 PM, Yao Qi wrote:
> gdb/doc:
>
> 2012-10-23  Yao Qi<yao@codesourcery.com>
>
> 	* gdb.texinfo (GDB/MI Async Records): Add '=register-changed'.
> 	* observer.texi (GDB Observers): Add observer
> 	'register_changed'.
> gdb:
>
> 2012-10-23  Yao Qi<yao@codesourcery.com>
>
> 	* mi/mi-cmd-var.c (mi_cmd_var_assign): Set
> 	mi_suppress_notification.register_changed to 1 and restore it
> 	on exit.
> 	* mi/mi-interp.c: Include "regcache.h".
> 	Declare mi_register_changed.
> 	(mi_interpreter_init): Install mi_register_changed to observer
> 	'register_changed'.
> 	(mi_register_changed): New.
> 	* mi/mi-main.h (struct mi_suppress_notification)
> 	<register_changed>: New.
>
> 	* value.c (struct value) <regnum>: Update comments.
> 	* eval.c (evaluate_subexp_standard):
>
> 	* valops.c (value_assign): Get frame info of 'toval' earlier.
> 	(value_assign): Call observer_notify_register_changed if
> 	'toval' is related to a register.
>
> 	* NEWS: Mention it.
>
> gdb/testsuite/
>
> 2012-10-23  Yao Qi<yao@codesourcery.com>
>
> 	* gdb.mi/mi-reg-changed.c: New.
> 	* gdb.mi/mi-reg-changed.exp: New.
> 	* gdb.mi/mi-cli.exp: Update tests for new MI notification.

Ping.  http://sourceware.org/ml/gdb-patches/2012-10/msg00401.html

-- 
Yao

Re: [RFC] MI notification on register changes

[Tom Tromey]

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

Yao> User can type command in console 'set $reg = foo' to modify register.
Yao> The modification may not really happen in registers, because register
Yao> is already saved in frame, and modification really goes to memory (frame).
Yao> The new MI notification this patch adds is about 'register change' from
Yao> user's point of view, instead of from machine's point of view.

Yao> Usually, MI frontend has a 'register view' to show the contents of
Yao> registers.  When users modify registers on a certain frame, this
Yao> notification is useful to tell MI frontend to refresh its 'register
Yao> view'.

This all seems reasonable to me.

Yao> This patch also extends the usage of field 'regnum' of 'struct value'.
Yao> Originally, this field is used to store the register number if content
Yao> is from register.  With this patch, this field also store the register
Yao> number if it "presents" a register.  For example, on non-innermost
Yao> frame, the value "reg" may be from memory instead of register.
Yao>    set $reg = foo

The register may be stored in memory, but it this case really ever
represented as lval_memory in gdb?  I thought the reason for
value::frame_id was to represent the notion of "register in a specific
frame", and that all registers are represented as lval_register.

Yao> +proc test_register_change { } { with_test_prefix "simple" {

For new tests I would prefer correct indentation.  That is,
with_test_prefix on its own line.  I think the existing tests are
written in this funny way to avoid a massive reindentation, but that
isn't a factor here.

Yao> +++ b/gdb/valops.c
Yao> @@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval)
 
Yao> +  /* Figure out which frame this is in currently.  */
Yao> +  frame = frame_find_by_id (VALUE_FRAME_ID (toval));
Yao> +

I find it pretty hard to convince myself that moving this out of the
switch can't cause some obscure problem.

How about hoisting the declaration of frame, initializing it to NULL,
and then changing this:

Yao> +  if (VALUE_REGNUM (toval) >= 0)
Yao> +    observer_notify_register_changed (VALUE_REGNUM (toval), frame,
Yao> +				      inferior_ptid,
Yao> +				      value_contents (val));

... to read 'if (frame != NULL)'?

Tom

Re: [RFC] MI notification on register changes

[Yao Qi]

Tom, thanks for the review.

On 11/02/2012 04:48 AM, Tom Tromey wrote:
> Yao> This patch also extends the usage of field 'regnum' of 'struct value'.
> Yao> Originally, this field is used to store the register number if content
> Yao> is from register.  With this patch, this field also store the register
> Yao> number if it "presents" a register.  For example, on non-innermost
> Yao> frame, the value "reg" may be from memory instead of register.
> Yao>    set $reg = foo
> 
> The register may be stored in memory, but it this case really ever
> represented as lval_memory in gdb?  I thought the reason for

Yes.

> value::frame_id was to represent the notion of "register in a specific
> frame", and that all registers are represented as lval_register.
> 

That is a good point.  I didn't realize that value::frame_id is for
register.  Then, the patch to frame used in 'register_change' observer
should be got in a different way.  New version reflects this.

> Yao> +proc test_register_change { } { with_test_prefix "simple" {
> 
> For new tests I would prefer correct indentation.  That is,
> with_test_prefix on its own line.  I think the existing tests are
> written in this funny way to avoid a massive reindentation, but that
> isn't a factor here.
> 

Fixed.

> Yao> +++ b/gdb/valops.c
> Yao> @@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval)
>   
> Yao> +  /* Figure out which frame this is in currently.  */
> Yao> +  frame = frame_find_by_id (VALUE_FRAME_ID (toval));
> Yao> +
> 
> I find it pretty hard to convince myself that moving this out of the
> switch can't cause some obscure problem.
> 
> How about hoisting the declaration of frame, initializing it to NULL,
> and then changing this:
> 

I hoist the declaration of frame, as you suggested, but set it to the
return value of deprecated_safe_get_selected_frame.  It will be updated
later if 'VALUE_LVAL (toval)' is lval_register, otherwise, we will use it
to pass to observer.

> Yao> +  if (VALUE_REGNUM (toval) >= 0)
> Yao> +    observer_notify_register_changed (VALUE_REGNUM (toval), frame,
> Yao> +				      inferior_ptid,
> Yao> +				      value_contents (val));
> 
> ... to read 'if (frame != NULL)'?

Done.  Besides these changes, the test case mi-reg-changed.exp is
updated a little bit to choose a call-saved register for the testing.

-- 
Yao

gdb/doc:

2012-11-02  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (GDB/MI Async Records): Add '=register-changed'.
	* observer.texi (GDB Observers): Add observer
	'register_changed'.
gdb:

2012-11-02  Yao Qi  <yao@codesourcery.com>

	* mi/mi-cmd-var.c (mi_cmd_var_assign): Set
	mi_suppress_notification.register_changed to 1 and restore it
	on exit.
	* mi/mi-interp.c: Include "regcache.h".
	Declare mi_register_changed.
	(mi_interpreter_init): Install mi_register_changed to observer
	'register_changed'.
	(mi_register_changed): New.
	* mi/mi-main.h (struct mi_suppress_notification)
	<register_changed>: New.
	* value.c (struct value) <regnum>: Update comments.
	* valops.c (value_assign): Get frame info of 'toval' earlier.
	(value_assign): Call observer_notify_register_changed if
	'toval' is related to a register.

	* NEWS: Mention it.

gdb/testsuite/

2012-11-02  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-reg-changed.c: New.
	* gdb.mi/mi-reg-changed.exp: New.
	* gdb.mi/mi-cli.exp: Update tests for new MI notification.
---
 gdb/NEWS                                |    2 +
 gdb/doc/gdb.texinfo                     |    5 +
 gdb/doc/observer.texi                   |    5 +
 gdb/eval.c                              |   12 ++-
 gdb/mi/mi-cmd-var.c                     |   12 ++-
 gdb/mi/mi-interp.c                      |   49 ++++++++++
 gdb/mi/mi-main.h                        |    2 +
 gdb/testsuite/gdb.mi/mi-cli.exp         |    6 +-
 gdb/testsuite/gdb.mi/mi-reg-changed.c   |   30 ++++++
 gdb/testsuite/gdb.mi/mi-reg-changed.exp |  159 +++++++++++++++++++++++++++++++
 gdb/valops.c                            |   10 ++-
 gdb/value.c                             |    4 +-
 12 files changed, 283 insertions(+), 13 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 069b84f..908f97d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -64,6 +64,8 @@ py [command]
      async record "=record-started" and "=record-stopped".
   ** Memory changes are now notified using new async record
      "=memory-changed".
+  ** Register changes are now notified using new async record
+     "=register-changed".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 21db475..f0a5e5d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27704,6 +27704,11 @@ written in an inferior.  The @var{id} is the identifier of the
 thread group corresponding to the affected inferior.  The optional
 @code{type="code"} part is reported if the memory written to holds
 executable code.
+
+@item =register-changed,name=@var{name},value=@var{value},group-id=@var{id},frame=@var{frame}
+Reports that register @var{name} on frame @var{frame} was changed to
+@var{value} in an inferior.  The @var{id} is the identifier of the
+thread group corresponding to the affected inferior.
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 106475b..54c566e 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -98,6 +98,11 @@ inferior has stopped.
 The target's register contents have changed.
 @end deftypefun
 
+@deftypefun void register_changed (int @var{reg_num}, struct frame_info *@var{frame}, ptid_t @var{inferior_ptid}, const gdb_byte *@var{buf})
+The content of register @var{reg_num} in frame @var{frame} inferior
+@var{inferior_ptid} has changed to the new content pointed by @var{buf}.
+@end deftypefun
+
 @deftypefun void executable_changed (void)
 The executable being debugged by GDB has changed: The user decided
 to debug a different program, or the program he was debugging has
diff --git a/gdb/eval.c b/gdb/eval.c
index 26e0cc8..b099499 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -939,7 +939,17 @@ evaluate_subexp_standard (struct type *expect_type,
 	if (val == NULL)
 	  error (_("Value of register %s not available."), name);
 	else
-	  return val;
+	  {
+	    /* Store the register number if it is not a user register.
+	       We don't have an easy way to get the number of cooked/frame
+	       register for a given user register, because on some arch,
+	       a 64-bit user register is composed by two 32-bit frame
+	       registers.  */
+	    if (regno < (gdbarch_num_regs (exp->gdbarch)
+			 + gdbarch_num_pseudo_regs (exp->gdbarch)))
+	      VALUE_REGNUM (val) = regno;
+	    return val;
+	  }
       }
     case OP_BOOL:
       (*pos) += 2;
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index dc47bc1..41cd889 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -618,6 +618,7 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
   struct varobj *var;
   char *expression, *val;
   struct cleanup *cleanup;
+  int *p;
 
   if (argc != 2)
     error (_("-var-assign: Usage: NAME EXPRESSION."));
@@ -630,11 +631,14 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
 
   expression = xstrdup (argv[1]);
 
-  /* MI command '-var-assign' may write memory, so suppress memory
-     changed notification if it does.  */
-  cleanup
-    = make_cleanup_restore_integer (&mi_suppress_notification.memory);
+  /* MI command '-var-assign' may write memory or register, so suppress
+     memory or register changed notification if it does.  */
+  p = &mi_suppress_notification.memory;
+  make_cleanup_restore_integer (p);
   mi_suppress_notification.memory = 1;
+  p = &mi_suppress_notification.register_changed;
+  cleanup = make_cleanup_restore_integer (p);
+  mi_suppress_notification.register_changed = 1;
 
   if (!varobj_set_value (var, expression))
     error (_("-var-assign: Could not assign "
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index d3c3d81..5c78b9a 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -36,6 +36,7 @@
 #include "solist.h"
 #include "gdb.h"
 #include "objfiles.h"
+#include "regcache.h"
 
 /* These are the interpreter setup, etc. functions for the MI
    interpreter.  */
@@ -79,6 +80,8 @@ static void mi_breakpoint_modified (struct breakpoint *b);
 static void mi_command_param_changed (const char *param, const char *value);
 static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
 			       ssize_t len, const bfd_byte *myaddr);
+static void mi_register_changed (int regnum, struct frame_info *frame,
+				 ptid_t ptid, const gdb_byte *buf);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -142,6 +145,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
+      observer_attach_register_changed (mi_register_changed);
 
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
@@ -885,6 +889,51 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about the register change.  */
+
+static void
+mi_register_changed (int regnum, struct frame_info *frame, ptid_t ptid,
+		     const gdb_byte *buf)
+{
+  if (!mi_suppress_notification.register_changed)
+    {
+      struct mi_interp *mi = top_level_interpreter_data ();
+      struct ui_out *mi_uiout
+	= interp_ui_out (top_level_interpreter ());
+      struct gdbarch *gdbarch = get_frame_arch (frame);
+      int regsize = register_size (gdbarch, regnum);
+      ULONGEST val = extract_unsigned_integer (buf, regsize,
+					       gdbarch_byte_order (gdbarch));
+      struct thread_info *ti = find_thread_ptid (ptid);
+      struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid));
+      struct ui_out *saved_uiout;
+
+      gdb_assert (ti);
+      gdb_assert (inf);
+
+      target_terminal_ours ();
+
+      fprintf_unfiltered (mi->event_channel, "register-changed");
+
+      ui_out_redirect (mi_uiout, mi->event_channel);
+
+      ui_out_field_string (mi_uiout, "name",
+			   gdbarch_register_name (gdbarch, regnum));
+      ui_out_field_fmt (mi_uiout, "value", "0x%s", phex_nz (val, regsize));
+      ui_out_field_int (mi_uiout, "group-id", inf->num);
+      ui_out_field_int (mi_uiout, "thread-id", ti->num);
+
+      saved_uiout = current_uiout;
+      current_uiout = mi_uiout;
+      print_stack_frame (frame, 0, SRC_AND_LOC);
+      current_uiout = saved_uiout;
+
+      ui_out_redirect (mi_uiout, NULL);
+
+      gdb_flush (mi->event_channel);
+    }
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index a815fbe..0f4b064 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -43,6 +43,8 @@ struct mi_suppress_notification
   int traceframe;
   /* Memory changed notification suppressed?  */
   int memory;
+  /* Register changed notification suppressed?  */
+  int register_changed;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp
index bab2373..ad0cdb8 100644
--- a/gdb/testsuite/gdb.mi/mi-cli.exp
+++ b/gdb/testsuite/gdb.mi/mi-cli.exp
@@ -188,12 +188,8 @@ mi_gdb_test "-interpreter-exec console \"help set args\"" \
   {\~"Set argument list to give program being debugged when it is started\.\\nFollow this command with any number of args, to be passed to the program\.".*\^done} \
   "-interpreter-exec console \"help set args\""
 
-# NOTE: cagney/2003-02-03: Not yet.
-# mi_gdb_test "-interpreter-exec console \"set \$pc=0x0\"" \
-#   {.*=target-changed.*\^done} \
-#   "-interpreter-exec console \"set \$pc=0x0\""
 mi_gdb_test "888-interpreter-exec console \"set \$pc=0x0\"" \
-  {888\^done} \
+  {.*=register-changed,name=\".*\",value=\"0x0\",group-id=\".*\",frame=.*\^done} \
   "-interpreter-exec console \"set \$pc=0x0\""
 
 #mi_gdb_test "-interpreter-exec console \"\"" \
diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.c b/gdb/testsuite/gdb.mi/mi-reg-changed.c
new file mode 100644
index 0000000..3d6950b
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-reg-changed.c
@@ -0,0 +1,30 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static int
+middle (int x)
+{
+  x = 2 * x;
+  x++;
+  return x;
+}
+
+int
+main (int argc, char **argv)
+{
+  return middle (argc);
+}
diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.exp b/gdb/testsuite/gdb.mi/mi-reg-changed.exp
new file mode 100644
index 0000000..9934b4d
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-reg-changed.exp
@@ -0,0 +1,159 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile .c
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested mi-reg-changed.exp
+     return -1
+}
+
+proc test_register_change { } {
+    with_test_prefix "simple" {
+	if [mi_gdb_start] {
+	    return
+	}
+	mi_run_to_main
+
+	global hex
+	global decimal
+	global expect_out
+
+	set reg ""
+	# Extract the name of a register.
+	mi_gdb_test "-data-list-register-names" \
+	    "\\\^done,register-names=\\\[\"(\[a-zA-Z0-9\]+)\".*\\\].*" \
+	    "-data-list-register-names"
+	set reg $expect_out(3,string)
+
+	if { $reg == "" } {
+	    fail "get the register name"
+	    return
+	}
+
+	set reg_val ""
+	if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	    set reg_val $expect_out(3,string)
+	}
+	if { $reg_val == "" } {
+	    fail "get the content of register ${reg}"
+	    return
+	}
+
+	mi_gdb_test "set \$${reg} = ${reg_val}" \
+	    ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}.*\\^done" \
+	    "change reg ${reg}"
+
+	mi_gdb_test "-var-create fp_var * \$${reg}" \
+	    "\\^done,name=\"fp_var\",numchild=\"0\",value=\".*\",type=\".*\",has_more=\"0\"" \
+	    "create objvar for reg ${reg}"
+	# GDB should not emit MI notification for register change requested
+	# from MI frontend.
+	mi_gdb_test "-var-assign fp_var ${reg_val}" \
+	    "\\^done,value=\".*\"" "change reg ${reg} thru. varobj"
+
+	mi_gdb_exit
+    }
+}
+
+proc test_register_on_frame { } {
+    with_test_prefix "frame" {
+	if [mi_gdb_start] {
+	    return
+	}
+	mi_run_to_main
+
+	global decimal
+	global hex
+	global expect_out
+	global mi_gdb_prompt
+
+	# A (register) value in memory in previous frame.  E.g.,
+	# call-saved register.
+	set reg ""
+	if [is_amd64_regs_target] {
+	    set reg "rbp"
+	} elseif [is_x86_like_target] {
+	    set reg "ebp"
+	} else {
+	    return
+	}
+
+	mi_gdb_test "-break-insert -t middle" \
+	    "\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"del\".*" \
+	    "-break-insert -t middle"
+	mi_execute_to "exec-continue" "breakpoint-hit" "middle" ".*" ".*" \
+	    ".*" { "" "disp=\"del\"" } "-exec-continue to middle"
+
+	set reg_val ""
+
+	if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	    set reg_val $expect_out(3,string)
+	}
+
+	mi_gdb_test "set \$${reg} = ${reg_val}" \
+	    ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"middle\",.*}.*\\^done" \
+	    "change reg ${reg}"
+
+	mi_gdb_test "up" ".*" "up"
+
+	set reg_val ""
+	if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] {
+	    set reg_val $expect_out(3,string)
+	}
+
+	set test "change reg ${reg} in upper frame"
+	set see_memory_change 0
+	set see_register_change 0
+	send_gdb "set \$${reg} = ${reg_val}\n"
+	# Changing register saved on frame will trigger two notifications,
+	# register change notification and memory change notification.  Check
+	# them.
+	gdb_expect {
+	    -re ".*=memory-changed,thread-group=\"i1\",addr=\"${hex}\",len=\"${hex}\"" {
+		set see_memory_change 1
+		exp_continue
+	    }
+	    -re ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}" {
+		set see_register_change 1
+		exp_continue
+	    }
+	    -re ".*${mi_gdb_prompt}$" {
+	    }
+	}
+
+	if $see_register_change {
+	    pass "${test}: register change"
+	} else {
+	    fail "${test}: register change"
+	}
+	if $see_memory_change {
+	    pass "${test}: memory change"
+	} else {
+	    fail "${test}: memory change"
+	}
+
+	mi_gdb_exit
+    }
+}
+
+test_register_change
+
+test_register_on_frame
+
+return 0
diff --git a/gdb/valops.c b/gdb/valops.c
index 502fb0d..dfea1cb 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval)
   struct type *type;
   struct value *val;
   struct frame_id old_frame;
+  struct frame_info *frame;
 
   if (!deprecated_value_modifiable (toval))
     error (_("Left operand of assignment is not a modifiable lvalue."));
@@ -1239,7 +1240,8 @@ value_assign (struct value *toval, struct value *fromval)
   /* Since modifying a register can trash the frame chain, and
      modifying memory can trash the frame cache, we save the old frame
      and then restore the new frame afterwards.  */
-  old_frame = get_frame_id (deprecated_safe_get_selected_frame ());
+  frame = deprecated_safe_get_selected_frame ();
+  old_frame = get_frame_id (frame);
 
   switch (VALUE_LVAL (toval))
     {
@@ -1305,7 +1307,6 @@ value_assign (struct value *toval, struct value *fromval)
 
     case lval_register:
       {
-	struct frame_info *frame;
 	struct gdbarch *gdbarch;
 	int value_reg;
 
@@ -1462,6 +1463,11 @@ value_assign (struct value *toval, struct value *fromval)
       set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
     }
 
+  if (VALUE_REGNUM (toval) >= 0 && frame != NULL)
+    observer_notify_register_changed (VALUE_REGNUM (toval), frame,
+				      inferior_ptid,
+				      value_contents (val));
+
   return val;
 }
 
diff --git a/gdb/value.c b/gdb/value.c
index 3feb1ca..7720cd1 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -318,7 +318,9 @@ struct value
      taken off this list.  */
   struct value *next;
 
-  /* Register number if the value is from a register.  */
+  /* Register number if the value presents a register.  The contents may
+     be from a register or memory, which is determined by field
+     'LVAL'.  */
   short regnum;
 
   /* Actual contents of the value.  Target byte-order.  NULL or not
-- 
1.7.7.6

Re: [RFC] MI notification on register changes

[Tom Tromey]

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

Tom> The register may be stored in memory, but it this case really ever
Tom> represented as lval_memory in gdb?  I thought the reason for

Yao> Yes.

We discussed this on irc a bit and I thought I'd email the list to keep
the thread up-to-date.

First, Yao sees that lval_memory on x86, but I don't see it on x86-64.
I wonder whether it is a bug elsewhere.

Second, Pedro pointed out this PR:

    http://sourceware.org/bugzilla/show_bug.cgi?id=7574

... which seems apropos.  It seems that the best thing is a generic
"target changed" notification for the reasons mentioned there.

Tom

Re: [RFC] MI notification on register changes

[Yao Qi]

On 11/03/2012 12:42 AM, Tom Tromey wrote:
> First, Yao sees that lval_memory on x86, but I don't see it on x86-64.
> I wonder whether it is a bug elsewhere.
>

Tom,
I also see lval_memory on x86-64, and I am a little surprised that you 
see lval_register.

(gdb) b middle
Breakpoint 1 at 0x40056d
(gdb) run
Starting program: gdb/testsuite/gdb.base/nodebug

Breakpoint 1, 0x000000000040056d in middle ()
(gdb) up
#1  0x000000000040059d in top ()
(gdb) p/x $rbp
$1 = 0x7fffffffe5a0
(gdb) set $rbp=0x7fffffffe5a0

Breakpoint 1, value_assign (toval=0xdaf7a0, fromval=0xe36170) at 
../../gdb/gdb/valops.c:1246
1246	  switch (VALUE_LVAL (toval))
top>p toval->lval
$1 = lval_memory

and I think it is reasonable to see 'lval_memory' instead of 
'lval_register' here.  When executing this 'set' command, the callchain 
looks like,

evaluate_subexp_standard
    |
    ' case OP_REGISTER: value_of_register

In value_of_register,

   frame_register (frame, regnum, &optim, &unavail,
		  &lval, &addr, &realnum, raw_buffer);
....
   VALUE_LVAL (reg_val) = lval;


The 'lval' can be set to 'lval_register' or 'lval_memory' in 
frame_register.  So I don't see anything wrong here.

> Second, Pedro pointed out this PR:
>
>      http://sourceware.org/bugzilla/show_bug.cgi?id=7574
>
> ... which seems apropos.  It seems that the best thing is a generic
> "target changed" notification for the reasons mentioned there.

'target changed' observer and 'register changed' observer serves 
differently.  'target changed' observer is mostly for keeping gdb 
internal states, such as regcache and frames, consistent, however, 
'register changed' observer is for external notifications.

AFAICS, the reason 'target changed' observer is not replaced by 'memory 
changed' observer and 'register change' observer is that GDB doesn't 
know the side effect of setting a register or a piece of memory, so GDB 
conservatively uses 'target changed' observer.

-- 
Yao

Re: [RFC] MI notification on register changes

[Tom Tromey]

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

Yao> I also see lval_memory on x86-64, and I am a little surprised that you
Yao> see lval_register.
[...]
Yao> and I think it is reasonable to see 'lval_memory' instead of
Yao> lval_register' here.  When executing this 'set' command, the callchain
Yao> looks like,

Thanks for walking me through it.
I agree this is the intended design.

>> Second, Pedro pointed out this PR:
>> http://sourceware.org/bugzilla/show_bug.cgi?id=7574
>> ... which seems apropos.  It seems that the best thing is a generic
>> "target changed" notification for the reasons mentioned there.

Yao> 'target changed' observer and 'register changed' observer serves
Yao> differently.  'target changed' observer is mostly for keeping gdb
Yao> internal states, such as regcache and frames, consistent, however,
Yao> register changed' observer is for external notifications.

Yao> AFAICS, the reason 'target changed' observer is not replaced by
Yao> memory changed' observer and 'register change' observer is that GDB
Yao> doesn't know the side effect of setting a register or a piece of
Yao> memory, so GDB conservatively uses 'target changed' observer.

I think the implication is that nearly any change can require the front
end to need to reset everything and so trying to make gdb differentiate
between the various events will never work out.  So, gdb might as well
emit something very generic rather than =register-changed.

For example, a register change can affect the variable view, the memory
view (if the register is from some more-outer frame and the user is
viewing the stack), the backtrace view, etc.

Or, a memory change can also affect any of these, including the register
view if the changed memory happens to be where a register is stored.

So, in general, it seems the only safe thing for a front end is to just
refresh everything on any change.

What do you think?

Tom

Re: [RFC] MI notification on register changes

[Yao Qi]

On 11/06/2012 04:10 AM, Tom Tromey wrote:
> I think the implication is that nearly any change can require the front
> end to need to reset everything and so trying to make gdb differentiate
> between the various events will never work out.  So, gdb might as well
> emit something very generic rather than =register-changed.
>

If GDB emits a very generic notification (for register change, memory 
change, etc), I am not sure how useful it is.  It would be noisy to 
frondend, IMO.
Do we have to emit this notification if the frontend requests to modify 
data in memory? because memory change may affect some register.

> For example, a register change can affect the variable view, the memory
> view (if the register is from some more-outer frame and the user is
> viewing the stack), the backtrace view, etc.
>
> Or, a memory change can also affect any of these, including the register
> view if the changed memory happens to be where a register is stored.
>

I agree with your points.

> So, in general, it seems the only safe thing for a front end is to just
> refresh everything on any change.
>

Not really.  That is true to GDB internal states update, because GDB is 
not clear what is the effect of a register change or memory change.  It 
is safe to reset everything, as you said.  However, for notifications to 
external clients, such as MI front-end, I am not sure it is a good idea 
to emit such notification to MI front-end "everything will be changed" 
just because GDB doesn't exactly know what will happen to the inferior.

My rationale here is like this: for the gdb internal state update, false 
positive (notify observers, but nothing is changed in fact) is fine and 
false negative (something is changed but observers are not notified) is 
not allowed.  For the external notification to clients, false negative 
is fine (it is a limitation of gdb, and will only happen in some corner 
cases), but false positive is noisy.

-- 
Yao

Re: [RFC] MI notification on register changes

[André Pönitz]

On Tue, Nov 06, 2012 at 02:04:59PM +0800, Yao Qi wrote:
> On 11/06/2012 04:10 AM, Tom Tromey wrote:
> >I think the implication is that nearly any change can require the front
> >end to need to reset everything and so trying to make gdb differentiate
> >between the various events will never work out.  So, gdb might as well
> >emit something very generic rather than =register-changed.
> >
> 
> If GDB emits a very generic notification (for register change,
> memory change, etc), I am not sure how useful it is.  It would be
> noisy to frondend, IMO.

Why?

As soon as a single bit changes somewhere, the only formally correct
response from a frontend is to re-fetch everything.

One can collect several such notification before triggering a full
refetch (no user will insist on updates everyt 50ms, and gdb won't
deliver that anyway), or one can take some risk and not refetch
everything, which in most cases would even be "good enough", but
in general a refetch is needed. 

[...]
> My rationale here is like this: for the gdb internal state update,
> false positive (notify observers, but nothing is changed in fact) is
> fine and false negative (something is changed but observers are not
> notified) is not allowed.  For the external notification to clients,
> false negative is fine (it is a limitation of gdb, and will only
> happen in some corner cases), but false positive is noisy.

I wouldn't call "false external positives" noisy, unless they generate
so much traffic that there is a performance bottleneck. A frontend
can consciously decide to ignore them.

Andre'

RE: [RFC] MI notification on register changes

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of André Pönitz
> Sent: Tuesday, November 06, 2012 3:00 PM
> To: Yao Qi
> Cc: Tom Tromey; gdb-patches@sourceware.org
> Subject: Re: [RFC] MI notification on register changes
> 
> On Tue, Nov 06, 2012 at 02:04:59PM +0800, Yao Qi wrote:
> > On 11/06/2012 04:10 AM, Tom Tromey wrote:
> > >I think the implication is that nearly any change can 
> require the front
> > >end to need to reset everything and so trying to make gdb 
> differentiate
> > >between the various events will never work out.  So, gdb 
> might as well
> > >emit something very generic rather than =register-changed.
> > >
> > 
> > If GDB emits a very generic notification (for register change,
> > memory change, etc), I am not sure how useful it is.  It would be
> > noisy to frondend, IMO.
> 
> Why?
> 
> As soon as a single bit changes somewhere, the only formally correct
> response from a frontend is to re-fetch everything.

IIUC, the notifications being proposed will only be triggered if
registers are changed by the user.  This is something that the frontend
needs to be told about because the assumption is that registers/memory
are stable when the program is suspended.  I don't believe a full
refresh is needed if such notifications contain the changed info.

Register changes caused by program execution are not targetted
by this notification, or at least I don't think they should be.

Marc


> One can collect several such notification before triggering a full
> refetch (no user will insist on updates everyt 50ms, and gdb won't
> deliver that anyway), or one can take some risk and not refetch
> everything, which in most cases would even be "good enough", but
> in general a refetch is needed. 
> 
> [...]
> > My rationale here is like this: for the gdb internal state update,
> > false positive (notify observers, but nothing is changed in fact) is
> > fine and false negative (something is changed but observers are not
> > notified) is not allowed.  For the external notification to clients,
> > false negative is fine (it is a limitation of gdb, and will only
> > happen in some corner cases), but false positive is noisy.
> 
> I wouldn't call "false external positives" noisy, unless they generate
> so much traffic that there is a performance bottleneck. A frontend
> can consciously decide to ignore them.
> 
> Andre'
> 

Re: [RFC] MI notification on register changes

[Pedro Alves]

Marc Khouzam wrote:

>> André Pönitz wrote:
>> On Tue, Nov 06, 2012 at 02:04:59PM +0800, Yao Qi wrote:
>>> If GDB emits a very generic notification (for register change,
>>> memory change, etc), I am not sure how useful it is.  It would be
>>> noisy to frondend, IMO.
>>
>> Why?
>>
>> As soon as a single bit changes somewhere, the only formally correct
>> response from a frontend is to re-fetch everything.
> 
> IIUC, the notifications being proposed will only be triggered if
> registers are changed by the user.  This is something that the frontend
> needs to be told about because the assumption is that registers/memory
> are stable when the program is suspended.  I don't believe a full
> refresh is needed if such notifications contain the changed info.

The issue is in the "contain the changed info" part.  In general, we don't have such
info available.  So there are cases where trusting such fine-grained
notifications will get you stale views.  For example, in the general case, if the
user changes registers/memory, we can't assume that memory/registers didn't change.
Some targets even have memory-mapped registers.  If GDB core shouldn't assume such
things, then "registers changed" or "memory changed" notifications are in general
misleading.  We'd have to add notes like "note that even though we're saying only
a register changed, everything else (memory, stack frame, whole stack, etc.)
might have changed too, so you're better off refreshing all your views." which may
be a bit silly.  We may be better off not giving ourselves and the frontends rope
to hang ourselves with.

The related question is then, why is a full refresh an issue, if that is something
the must already do after every stop, such as e.g., after each "step".  As PR 7574
hints at, things like "step" are the use cases users will notice and complain
the most if slow.  It seems like optimizing the frontends with selecting/partial updates
for some cases still leaves you with needing to optimize the full-refresh case
anyway for more important cases.

-- 
Pedro Alves

RE: [RFC] MI notification on register changes

[Marc Khouzam]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com] 
> Sent: Wednesday, November 07, 2012 12:01 PM
> To: Marc Khouzam
> Cc: 'André Pönitz'; 'Yao Qi'; 'Tom Tromey'; 
> 'gdb-patches@sourceware.org'
> Subject: Re: [RFC] MI notification on register changes
> 
> Marc Khouzam wrote:
> 
> >> André Pönitz wrote:
> >> On Tue, Nov 06, 2012 at 02:04:59PM +0800, Yao Qi wrote:
> >>> If GDB emits a very generic notification (for register change,
> >>> memory change, etc), I am not sure how useful it is.  It would be
> >>> noisy to frondend, IMO.
> >>
> >> Why?
> >>
> >> As soon as a single bit changes somewhere, the only 
> formally correct
> >> response from a frontend is to re-fetch everything.
> > 
> > IIUC, the notifications being proposed will only be triggered if
> > registers are changed by the user.  This is something that 
> the frontend
> > needs to be told about because the assumption is that 
> registers/memory
> > are stable when the program is suspended.  I don't believe a full
> > refresh is needed if such notifications contain the changed info.
> 
> The issue is in the "contain the changed info" part.  In 
> general, we don't have such
> info available.  So there are cases where trusting such fine-grained
> notifications will get you stale views.  For example, in the 
> general case, if the
> user changes registers/memory, we can't assume that 
> memory/registers didn't change.
> Some targets even have memory-mapped registers.  If GDB core 
> shouldn't assume such
> things, then "registers changed" or "memory changed" 
> notifications are in general
> misleading.  We'd have to add notes like "note that even 
> though we're saying only
> a register changed, everything else (memory, stack frame, 
> whole stack, etc.)
> might have changed too, so you're better off refreshing all 
> your views." which may
> be a bit silly.  We may be better off not giving ourselves 
> and the frontends rope to hang ourselves with.

Ok, I understand the problem now.
It should probably up to GDB to determine, if possible,
the safe cases which can specify the "changed info" and 
the unsafe cases which should not, therefore telling the 
frontend to do a full refresh.  Which brings your next
question:

> The related question is then, why is a full refresh an issue, 
> if that is something
> the must already do after every stop, such as e.g., after 
> each "step".  As PR 7574
> hints at, things like "step" are the use cases users will 
> notice and complain
> the most if slow.  It seems like optimizing the frontends 
> with selecting/partial updates
> for some cases still leaves you with needing to optimize the 
> full-refresh case
> anyway for more important cases.

Avoiding a full refresh is probably a result of inertia.
Since GDB allows the frontend to try to avoid a full
refresh by giving extra information in its notifications,
the idea of continuing with that approach seemed the right one.
For instance, GDB could have a single =breakpoints-changed
notif which would trigger a full refresh a breakpoints.
Same for variable objects (see -var-update).  Even
=threads-changed (although this may be a little more
justified if we think about it more deeply).

Fundamentally, erroneous data is the worse case.  So,
having a notification without "changed data" is better 
than an invalid or incomplete "changed data", or no
notification at all :).

So, if there are cases where a full refresh is needed
then I can understand that it may not be worth trying
to provide the "changed data" only once in a while.

Thanks for the explanation.

Marc

Re: [RFC] MI notification on register changes

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Since GDB allows the frontend to try to avoid a full
Marc> refresh by giving extra information in its notifications,
Marc> the idea of continuing with that approach seemed the right one.
Marc> For instance, GDB could have a single =breakpoints-changed
Marc> notif which would trigger a full refresh a breakpoints.
Marc> Same for variable objects (see -var-update).  Even
Marc> =threads-changed (although this may be a little more
Marc> justified if we think about it more deeply).

In some cases, like breakpoints, gdb has perfect information.
In these situations, sending small changes is easy and efficient.

For memory or registers, I think the PR established pretty clearly that
gdb doesn't have perfect information, or at the very least, that trying
to compute it perfectly would be a lot of work, and so it is preferable
to avoid doing so if possible.

For varobjs, gdb caches the previously sent data so that it can send
deltas.  I think this could be done with other the entities in question
-- registers, memory, stack frames -- if the communications overhead of
a full update is the rate-limiting step.  On the other hand, if the
computation in gdb (including communication with the inferior) is
rate-limiting, then this won't help at all.

Threads are a kind of funny case, see the "finding new threads in the
inferior" item here: http://sourceware.org/gdb/wiki/LocalRemoteFeatureParity

Tom

Re: [RFC] MI notification on register changes

[André Pönitz]

On Wed, Nov 07, 2012 at 11:54:09AM -0700, Tom Tromey wrote:
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> Since GDB allows the frontend to try to avoid a full
> Marc> refresh by giving extra information in its notifications,
> Marc> the idea of continuing with that approach seemed the right one.
> Marc> For instance, GDB could have a single =breakpoints-changed
> Marc> notif which would trigger a full refresh a breakpoints.
> Marc> Same for variable objects (see -var-update).  Even
> Marc> =threads-changed (although this may be a little more
> Marc> justified if we think about it more deeply).
> 
> In some cases, like breakpoints, gdb has perfect information.
> In these situations, sending small changes is easy and efficient.
>
> For memory or registers, I think the PR established pretty clearly that
> gdb doesn't have perfect information, or at the very least, that trying
> to compute it perfectly would be a lot of work, and so it is preferable
> to avoid doing so if possible.

Computing perfect updates is not possible.

GDB would need to keep track of all memory locations and registers used
to create the representation of the value, and even that would not be
perfect in the presence of "external" sources of information like the
result of system calls or similar.
 
> For varobjs, gdb caches the previously sent data so that it can send
> deltas.

And this is not sufficient for proper display. Consider:

In .gdbinit (or similar):

    python

    class Printer(object):

        def __init__(self, val):
            self.val = val

        def to_string(self):
            return ["foo", "bar"][int(gdb.parse_and_eval("d"))]
           

        def display_hint(self):
            return 'string'


    def P(val):
        if str(val.type) == "struct S":
            return Printer(val)
        return None

    gdb.pretty_printers = [ P ]

    end



In main.c:

    int d;

    struct S { } s;

    int main()
    {
        d = 1;
        d = 0;
        d = 1;
        d = 0;
        d = 1;
        d = 0;
        d = 1;
        d = 0;
        d = 1;
        d = 0;
        d = 1;
    }


With "full refetch" you get alternating values for s when stepping:


    (gdb) display d
    1: d = 0
    (gdb) display s
    2: s = "foo"
    (gdb) n
    41          d = 0;
    2: s = "bar"
    1: d = 1
    (gdb) 
    42          d = 1;
    2: s = "foo"
    1: d = 0
    (gdb) 
    43          d = 0;
    2: s = "bar"
    1: d = 1
    (gdb) 
    44          d = 1;
    2: s = "foo"
    1: d = 0
    (gdb) 
    45          d = 0;
    2: s = "bar"
    1: d = 1
    (gdb) 

Using MI varobj's you can get something like that:

    (gdb) -var-create d * d
    ^done,name="d",numchild="0",value="0",type="int",has_more="0"
    (gdb) -var-create s * s
    ^done,name="s",numchild="0",value="{...}",type="struct S",has_more="0"

    (gdb) n
    ^running
    *running,thread-id="all"
    (gdb) 
    ~"39\t    d = 0;\n"
    *stopped,frame={addr="0x080483e9",...,line="39"},....

    (gdb) -var-update *
    ^done,changelist=[{name="d",in_scope="true",type_changed="false",has_more="0"}]

    (gdb) n
    ^running
    *running,thread-id="all"
    (gdb) 
    ~"40\t    d = 1;\n"
    *stopped,frame={addr="0x080483e9",...,line="40"},....

    (gdb) -var-update *
    ^done,changelist=[{name="d",in_scope="true",type_changed="false",has_more="0"}]


Only the changes to d are announced.  For the frontend there is no
indication whatsoever that the display of s would need to be updated.

The only way to get the proper alternating display is to fully refetch
everything again:

    (gdb) p s
    &"p s\n"
    ~"$1 = \"foo\"\n"
    ^done
    (gdb) p d
    &"p d\n"
    ~"$2 = 0"
    ~"\n"
    ^done

    (gdb) n
    &"n\n"
    ^running
    *running,thread-id="all"
    (gdb) 
    ~"41\t    d = 0;\n"
    *stopped,frame={addr="0x080483fd",...,line="41"},....
    (gdb) 
    p s
    &"p s\n"
    ~"$3 = \"bar\"\n"
    ^done
    (gdb) 



Andre'

Re: [RFC] MI notification on register changes

[Yao Qi]

On 11/08/2012 01:00 AM, Pedro Alves wrote:
> The issue is in the "contain the changed info" part.  In general, we don't have such
> info available.  So there are cases where trusting such fine-grained
> notifications will get you stale views.  For example, in the general case, if the
> user changes registers/memory, we can't assume that memory/registers didn't change.
> Some targets even have memory-mapped registers.   If GDB core shouldn't assume such
> things, then "registers changed" or "memory changed" notifications are in general

It sounds like a 'memory model' (it is not proper here, but that is the 
term I can think of), a single write to register or memory may cause 
different effects.

If we want to solve this problem in GDB side, we need a precise and 
accurate target description to address them below, don't we?

   1) modify register FOO will update memory area 0xN ~ 0xM, and vice versa,
   2) register BAR is read only, or at least some bits are.
   3) this target doesn't have these special hardware features,

> misleading.  We'd have to add notes like "note that even though we're saying only
> a register changed, everything else (memory, stack frame, whole stack, etc.)
> might have changed too, so you're better off refreshing all your views." which may
> be a bit silly.  We may be better off not giving ourselves and the frontends rope
> to hang ourselves with.

Supposing gdb has this precise target description, gdb doesn't have to 
refresh everything else, otherwise gdb has to conservatively update 
everything like what it does nowadays.  Do you agree?

-- 
Yao