This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] MI notification on register changes
- From: Yao Qi <yao at codesourcery dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Fri, 2 Nov 2012 17:56:40 +0800
- Subject: Re: [RFC] MI notification on register changes
- References: <1350977008-28632-1-git-send-email-yao@codesourcery.com> <87lielqcba.fsf@fleche.redhat.com>
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