This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] MI notification on register changes


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]