* [RFC] MI notification on register changes @ 2012-10-23 7:24 Yao Qi 2012-11-01 0:36 ` ping: " Yao Qi 2012-11-01 20:48 ` Tom Tromey 0 siblings, 2 replies; 15+ messages in thread From: Yao Qi @ 2012-10-23 7:24 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* ping: [RFC] MI notification on register changes 2012-10-23 7:24 [RFC] MI notification on register changes Yao Qi @ 2012-11-01 0:36 ` Yao Qi 2012-11-01 20:48 ` Tom Tromey 1 sibling, 0 replies; 15+ messages in thread From: Yao Qi @ 2012-11-01 0:36 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-10-23 7:24 [RFC] MI notification on register changes Yao Qi 2012-11-01 0:36 ` ping: " Yao Qi @ 2012-11-01 20:48 ` Tom Tromey 2012-11-02 9:56 ` Yao Qi 1 sibling, 1 reply; 15+ messages in thread From: Tom Tromey @ 2012-11-01 20:48 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-01 20:48 ` Tom Tromey @ 2012-11-02 9:56 ` Yao Qi 2012-11-02 16:42 ` Tom Tromey 0 siblings, 1 reply; 15+ messages in thread From: Yao Qi @ 2012-11-02 9:56 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-02 9:56 ` Yao Qi @ 2012-11-02 16:42 ` Tom Tromey 2012-11-05 10:26 ` Yao Qi 0 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2012-11-02 16:42 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-02 16:42 ` Tom Tromey @ 2012-11-05 10:26 ` Yao Qi 2012-11-05 20:10 ` Tom Tromey 0 siblings, 1 reply; 15+ messages in thread From: Yao Qi @ 2012-11-05 10:26 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-05 10:26 ` Yao Qi @ 2012-11-05 20:10 ` Tom Tromey 2012-11-06 6:05 ` Yao Qi 0 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2012-11-05 20:10 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-05 20:10 ` Tom Tromey @ 2012-11-06 6:05 ` Yao Qi 2012-11-06 20:00 ` André Pönitz 0 siblings, 1 reply; 15+ messages in thread From: Yao Qi @ 2012-11-06 6:05 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-06 6:05 ` Yao Qi @ 2012-11-06 20:00 ` André Pönitz 2012-11-07 15:35 ` Marc Khouzam 0 siblings, 1 reply; 15+ messages in thread From: André Pönitz @ 2012-11-06 20:00 UTC (permalink / raw) To: Yao Qi; +Cc: Tom Tromey, gdb-patches 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' ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC] MI notification on register changes 2012-11-06 20:00 ` André Pönitz @ 2012-11-07 15:35 ` Marc Khouzam 2012-11-07 17:01 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Marc Khouzam @ 2012-11-07 15:35 UTC (permalink / raw) To: 'André Pönitz', 'Yao Qi' Cc: 'Tom Tromey', 'gdb-patches@sourceware.org' > -----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' > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-07 15:35 ` Marc Khouzam @ 2012-11-07 17:01 ` Pedro Alves 2012-11-07 18:18 ` Marc Khouzam 2012-11-09 1:47 ` Yao Qi 0 siblings, 2 replies; 15+ messages in thread From: Pedro Alves @ 2012-11-07 17:01 UTC (permalink / raw) To: Marc Khouzam Cc: 'André Pönitz', 'Yao Qi', 'Tom Tromey', 'gdb-patches@sourceware.org' 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC] MI notification on register changes 2012-11-07 17:01 ` Pedro Alves @ 2012-11-07 18:18 ` Marc Khouzam 2012-11-07 18:54 ` Tom Tromey 2012-11-09 1:47 ` Yao Qi 1 sibling, 1 reply; 15+ messages in thread From: Marc Khouzam @ 2012-11-07 18:18 UTC (permalink / raw) To: 'Pedro Alves' Cc: 'André Pönitz', 'Yao Qi', 'Tom Tromey', 'gdb-patches@sourceware.org' > -----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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-07 18:18 ` Marc Khouzam @ 2012-11-07 18:54 ` Tom Tromey 2012-11-07 21:09 ` André Pönitz 0 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2012-11-07 18:54 UTC (permalink / raw) To: Marc Khouzam Cc: 'Pedro Alves', 'André Pönitz', 'Yao Qi', 'gdb-patches@sourceware.org' >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-07 18:54 ` Tom Tromey @ 2012-11-07 21:09 ` André Pönitz 0 siblings, 0 replies; 15+ messages in thread From: André Pönitz @ 2012-11-07 21:09 UTC (permalink / raw) To: Tom Tromey Cc: Marc Khouzam, 'Pedro Alves', 'Yao Qi', 'gdb-patches@sourceware.org' 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' ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC] MI notification on register changes 2012-11-07 17:01 ` Pedro Alves 2012-11-07 18:18 ` Marc Khouzam @ 2012-11-09 1:47 ` Yao Qi 1 sibling, 0 replies; 15+ messages in thread From: Yao Qi @ 2012-11-09 1:47 UTC (permalink / raw) To: Pedro Alves Cc: Marc Khouzam, 'André Pönitz', 'Tom Tromey', 'gdb-patches@sourceware.org' 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-11-09 1:47 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-10-23 7:24 [RFC] MI notification on register changes Yao Qi 2012-11-01 0:36 ` ping: " Yao Qi 2012-11-01 20:48 ` Tom Tromey 2012-11-02 9:56 ` Yao Qi 2012-11-02 16:42 ` Tom Tromey 2012-11-05 10:26 ` Yao Qi 2012-11-05 20:10 ` Tom Tromey 2012-11-06 6:05 ` Yao Qi 2012-11-06 20:00 ` André Pönitz 2012-11-07 15:35 ` Marc Khouzam 2012-11-07 17:01 ` Pedro Alves 2012-11-07 18:18 ` Marc Khouzam 2012-11-07 18:54 ` Tom Tromey 2012-11-07 21:09 ` André Pönitz 2012-11-09 1:47 ` Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox